Re: [PATCH] reftable: mark unused parameters in empty iterator functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 28, 2024 at 10:12:22AM -0700, Junio C Hamano wrote:

> Jeff King <peff@xxxxxxxx> writes:
> 
> > This should go on top of ps/reftable-drop-generic. Arguably this could
> > have been done as part of the conflict resolution when merging into next
> > alongside jk/mark-unused-parameters, but at this point I think a
> > separate patch is the best way forward.
> 
> As marking with UNUSED used to be optional before -Wno-unused-param
> got removed, I agree that it is better to apply this on top to be
> explicit, rather than burying it in an evil merge with a topic that
> marked other unrelated parameters as UNUSED.

[warning: philosophical rambling ahead]

The reason I mentioned the merge here is that I think you could argue
this is a mis-merge that already happened. Forget for a moment the
recent series that makes UNUSED non-optional, and consider the merge of
jk/mark-unused-parameters and ps/reftable-drop-generic.

The former updated code in reftable/generic.c, and the latter removed
that file entirely, so there's a conflict. And it's tempting to say "ok,
we don't care about this code anymore, so take the deletion", which is
what your merge did. But the code in question was actually moved in that
series, via f2406c81b9 (reftable/generic: move generic iterator code
into iterator interface, 2024-08-22). So I think the correct resolution
for the merge is to move those updates along with the code into the new
file; otherwise we are accidentally reverting part of what
jk/mark-unused-parameters did.

That said, I think you can get pretty philosophical here. Did the
reftable topic move the code, or did it delete some old code and add
some new code that needed the same change? :)

I also think it's pretty hard to notice these kinds of resolutions in
practice. There's a conflict at the point of deletion, but there's
nothing in the merge workflow that tells you "btw, this code is now over
here, so you should port the modifications from the side branch over".

Interestingly, I think a rebase of one topic onto the other might have
made it more clear, since the code movement happened in its own step
(which would make what was going on more obvious). I guess Michael
Haggerty's "imerge" would probably show something similar (actually, I
just tried it, and it indeed hones in on f2406c81 and 4695c3f3 as the
source of the conflict).

Anyway, I am not proposing to do anything different. _If_ we considered
the merge of the two topics that is in next to be an incorrect
resolution, we could repair that when merging to master. But I think
doing so is complicated. And certainly the philosophy of "if something
is tricky, try to be as explicit as possible" seems like a good one
here.

Mostly I just found it kind of an interesting case. :)

-Peff




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux