[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-kimageshop
Subject:    Re: Merge Request Workflow on GitLab
From:       Wolthera <griffinvalley () gmail ! com>
Date:       2019-04-20 16:01:24
Message-ID: CAN80MtEnYZjJMw=MJuLirC_uQwVq7T8Gyc0hwZ=hPMNZzg08Yw () mail ! gmail ! com
[Download RAW message or body]

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@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


[prev in list] [next in list] [prev in thread] [next in thread]