Git hooks check old commits when merging

From https://gitlab.com/gitlab-org/gitlab-ee/issues/279#note_3746260

git config --local user.email "doe@example.com"
echo ${RANDOM} > README && git add README && git commit -m"Testing 1" README && git push
git checkout -b test-branch && git push -u origin test-branch
git config --local user.email "john.doe@example.com"
echo ${RANDOM} > README.tb && git add README.tb && git commit -m"Testing tb 1" README.tb && git push
git checkout master
git config --local user.email "jane.doe@example.com"
echo ${RANDOM} > README.m && git add README.m && git commit -m"Testing m 1" README.m && git push
git checkout test-branch

Now enable the member_check:

git config --local user.email "<Your valid member e-mail>"
git merge -m"Test merge" master
git push

mentioned in issue #279 (closed)

/cc @boudekerk

The above will fail on "jane.doe@example.com", which was the author on master. When doing the merge the other way, it will fail on "john.doe@example.com" which was the author on test-branch. In other words, it seems to check old commits from the other side of the merge only.

This and #279 (closed) are the two cases we found, it would be appreciated if you, while working on this, could have a look if there could be any others. As said before, the git command below should always only return the commits that need to be checked, regardless of the operation IIUC.

Thank you for looking into this.

@jameslopez Just adding some extra information from #279 (closed). Possibly unnecessary, please ignore if so:

git rev-list <newrev> --not --all is a standard git way of getting only new commits in a pre-receive hook, this should always return only the commits to be checked. See this StackOverflow post: http://stackoverflow.com/a/22547375/114983
That can easily be confirmed with this pre-receive hook:

#!/bin/bash
INPUT="$(cat -)"
echo "======================================================================="
echo \"${INPUT}\"
echo "======================================================================="
git rev-list $(echo "${INPUT}" | awk '{print $2}') --not --all
echo "======================================================================="

This was how a very similar issue was solved in a different hook:

• https://github.com/gnustavo/Git-Hooks/issues/24
• https://github.com/gnustavo/Git-Hooks/issues/26

Thanks @boudekerk for giving me more context. I will have a look as soon as I can.

mentioned in merge request !310 (merged)

@boudekerk this was simpler than I thought, as I just removed the check to see if is a new branch and it seemed to work. This should also fix all other cases, in theory. MR still has to be reviewed, etc.. https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/310

This is not the best fix and I've created a new issue in order to fix this properly: https://gitlab.com/gitlab-org/gitlab-ee/issues/429

The examples provided for gnustavo/Git-Hooks/ are literally just calling git rev-list --not --all. But we are doing a similar thing here, just not as nice performance-wise.

@jameslopez Thanks, I'll see if I can test this.

@jameslopez Both merging an branching now seems to work as expected with the hook enabled. Thank you.

great to hear! thanks for testing this

Status changed to closed by commit 4bcbd8e5a6fb6415792df7120ecf7937bc610871

mentioned in commit 92f2e4a2

mentioned in issue #587 (closed)

mentioned in merge request !412 (closed)

Mentioned in commit pfjason/gitlab-ce@4bcbd8e5

mentioned in issue #1736 (closed)

mentioned in commit 5994b3b6