As a first step to solve #1074960: Let maintainers set a default branch on git repositories we need methods to set and get the default branch of a repository. To set a default branch a symbolic ref from HEAD to refs/heads/<branchname>
must be created. Then, when cloning, <branchname>
will be checked out by default.
The attached patch adds a testcase for this and implements it. Please review.
Because of #1291272: VersionControlGitTestCase::versioncontrolCreateRepoFromClone() trys to call protected method VersioncontrolGitRepository::exec() the testCloning() method will fail - the secound patch makes the exec() method public to show that it would work, if #1291272: VersionControlGitTestCase::versioncontrolCreateRepoFromClone() trys to call protected method VersioncontrolGitRepository::exec() was fixed. It is only the test case that fails because of this, setDefaultBranch() works either way.
Comment | File | Size | Author |
---|---|---|---|
#12 | 1291308-default-branch-fixup-12.patch | 858 bytes | Niklas Fiekas |
#8 | 1291308-default-branch-8.patch | 9.89 KB | Niklas Fiekas |
#4 | 1291308-default-branch-schema.patch | 2.29 KB | Niklas Fiekas |
#4 | 1291308-default-branch-override-controller.patch | 3.79 KB | Niklas Fiekas |
#2 | 1291308-default-branch-2.patch | 3.44 KB | Niklas Fiekas |
Comments
Comment #1
pillarsdotnet CreditAttribution: pillarsdotnet commentedAs requested. Since I can't fault the code, I'll nit-pick the documentation. Coding standards require that function summaries should start with a third-person present indicative verb, and fit on a single 80-character line.
Should perhaps be:
Should perhaps be:
Should perhaps be:
Should perhaps be:
Should perhaps be:
Should perhaps be:
Should perhaps be:
Should be:
Should perhaps be:
Should perhaps be:
Comment #2
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThank you for your review, pillarsdotnet.
Both patches are the same except the latter contains this hunk:
The new versions contain all your suggestions except:
to
That one was just from the context and I didn't touch it in my patch. Maybe this could be done in a seperate issue together with other coding-style / comment fixes.
Comment #3
sdboyer CreditAttribution: sdboyer commentedVery happy to see somebody stepping up for this kinda stuff. Unfortunately I have to crush this a bit with many additional requirements, but @Niklas Fiekas was pretty gracious about that in irc :) brief summary of those issues:
setDefaultBranch()
) that passes through a job queue, and is picked up by workers on the other side. This works kinda well enough for the async write situation. It's not robust enough to really deserve the title 'framework,' but it's good enough to not require rearchitecting.getDefaultBranch()
is an excellent example of this. This is a very different kind of challenge than the async one. It's already a requirement, though, for the new, Drupal-native version of a repository browser to go in. GitHub uses BERT, an RPC system, to do it; we're looking at a node.js-backed solution. The transport layer doesn't really matter that much, though, tbh.getDefaultBranch()
isn't something we necessarily want to require a blocking network + io call. We still need the sync framework described above, but for this specific case, it would be better to have the data stored locally - we can treat that as canonical of the user's preference, which lets us clean up any potential anomalies on disk. Doing that means adding a{versioncontrol_git_repositories}
table, with adefault_branch
field, and corresponding CRUD to the various entities that manage it (VersioncontrolGitRepository
,VersioncontrolGitRepositoryController
. We could do this with an overridden controller or a hook, but I prefer overriding).All that said, I don't really mind keeping these methods on the
VersioncontrolGitRepository
object itself; they make logical sense as part of the API. But they need to connect to these indirection layers instead of doing the work directly themselves.Comment #4
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for summarizing this. Two new patches.
The first one defines the schema of the
versioncontrol_git_repositories
table and creates it.The secound extends the VersioncontrolRepositoryController with a VersioncontrolGitRepositoryController that loads data from that table and adds CRUD to the VersioncontrolGitRepository class.
Comment #5
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThis issue is blocked by #1293720: Allow backend specific repository controllers.
Comment #6
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedUpdate:
I forked this on github, because I can rebase there to give you a linear history (at least until someone forks me): https://github.com/niklasf/versioncontrol_git
We've got the schema and the controller. Next are getDefaultBranch() and setDefaultBranch(). What do they do?Can we do something like that in getDefaultBranch()?How do I queue a write operation?Comment #7
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedUpdate:
Implemented getter an setter.
I don't think we need an update function, since all default branches should be master.Is this it, or do we need something else (except a UI)?Comment #8
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThe attached patch:
versioncontrol_git_repositories
table.VersioncontrolGitRepositoryController
(that depended on #1293720: Allow backend specific repository controllers).VersioncontrolGitRepository::$default_branch
.fetchDefaultBranch()
method to the worker.I rewrote the history to make those steps single commits in my fork on github: https://github.com/niklasf/versioncontrol_git/compare/11a4940156...8ea7c...
Comment #9
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for reviewing. I am marking this as fixed, becuase it's merged in.
Comment #11
sdboyer CreditAttribution: sdboyer commentedThe original 6203 update created the table, but it did not create records in that table for each currently existing repository. As a result, no subsequent update operations work as they assume those records to already exist.
Please add a 6205 update that creates records, even if they're empty ones, for all (git) repositories.
I'm sure this was my oversight, letting this in as it is.
Comment #12
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedPlease test this one.
Comment #13
sdboyer CreditAttribution: sdboyer commentedYep, looks good. Go ahead and push it.
Comment #14
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedhttp://drupalcode.org/project/versioncontrol_git.git/commitdiff/e84b44d.