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

[Jeff King <peff@peff.net>]

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

> 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