Re: [PATCH 2/2] object-name: make ambiguous object output translatable

From: Jeff King <peff@peff.net>
Date: Oct 4, 2021, 12:07 UTC
Message-ID: <YVrudGOcUxblsfPY@coredump.intra.peff.net>
In-Reply-To: <877detni03.fsf@evledraar.gmail.com>

On Mon, Oct 04, 2021 at 01:16:24PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 12 quoted lines

> An unstated motivation of mine here is that I've got a series that
> changes the advise() function itself so that it automatically adds the
> "and run xyz to disable this message".
> 
> Now some don't emit it, some don't even have associated configuration or
> documentation. It's a mess.
> 
> I originally hacked this up because this is the one in-tree user of
> advise() that constructs output incrementally. So for that improvement
> to advise() it either needs to be changed to not do so (this patch), or
> I'd need an advise_no_template() or advise_hint_line() or whatever as a
> workaround.

OK, that makes sense. In general, I think it's much easier if those motivations _aren't_ unstated. Both for the benefit of reviewers, but also folks reading commit messages later who wonder "hey, it looks like we didn't need this hunk, and it is causing a hassle, so why can't I just revert it".

But...

> I didn't mean to be too subterfuge-y about it. It's just hard to find a
> balance between a single long series & a few shorter ones, and when to
> distract reviewers with "this design choice is also because of XYZ
> tangentally related end-goal".

...yeah, if you have patches that say "do X, because later maybe we'll do Y", then it is often hard to evaluate them if Y is not in the same series. And _especially_ so if there is some other Z happening in the current series with X, because even talking about X is muddling things.

So in an ideal world, you'd not do X at all (in this case, touch the advise() lines), and leave Y (rolling up a buf to hand to a single advise() line) as a preparatory patch in a series that does Z (your change to advise() to print the extra stuff).

Things don't always break down that way, but I think they do here. Nothing you want to do here is semantically related to the later change to advise() you want to make. There are textual dependencies, which means you'll want to wait for one series to graduate before the other, but that's already the case if you stuff the preparatory patch in this series.

-Peff