[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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic