[RFC] Revisiting linear history vs merge commits

[mehdi_amini]

@beanz

For complex changes we could allow PRs to contain multiple commits. GitHub’s UI allows for commits that are part of a PR to be viewed individually. This puts some work on the author to be able to structure their history into a cohesive “story” that a reviewer can follow, but it does largely match current workflows with one big change:

This would require to amend/rebase/force-push through the iteration of the reviews right? In this case how does one follow the review threads across these force-pushes when the review spans different reviewers / area of the project / revisions? GitHub UI does not have a good reputation at the moment for this (admittedly it may be improving).

@beanz

One advantage of a merge is that the child commits don’t need to be fully functional

Why is it an advantage? My impression is that it is a deficiency but I may be missing some arguments.

@beanz

For example: a change that alters an LLVM API could be introduced in a PR where one commit is the change just in LLVM with any expected test, documentation, etc. Subsequent commits on the PR could update the usage of the API across the repo covering the sub-projects.

In these rare cases, I’ve been sending the two phab revision, but squashing them before push. This isn’t the common case I’ve seen for stacked revision though where each change stands in itself.

@beanz

Since our transition to GitHub, git has added the --first-parent flag to both git bisect and git log, and many git GUI tools have been updated to support first-parent filtering.

--first-parent is great, but the current status makes it so that commits in stack are approved and landed individually: they also get tested, bisected, and reverted individually. We’d lose all this.
Also the log for --first-parent because less readable by pointing to the merge commit instead of the commit that touches the file.
For example in TensorFlow right now:

$ git log -n 3 tensorflow/core/kernels/resource_variable_ops.cc 
commit 0409fd1cffa6342561c497756d276499f71a9df7
Author: Jeff <jefferycole100@gmail.com>
Date:   Thu Jul 28 00:10:38 2022

    Fixed spacing

commit 29c264087a94834da2758210a3b2bd058c825227
Author: Jeff <jefferycole100@gmail.com>
Date:   Thu Jul 28 00:00:50 2022

    Register more types for AssignVariableOp

commit 769eddaf479c8debead9a59a72617d6ed6f0fe10
Author: Jean-Baptiste Lespiau <jblespiau@google.com>
Date:   Wed Jun 1 03:13:29 2022

    Replace `tensorflow::Status::OK()` with tensorflow::OkStatus()`.
    
    PiperOrigin-RevId: 452199902

But with first-parent:

$ git log --first-parent -n 2 tensorflow/core/kernels/resource_variable_ops.cc 
commit 9c204384df41bf354e76aecfb8ddb58f637b2b5e
Merge: fd04e10f93ba 0409fd1cffa6
Author: TensorFlower Gardener <gardener@tensorflow.org>
Date:   Mon Aug 22 19:26:28 2022

    Merge pull request #56936 from jswag180:master
    
    PiperOrigin-RevId: 469253884

commit 769eddaf479c8debead9a59a72617d6ed6f0fe10
Author: Jean-Baptiste Lespiau <jblespiau@google.com>
Date:   Wed Jun 1 03:13:29 2022

    Replace `tensorflow::Status::OK()` with tensorflow::OkStatus()`.
    
    PiperOrigin-RevId: 452199902

The first two commits belonged to what would be for us a “stack” in a pull-request and we just see the merge instead.

Some annoyance of merges (that I don’t think you mentioned?) is also that we don’t have an easy versioning: the mental model isn’t as simple as counting revisions.