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

List:       webkit-dev
Subject:    Re: [webkit-dev] WebKit Transition to Git
From:       Maciej Stachowiak <mjs () apple ! com>
Date:       2020-10-29 2:57:27
Message-ID: D2F3D728-4EDA-473E-8C22-7A668E89D0B9 () apple ! com
[Download RAW message or body]



> On Oct 13, 2020, at 2:37 PM, Konstantin Tokarev <annulen@yandex.ru> \
> wrote: 
> 
> 
> 13.10.2020, 22:33, "Maciej Stachowiak" <mjs@apple.com>:
> > > On Oct 2, 2020, at 10:59 AM, Michael Catanzaro <mcatanzaro@gnome.org> \
> > > wrote: 
> > > On Fri, Oct 2, 2020 at 6:36 pm, Philippe Normand <philn@igalia.com> \
> > > wrote:
> > > > Would you also consider preventing merge commits in order to keep a
> > > > clean mainline branch?
> > > 
> > > Big +1 to blocking merge commits. Merge commits in a huge project \
> > > like WebKit would make commit archaeology very frustrating. (I assume \
> > > this is implied by the monotonic commit identifiers proposal, but it \
> > > doesn't exactly say that.)
> > 
> > I'm assuming your objection is to regular merges, but how do you feel \
> > about squash merges? Or do you think all PRs should be landed by \
> > rebasing?
> 
> I'm not Michael but will add my 2 dollars anyway :)
> 
> In these two approaches commits inside PR have different meaning, and \
> workflow is different. 
> Below I use a term "atomic change" to describe minimal code change which \
>                 is a self-contained work unit with following properties:
> * It implements well-defined task which can be summarized as a short \
>                 English sentence (typical soft limit is 60 characters)
> * It doesn't introduce defects (e.g. bugs, compilation breakages, style \
>                 errors, typos) which were discovered during review \
>                 process
> * It doesn't include any code changes unrelated to main topic. This \
> separation is sometimes subjective, but it's usually recommended to split \
> refactoring and implementation of feature based on that, bug fix and new \
> feature, big style change and fix or feature. 
> AFAIU our current review process has similar requirements to patches \
> submitted to Bugzilla, though sometimes patches include unrelated \
> changes. This can be justified by weakness of webkit-patch/Bugzilla \
> tooling which has no support for patch series, and by fact that SVN \
> doesn't support keeping local patch series at all. 
> 1. Workflow 1 - "Squash merge" policy
> 
> * Whole PR is considered to be a single atomic change of WebKit source \
> tree. If work is supposed to be landed as a series of changes which \
> depend on each other (e.g. refactoring and feature based on it, or \
> individual separate features touching same parts of code), each change \
> needs a separate PR, and, as a consequence, only one of them can be \
>                 efficiently reviewed at the moment of time
> * Commits in PR represent review iterations or intermediate \
>                 implementation progress
> * Reviewers' comments are addressed by pushing new commits without \
> rewriting history, which works around GitHub's lack of "commit \
> revisions". Also this workflow has lower entry barrier for people who \
> haven't mastered git yet, as it requires only "git commit" and "git push" \
> without rebases. 
> 2. Workflow 2 - "Rebase" ("cherry-pick")) or "Merge" policy
> 
> * PR is considered to be a series of atomic changes. If work consists of \
>                 several atomic changes, each commit represent an atomic \
>                 change
> * Review iterations are done by fixing commits in place and reuploading \
> entire series using force push (of course if review discovers that \
> substantial part of work is missing it can be added as a new atomic \
>                 commit to the series)
> * It's possible to review each commit in the series separately
> * Workflow requires developers to have more discipline and experience \
> with using git rebase for history rewriting. Entry barrier can be lowered \
> by providing step by step instructions like e.g. [1].  
> 
> Workflow 1 is more like what we have now with Bugzilla.
> 
> Workflow 2 is used by many projects which use git for a long time and \
> value high-quality commit history. It's used e.g. by Linux kernel, or \
> projects which use Gerrit as a review tool (Chromium, Android, Qt, etc). \
> It advantages for developers (who can submit more work to review at the \
> same time without risk of mixing up unrelated changes together), \
> reviewers (it's easier to review atomic changes than those with too high \
> or too low granularity), maintainers of stable branches (they can pick \
> bug reports and avoid at least some unneeded refactorings, features and \
> style improvements). 
> Workflow 1 is the most obvious way to use GitHub, because it doesn't make \
> any hints about existence of workflow 2. However, many projects which use \
> GitHub for code review and value high-quality history are using workflow \
> 2. For example, see Ryosuke's example with whatwg/html repo. 
> Workflow 2 is facilitated by Gerrit, which doesn't require using force \
> pushes and makes it immediately obvious for new user if they start adding \
> new fixup commits instead of editing those being reviewed. 
> Workflow 2 can use both rebase and merge strategies, and merge isn't so \
> evil in this case. However, using merge moves responsibility for solving \
> conflicts from contributor to repository maintainers, which doesn't scale \
> well. Projects which accept patches via mailing lists, like Linux kernel, \
> imply that reviewed patch series must apply to the tip of the tree, so \
> while there is no explicit "rebase" it is implied. 
> In conclusion, I recommend using 2 with "rebase" policy, but YMMV.

One big downside of Workflow 2, besides its lack of beginner-friendliness \
when revising patches, is that it requires the reviewer to review each \
commit, and not just the total diff of the PR, to ensure that it is an \
"atomic change" by your definition. And our CI would have to run against \
each commit in the PR, not just the PR overall, again, to ensure that it \
meets the definition of "atomic change". This seems like a lot of extra \
cost to enable something that IMO should not be needed much of the time. \
Furthermore, if anyone has developed a PR using branch commits that \
introduce defects, their PR would immediately fail CI and review, even if \
the net diff of the PR is good; they'd either have to do extra work to fix \
up every individual commit, or squash down their changes before posting the \
PR. I suspect this is a more common way to incrementally work on a change \
than every commit being individually valid and committable to main.

For these reasons, I'd prefer Workflow 1. If we did Workflow 2, I'd prefer \
the Rebase variant to the Merge variant in order to keep commit history \
simple.

 - Maciej

> 
> [1] https://wiki.qt.io/Gerrit_Introduction#Updating_a_Contribution_With_New_Code
>  
> 
> 
> -- 
> Regards,
> Konstantin

_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


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