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.

▲ adityaathalye 12 minutes ago | parent | next [-]

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

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

	▲	adityaathalye 6 minutes ago \| parent [-]
		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

▲ tcoff91 9 hours ago | parent | prev [-]

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 8 hours ago | parent [-]

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 5 hours ago | parent [-]

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 23 minutes ago | parent | next [-]

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.

▲

steveklabnik 5 hours ago | parent | prev [-]

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

▲

verst 2 hours ago | parent [-]

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 32 minutes ago \| parent [-]
		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.