12

A common scenario in software development is code reviewing somebody else's code. A common tool for doing this is opening a Pull Request.

My question is, when issues are found in the review, should the changes

  1. be committed separately (new commit)
  2. or should the existing commit be modified (assuming nobody is branching from your earlier commit... since re-writing history from a shared branch is bad news).

For the first scenario, it's easy to track incremental changes, though it adds some noise to the commit history. The second options has the reverse pros and cons.

Improve this question
4
  • 15
    You say "noise" to the commit, but I read that is accurate history. Why try to mask what really happened in commit history? A code review is a code review, no need to paint it as something else. My vote would be go to the separate commit, and not the rebase in this case.
    – Thomas Stringer
    Apr 3, 2015 at 16:29
  • 3
    Usually I do both. Post each commit separately, then once the review is complete rebase and merge. GitHub keeps discussions on the pull request even after those commits are removed or replaced, so there's no substantial loss of history from rebasing. You get the best of both worlds.
    – Ajedi32
    Apr 3, 2015 at 20:41
  • 1
    i have mixed feelings regarding if i committed something that i somehow later determined crashes the system. those commits, if i discover their defect of my making soon, are ones i would like to rollback from history. but they're just bits, they don't cost that much, so probably the most secure, cost-effective, and consistent thing to do (as in doctrine) is always commit separately and leave the car wreck in the ditch for everyone to see now and forevermore. .... and you can mark each flawed branch with an unmistakable message to not build on it, so no shared branch.
    – robert bristow-johnson
    Apr 3, 2015 at 21:42
  • Can you explain what "rebasing" is and when I would want it?
    – Kilian Foth
    Apr 4, 2015 at 6:33

1 Answer 1

Reset to default
24

You assume the fix does not introduce any new issues and fixes the old ones complete. But lots of fixes are worth a review on their own - and that is probably much easier when the incremental changes can be reviewed separately.

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.