14.10.2020, 02:01, "Ryosuke Niwa" <rn...@webkit.org>:
> On Tue, Oct 13, 2020 at 3:53 PM Konstantin Tokarev <annu...@yandex.ru> wrote:
>> 14.10.2020, 01:45, "Ryosuke Niwa" <rn...@webkit.org>:
>> > On Tue, Oct 13, 2020 at 3:40 PM Konstantin Tokarev <annu...@yandex.ru>
>> wrote:
>> >> 14.10.2020, 01:30, "Ryosuke Niwa" <rn...@webkit.org>:
>> >> > On Tue, Oct 13, 2020 at 2:37 PM Konstantin Tokarev <annu...@yandex.ru>
>> wrote:
>> >> >> 13.10.2020, 22:33, "Maciej Stachowiak" <m...@apple.com>:
>> >> >> >> On Oct 2, 2020, at 10:59 AM, Michael Catanzaro
>> <mcatanz...@gnome.org> wrote:
>> >> >> >>
>> >> >> >> On Fri, Oct 2, 2020 at 6:36 pm, Philippe Normand
>> <ph...@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
