Don't execute git hooks if you create branch as part of other change
What does this MR do?
Currently, our procedure for adding a commit requires us to execute CreateBranchService
before file creation.
It's OK, but also we do execute git hooks
(the PostReceive
sidekiq job) as part of this process.
However, this hook is execute before the file is actually committed, so the ref is updated.
Secondly, we do execute a git hooks
after committing file and updating ref.
This results in duplicate PostReceive
jobs, where the first one is completely invalid.
This change makes the branch creation, something that is intermediate step of bigger process (file creation or update, commit cherry pick or revert) to not execute git hooks.
Why was this MR needed?
Adds a to CreateBranchService
an option to disable hooks execution.
Does this MR meet the acceptance criteria?
-
CHANGELOG entry added -
Documentation created/updated -
API support added - Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Merge request reports
Activity
cc @DouweM
Mentioned in issue #23439 (closed)
Mentioned in issue #23930 (closed)
Added 207 commits:
-
a431ca0f...e4c05de7 - 206 commits from branch
master
- 4b20cb5d - Merge remote-tracking branch 'upstream/master' into fix-git-hooks-when-creating-file
-
a431ca0f...e4c05de7 - 206 commits from branch
Added 1 commit:
- 92aa4028 - Add a test to make sure hooks are fire only once
@grzesiek Please help review, thanks! I don't like the idea of using a flag like that, but well it works and I have no significantly better idea right now. So I guess I'll just ask for opinions :P
Reassigned to @grzesiek
@godfat I also believe that we should avoid using flag parameter, especially when executing service. One option to avoid flag parameter with services would be using fluent interface to setup service before calling
execute
, but this is something we do not practice right now.Consider having:
CreateBranchService.new(project, current_user) .target_branch(@target_branch) .source_branch(@source_branch) .source_project(@source_project) .without_hooks .execute!
Instead of
CreateBranchService.new(project, current_user).execute(@target_branch, @source_branch, source_project: @source_project, with_hooks: false)
This is something I wish we had, but because we do not, it is just a random thought of mine.
More significant problem with this MR is probably passing flag parameter to
Repository#add_branch
. We can try to find workarounds for that, but I'm not sure if we would find something much better. The problems stems from thatadd_branch
does have more than a single responsibility. It adds a branch, but also fires hooks. Implementation for later problem may belong better to different object or different layer. At this point I'm not sure if we can do any better without refining current model. Passing to @DouweM.Reassigned to @DouweM
@grzesiek Not sure if I like fluent interface better, but at least it would better protect us from typo in hashes. Since you talked about single responsibility, what about splitting
CreateBranchService
toCreateBranchWithHooksService
andCreateBranchService
? I am a bit worried that we could end up with combination of classes for options, but I guess it's fine in this case.Thanks for reviewing!
"Executing git hooks" is really just simulating a push, so that access is verified, push rules are run, the branch creation shows up in the activity feed, we optionally post "restored branch" notes to MRs etc. It is not safe to skip it, since behavior in GitLab or the project's workflow (through push rules) depends on every repo action having these hooks run.
add_branch
technically does have more than one responsibility, but I don't think we should ever add a branch without running to accompanying hooks.I agree that https://gitlab.com/gitlab-org/gitlab-ce/issues/23439 is a problem, but I think the fix is not to create the branch and just not tell GitLab about it. The proper fix seems to create the branch with the new commit in one go, so that it is a single repository action, which would mean moving away from using
CreateBranchService
here.Reassigned to @ayufan
Reassigned to @godfat
@ayufan I'll play around and see if I could come up with a good approach.
@godfat Were you able to come up with another approach?
@stanhu I am about to try it in the next few hours. Would let you know how it goes tomorrow :P
Some quick investigation makes me feel that we should actually make
Repository#update_branch_with_hooks
create a new branch if target branch doesn't exist, instead of raisingGitlab::Git::Repository::InvalidRef
as for now because otherwise we might need to touch a lot of places to make it create a new branch in one action.The only place we're rescuing
Gitlab::Git::Repository::InvalidRef
is inRepository#copy_gitattributes
, which I guess has nothing to do withRepository#update_branch_with_hooks
so this might not break anything.I realized that this is slightly beyond my knowledge about how we currently manipulating Git, and it's also more complex than I originally thought. So this might take a bit more effort than I thought (love the challenge, but I probably don't have enough time to do everything for what assigned to me for %8.14
). I'll push a few changes first but I would expect a few rounds of changes in the coming days.Added 417 commits:
-
92aa4028...889dcf60 - 415 commits from branch
master
- f5bc41b3 - Merge remote-tracking branch 'upstream/master' into fix-git-hooks-when-creating-file
- 3128641f - Revert "Don't execute git hooks if you create branch as part of other change"
-
92aa4028...889dcf60 - 415 commits from branch
Added 1 commit:
-
0b5a2eef - Add
source_branch
option for various git operations
-
0b5a2eef - Add
Added 2 commits:
Added 1 commit:
- eddee5fe - Make sure we create target branch for cherry/revert
Added 1 commit:
- d8fe2fac - Make sure we have the branch on the other project
Added 1 commit:
- a68a6201 - Don't pass source_branch if it doesn't exist
Added 1 commit:
- 39d83f72 - Add a few comments to explain implementation detail
Reassigned to @DouweM
- Resolved by Lin Jen-Shin
- Resolved by Douwe Maan
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin
- Resolved by Lin Jen-Shin