Re: [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 16, 2021, 12:39 UTC
Message-ID: <nycvar.QRO.7.76.6.2108161431500.55@tvgsbejvaqbjf.bet>
In-Reply-To: <20210815213453.GB10013@schmidt.localdomain>

Hi Kim,

On Sun, 15 Aug 2021, Kim Altintop wrote:

Show 6 quoted lines

> On Sat, 14 Aug 2021 Johannes Schindelin wrote:
>
> > My only comment is that I would find the diff to `upload-pack.c` much
> > easier to parse if the `arg` variable hadn't been renamed.
>
> Can you explain why?

Yes. I prefer patches to be really obvious. That way, it is really easy to spot bugs.

In this instance, the same patch that introduces a conditional block _also_ renames an involved variable.

To satisfy myself that the patch does what is intended, I therefore have to virtually split the patch into the rename part and the added-conditional-block part.

It would be easier for me if the modifications were presented as two separate patches. And I could imagine that I am not alone in this: you yourself might also have an easier time looking at the commits in six months from now if those concerns are separated into their own commit.

Show 13 quoted lines

> Just because the diff would be smaller? I can see that in a larger patch
> it might have been preferable to put the rename into a separate commit,
> but in a hunk-sized change it seemed fine. It is also that this
> particular naming ("refname_nons") is used in other places in
> upload-pack.c, so it seemed obvious that, if I introduce namespace
> handling where it was previously missing, the terminology (if you will)
> should be the same.
>
> From you comment, it seems like the proposer of a patch should assume
> that the reviewers only look at the diff as sent in the email, and not
> any context. Junio's response suggests something else, but I guess it's
> fair that if someone feels like they got CC'ed by mistake, they're not
> going to spend too much time.

In this instance, I indeed did not spend more time than on reviewing the patch, simply because I am (currently, at least) not all that familiar with the `upload-pack.c` machinery. I probably touched it in the past, but for the moment, all I can comment on is the shape of the patch series, which is what I did.

> So my question from above stands: are there better ways to find the
> right people to CC, especially for newbies?

When I look for reviewers in projects other than the ones where I know the usual reviewers' special areas of interest, I like to pick a function at the center of my contribution, then look at `git log -L :<function>:<file>` and try to figure out who was the last person to implement non-stylistic changes on that function. This has worked relatively well for me, in the past. Maybe it can help you here, too?

Ciao, Dscho