[webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

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.

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.

- R. Niwa