Re: Pain points in PRs [was: Re: RFC: Moving git-gui development to GitHub]

Son Luong Ngoc [off-list ref] writes:

On Mon, Apr 19, 2021 at 02:52:16PM -0700, Junio C Hamano wrote:

quoted

Interesting.

I recently had a similar experience with Gerrit, where a patch I
have seen quite a few times on Gerrit at $WORK had an embarrassing
syntactic issues I did not discover until it hit the public mailing
list.  It may be different from reviewer to reviewer, but at least
to me, e-mailed workflow forces me to apply the patch to my tree
before I can say anything non-trivially intelligent about it and
once applied to the tree, it actually let's me play with the code
(like, say, asking the compiler to give its opinion on it).

I think this is very much the point of having a good CI pipeline:
  - Apply patches into tree
  - Compile
  - Run relevant tests

It is true that CI can spot -Wdecl-after-stmt, but CI only covers
just one part of what is needed while I do my reviews.  It would
also be doable with web interface to look at all the places that
functions modified by the patch are referred to, and to check if the
change makes sense in the context of the entire tree.  It would also
be doable with web interface to looking at the evolution of the code
being changed.  There are some things, like building and using for
everyday life, running the built binary under debuggers, etc., that
may be harder to do with web interface, but I am sure many things
would become doable given enough time and effort.  However.

Yes, having context beyond the diff is very important for Code Review.
This is why I strongly recommend SourceGraph usages to folks I know.
...
So I guess mordern toolings are available for these usecases, but
fragmented and subjective to personal workflow.

My point in the message you are responding to was that I can do all
what is necessary locally, with my favorite toolset, once I apply a
patch to my tree.  The only thing that Gerrit allowed me to skip in
my recent adventure was to download the patch and apply to a newly
created topic branch locally to my tree, before I can start doing
some of the things (e.g. "look at the patch, examine with larger
context as needed", "grep for the symbols at the same revision in
paths that are not touched by the patch") that was needed to review.
And while I know I shouldn't blame the tool for this, but it did
mislead me to false sense of "I've reviewed this change well enough",
when I haven't.

By the way, I've been playing with "b4 am" and it's been a pleasant
experience so far.

Thanks.