← Back to context

Comment by kardianos

12 hours ago

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.

9 comments

kardianos

Reply
tcoff91  11 hours ago

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  11 hours ago

    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  8 hours ago

      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?

      4 replies →

adityaathalye  3 hours ago

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)