3 ms·

I can perfectly understand the appeal of keeping the history clean by requiring ff-only merge commits, but I fail to see any proper case where squash rebase is

I can perfectly understand the appeal of keeping the history clean by requiring ff-only merge commits, but I fail to see any proper case where squash rebase is actually useful in any way. It only loses potentially useful information and doesn't give anything in return. The only usefulness I can image getting out of it is throwing away garbage commit messages and fixup commits made by people who didn't bother to get them right, which sounds like something to fix in the first place. When you want to maintain linear history, FF-only merge commits allow you to have cake (keep properly atomic changes retained) and eat it (filter out the detail in git log when it's unwanted). Squashing (eating cake) or rebasing with no merge commit (having cake) only makes things worse from there.

Gigachad 4y ago
Every PR I have worked on starts with some good commits but then they become useless when review happens and a bunch of commits that are essentially “do the suggested thing”. Better to just squash it all in to one commit for the whole PR. Ideally the PR is small enough to be logical as one commit.
- seba_dos1 4y ago
  > Better to just squash it all in to one commit for the whole PR. Absolutely not. It's better to edit your PR to contain suggested changes in a clear, atomic history before presenting it for another round of review. Why would I want to present commits that "do the suggested thing" to the reviewer in the first place? That doesn't make any sense and it shouldn't pass the review.
  - agsnu 4y ago
    Having a commit in isolation that "does the suggested thing" reduces the cognitive load on the reviewer(s) because you can see the conversation history and the specific changes in response to that comment without having to re-parse the entire branch changeset from first principles. Basically the trade off is, re-write history before review or after. If you re-write before review, you lose some context about the evolution of the changeset - also the GitHub UI can get confusing (because some conversation comments will apply to commits that no longer exist on the PR branch). Keeping the pre-review iteration visible in the context of the PR can make it easier for someone else to come along and join the discussion later. If your feature branches/PRs are short-lived, then the size of a squashed merge is still quite small, and the pre-review iteration is irrelevant. If for some reason the pre-review iteration is interesting, it's still there in the context of the PR, which is back-referenced from the commit message.
    - seba_dos1 4y ago
      > Having a commit in isolation that "does the suggested thing" reduces the cognitive load on the reviewer(s) because you can see the conversation history and the specific changes in response to that comment without having to re-parse the entire branch changeset from first principles. When I do a review this way, conversation history is retained and I can easily see the diff between rounds of reviews in the UI. I'm mostly using GitLab. Are GitHub's reviews really this bad? When working on a project that maintains linear history, you should imagine your merge requests to be like patchsets sent via e-mail. If you send a set of three patches to Linux and you get some feedback in return, you don't reply by sending a fourth patch on top. You reply by sending a V2 of your patchset. Previous discussion is still there attached to V1, and everything is clear for both the submitter and to reviewer, and even to bystanders reading the discussion in future. That's pretty much what you get when you force-push an updated patchset onto your MR branch in GitLab. > then the size of a squashed merge is still quite small In my experience, only trivial MRs fit comfortably into a single commit. Usually a good MR contains somewhere around 3-10 commits. More than that and it's probably worth splitting to make review easier, less than that and you're into single bug fix territory where this whole discussion doesn't matter much. > and the pre-review iteration is irrelevant. Pre-merge iteration, either before or during review, is always irrelevant for the repository. It should only be retained in your review tool (GitLab does that well) and referenced in merge commit (after all, its whole purpose is to do just that and group commits together).
kortex 4y ago
I think it depends a lot on your process as well. I should have added that we use Jira+Confluence and every merge commit starts with ENG-1234 | Does the needful with foobar So most of the context is in the tickets, which then point to design docs. The code has very high percentage of docstrings, very high type coverage (for Python anyways), so there's really not much to be had in commit messages. We are a small (<20) team of engineers in a fast-moving year-old startup. Very tight slack comms. PRs to main are often single-digit lines, typically a few dozen, and anything more than a few hundred LOC is considered suboptimal. I think if you are running an open-source project, with lots of disparate contributors, using Git as your main knowledge base, with bigger PRs, it's a very different ballgame. I think in that situation I would be team history.

Gigachad 4y ago

Every PR I have worked on starts with some good commits but then they become useless when review happens and a bunch of commits that are essentially “do the suggested thing”. Better to just squash it all in to one commit for the whole PR. Ideally the PR is small enough to be logical as one commit.

seba_dos1 4y ago
> Better to just squash it all in to one commit for the whole PR. Absolutely not. It's better to edit your PR to contain suggested changes in a clear, atomic history before presenting it for another round of review. Why would I want to present commits that "do the suggested thing" to the reviewer in the first place? That doesn't make any sense and it shouldn't pass the review.
- agsnu 4y ago
  Having a commit in isolation that "does the suggested thing" reduces the cognitive load on the reviewer(s) because you can see the conversation history and the specific changes in response to that comment without having to re-parse the entire branch changeset from first principles. Basically the trade off is, re-write history before review or after. If you re-write before review, you lose some context about the evolution of the changeset - also the GitHub UI can get confusing (because some conversation comments will apply to commits that no longer exist on the PR branch). Keeping the pre-review iteration visible in the context of the PR can make it easier for someone else to come along and join the discussion later. If your feature branches/PRs are short-lived, then the size of a squashed merge is still quite small, and the pre-review iteration is irrelevant. If for some reason the pre-review iteration is interesting, it's still there in the context of the PR, which is back-referenced from the commit message.
  - seba_dos1 4y ago
    > Having a commit in isolation that "does the suggested thing" reduces the cognitive load on the reviewer(s) because you can see the conversation history and the specific changes in response to that comment without having to re-parse the entire branch changeset from first principles. When I do a review this way, conversation history is retained and I can easily see the diff between rounds of reviews in the UI. I'm mostly using GitLab. Are GitHub's reviews really this bad? When working on a project that maintains linear history, you should imagine your merge requests to be like patchsets sent via e-mail. If you send a set of three patches to Linux and you get some feedback in return, you don't reply by sending a fourth patch on top. You reply by sending a V2 of your patchset. Previous discussion is still there attached to V1, and everything is clear for both the submitter and to reviewer, and even to bystanders reading the discussion in future. That's pretty much what you get when you force-push an updated patchset onto your MR branch in GitLab. > then the size of a squashed merge is still quite small In my experience, only trivial MRs fit comfortably into a single commit. Usually a good MR contains somewhere around 3-10 commits. More than that and it's probably worth splitting to make review easier, less than that and you're into single bug fix territory where this whole discussion doesn't matter much. > and the pre-review iteration is irrelevant. Pre-merge iteration, either before or during review, is always irrelevant for the repository. It should only be retained in your review tool (GitLab does that well) and referenced in merge commit (after all, its whole purpose is to do just that and group commits together).

kortex 4y ago

I think it depends a lot on your process as well. I should have added that we use Jira+Confluence and every merge commit starts with ENG-1234 | Does the needful with foobar So most of the context is in the tickets, which then point to design docs. The code has very high percentage of docstrings, very high type coverage (for Python anyways), so there's really not much to be had in commit messages. We are a small (<20) team of engineers in a fast-moving year-old startup. Very tight slack comms. PRs to main are often single-digit lines, typically a few dozen, and anything more than a few hundred LOC is considered suboptimal. I think if you are running an open-source project, with lots of disparate contributors, using Git as your main knowledge base, with bigger PRs, it's a very different ballgame. I think in that situation I would be team history.