Merge Request Workflow on GitLab

[Wolthera]

I've also started a stub in the contributor's manual:
https://docs.krita.org/en/untranslatable_pages/patch_review_guide.html

Once we've tackled this during the meeting it'd be great if someone
could go in and fold in everything we've discussed.

On Sat, Apr 20, 2019 at 4:54 PM Alvin Wong <alvin+kimageshop at alvinhc.com> wrote:
>
> Hi all,
>
> Now that the main Krita repository has been migrated to KDE’s GitLab, there’s going to be a change in workflow for developers and contributors. We should discuss the workflow on Monday’s meeting. A major difference is with Merge Request.
>
> Merge Request is the equivalent of Differential Revision on Phabricator, but instead of uploading a diff (either manually or via the Arcanist tool), a Merge Request is created from a branch. They are conceptually similar except that a Merge Request can include more than one commit but each Differential Revision can only contain one diff.
>
> Here I would like to present my opinion on the Merge Request workflow. These opinions are up for debate, so feel free to disagree with me.
>
>
> Making Merge Requests:
>
> How to create a merge request: https://invent.kde.org/help/gitlab-basics/add-merge-request.md
>
> 1. For newcomers without commit access, they need to log into GitLab with their KDE Identity, fork the KDE/Krita project, push their changes onto their fork, then create a pull request on the KDE/Krita project.
>
> 2. For developers with commit access, they can still commit directly onto master, but they should make use of Merge Requests to request for comments on their changes when appropriate (we might need a better definition for this). Since they have commit access, they can push a regular branch into Krita’s main repo and create a Merge Request with it. They also have the option to create a personal fork and create Merge Requests with it instead.
> - In my opinion both ways should be acceptable.
>
>
> Editing Merge Requests:
>
> This can be a bit tricky. On Phabricator, updating a Differential Revision means replacing the old diff with a new one. But Merge Requests are backed by a branch with commits, which mean updating a Merge Requests will require updating the branch -
>
> 1. Making new changes that are an extension to the previous commit(s): Just add new commits on top of it and push to the same branch. GitLab will update the Merge Request accordingly. Do this whenever this makes sense.
>
> 2. Fixing mistakes in the existing commit(s): For this I will strongly suggest to *not* add new commits on top. Ideally, every single intermediate commits on a branch should contain correct compilable code. In my opinion, you should just amend the past commit(s) and force push the new branch.
> - Force-pushing sounds scary, but in this case it really isn’t.
> - This is akin to replacing a diff on Phabricator.
> - GitLab keeps track of the old commits just like how Phabricator keeps track of the old diffs, so no history is really lost.
> - The master branch has “protected” status, so you can’t accidentally force push to master. (As a side note, future release branches will also need this applied.)
>
> 2a. An alternative is to push fix-up commits on top, and only squash or interactive-rebase when the Merge Request is ready to be merged. This way you don’t need to force-push until the end.
>
> 3. Rebasing changes on top of master: I don’t have a strong opinion but I suggest to only do this for small changes with no more than a handful of commits. On the other hand, an interactive rebase can also be used to fix up mistakes in the previous commits. After it’s done, just force push the rebased branch, like above.
>
> 4. Merging master into the branch: I think doing this is acceptable if it’s a long-running feature branch and is still WIP (i.e. you will still be adding commits on top of it). But I strongly suggest against doing this if the branch is ready to be merged into master, as this will either become a “foxtrot merge” or a “Portuguese man o’ war merge” (see this blog post [1] for more).
>
> [1]: https://blog.developer.atlassian.com/stop-foxtrots-now/
>
>
> Reviewing/approving Merge Requests:
>
> Proper merge request and approvals are currently limited to GitLab EE so it’s not available to us (see GitLab Issue #42096 [2]). Let’s hope this feature will land in CE in the future. But for now, we’re limited to using mentions, comments and labels. This needs further discussions.
>
> [2]: https://gitlab.com/gitlab-org/gitlab-ce/issues/42096
>
>
> Merging Merge Requests:
>
> In the past, Phabricator diffs are usually manually applied on top of master and committed. In rare cases where a diff was created from the squashed diff of a feature branch, the branch is merged into master. GitLab currently favours a merge workflow, where every Merge Request is merged by a merge commit, though it seems like we might switch it to the “fast-forward merge” model [3]. Even if the project has switched to the “fast-forward merge” model, we should still have the option to do an explicit merge commit using the git command line.
>
> I am fine with rebasing single-commit Merge Requests and push directly to master without a merge commit, as in this case the merge commits simply add noise with no additional info. What I would like to see is the commit messages being amended to include a reference to its corresponding Merge Requests so it can be found easily. But if GitLab’s “fast-forward merge” model doesn’t do this by default, then so be it.
>
> For larger Merge Requests with multiple well-defined commits, I tend to prefer the merge commit workflow, as it preserve the context of the individual changes in history (especially useful with git blame), unlike a squashed commit. An explicit merge also naturally organizes the related commits in a group. In contrast, rebasing the commits does not provide this type of organization – all you see on master is a bunch of commits with no indication that they are directly related (though the commit messages may provide a hint).
>
> [3]: https://invent.kde.org/help/user/project/merge_requests/fast_forward_merge.md
>
>
> Best Regards,
> Alvin



-- 
Wolthera