11

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.

  • 14
    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 '15 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 '15 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 '15 at 21:42
  • Can you explain what "rebasing" is and when I would want it? – Kilian Foth Apr 4 '15 at 6:33
23

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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