Use gitlab_git create branch instead of gitlab_shell to prevent shelling out for better performance

This change switches the use of gitlab-shell for gitlab_git. The main reason is performance because the latter uses Rugged instead of shelled out git executions, so we save the forking time and memory overhead.

In general this change is quite small and simple and is just an initial step to do the same thing with tagging.

There is still one thing left to do that is beyond the scope for now: handle exceptions correctly to not let Rugged leak out of gitlab_git.

cc @DouweM

Fixes #3329 (closed)

Just a final comment, I could not find any previous spec to test the code, so I just did manual testing.

I did added tests in gitlab_git to evaluate that it behaves as expected.

mentioned in merge request !1742 (merged)

Added 1 commit:

• 26fab2e3 - Use custom exception for failure cases when creating a branch

mentioned in merge request !1757 (closed)

As I understand it requires https://gitlab.com/gitlab-org/gitlab_git/merge_requests/54 to be merged first and release new version of gitlab_git

Yes. https://gitlab.com/gitlab-org/gitlab_git/merge_requests/55 is pending as well.

@pcarranza I released gitlab_git 7.2.20 with the new Repository methods, so you can update this MR.

Also, please remove the now-redundant methods from Gitlab::Shell!

mentioned in merge request gitlab-shell!24

@pcarranza To fully resolve #3329 (closed), we also need to move away from Gitlab::Shell#rm_branch, Gitlab::Shell#rm_tag and Gitlab::Shell#update_repository_head (if possible). Would you be up for that?

@pcarranza As for the failing tests, do you see a Retry button?

@DouweM This MR still needs to update the gem ;)

@razer6 I've kicked off a build to test all the gem upgrades for this. Hang on. :)

@pcarranza Please rebase/merge master. With !1768 (merged) merged we're now pointing at a gitlab_git with the methods you added, and this MR should start to work.

Added 100 commits:

• 26fab2e3...264dc3f7 - 98 commits from branch gitlab-org:master
• f4dbf316 - Use gitlab_git create branch instead of gitlab_shell to prevent shelling out for better performance
• 3583164b - Use custom exception for failure cases when creating a branch

@DouweM Yes I would be up, I'll work on dropping those rm methods first and then check update_repository_head finally.

Out of curiosity, will gitlab-shell be not necessary anymore if those are provided too?

@pcarranza It would still be necessary for actual file system actions like cloning or forking a repository, but at least it would no longer be used for simple Web UI Git actions.

@pcarranza I'm seeing some failed builds. 2 seem to be flukes (a retry should solve them) but the third one seems real.

Yes, you are right, the message has changed a bit:

Error summary:
  Failures (2)
    Project Commits Branches :: I create a branch with invalid reference :: Then I should see new an error that ref is invalid
    Project Commits Branches :: I create a branch that already exists :: Then I should see new an error that branch already exists

Will check later

@pcarranza Do you think you'll have time to finish this in the near future? If not, please let me know and I will be happy to apply the finishing touches.

Hey @DouweM, I have it rough this week as I am in a business trip.

I can pick it up next week.

@pcarranza Sounds good!

I tried to rebase and found some conflicts.

@DouweM Is this branch still relevant?

@pcarranza If you check app/models/repository.rb, you'll see that some of your changes have already been applied, like add_branch which now calls rugged.branches.create directly without going through either gitlab-shell or gitlab_git. The tag logic still uses gitlab-shell though, and the now-deprecated commands haven't been pulled out of gitlab-shell yet.

I think the current consensus is that calling rugged methods directly from gitlab-ce is OK, but gitlab_git should be used for methods that wrap git CLI command and parse the output.

If you would make the still-pending changes to gitlab-ce and gitlab-shell, that would be greatly appreciated. Touching gitlab_git is most likely no longer necessary since we're just calling rugged methods.

👍 Will do

@pcarranza Could you please rebase/merge master and add a CHANGELOG item?

Status changed to closed

This was superseded by !1918 (merged), cleaned in CE by !3746 (merged), and cleaned in gitlab-shell by gitlab-shell!52 (merged).