Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-670

Lift requirement on post-review commit squashing

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      Background

      Our developer guide page on the DM workflow has a couple of statements instructing developers to not leave post-review changes as separate commits:

      Code Review/Merging says:

      If it has not been done already, the developer should rebase the ticket branch against the latest master. During this rebase, we recommend squashing any fixup commits into the main commit implementing that feature. Git commit history should not record the iterative improvements from code review.

      Appendix/Rebase commits from code reviews says:

      Authors should rebase (i.e., git rebase -i master) their ticket branch to squash the post-review fixes to the pre-review commits. The end-goal is that a pull request, when merged, should have a coherent development story and look as if the code was written correctly the first time.

      My understanding is that this policy was put in place to make the commit history easier to understand for later readers. Readable commit history is certainly a fine goal, for the same reasons as readable code – it helps clarify the developer's intent and the circumstances or assumptions behind design decisions.

      However, this policy has had, in practice, several undesirable consequences:

      • Merging review corrections with the existing code history is a lot of work because later changes to the same lines (e.g., from refactoring in steps) need to be rebased onto the corrections. In the worst case, responding to a review can take longer than either the original work or the review itself, slowing down overall Stack development.
      • The work needed scales roughly quadratically with the number of existing commits, disproportionately penalizing developers who are conscientious about focused, atomic commits.
      • Similarly, anything that makes the review process slow discourages developers from organizing their work in fine-grained tickets, which itself slows down the review process and makes reviews more stressful for both parties.
      • The difficulty in implementing changes suggested in review makes developers resistant to changes they might otherwise agree with ("it's too much work to go back and fix it"), lowering code quality.
      • I have heard at least one developer say that they tend to squash most or all their commits to sidestep the difficulty of rebasing review changes. This negates the original goal of making the commit history easier to follow.

      I understand Astropy has adopted the opposite policy, recommending that review corrections not be squashed. Any insights into how that worked for them would be greatly appreciated.

      Proposal

      While the actual wording in the guide is quite mild, with phrases like "should" or "recommend" (our stringency level policy does not apply to this page, so these words mean what a typical English speaker expects them to mean), as far as I know most developers see the squashing as something they are expected to do unconditionally. I don't think making the wording even looser ("you might consider"?) will change that.

      I therefore propose removing the sentences "During this rebase... from code review." from the first quote and the entire "Rebase commits from code reviews..." section from the appendix. I'm not suggesting we forbid review squashing, as it is a useful practice when the changes are easy to apply retroactively (e.g., documentation typos).

      I believe this change will improve our workflow without making the commit history any harder to examine than it is at present.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                krzys Krzysztof Findeisen
                Reporter:
                krzys Krzysztof Findeisen
                Watchers:
                Fred Moolekamp, John Parejko, John Swinbank, Kian-Tat Lim, Krzysztof Findeisen, Merlin Fisher-Levine, Paul Price, Simon Krughoff, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel