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 w= rote: > > Hi all, > > Now that the main Krita repository has been migrated to KDE=E2=80=99s Git= Lab, there=E2=80=99s going to be a change in workflow for developers and co= ntributors. We should discuss the workflow on Monday=E2=80=99s meeting. A m= ajor 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 ex= cept that a Merge Request can include more than one commit but each Differe= ntial Revision can only contain one diff. > > Here I would like to present my opinion on the Merge Request workflow. Th= ese 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 th= eir 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 t= his). Since they have commit access, they can push a regular branch into Kr= ita=E2=80=99s 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 inst= ead. > - In my opinion both ways should be acceptable. > > > Editing Merge Requests: > > This can be a bit tricky. On Phabricator, updating a Differential Revisio= n means replacing the old diff with a new one. But Merge Requests are backe= d by a branch with commits, which mean updating a Merge Requests will requi= re updating the branch - > > 1. Making new changes that are an extension to the previous commit(s): Ju= st add new commits on top of it and push to the same branch. GitLab will up= date the Merge Request accordingly. Do this whenever this makes sense. > > 2. Fixing mistakes in the existing commit(s): For this I will strongly su= ggest to *not* add new commits on top. Ideally, every single intermediate c= ommits on a branch should contain correct compilable code. In my opinion, y= ou should just amend the past commit(s) and force push the new branch. > - Force-pushing sounds scary, but in this case it really isn=E2=80=99t. > - This is akin to replacing a diff on Phabricator. > - GitLab keeps track of the old commits just like how Phabricator keeps t= rack of the old diffs, so no history is really lost. > - The master branch has =E2=80=9Cprotected=E2=80=9D status, so you can=E2= =80=99t 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 i= nteractive-rebase when the Merge Request is ready to be merged. This way yo= u don=E2=80=99t need to force-push until the end. > > 3. Rebasing changes on top of master: I don=E2=80=99t have a strong opini= on but I suggest to only do this for small changes with no more than a hand= ful of commits. On the other hand, an interactive rebase can also be used t= o fix up mistakes in the previous commits. After it=E2=80=99s done, just fo= rce push the rebased branch, like above. > > 4. Merging master into the branch: I think doing this is acceptable if it= =E2=80=99s a long-running feature branch and is still WIP (i.e. you will st= ill be adding commits on top of it). But I strongly suggest against doing t= his if the branch is ready to be merged into master, as this will either be= come a =E2=80=9Cfoxtrot merge=E2=80=9D or a =E2=80=9CPortuguese man o=E2=80= =99 war merge=E2=80=9D (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=E2=80=99s not available to us (see GitLab Issue #42096 [2]). Let=E2=80= =99s hope this feature will land in CE in the future. But for now, we=E2=80= =99re limited to using mentions, comments and labels. This needs further di= scussions. > > [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 mas= ter 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 current= ly favours a merge workflow, where every Merge Request is merged by a merge= commit, though it seems like we might switch it to the =E2=80=9Cfast-forwa= rd merge=E2=80=9D model [3]. Even if the project has switched to the =E2=80= =9Cfast-forward merge=E2=80=9D 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 ad= d noise with no additional info. What I would like to see is the commit mes= sages being amended to include a reference to its corresponding Merge Reque= sts so it can be found easily. But if GitLab=E2=80=99s =E2=80=9Cfast-forwar= d merge=E2=80=9D model doesn=E2=80=99t do this by default, then so be it. > > For larger Merge Requests with multiple well-defined commits, I tend to p= refer the merge commit workflow, as it preserve the context of the individu= al 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 or= ganization =E2=80=93 all you see on master is a bunch of commits with no in= dication that they are directly related (though the commit messages may pro= vide a hint). > > [3]: https://invent.kde.org/help/user/project/merge_requests/fast_forward= _merge.md > > > Best Regards, > Alvin --=20 Wolthera