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.
- R. Niwa
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
