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

[Son Luong Ngoc <sluongng@gmail.com>]

Hi Junio,

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

I'm not sure about Github PR, but Gitlab's MR workflow also provide a
merge queue implementation(Merge Train) coupled with CI to ensure the
merge result is accurately verified against tests.

What might be missing from (most) CI services is a bisect pipeline that
help us identify culprit commit that broke the tests, but that could be
engineered.

> The experience I had with Gerrit at $WORK gave me side-to-side diff
> with context with arbitrary on-demand width, even with per-word
> differences highlighted, and it may be wonderful that I can get all
> of these _without_ having to apply the patch myself, but what it
> gave me stopped there.  There are a lot more things that need to
> happen beyond looking at what changed in the context of the files
> during a review, from grepping in the tree for functions and
> variables used in the patch to see their uses in other parts of the
> system that the patch does not touch, to make various trial merges
> to different topics that are in flight, and Gerrit didn't help me an
> iota, but still gave me a (false) impression that I _did_ review the
> patch fully, when I only have scraped its surface, and the worst
> part of the story was that the UI feld so nice that I didn't even
> realize that I was doing a lot more shoddy job in reviewing than
> what I usually do to e-mailed patches.
> 

Yes, having context beyond the diff is very important for Code Review.
This is why I strongly recommend SourceGraph usages to folks I know.

  > https://sourcegraph.com/github.com/git/git/-/blob/builtin/repack.c#L61:13
  > https://sourcegraph.com/github.com/git/git/-/commit/9218c6a40c37023a1f434222d501218cf8157857#diff-01ec5e99d04fb7ba9753f219ab638469R64

(I have no affiliation with SourceGraph, just really enjoy their product)

A mordern codesearch service like sourcegraph could help decorate diff
with relevant code intelligent like finding references, definitions and
assist with the Code Review process.

Afaik, sourcegraph has been building more integrations with Github and
Gitlab, not too sure about Gerrit (but Im sure it's not far reach given
their GraphQL API).

So I guess mordern toolings are available for these usecases, but
fragmented and subjective to personal workflow.

Regards,
Son Luong.