git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Son Luong Ngoc <sluongng@gmail.com>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>
Subject: Re: Pain points in PRs [was: Re: RFC: Moving git-gui development to GitHub]
Date: Tue, 20 Apr 2021 13:17:11 -0700	[thread overview]
Message-ID: <xmqqa6ps6794.fsf@gitster.g> (raw)
In-Reply-To: <YH6Hj2/fGimrLuZ+@C02YX140LVDN.corpad.adbkng.com> (Son Luong Ngoc's message of "Tue, 20 Apr 2021 09:49:35 +0200")

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

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

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

  reply	other threads:[~2021-04-20 20:17 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-23 20:13 RFC: Moving git-gui development to GitHub Pratyush Yadav
2019-10-24  2:06 ` Junio C Hamano
2019-10-24  7:37   ` Birger Skogeng Pedersen
2019-10-24 17:16     ` Denton Liu
2019-10-24 19:06       ` Pratyush Yadav
2019-10-24 21:29       ` Pratyush Yadav
2019-10-25  5:33         ` Birger Skogeng Pedersen
2019-10-25 17:47           ` Pratyush Yadav
2019-10-24 19:46 ` Elijah Newren
2019-10-25 18:36   ` Pratyush Yadav
2019-10-26 18:25   ` Jakub Narebski
2019-10-28 10:13     ` Konstantin Ryabitsev
2019-10-30  6:21     ` Elijah Newren
2019-11-20 12:19       ` Birger Skogeng Pedersen
2019-11-20 17:13         ` Elijah Newren
2021-04-19 20:33           ` Pain points in PRs [was: Re: RFC: Moving git-gui development to GitHub] SZEDER Gábor
2021-04-19 21:52             ` Junio C Hamano
2021-04-20  7:49               ` Son Luong Ngoc
2021-04-20 20:17                 ` Junio C Hamano [this message]
2021-04-22 20:22             ` Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqa6ps6794.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    --cc=sluongng@gmail.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).