How to handle Git (or other VCS) allowed branch/tag names?

haxney's picture

I'm working on the Git hooks (specifically the 'update' hook right now) and am trying to figure out what sorts of things we would want in a pre-commit (or pre-push, in the case of DVCSs) hook.

What I have now

For now, I have only been checking whether the user attempting to push has an account with the given repository. Combined with the "admin must approve accounts" setting on repositories, this seems good enough to be useful.

Do we want more?

I seem to recall there being restrictions on what branch and tag names were allowed in the CVS repository, since they were globally visible, but with one repository per project (for drupal.org), this would seem to be a non-issue.

Do we still want some sort of restriction on who can commit where, aside from membership with the repository? I can envision something like having certain users able to push to certain branches (this would apply more to DVCSs). This would allow for a style where there is a "master" branch, to which only the project admin has push access, but anyone can create other branches. That is what I'm planning on working on later (the item "Multiple project branches" on my project wiki), but is there anything else we would want?

Comments

Tough one

jpetso's picture

Yeah, that's a difficult issue, complicated by the fact that DVCS users commit code before the server has any chance to get to see them. So I'm not even sure if we really want to restrict commits, as that would potentially force the developer to rewrite commit history even if current HEAD is ok.

On the other hand, a couple of important checks are done as part of (commit/branch/tag) operation checks. Checking label (branch/tag) operations for correct label names is indispensable for drupal.org as they determine version numbers of project releases. I could imagine that a project's "official repository" on d.o (the maintainer one) allows additional branches like DRUPAL-6--1 but other people's repositories can only upload master. (In addition, we probably want the website to allow whether to have a single "official" repository with branches, or whether to have multiple "official" repositories for a single branch each. But that's mostly stuff for later, probably.)

The other thing that operation checks has to check is whether a person has commit access not only to a given repository but also to a directory inside that repository. Using Git for one project each would lower the need for such detailed access control, but again d.o makes an exception here: translators can commit freely into the translations/ (D6) and po/ (D5) directories of a project, without being listed in the maintainer list of a project.

So all in all, I think we won't get around checking commits like for CVS, and expect the admin to configure the checks in a sensible way so that rewriting history is not necessary except when something's really wrong (such as a translator committing into a module's main directory).

And yeah, I'm the first person who'll claim that d.o's commit restrictions are ridiculously strict and unprecedented especially in distributed version control, it's so paranoid it's a joke. But the masses of unattentive amateur committers on d.o make the enforcement of branch and tag names necessary as the CVS repository would otherwise turn out to be a big mess, and if we want to see Git on d.o we probably won't get around allowing a similar set of restriction capabilities. Just ask dww if you want to know what I mean.

Oh, and speaking of which... please cross-post this discussion in the "Issue tracking and software releases" group so that dww (and other project* people that might be missing from the Revision Control Management group) can chime in too.

Checking label (branch/tag)

jpetso's picture

Checking label (branch/tag) operations for correct label names is indispensable for drupal.org as they determine version numbers of project releases. I could imagine that a project's "official repository" on d.o (the maintainer one) allows additional branches like DRUPAL-6--1 but other people's repositories can only upload master.

Heck, I think I haven't thought enough outside the box here. The main reason why drupal.org CVS forbids branches and tags other than those for official releases (branches: HEAD and DRUPAL-{5,6,7}--$n, tags: DRUPAL-{5,6,7}--$n-[UNSTABLE,ALPHA,BETA,RC]$m) is that tags are global for the whole CVS repository, and especially the branches can't be deleted after they have been created. So the current label restrictions prevent the CVS repository from drowning in branch chaos.

With DVCS though, that should not be a problem at all. On the one hand, branches can be renamed and deleted anyways, and on the other hand it's all local to the respective repository. So I see little reason to deny people the creation of non-release branches. Maybe even in the projects' official repositories. So, that should be less of an issue than I originally thought.

So I'm not even sure if we

haxney's picture

So I'm not even sure if we really want to restrict commits, as that would potentially force the developer to rewrite commit history even if current HEAD is ok.

Well, to be completely thorough, we would probably need to check each commit to make sure that the committer didn't (for example) change a directory they weren't supposed to and then change it back before the end of their commit. This isn't such a huge deal, as the changes in the "restricted directories" would have to result in no overall change for the whole push (otherwise a change would show up at the end of the push and that would trigger the permission restrictions). Even if this were the case, a malicious (or inexperienced) user could still muddy the commit history for files for which they weren't supposed to have access.

Checking label (branch/tag) operations for correct label names is indispensable for drupal.org as they determine version numbers of project releases.

This makes sense, and is fairly easy to do, as the update hook of Git runs once for each branch or tag being updated.

I could imagine that a project's "official repository" on d.o (the maintainer one) allows additional branches like DRUPAL-6--1 but other people's repositories can only upload master.

I have comments about this, but they should really be discussed elsewhere, as they are about multiple branches/repositories rather than permissions for branch/tag names.

The other thing that operation checks has to check is whether a person has commit access not only to a given repository but also to a directory inside that repository.

That would be done as an implementation of hook_versioncontrol_write_access(), correct? In this case, it really wouldn't be up to the VCS hook, since it is merely responsible for gathering the data from the hook event and passing it to the VCS api, right?

That means that the only access checking that the update or pre-commit hook would need to do is a call to versioncontrol_has_write_access(), since all other access control will be done as implementations of the write_access hook, right?

Oh, and speaking of which... please cross-post this discussion in the "Issue tracking and software releases" group so that dww (and other project* people that might be missing from the Revision Control Management group) can chime in too.

Done.

Access checks

jpetso's picture

(jpetso) The other thing that operation checks has to check is whether a person has commit access not only to a given repository but also to a directory inside that repository.

(dhax) That would be done as an implementation of hook_versioncontrol_write_access(), correct? In this case, it really wouldn't be up to the VCS hook, since it is merely responsible for gathering the data from the hook event and passing it to the VCS api, right?

Right. In fact, the Commit Restrictions module that ships with Version Control API already implements such a check, but we would need to make sure to propagate those directory settings to cloned repositories and make sure that the appropriate people (translators) have access to those repositories too. Or something.

That means that the only access checking that the update or pre-commit hook would need to do is a call to versioncontrol_has_write_access(), since all other access control will be done as implementations of the write_access hook, right?

Right, that's the plan.

Issue tracking and software releases

Group organizers

Group notifications

This group offers an RSS feed. Or subscribe to these personalized, sitewide feeds: