4 ms·

I would agree with your points if not for the fact that "clean up commits" or "typo fix" is a necessary result of PR's. Your teammate will request changes in y

I would agree with your points if not for the fact that "clean up commits" or "typo fix" is a necessary result of PR's. Your teammate will request changes in your code, and the only way to cleanly communicate "yes I made that change, and ONLY that change" is through these clean-up commits. Otherwise, if you amend/force-push or open an entirely new PR, 99% of the diff are things that your team has already seen and reviewed. Squash merges let you clearly communicate how you addressed PR comments, while also keeping the master history clean.

jcelerier 5y ago
> Otherwise, if you amend/force-push or open an entirely new PR, 99% of the diff are things that your team has already seen and reviewed. gerrit has solved this issue for years by showing the diffs between each successive revision of a patch. e.g. look here the files at different origin patchsets : https://codereview.qt-project.org/c/qt/qtwayland/+/321246/3..6 https://codereview.qt-project.org/c/qt/qtwayland/+/321246/3....
- jonhohle 5y ago
  This was a feature of ReviewBoard as well. The history of code review changes was maintained by the tool separately from commit history.
- pjc50 5y ago
  It took me a while to get a workflow that actually works with Gerrit, and I occasionally think of trying to do some kind of autosquash so I can have successive commits locally. In practice I just use --amend all the time. Careful use of setting upstream and of course pull=rebase makes keeping up with trunk manageable.
- servilio 5y ago
  You can also use git-range-diff[1]. [1] http://git-scm.com/docs/git-range-diff http://git-scm.com/docs/git-range-diff
  - jcelerier 5y ago
    how does that work if the old version of the amended commit has been gc'd ?
- piotrkaminski 5y ago
  And as it happens, Reviewable also deals with this perfectly fine. In fact, it appears that most any code review tool save GitHub is perfectly all right with amend/force-push...
leafmeal 5y ago
Fixup commits are a good way to work around this. https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---fixupltcommitgt https://git-scm.com/docs/git-commit#Documentation/git-commit... You still create your "clean up" commits, but it's done in a way where git can automatically squash everything back together in the end. See also https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---autosquash https://git-scm.com/docs/git-rebase#Documentation/git-rebase...
- maxioatic 5y ago
  Huh, never knew about this. Seems quite useful. Thanks!
mrkeen 5y ago
I don't mind this. I want my reviewers to look at the state of the new code - not the diff, and not the diff of the diff. Address PR comments using the PR comments.

jcelerier 5y ago

> Otherwise, if you amend/force-push or open an entirely new PR, 99% of the diff are things that your team has already seen and reviewed. gerrit has solved this issue for years by showing the diffs between each successive revision of a patch. e.g. look here the files at different origin patchsets : https://codereview.qt-project.org/c/qt/qtwayland/+/321246/3..6 https://codereview.qt-project.org/c/qt/qtwayland/+/321246/3....

jonhohle 5y ago
This was a feature of ReviewBoard as well. The history of code review changes was maintained by the tool separately from commit history.
pjc50 5y ago
It took me a while to get a workflow that actually works with Gerrit, and I occasionally think of trying to do some kind of autosquash so I can have successive commits locally. In practice I just use --amend all the time. Careful use of setting upstream and of course pull=rebase makes keeping up with trunk manageable.
servilio 5y ago
You can also use git-range-diff[1]. [1] http://git-scm.com/docs/git-range-diff http://git-scm.com/docs/git-range-diff
- jcelerier 5y ago
  how does that work if the old version of the amended commit has been gc'd ?
piotrkaminski 5y ago
And as it happens, Reviewable also deals with this perfectly fine. In fact, it appears that most any code review tool save GitHub is perfectly all right with amend/force-push...

leafmeal 5y ago

Fixup commits are a good way to work around this. https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---fixupltcommitgt https://git-scm.com/docs/git-commit#Documentation/git-commit... You still create your "clean up" commits, but it's done in a way where git can automatically squash everything back together in the end. See also https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---autosquash https://git-scm.com/docs/git-rebase#Documentation/git-rebase...

maxioatic 5y ago
Huh, never knew about this. Seems quite useful. Thanks!

mrkeen 5y ago

I don't mind this. I want my reviewers to look at the state of the new code - not the diff, and not the diff of the diff. Address PR comments using the PR comments.