Hi Kim,
On Sun, 15 Aug 2021, Kim Altintop wrote:
> 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.
> 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
