[webkit-dev] WebKit Transition to Git

[Konstantin Tokarev]

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]. 


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