1

A common situation I have been encountering is a developer submits a pull request, a reviewer then makes some comments, the developer fixes the problems and then uses git rebase to update the original commit so the history is easier to read in the future. The problem now is its seemingly impossible to see what has changed between the original commit and the new changes that were added to the commit.

This makes reviewing the code difficult because its not possible to see if the new changes fixed the raised issue or if other changes were made that caused new issues so the whole PR must be reviewed again or blindly accepted.

Does git provide tools to view changes caused by rebasing or is there an alternative workflow that allows the git history to contain single commits per change without extra "Fixes" commits but still allows reviewers to see each change as the code was in the PR stage. For context I am using bitbucket but I would be interested if raw git or other websites can do this.

Qwertie
  • 5,784
  • 12
  • 45
  • 89

2 Answers2

2

There is no native (GitHub or BitBucket or GitLab) solution.

GitHub PR keep a reference to the old history (before each rebase), but comparing old with new history is too cumbersome.

The alternative would be to encourage the developer to rebase/clarify the history:

  • only at the end of the review process, for a final test, before accepting the PR.
  • not during the review process, where the problem articulated in the OP's question would arise.
VonC
  • 1,262,500
  • 529
  • 4,410
  • 5,250
  • Note: on GitHub, you can request a second round of review now: https://stackoverflow.com/a/54817351/6309 – VonC Feb 27 '19 at 07:49
0

So I hope I understood what you just described. From what I understand, basically after the reviewer posts comments on the pull-request(PR), the developer updates the said commit and then after it's pushed, you will still see just one commit (in which it is the new and updated one after the fix, and the original commit is 'gone'). Yes?

If so, I do recommend to update your workflow. It's as easy as just commiting the fix after the original commit. No need to rebase/change history as this is still readable.

develop   o---o---o
                   \
feature             A

then let's say A is the original commit and is now for PR, and a reviewer reviews commit A. Next thing to do by the dev is just simply commit after A:

develop   o---o---o
                   \
feature             A---A'

With A' being the 'fix' after the reviewer's comment. You can check the delta from A' to A by invoking:

git diff hashidAprime hashidA

I would suggest to rebase the branch only when updating from the develop/trunk. Do not rebase when you are still working. Now if you are kind of 'trapped' with that workflow in which you are strictly one commit per feature before PR, then i suggest everytime you make a fix you just note it in the commit message. Kind of a changelog if you may. But that would only show descriptions of what has changed and not the code blocks that were affected. I would personally pick just committing and committing everytime there's a review. It will still look neat, you don't really need to be strict to one commit if you ask me.

scarletlights
  • 9
  • 2
  • The problem with not rebasing is git blame becomes less useful. We often look at the git blame to see why a line does something or why it was changed. These PR fixes break this workflow because a line gets changed and then it gets changed again with a less useful commit comment like "Fix issue from review" – Qwertie Feb 27 '19 at 09:18
  • Hmm.. from what I'm seeing i don't think there's a way for you to have both. I mean the only way for you to see all the delta between fixes and reviews is for you not to rebase after reviews. But now you also don't want commits that are fixes to code reviews. If you ask me, not rebasing might be the better option (at least for your needs). Then just compromise the git blame part. Do git blame and then if theres not enough info just go to that commit hash and then view the string of fixes (commits) that preceed it. – scarletlights Feb 28 '19 at 06:52