[webkit-dev] WebKit Transition to Git

Tue Oct 13 18:12:19 PDT 2020

On Tue, Oct 13, 2020 at 2:37 PM Konstantin Tokarev <annulen at yandex.ru>
wrote:

>
>
> 13.10.2020, 22:33, "Maciej Stachowiak" <mjs at apple.com>:
> >>  On Oct 2, 2020, at 10:59 AM, Michael Catanzaro <mcatanzaro at gnome.org>
> wrote:
> >>
> >>  On Fri, Oct 2, 2020 at 6:36 pm, Philippe Normand <philn at 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).
>

Slight correction: Chromium's Gerrit instance actually uses Workflow 1 -
the "squash merge" policy. The developer can have multiple Git commits on a
single branch and associated with a single Gerrit code review. Each time
the branch is uploaded into the review tool, all of those commits' diffs
are squashed together into the current patchset - the new version of the
review.

Some sub-projects' Gerrit instances, such as ANGLE
<https://chromium.googlesource.com/angle/angle>'s, use Workflow 2. The
developer is expected to use "git commit --amend" each time to modify their
work. Each Git commit corresponds to exactly one code review on Gerrit.

Having gotten used to Workflow 1, and only rarely maintaining multiple
dependent CLs in flight, I find Workflow 1 generally easier to use.

-Ken

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.
>
> [1]
> https://wiki.qt.io/Gerrit_Introduction#Updating_a_Contribution_With_New_Code
>
>
>
> --
> Regards,
> Konstantin
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>

-- 
I support flexible work schedules, and I’m sending this email now because
it is within the hours I’m working today.  Please do not feel obliged to
reply straight away - I understand that you will reply during the hours you
work, which may not match mine. (credit: jparent@)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20201013/cb72283e/attachment.htm>