3 ms·

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

by 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).

by agsnu 4y ago

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).