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

List:       webkit-dev
Subject:    Re: [webkit-dev] WebKit Transition to Git
From:       Konstantin Tokarev <annulen () yandex ! ru>
Date:       2020-10-13 22:40:34
Message-ID: 357951602628699 () mail ! yandex ! ru
[Download RAW message or body]



14.10.2020, 01:30, "Ryosuke Niwa" <rniwa@webkit.org>:
> On Tue, 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].
> 
> I really dislike this workflow due to its inherent complexity. Having
> to use Git is enough of a burden already. I don't want to deal with an
> extra layer of complexity to deal with.

There is simplified version of workflow 2 when you have only one commit in PR. In \
this case you can easily edit this single commit with gic commit --amend or GUI tools \
to address review comments. At the same time those who are more comfortable with git \
can use longer patch series.


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