On Tue, Oct 13, 2020 at 3:53 PM Konstantin Tokarev <annu...@yandex.ru> wrote: > > > 14.10.2020, 01:45, "Ryosuke Niwa" <rn...@webkit.org>: > > On Tue, Oct 13, 2020 at 3:40 PM Konstantin Tokarev <annu...@yandex.ru> > > wrote: > >> 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. > > > > Except that reviewers would still have to review each commit > > separately, and the time comes to revert someone's patch, we still > > need to remember how to revert a sequence of commits that belong to a > > single PR. > > Workflow 2 assumes that you forget about PR after it was merged and operate > on its commits as equal parts of history. > > In this sequence of commits each one can be reverted on their own merits, > like separate (but consequential) Bugzilla patches in current workflow. > Sometimes it's not possible to revert one patch without reverting a few others > or solving conflicts, but you rarely think about reverting a whole range of > patches unless it becomes really necessary.
Currently, when we revert a patch, we reopen the bug. If we're reverting individual commits and they don't all correspond to a single PR, then we would need a new system for tracking the partial(?) introduction of the original issue that PR fixed. This is extremely confusing because a single PR may have many to many relationships with Bugzilla bugs / GitHub issues. In which case, there isn't a clear communication of what got reverted and what needs to happen other than the history in Git. Again, I dislike all these complexities that come with workflow 2. Contributing to WebKit is already too damn complicated. Please don't make it even more complicated. - R. Niwa _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev