[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 23:12:08
Message-ID: 1487111602630264 () mail ! yandex ! ru
[Download RAW message or body]



14.10.2020, 02:01, "Ryosuke Niwa" <rniwa@webkit.org>:
> On Tue, Oct 13, 2020 at 3:53 PM Konstantin Tokarev <annulen@yandex.ru> wrote:
> > 14.10.2020, 01:45, "Ryosuke Niwa" <rniwa@webkit.org>:
> > > On Tue, Oct 13, 2020 at 3:40 PM Konstantin Tokarev <annulen@yandex.ru> wrote:
> > > > 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.
> > > 
> > > Except that reviewers would still have to review each commit
> > > separately, and the time comes to revert someone's patch, we still
> > > need to remember how to revert a sequence of commits that belong to a
> > > single PR.
> > 
> > Workflow 2 assumes that you forget about PR after it was merged and operate
> > on its commits as equal parts of history.
> > 
> > In this sequence of commits each one can be reverted on their own merits,
> > like separate (but consequential) Bugzilla patches in current workflow.
> > Sometimes it's not possible to revert one patch without reverting a few others
> > or solving conflicts, but you rarely think about reverting a whole range of
> > patches unless it becomes really necessary.
> 
> Currently, when we revert a patch, we reopen the bug. If we're
> reverting individual commits and they don't all correspond to a single
> PR, then we would need a new system for tracking the partial(?)
> introduction of the original issue that PR fixed. This is extremely
> confusing because a single PR may have many to many relationships with
> Bugzilla bugs / GitHub issues. In which case, there isn't a clear
> communication of what got reverted and what needs to happen other than
> the history in Git.

Each commit could have a reference to issue it solves, which could be set up
to be reopened automatically after revert. I guess webkitbot could do that.

> 
> Again, I dislike all these complexities that come with workflow 2.
> Contributing to WebKit is already too damn complicated. Please don't
> make it even more complicated.

FWIW, having to create individual PR for every patch in a series (and wait before
previous PR is merged to avoid confusion, because of git branch containing
previous commit reviewed elsewhere) is also a complication which decrease 
developers' productivity.

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