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

Hi Kim,

On Sun, 15 Aug 2021, Kim Altintop wrote:

On Sat, 14 Aug 2021 Johannes Schindelin wrote:

quoted

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.

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