[kardianos]

I continue to use gerrit explicitly because I cannot stand github reviews. Yes, in theory, make changes small. But if I'm doing larger work (like updating a vendored dep, that I still review), reviewing files is... not great... in github.

[tcoff91]

Most editors have some kind of way to review github PRs in your editor. VSCode has a great one. I use octo.nvim since I use neovim.

[nine_k]

Can these tools e.g. do per-commit review? I mean, it's not the UI what's the problem (though it's not ideal), it's the whole idea of commenting the entire PR at once, partly ignoring the fact that the code in it changes with more commits pushed.

Phabricator and even Gerrit are significantly nicer.

[dathanb82]

Unless you have a “every commit must build” rule, why would you review commits independently? The entire PR is the change set - what’s problematic about reviewing it as such?

[riffraff]

There's a certain set of changes which are just easier to review as stacked independent commits.

Like, you can do a change that introduced a new API and one that updates all usages.

It's just easier to review those independently.

Or, you may have workflows where you have different versions of schemas and you always keep the old ones. Then you can do two commits (copy X to X+1; update X+1) where the change is obvious, rather than seeing a single diff which is just a huge new file.

I'm sure there's more cases. It's not super common but it is convenient.

[strokirk]

Wouldn’t it be easier to do those as stacked PRs then?

[steveklabnik]

In stacked diffs system, each commit is expected to land cleanly, yes.

[verst]

But isn't that why you would squash before merging your PR? If you define a rule that PRs must be squashed you would still have the per commit build.

[steveklabnik]

Squash merge is an artifact of PRs encouraging you to add commits instead of amending them, due to GitHub not being able to show you proper interdiffs, and making comments disappear when you change a diff at that line. In that context, when you add fixup commits, sure, squashing makes sense, but the stacked diffs approach encourages you to create commits that look like you want them to look like directly, instead of requiring you to roll them up at the end.

[Sebb767]

> Unless you have a “every commit must build” rule, why would you review commits independently?

Security. Imagine commit #1 introduces a security vulnerability (backdoor) and the features. Then #2 introduces a non-obvious, harmless bug and closes the vulnerability introduced in #1 [0]. At some point, the bug will surface and rolling back commit #2 will be an easy fix, re-introducing your bug.

Alternatively, one of the earlier commits might, for example, contain credential dumping code. Once that commit is mainlined, CI might either automatically run on it or will be able to be run on it since it's no longer marked as unsafe PR.

[0] Think something like #1 introduces array access and #2 adds a bounds-check in a function a layer above - a reviewer with the whole context will see the bounds check and (possibly) consider it fine, but to someone rolling back a commit the necessity will not be obvious.

[adityaathalye]

Same team, and a rare hill I'm willing to die on.

Rant incoming...

Boy do I hate Github/Lab/Bucket style code reviews with a burning passion. Who the hell loses code review history? A record of the very thing that made my code better? The "why" of it all, that I am guaranteed to forget tomorrow morning.

Nobody would be using `--force` or `--force-with-lease` as a normal part of development workflow, of their own volition, if they had read that part of the git-push manpage and been horrified (as one should be).

The magit key sequence for this abominable operation is `P "f-u"`. And every single time I am forced to do it, I read "f-u" as it ought to be read.

Rebase-push is the way to do it (patch sets in Gerrit).

Rebase-force-push is absolutely not.

You see, any development workflow inevitably has to integrate changes from at least one other branch (typically latest develop or master), without destroying change history, nor review history. Gerrit makes this trivial.

It's a bit difficult to convey exactly why I'm so rah-rah Gerrit, because it is a matter of day-to-day experience of

  - Well, a single commit of a few lines to maybe a hundred lines *is* the correct unit of code review, rebase, revert etc. Manually "Sizing PRs" to that review context size is utter BS. I have better things to do in life than to book-keep PR sizes. Make a single well-contained, revertible commit. Then keep making those. And now you have a commit history that is clean, that you can merge, bisect, and bulk-revert at will. Octopus merges are a good thing. `git-log` is *designed* to let us view changes in any sequence we wish, *including* the so-called "linear" history. `git log --online`.

  - Trivial for committer to send up reviews-preserving rebase-push responses to commit reviews (NO force-push, ever --- that's an "admin" action to *evict* / permanently wipe out disaster scenarios such as when someone accidentally commits and pushes out a plaintext secret or a giant blob of the executable of the source code etc.).

  - Fast-for-the-reviewer, per-commit, diff-based, inline-commenting code reviews.

  - The years-apart experience of being able to dig into any part of one's (immutable) software change history to offer a teaching moment to someone new to the team.

... to name a few key ones.

(edit: add point about review size)

[adityaathalye]

Slapping this "stacked diff" business on top of something so broken as Github/lab/bucket is a concrete example of... https://en.wikipedia.org/wiki/Lipstick_on_a_pig