[webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Tue, Oct 13, 2020 at 4:12 PM Konstantin Tokarev <annulen at yandex.ru> wrote:
>
>
>
> 14.10.2020, 02:01, "Ryosuke Niwa" <rniwa at webkit.org>:
> > On Tue, Oct 13, 2020 at 3:53 PM Konstantin Tokarev <annulen at yandex.ru> wrote:
> >>  14.10.2020, 01:45, "Ryosuke Niwa" <rniwa at webkit.org>:
> >>  > On Tue, Oct 13, 2020 at 3:40 PM Konstantin Tokarev <annulen at yandex.ru> wrote:
> >>  >> 14.10.2020, 01:30, "Ryosuke Niwa" <rniwa at webkit.org>:
> >>  >> > 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].
> >>  >> >
> >>  >> > 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.

So now we're talking about each PR containing multiple commits each of
which fixes some distinct issue?

I don't think we should have a single PR which fixes multiple distinct
issues like 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.

I've contributed to WebKit for 11 years and this has never been an
issue because I've never found a need to upload a patch up review
before the previous patch had been landed.

This is only an issue if you're working on a large feature and trying
to upload a series of patches for a review. I do get that something
like that might be useful when reviewing a large WIP feature but then
you can just point to a branch in your fork for the entire patch /
diff. There isn't really a need to have a PR for it.

- R. Niwa