Use fixup commits during code review

[alevkoy]

Introduction

During code review, when PR authors make changes, they should upload fix-up commits containing those changes instead of changing the existing commits and force-pushing. This will streamline reviewer tasks. Once the review is nominally complete, the reviewer should perform a squash rebase and force-push as usual for merging.

Problem description

Code reviews may go through several rounds of review, in which the author uploads new code to address previous comments, typically by amending their previous commits. Due to limitations of GitHub, it may not be immediately obvious to reviewers which parts of the uploaded code are new to them, so they need to review any files that have changed, usually looking at the entire delta of the PR again. This is particularly burdensome on large PRs with several active reviewers.

Proposed change

Authors should avoid pushing amended commits during code review. Instead, they should push fix-up commits addressing reviewer comments. When reviewers are satisfied, authors should squash in the fix-up commits and force-push prior to final approval and merging.

Detailed RFC

Background

Git's interactive rebase supports squash and fixup operations on commits. squash combines two or more commits into one and combines their commit messages into one. fixup combines the commits into one but only retains the first commit message. "Fix-up commits" or "squash commits" are commits that a user intends to fixup or squash later, typically indicating the base commit onto which the new commit will be fixed up or squashed, e.g.

deadbeef fixup: Declare API # Commit message: Adds comments
2468abab Define API
1234abcd Declare API

Proposed change (Detailed)

• During code review, authors will avoid pushing amended commits and instead push fix-up commits containing their new changes.
• Reviewers will review new commits until they are satisfied that the new commits address the concerns they raised about the previous commits.
• When reviewers have satisfied their concerns, authors will squash the previously created fix-up commits and force-push, retaining the original base commit with git rebase --keep-base.
• Reviewers will verify that the author did not make any significant changes during this operation, which should be straightforward in GitHub's compare UI. Reviewers will also verify that the overall commit structure is still to their liking. If those things aren't true, review will continue.
• Reviewers will approve and merge the PR.

Dependencies

I think the blast radius of this is pretty minimal. We could try it on a few PRs and see how it goes before making any policy changes.

Concerns and Unresolved Questions

• Git has facilities to automate fixup and squash operations like so:

  git commit # Original commit 1234, "Add a feature"
  # Fix formatting
  git commit --fixup=1234 # Create a fix-up commit
  # Now there are two commits
  git rebase --autosquash
  # Now there is just one commit, "Add a feature," with correct formatting
  

  These operations create and rely on commit headlines with the format fixup: Previous headline or squash: Previous headline.

  Authors could use this feature to automate some of the steps. However, this would get in the way of using --autosquash locally on that PR for their own purposes. I think it should be fine if authors use FIXUP: Previous message or similar for the fix-up commits they want to upload for review. The reviewer experience should be the same either way.

• This process is probably overkill for small PRs of a few lines that are trivial to re-review. I think fix-ups should be recommended for large PRs.

Alternatives

Habitually not rebasing

This proposal primarily addresses weaknesses of GitHub's UI for before-after comparison of force pushes. One annoying behavior is that it will show diffs for every file in the repository, not just those changed by the commits. This makes it basically unusable if the author has rebased the PR. Authors could avoid this by only using git rebase --keep-base during the review. Reviewers would then be able to use GitHub to see just the relatively small changes that the author made in response to review comments.

However, this would retain other weaknesses of the GitHub push comparison UI:

• It does not show in which commits the new changes will ultimately land.
• It also does not allow the reviewer to comment in the comparison view, so they need to navigate to a different page with more code and find the code they were looking at previously.
• It only works for the delta introduced by the latest push. (Actually, I don't know if this is true. I've seen some PRs with multiple compare buttons and some with only one.)

[henrikbrixandersen]

This seems overly complicated IMO.

If people stick to interactively rebasing against the same base as the initial/last push on a given PR, the GH UI works fine for listing the actual changes compared to the previous revision of the PR.

If a rebase against upstream main is needed, this can be done as a separate force-push (without other changes) and the reason for the rebase can be noted in a comment on the PR.

[alevkoy]

I think you're right about deliberately avoiding/isolating rebases. We should probably do that anyway.

You might be right about this being overly complicated. It seems simple to me, because I'm used to Gerrit, and this somewhat replicates the Gerrit review flow. (Single-commit diffs, one review cycle at a time.)

I created the test PR above so that people could try it and see how it feels. If you ask me to change something, I'll make a fix-up, and you can take a look.

[mbolivar-nordic]

Process WG: no consensus

• general agreement that we are missing features from gerrit and the review experience on github is not ideal
• some discussion of allowing this as optional workflow and failing CI if fixup commits are present
• multiple parties voiced concerns that this will increase review workload rather than make it easier on balance

---

• @alevkoy I opened up kernel: Declare a useless function zephyr-testing#203 as an example of how this might work. I don't know that this is absolutely the way we should do this, but I'm hoping an example will resolve ambiguity about how this might work. Another idea that was suggested is separating your rebases from your fixup changes
• @carlescufi I want to understand the difference between rebasing and other force pushes. You're referring to GitHub's feature that shows the difference between the previous push and the last revision, where if you mix a rebase and functional changes, it becomes useless since it mixes in everything that was merged to main since then?
• @alevkoy yes, that's the problem I'm describing
• @carlescufi OK, because even if you try to separate rebases from force pushes, it doesn't solve the problem fully. So the discussion is whether introducing the complexity of separate fixup commits is worth it, right? One of the problems I see is that even if everything is perfect and we have fixup commits, we have to rebase and then ask everyone for +1s since pushing removes all approvals
• @alevkoy if they're doing it right, that least diff should be empty
• @nashif I'm still struggling to understand what you achieve. You can add fixup commits in PRs today if that's your way of developing, and then you have to figure out how all of this ends up before it gets merged
• @carlescufi this is about standardizing this, right?
• @nashif but how is this going to work? If I as a reviewer go in and see this, is the expectation that this is merged as-is?
• @alevkoy all this is supposed to help with is the experience of being a reviewer. If you as a reviewer ask for a change, I'll do it as a fixup so it's obvious to see that I made a change.
• @nashif what does it look like when it's merged?
• @alevkoy each fixup commit will be squashed into the previous commit with the same name
• @henrikbrixandersen if we could add a policy to not combine fixups with rebases against another base, the github UI works perfectly
• @keith-zephyr it works OK
• @stephanosio if you do multiple force pushes, it only shows the last force-push, for example
• @henrikbrixandersen another thing you can do as a reviewer is tick a box that says you've reviewed a file, pushing a change which does not affect that file keeps it collapsed after force push
• @dleach02 that's not correct; every force push shows up as a separate line in the GitHub UI with a diff from the previous version
• @stephanosio I think if you force push, then add a comment, then force push, you see both. But multiple force pushes in a row without intervening comments lose the history of force pushes
• @fabiobaltieri if the conversation is about fixup commits or not, you're making life easier for the specific reviewer that requested the change, but not other reviewers that jump in later. I feel if we do this, we'll end up in a situation where reviews will take even more time and confusing. It introduces more surface for errors
• @MaureenHelm I think he makes a good point (agreement from @nashif @dleach02 )
• @carlescufi I do see that at least in some PRs, force pushes do give you the difference between consecutive force pushes, but in merged PRs, you get a changed UI where github says there isn't anything to compare. My best guess is it works on open PRs, but not merged PRs
• @nashif Just a comment that when we switched to GitHub, force pushes worked better when we were doing some tests with force pushes in PRs. A second comment is that there are many GitHub users. We are trying to change something that is becoming the standard. I'm not sure there's any chance we can get the level of features we got from Gerrit. So what's the compromise here? I think people are mostly happy
• @carlescufi the way we work is different from the way most people on GitHub work. Most people on GitHub use merges and react to review comments by adding additional commits on top. That's the difference and why we miss Gerrit. We are working like Gerritt on GitHub
• @mbolivar-nordic I'll add that I'm not happy
• @nashif sure but you move on and get used to it
• @dleach02 right, but this is a forum where people can propose improvements
• @nashif every way you try to fix this will have disadvantages. We also have a large community; this proposal will affect the way everyone works, etc. You could use fixup commits right now if you wanted, in your PRs.
• @keith-zephyr this discussion was meant more as a way of asking if we wanted to propose this as a potential workflow. We're not trying to say this is a requirement
• [discussion of how this would work follows]
• @carlescufi we could have this as long as we have a compliance check that prevents merge if there is a fixup commit in the PR
• @galak I feel that any workflow not enforced by tooling is subject to pseudo-chaos, which is the state we are in now. I feel we're having a hard enough time enforcing basic concepts that @keith-zephyr has already documented like pulling separable things out into separate PRs. This is just going to be one of those things -- you can do it, we document how. But then is it more burden on the reviewers -- who are the finite resource -- than not introducing the new workflow? I understand the idea but I worry about anything not enforced by tooling. The community is large enough now that it will be difficult to shift workflow without tooling enforcement.
• @nashif but the tooling will enforce it -- the tooling would reject the PR if it has a fixup
• @galak yes, but my gripe is that reviewers have to understand a new workflow from what was there before. It's not like everyone will do this. The review resource is finite, so my argument is we should not be putting the burden on them. The developers should be more constrained than reviewers. If we can't enforce this, I don't think we should introduce it
• @nashif it depends on how you review -- some people review by looking at the whole change. But I think it helps a lot, since you can see concretely as a reviewer that my comment was addressed. I don't know if it's going to be a burden. In my opinion this is going in the right direction as long as it doesn't change how we merge
• @stephanosio a small concern: if we have fixup commits, we don't have a clear picture of how this will look like squashed together unless you autosquash locally
• @alevkoy I think one of the things the reviewer will need to do is make sure the commit structure makes sense after the autosquash
• @stephanosio but that's after you've decided it's OK. In the interim, it's hard to guess while fixup commits are still being added
• @alevkoy I'll concede that if you show up in the middle, it could be confusing
• @dleach02 depending on how you review; if it's commit by commit or entire PR diff based
• @MaureenHelm this has the potential to cause reviewers to have to come back multiple times to +1 review. Let's say you ask for changes, then review a fixup commit, then there's a force push with the squash, and you have to come back a third time. I think it's just shifting the burden. I recognize the value of fixup commits, but because we don't have any automatic tooling, I think it increases the number of times reviewers have to come back and re-review pull requests. To @stephanosio 's point, it makes assumptions there's only one person actively reviewing; with multiple reviewers, I think it gets complicated really quickly. I do appreciate the effort here since this is a feature in Gerritt we all want and don't have. I'm not convinced this is a lost cause, but I don't think this is the right solution

[marc-hb]

@carlescufi the way we work is different from the way most people on GitHub work. Most people on GitHub use merges and react to review comments by adding additional commits on top. That's the difference and why we miss Gerrit. We are working like Gerrit on GitHub

... as discussed already in #39194 and many others before.

This fixup workflow seems very popular on Github, maybe the most popular? It is documented on github and works really well as long as the intention is to squash all commits into a SINGLE final commit. The web UI has the button to do exactly that and "atomically" merge at the same time: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges

When you select the Squash and merge option on a pull request on GitHub.com, the pull request's commits are squashed into a SINGLE commit. Instead of seeing all of a contributor's individual commits from a topic branch, the commits are combined into ONE commit and merged into the default branch

Single final commit = no force push and no command line needed, no accidental rebase possible, no fixup->commit mapping ambiguity, no awkward interval between squash and merge where reviewers have to come and approve again and potentially request last minutes changes, etc. #52731 (comment)

Fixups spread over multiple commits in the same PR is technically possible but one order of magnitude more complicated and much less charted territory.

One "creative" solution that has apparently been explored to review series is to spread each logical change (and final single commit) in a separate per pull request and then somehow link pull requests to one another: isaacs/github#959.
At best very different from the current flow.

my gripe is that reviewers have to understand a new workflow from what was there before.

Maybe less of a "finite" resource but submitters will also have a hard time learning new tricks. Even "Github natives" will wonder why and when they should fixup versus amend: depending on the PR "size", number of commits, subsystem preference?

git rebase --keep-base

Seconding comments above: promoting a general --keep-base approach would really help in any case and it can start right now; zero change required. Already today you can see the huge difference it makes with the few people already using it.

Just for fun: Rebase Considered Harmful (he's wrong)

@dleach02 that's not correct; every force push shows up as a separate line in the GitHub UI with a diff from the previous version

Only when they're not too close in time.

[desowin]

This process is probably overkill for small PRs of a few lines that are trivial to re-review. I think fix-ups should be recommended for large PRs.

It might work for small PRs, but it'll become absolute madness with large PRs. I can easily imagine over 100 fixup commits in large PR and this is going to be unmanageable.

You might be right about this being overly complicated. It seems simple to me, because I'm used to Gerrit, and this somewhat replicates the Gerrit review flow. (Single-commit diffs, one review cycle at a time.)

I think https://sapling-scm.com/docs/addons/reviewstack/ is more similar to Gerrit than the proposed fixup commits.

@carlescufi I want to understand the difference between rebasing and other force pushes. You're referring to GitHub's feature that shows the difference between the previous push and the last revision, where if you mix a rebase and functional changes, it becomes useless since it mixes in everything that was merged to main since then?

Even if you separate the pushes, the GitHub compare becomes useless because it seems to work only for the last push. If the author does multiple pushes in between one of the reviewers look at the PR - the diff feature is no longer working for the reviewer. That is, the reviewer won't see the changes since he last reviewed, but at most the changes in last force push.

My workaround to GitHub review system is:

• Create ~/review/PR-number-short-desc
• Fetch the changes locally, export all commits from PR with git format-patch to ~/review/PR-number-short-desc/1-todo
• Review on GitHub, after submitting review rename 1-todo to 1-reviewed
• When new changes are in, fetch again and do 2-todo, etc.
• Sometimes I do not finish review before new version comes in, and in such cases I create 3-todo and essentially leave 2-todo as is (i.e. I do not rename it to 2-reviewed)
• To check differences, I compare the exported patches (far from ideal, but works much better for me than GitHub UI)

@carlescufi we could have this as long as we have a compliance check that prevents merge if there is a fixup commit in the PR

Definitely. Git history with a lot of fixup commits is not fun at all to navigate/bisect.

[alevkoy]

Seconding comments above: promoting a general --keep-base approach would really help in any case and it can start right now; zero change required. Already today you can see the huge difference it makes with the few people already using it.

I'm all for this. Maybe it will help enough that we won't feel like changing anything else. If it doesn't, at least we'll have a clearer idea of how much pain remains. As @galak noted yesterday, fixups would be a new process that people would have to learn. --keep-base still would be for some people, but it's easier to explain, fights GH less, and maybe provides a significant chunk of the benefit.

One "creative" solution that has apparently been explored to review series is to spread each logical change (and final single commit) in a separate per pull request and then somehow link pull requests to one another: isaacs/github#959.

Wow, that is a sad thread. But maybe we can try some of the third-party hacks that people list in there.

[mbolivar-nordic]

Process WG:

• @keith-zephyr I think @alevkoy 's take is that absent consensus, we can close this. The hope is that we can encourage submitters to separate force pushes which are rebases from force pushes which include changes
• @fabiobaltieri we can't enforce this, so this is meant as a guideline, right?
• @keith-zephyr we can't enforce this, but we can encourage it. It's already in my 'workflow suggestions' section in the ongoing committer expectations PR

@alevkoy please reopen if we misunderstood anything

[marc-hb]

There is now a Google Summer of Code project idea to add git range-diff and solve some of these problems in... Gitlab: https://gitlab.com/gitlab-org/gitlab/-/issues/24096
Give it a thumb up if you have a gitlab.com account. Competition never hurts.