#1325 Add a git commit-hook to projxp to block merge commits.
Closed: Fixed None Opened 15 years ago by mcpierce.

This hook script will block developers from doing merge commits, returning an error instead.


Merge commit blocking hook script.
update

Forgot to add steps for doing the setup:

{{{
repo=REPO_LOCATION
cp -a update $repo/hooks/update
git --git_dir=$repo config hooks.denymerge.master true
git --git_dir=$repo config hooks.denymerge.devel true
}}}

Just making a note for whoever does this - We'll probably want this in our scripts module on puppet deployed to /usr/local/bin/ and symlinked from there since the git dir's are mounted noexec.

Will anybody have time to take care of this ticket soon?

Apologies, $REAL_LIFE got in the way. This should be done, let me know if it doesn't work right or something

Now I'm not able to commit to my project's repo. Is it possibly related to this commit hook?

Sure. The hook is exactly as you'd requested it though. What error are you getting? I'll remove the hook in the meantime so you can keep working.

The pushes would be consistently rejected. I use "git push origin devel" to push into our devel branch, and those were rejected. Now with the hook removed the push is successful.

I'll work with my coworker who provided the hook and see if he can identify reasons why it would have resulted in pushes being rejected.

Did you get a chance to figure out why the script might not have worked? We're working on cleaning up hosted and found this unreferenced script hanging out there :)

Is there a diff from the old update hook, or is this hook entirely new? I ask because it does substantially more than just check for merge commits. Several of its other sanity checks could be cause for failure.

{{{
The test used in the script is:
44 is_merge_commit()
45 {
46 git rev-parse --verify --quiet $1^2 > /dev/null
47 }
}}}

There is a basic logic flaw in that it only tests the new value of the ref. A push could include multiple commits, any of which could be a merge (not just the last). To be rigorous, you would need to walk from the new value back to the old value (assuming a fastforward push) and check each step for a merge. I'm not sure what the most efficient way to do this is.

I tried applying in a local test repo and ran into a few of the sanity checks myself:
- lack of an edited description file
- whitespace checks

You also need to set hooks.denymerge.master for the repo in order to trigger enforcement of the merge check.

Once I'd dealt with these the script worked fine for me. However note the limitation of the check. The following hypothetical sequence of events completely dodges it:
1. merge locally
2. try to push -- failure
3. make another non-merge commit
4. try to push -- success

From what I can tell, the update hook attached here is mostly the same as what we've got installed on hosted in /usr/share/git-core/custom-hooks/update. That hook came from Jim Meyering's work at http://lists.gnu.org/archive/html/bug-gnulib/2008-10/msg00221.html. I'll attach a diff from the stock git-1.5.5.6 update hook to the one submitted in this bug.

I haven't tested thoroughly various ways this hook can fail. I'm sure there are a few ways to get around it. But folks sometimes request such hooks so we've got one available. Well tested suggestions for improvement are, of course, welcome.

Personally, I suggest social pressure as a better method to handle pathological case where folks with commit access don't understand that pushing local merges isn't kosher by a project's standards. If someone tries hard to push crap and they have access, they'll get it in sooner or later and it's their commit privileges which should be removed. ;)

Diff from git-1.5.5.6 update hook and the hook script attached previously
update.diff

The hook attached was the same one from Jim Meyering. But, at this point, I don't think i want to go forward with the hook.

I just committed a fix to the custom update hook we have on hosted that should properly implement the test for merge commits. It loops over each commit in the range of old to new and tests them via is_merge_commit. This closely matches what Jim has done in an updated hook for the ovirt-server repository¹.

If you'd like to enable this hook, let us know. We can hook up in #fedora-admin sometime and enable it when you're ready to test, to minimize any problems if it doesn't work as you want.

Sorry for not finding time to fix this much sooner.

For completeness, the new hook contains this code:

{{{
case $deny_merge,$oldrev in
true,$zero)
# XXX Is there a way to get the list of
# revisions being added if this is an initial
# push to an empty repo?
echo " Unable to detect/deny merges in initial push." >&2
;;
true,
)
for rev in $(git rev-list --reverse $oldrev..$newrev); do
is_merge_commit $rev && {
printf "error:
* %s\n" \
"You may not push merge commits to branch $branch." \
"Did you forget to rebase? ($rev)" >&2
exit 1
}
done
;;
esac
}}}

¹ http://git.et.redhat.com/?p=ovirt-server.git;a=blob_plain;f=git-hook/update;hb=refs/heads/vcs-admin

Closing per 4/14 comment.

Login to comment on this ticket.

Metadata