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.


-- 
Regards,
Konstantin
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Reply via email to