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

            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Ok then, I'd like to amend the RFC proposal to the following:

            • Removing "During this rebase... from code review." as originally suggested.
            • Adding the following paragraph to Resolving a review, between the paragraphs beginning with "Code reviews are a collaborative check-and-improve process..." and "If the review becomes stuck on a design decision...":
              • Changes in response to a review can be made either by squashing changes onto the main commit implementing that feature, or as new commits. Any new commits must be as well-organized and well-documented as the original work (see [Commit Organization Best Practices]); do not simply commit all changes as "Response to review".

            • Replacing the "Rebase commits from code reviews" section in the appendix with the following:
              • Commits from code reviews should follow the same standards as original commits

                Code review will result in additional commits that address code style, documentation and implementation issues. Authors may rebase (i.e., git rebase -i master) their ticket branch to squash the post-review fixes to the pre-review commits. Any commits not squashed in this way should represent [discrete logical changes] and have [informative commit messages], as if the changes had been made before review.

            (All square brackets represent links to existing sections on the same page.)

            Show
            krzys Krzysztof Findeisen added a comment - - edited Ok then, I'd like to amend the RFC proposal to the following: Removing "During this rebase... from code review." as originally suggested. Adding the following paragraph to Resolving a review , between the paragraphs beginning with "Code reviews are a collaborative check-and-improve process..." and "If the review becomes stuck on a design decision...": Changes in response to a review can be made either by squashing changes onto the main commit implementing that feature, or as new commits. Any new commits must be as well-organized and well-documented as the original work (see [Commit Organization Best Practices]); do not simply commit all changes as "Response to review". Replacing the "Rebase commits from code reviews" section in the appendix with the following: Commits from code reviews should follow the same standards as original commits Code review will result in additional commits that address code style, documentation and implementation issues. Authors may rebase (i.e., git rebase -i master ) their ticket branch to squash the post-review fixes to the pre-review commits. Any commits not squashed in this way should represent [discrete logical changes] and have [informative commit messages], as if the changes had been made before review. (All square brackets represent links to existing sections on the same page.)
            Hide
            krughoff Simon Krughoff added a comment -

            I have no issues with that language.

            Show
            krughoff Simon Krughoff added a comment - I have no issues with that language.
            Hide
            ktl Kian-Tat Lim added a comment -

            I am almost OK with the amended proposal. I think two things are missing: the motivation for squashing (to end up with easy-to-manipulate commits that are relevant to a particular feature or fix) and therefore a preference for squashing if it is not difficult. As written, the motivation has been entirely removed, giving the impression that this is purely a developer preference rather than a cost-benefit trade-off.

            Show
            ktl Kian-Tat Lim added a comment - I am almost OK with the amended proposal. I think two things are missing: the motivation for squashing (to end up with easy-to-manipulate commits that are relevant to a particular feature or fix) and therefore a preference for squashing if it is not difficult . As written, the motivation has been entirely removed, giving the impression that this is purely a developer preference rather than a cost-benefit trade-off.
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            Third time's the charm:

            • Remove "During this rebase... from code review." as originally suggested.
            • Add the following paragraph to Resolving a review, between the paragraphs beginning with "Code reviews are a collaborative check-and-improve process..." and "If the review becomes stuck on a design decision...":
              • Changes in response to a review should be made by squashing changes onto the main commit implementing that feature, where practical. This avoids cluttering the final Git commit history with iterative improvements from code review. If this is not practical, changes may be made by new commits, which must be as well-organized and well-documented as the original work (see [Commit Organization Best Practices]). In no event should you simply commit all changes as "Response to review".

            • Reword the "Rebase commits from code reviews" section in the appendix as follows:
              • Code review will result in additional commits that address code style, documentation and implementation issues. Where possible, authors should rebase (i.e., git rebase -i master) their ticket branch to squash the post-review fixes to the pre-review commits. The preference 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.

                If such a rebase is too difficult (e.g., because it would result in excessive merge conflicts), then post-review commits may be left appended to the pre-review commits. Any commits not squashed in this way should represent [discrete logical changes] and have [informative commit messages], as if the changes had been made before review.

            (I've pushed back the end-date to tomorrow to allow for any final feedback.)

            Show
            krzys Krzysztof Findeisen added a comment - - edited Third time's the charm: Remove "During this rebase... from code review." as originally suggested. Add the following paragraph to Resolving a review , between the paragraphs beginning with "Code reviews are a collaborative check-and-improve process..." and "If the review becomes stuck on a design decision...": Changes in response to a review should be made by squashing changes onto the main commit implementing that feature, where practical. This avoids cluttering the final Git commit history with iterative improvements from code review. If this is not practical, changes may be made by new commits, which must be as well-organized and well-documented as the original work (see [Commit Organization Best Practices]). In no event should you simply commit all changes as "Response to review". Reword the "Rebase commits from code reviews" section in the appendix as follows: Code review will result in additional commits that address code style, documentation and implementation issues. Where possible, authors should rebase (i.e., git rebase -i master ) their ticket branch to squash the post-review fixes to the pre-review commits. The preference 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. If such a rebase is too difficult (e.g., because it would result in excessive merge conflicts), then post-review commits may be left appended to the pre-review commits. Any commits not squashed in this way should represent [discrete logical changes] and have [informative commit messages], as if the changes had been made before review. (I've pushed back the end-date to tomorrow to allow for any final feedback.)
            Hide
            krzys Krzysztof Findeisen added a comment -

            In the absence of any further objections, I'm adopting with the text in the previous comment.

            Show
            krzys Krzysztof Findeisen added a comment - In the absence of any further objections, I'm adopting with the text in the previous comment.

              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: