3 ms·

How do people with workflows that don't do any squashing do code review? To me it always seems the main consideration deciding on commit size is about being con

How do people with workflows that don't do any squashing do code review? To me it always seems the main consideration deciding on commit size is about being considerate of the reviewer. Don't want to harass them with huge commits but also don't want to send barrages of tiny uncontextualized changes.

icedchai 3y ago
Generally, I look at the diff with the base branch, not individual commits (unless there is a reason to...)
mostlylurks 3y ago
> How do people with workflows that don't do any squashing do code review? Going through the commits one-by-one or just looking at the entire diff both work just fine in most cases. In the former case, the commit messages (even if short one-liners) actually help understand the story of how and why the changes ended up taking the shape they did, so it's usually actually easier than just reading through one big diff. > To me it always seems the main consideration deciding on commit size is about being considerate of the reviewer. Don't want to harass them with huge commits but also don't want to send barrages of tiny uncontextualized changes. Commits should be atomic. Their size is irrelevant to that consideration. An atomic change may be one character, or twenty thousand lines (if those changes constitute an atomic (= singular and indivisible) change). This usually doesn't result in a barrage of tiny changes that are difficult to understand on their own, but even if it did, commit messages, the sequence of commits, the full diff of the MR/PR, as well as the attached information on the issue tracker you use (which you presumably have if you're doing code review) all provide more than enough context.
cryptonector 3y ago
When I do code reviews, - first I review the commit list (including commentary), - then I review all the diffs in one go unless it turns out that it's better to use a different approach, in which case I review all the small commits, then all the large commits.
Spivak 3y ago
My solution to this is "stacked PRs." main <- PR #1 <- PR #2 <- PR #3 <- PR #4 You can review them in order where the diff view in the PR shows chunks of changes logically grouped together and can be commented on/amended separately. Once everyone is satisfied with the patch set #4 is pulled into #3 is pulled into #2 ... and it shows up in main as 4 commits each with an independent PR history attached.
- dfawcus 3y ago
  That then becomes a problem if the first PR is accepted and rebased while the others are outstanding. Or at least it is with bitbucket. If the first PR is instead merged, the commits vanish from the start of the sequence in the other PRs.

icedchai 3y ago

Generally, I look at the diff with the base branch, not individual commits (unless there is a reason to...)

mostlylurks 3y ago

> How do people with workflows that don't do any squashing do code review? Going through the commits one-by-one or just looking at the entire diff both work just fine in most cases. In the former case, the commit messages (even if short one-liners) actually help understand the story of how and why the changes ended up taking the shape they did, so it's usually actually easier than just reading through one big diff. > To me it always seems the main consideration deciding on commit size is about being considerate of the reviewer. Don't want to harass them with huge commits but also don't want to send barrages of tiny uncontextualized changes. Commits should be atomic. Their size is irrelevant to that consideration. An atomic change may be one character, or twenty thousand lines (if those changes constitute an atomic (= singular and indivisible) change). This usually doesn't result in a barrage of tiny changes that are difficult to understand on their own, but even if it did, commit messages, the sequence of commits, the full diff of the MR/PR, as well as the attached information on the issue tracker you use (which you presumably have if you're doing code review) all provide more than enough context.

cryptonector 3y ago

When I do code reviews, - first I review the commit list (including commentary), - then I review all the diffs in one go unless it turns out that it's better to use a different approach, in which case I review all the small commits, then all the large commits.

Spivak 3y ago

My solution to this is "stacked PRs." main <- PR #1 <- PR #2 <- PR #3 <- PR #4 You can review them in order where the diff view in the PR shows chunks of changes logically grouped together and can be commented on/amended separately. Once everyone is satisfied with the patch set #4 is pulled into #3 is pulled into #2 ... and it shows up in main as 4 commits each with an independent PR history attached.

dfawcus 3y ago
That then becomes a problem if the first PR is accepted and rebased while the others are outstanding. Or at least it is with bitbucket. If the first PR is instead merged, the commits vanish from the start of the sequence in the other PRs.