← Back to context

Comment by yokoprime

13 hours ago

Haha, good luck working with a team with more than 2 people. A good reviewer looks at the end-state and does not care about individual commits. If im curious about a specific change i just look at the blame.

13 comments

yokoprime

Reply
tasuki  12 hours ago

> A good reviewer looks at the end-state and does not care about individual commits.

Then I must be a bad reviewer. In a past job, I had a colleague who meticulously crafted his commits - his PRs were a joy to review because I could go commit by commit in logical chunks, rather than wading through a single 3k line diff. I tried to do the same for him and hope I succeeded.

  • theshrike79  12 hours ago

    And then someone comments on a thing, they change it and force-push another "clean" history on top and all of your work is wasted because the PR is now completely different =)

  • KptMarchewa  12 hours ago

    Split the PR rather than force me to wade through your commit history. Use graphite or something else that allows you to stack PRs.

    • jlokier  9 hours ago

      Why is it "wade through" if there are 10 clearly distinct but dependent commits, but comfortable if it's 10 stacked PRs instead? They are basically the same thing, presented ever so slightly differently.

      I think in most teams I've worked with, the majority of developers (> 85%) barely undestand what Git is doing or what things mean inside GitHub, have never seen commit history as a graph, have never run something like "git log --oneline --graph --decorate" or "--format", and have never heard of "git range-diff" which is very useful for following commit/PR/unit changes.

      Personally I review using "git" itself, so I see the graph structure either way, and there's little difference between stacked PRs, commit chains in a single PR, or even feature branches, from that point of view. Even force-push branch updatea aren't difficult to review, because of the reflog and "git range-diff". The differences are mainly in what kinds of behaviour the web-based tooling promotes in the rest of the team, which does matter, and depends on the team.

      I agree with you if you're using Graphite instead of GitHub. Having a place to give feedback and/or approval on the individual "units" (commits in a PR, or PRs in a stack) is useful, grouping dependent but distinct changes is useful, and diff'd commit evolution within each unit PR in response to back-end-forth review feedback is useful in some collaborative settings. Though, if you know "git range-diff" and reflog, that shows diff'd commit evolution quite well.

      In GitHub, people are confused by stacked PRs both conceptually and due to the GitHub UX around them. Most times when I've posted a stacked PR to a GitHub project, other people didn't realise it was stacked, and occasinally someone else has merged the tip of a stack made by me, and been surprised to see all the dependent PRs merged automatically as a side effect. Usually before they get to reviewing those other PRs :-)

      People understand commit sequences in a PR, though I've rarely seen people treat the individual commits as units for review when using GitHub, unfortunately. In the Linux kernel world where Git was born, the PR flow is completely different from GitHub: Their system tends to result in feedback on individual commits. It also encourages better quality feedback, with less nitpicking, and better quality commits.

jfengel  11 hours ago

Sometimes I have to go back and fix a bug that appeared during another branch. Having the original commits helps me bisect it.

Not often, but given that it costs me nothing to have it all in my tree, I'd rather have it than not.

hhjinks  13 hours ago

You review code not to verify the actual output of the code, but the code itself. For bugs, for maintainability. Commit hygiene is part of that.

seba_dos1  13 hours ago

I have no troubles working on big FLOSS projects where reviews usually happen at the commit level :)