top | item 41655516

(no title)

alex23478 | 1 year ago

I think this really boils down how your team is using Git and which code review tool you're using. (I've never used Gerrit personally, but as far as I understand it, we wouldn't have this conversation, since it aims to refine a single change by re-submitting a commit over and over again?)

For GitHub/GitLab reviews, I'm totally with you - this makes it more convenient for the reviewer to check that/how you've responded to feedback.

But now if you merge this without squashing, your Git history on the main branch contains all your revisions, which makes operations like bisecting or blame more complicated.

For me personally, the sweet spot is currently a mix of stacked-commits and the PR workflow: Use a single commit as the unit of review, and polish that commit using a PR, and use the commit descriptions within that PR to tell the story of you responding to feedback. Then, squash merge that commit.

This provides both a good review experience and a tidy history. If you find a strange bug later and can't grasp how someone could have come up with that, the PR still has all the history.

Together with tools such as git-branchless, this also makes working on stacked-PRs a breeze.

discuss

order

DanielHB|1 year ago

I have used standard github and graphite reviews. I tend to prefer what I mentioned in my original post than graphite stacked PR review (which are essentially atomic commits)

Yes I also advocate for squash-before-merge, so a lot of little commits is doesn't show up in the main history.

> For me personally, the sweet spot is currently a mix of stacked-commits and the PR workflow: Use a single commit as the unit of review, and polish that commit using a PR, and use the commit descriptions within that PR to tell the story of you responding to feedback. Then, squash merge that commit.

To me time spent on commit polishing (and dealing with conflicts) is time not spent on product. Author comments on PR review, squash-before-merge, and sit-together with reviewer for big PRs to me seems a better compromise. I don't think super polished git history is worth the extra effort for most types of product, as long as I can track a change down to a PR discussion that is enough to me. From there I can track PR review commit changes individually if needed.

Like it is so uncommon for me to go digging on git history that deeply, usually all I care is "code behaving weird && line changed < 1 month ago then probably a bug introduced then"

Of course if you are working on aviation software and the like maybe the priorities are different. But I have spent way too much time dealing with rebase conflicts when trying to chop up my PRs into smaller commits. Dealing with these conflicts often introduces bugs too.

usr1106|1 year ago

Why are people talking about stacked PRs/MRs? Shouldn't they be called queued? A stack is LIFO and a queue is FIFO.

(Of course in some special case you might want to merge a later one earlier, but I don't think that's the normal case people are talking about.)

DanielHB|1 year ago

Why is it a "Pull Request" instead of a "Push Request"?

Someone named it that way and it stuck.