Fix for triggering git hooks configured through Project settings.
- We should skip hooks only for new branch.
- The spec was using a wrong rev as the blank rev.
REF: #587 (closed)
cc/ @jameslopez
Merge request reports
Activity
@jameslopez I've seen that you were working on this refactoring a couple of months ago, can you please give a look?
thanks for pinging me @rdavila, I'll have a look
@rdavila so, unless I have missed something in your code, I think you are just reverting back to the first fix I've put for git-hooks checking old commits on new branches - https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/281.
Basically, when you push a new branch from another branch that had old commits (that already triggered the git hooks), the git-hooks get triggered once again for those commits - when they shouldn't. With that fix, that wouldn't occur, but another problem arose - https://gitlab.com/gitlab-org/gitlab-ee/issues/428 which is basically the same situation but while merging.
So in that situation, we want to check that it's an old commit but not necessarily from a new branch. Hence why I had to remove the
blank_oldrevcheck...@rdavila Not sure if this will fix it, but now I recalled that while implementing that fix, I thought of a better way of solving the problem and left it as a technical debt issue on: https://gitlab.com/gitlab-org/gitlab-ee/issues/429
401 401 project.git_hook.update(commit_message_regex: "Change some files") 402 402 403 403 # push to new branch, so use a blank old rev and new ref @rdavila actually what is wrong here is the comment - we should remove it. In the previous test, we use a new branch, so the comment made sense, but in this case I think we are testing old commits, like the case when there's a merge... So the comment got copied & pasted here but shouldn't be on this particular test...
Edited by James Lopez
@rdavila if this is quite tricky I'll be happy to help or take over, so feel free to bug me on slack
-
@jameslopez thanks for reviewing, I'll leave it for you since you have worked on it and have a better understanding of the problem. Just a small tip: try to do some manual testing because it was failling for single commits too.
mentioned in commit 7d286ac6
Mentioned in commit 7d286ac6
