← Back to context

Comment by Arainach

16 hours ago

Looking at multiple commits is not a good workflow:

* It amounts to doing N code reviews at once rather than a few small reviews which can be done individually

* Github doesn't have any good UI to move between commits or to look at multiple at once. I have to find them, open them in separate tabs, etc.

* Github's overall UX for reviewing changes, quickly seeing a list of all comments, etc. is just awful. Gerrit is miles ahead. Microsoft's internal tooling was better 16 years ago.

* The more commits you have to read through at once the harder it is to keep track of the state of things.

6 comments

Arainach

Reply
Hamuko  16 hours ago

>It amounts to doing N code reviews at once rather than a few small reviews which can be done individually

I truly do not comprehend this view. How is reviewing N commits different from/having to do less reviews reviewing N separate pull requests? It's the same constant.

  • Arainach  16 hours ago

    Small reviews allow moving faster for both the author and reviewer.

    A chain of commits:

    * Does not go out for review until the author has written all of them

    * Cannot be submitted even in partial form until the reviewer has read all of them

    Reviewing a chain of commits, as the reviewer I have to review them all. For 10 commits, this means setting aside an hour or whatever - something I will put off until there's a gap in my schedule.

    For stacked commits, they can go out for review when each commit is ready. I can review a small CL very quick and will generally do so almost as soon as I get the notification. The author is immediately unblocked. Any feedback I have can be addressed immediately before the author keeps building on top of it.

  • tcoff91  16 hours ago

    Let's compare 2 approaches to delivering commits A, B, C.

    Single PR with commits A, B, C: You must merge all commits or no commits. If you don't approve of all the commits, then none of the commits are approved.

    3 stacked PRs: I approve PR A and B, and request changes on PR C. The developer of this stack is on vacation. We can incrementally deliver value by merging PRs A and B since those particular changes are blocking some other engineer's work, and we can wait until dev is back to fix PR C.

tcoff91  16 hours ago

It's crazy that you're getting downvoted for this take.

This isn't reddit people. You're not supposed to downvote just because you disagree. Downvotes are for people who are being assholes, spamming, etc...

If you disagree with a take, reply with a rebuttal. Don't just click downvote.