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?

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/23439

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

Compare with previous version

I think we're missing a lot of tests here, and I am thinking where I should start adding them.

Looks like the only test we have is Files::UpdateService, which isn't testing against creating a new branch. Adding a bunch of tests would take a lot of time, I think. So I'll simply try to write a minimal test for now.

Added 1 commit:

• 92aa4028 - Add a test to make sure hooks are fire only once

Compare with previous version

@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 that add_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 to CreateBranchWithHooksService and CreateBranchService? 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

@godfat Could you work on this as @DouweM suggests?

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 raising Gitlab::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 in Repository#copy_gitattributes, which I guess has nothing to do with Repository#update_branch_with_hooks so this might not break anything.