[prev in list] [next in list] [prev in thread] [next in thread] 

List:       webkit-dev
Subject:    Re: [webkit-dev] WebKit Transition to Git
From:       Konstantin Tokarev <annulen () yandex ! ru>
Date:       2020-10-13 21:37:22
Message-ID: 1520211602619298 () mail ! yandex ! ru
[Download RAW message or body]



13.10.2020, 22:33, "Maciej Stachowiak" <mjs@apple.com>:
> > On Oct 2, 2020, at 10:59 AM, Michael Catanzaro <mcatanzaro@gnome.org> wrote:
> > 
> > On Fri, Oct 2, 2020 at 6:36 pm, Philippe Normand <philn@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]. 


Workflow 1 is more like what we have now with Bugzilla.

Workflow 2 is used by many projects which use git for a long time and value \
high-quality commit history. It's used e.g. by Linux kernel, or projects which use \
Gerrit as a review tool (Chromium, Android, Qt, etc). It advantages for developers \
(who can submit more work to review at the same time without risk of mixing up \
unrelated changes together), reviewers (it's easier to review atomic changes than \
those with too high or too low granularity), maintainers of stable branches (they can \
pick bug reports and avoid at least some unneeded refactorings, features and style \
improvements).

Workflow 1 is the most obvious way to use GitHub, because it doesn't make any hints \
about existence of workflow 2. However, many projects which use GitHub for code \
review and value high-quality history are using workflow 2. For example, see \
Ryosuke's example with whatwg/html repo.

Workflow 2 is facilitated by Gerrit, which doesn't require using force pushes and \
makes it immediately obvious for new user if they start adding new fixup commits \
instead of editing those being reviewed.

Workflow 2 can use both rebase and merge strategies, and merge isn't so evil in this \
case. However, using merge moves responsibility for solving conflicts from \
contributor to repository maintainers, which doesn't scale well. Projects which \
accept patches via mailing lists, like Linux kernel, imply that reviewed patch series \
must apply to the tip of the tree, so while there is no explicit "rebase" it is \
implied.

In conclusion, I recommend using 2 with "rebase" policy, but YMMV.

[1] https://wiki.qt.io/Gerrit_Introduction#Updating_a_Contribution_With_New_Code



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


[prev in list] [next in list] [prev in thread] [next in thread]