13.10.2020, 22:33, "Maciej Stachowiak" <[email protected]>:
>> On Oct 2, 2020, at 10:59 AM, Michael Catanzaro <[email protected]> wrote:
>>
>> On Fri, Oct 2, 2020 at 6:36 pm, Philippe Normand <[email protected]> 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
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-dev
