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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 20, 2021, 20:17 UTC
Message-ID: <xmqqa6ps6794.fsf@gitster.g>
In-Reply-To: <YH6Hj2/fGimrLuZ+@C02YX140LVDN.corpad.adbkng.com>

Son Luong Ngoc <sluongng@gmail.com> writes:

Show 18 quoted lines

> On Mon, Apr 19, 2021 at 02:52:16PM -0700, Junio C Hamano wrote:
>> 
>> 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.

Show 5 quoted lines

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