3 ms·

> If you rebase locally and force push the branch to maintain the clean set of commits after addressing review comments Why would you do that, in general? That

> If you rebase locally and force push the branch to maintain the clean set of commits after addressing review comments Why would you do that, in general? That seems needlessly hostile to reviewers, since then they have to go through your diff and make sure you caught every place their comment applied (e.g. "did they actually handle the else-case everywhere they're testing on $var?"). Is this common practice in situations* where it harms diff readability? If so, why? * There are totally cases where that's necessary, but they don't seem like more than a small minority of comment/feedback/amendment cases. These are times like "you put all of this code in the totally wrong place" or "this is missing a fundamental assumption that would require you to totally rearchitect every change you wrote". In those cases, a rebase (plus clearly calling out "everything was rebased starting here please start reading from here on!") or a brand new branch is in order.

u801e 8y ago
> Why would you do that, in general? The main purpose is to maintain a clean set of commits. So rather than having a history like: Add method and unit tests Update version to 1.2.3 Fixed issue in method Fixed syntax error Addressing review comments you only have the first two commits. > That seems needlessly hostile to reviewers Why would doing something like this be considered hostile? The problem here is the tool used for reviewing code, not the submitter or reviewers. Other review tools have had the feature of viewing the diff of the branch across rebases for quite some time now. The only other alternative is to rely on the submitter to make sure they squash the amendments they make into the correct commit. That in itself is more difficult after making multiple commits of that nature. What usually happens in that case is that those commits end up as part of the master branch. This makes it more difficult to revert certain changes because they're split across multiple commits and it also makes it more difficult to track down the source of a bug. It also makes it more difficult to see which commit was responsible for a given change when using git blame.

u801e 8y ago

> Why would you do that, in general? The main purpose is to maintain a clean set of commits. So rather than having a history like: Add method and unit tests Update version to 1.2.3 Fixed issue in method Fixed syntax error Addressing review comments you only have the first two commits. > That seems needlessly hostile to reviewers Why would doing something like this be considered hostile? The problem here is the tool used for reviewing code, not the submitter or reviewers. Other review tools have had the feature of viewing the diff of the branch across rebases for quite some time now. The only other alternative is to rely on the submitter to make sure they squash the amendments they make into the correct commit. That in itself is more difficult after making multiple commits of that nature. What usually happens in that case is that those commits end up as part of the master branch. This makes it more difficult to revert certain changes because they're split across multiple commits and it also makes it more difficult to track down the source of a bug. It also makes it more difficult to see which commit was responsible for a given change when using git blame.