[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]