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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 4, 2021, 11:16 UTC
Message-ID: <877detni03.fsf@evledraar.gmail.com>
In-Reply-To: <YVrJj0Ltuc1Tcm7t@coredump.intra.peff.net>

On Mon, Oct 04 2021, Jeff King wrote:

Show 55 quoted lines

> On Mon, Oct 04, 2021 at 10:26:10AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >> +		/*
>> >> +		 * TRANSLATORS: The argument is the list of ambiguous
>> >> +		 * objects composed in show_ambiguous_object(). See
>> >> +		 * its "TRANSLATORS" comment for details.
>> >> +		 */
>> >> +		advise(_("The candidates are:\n\n%s"), sb.buf);
>> >
>> > Here's where the extra newline.
>> >
>> > I understand why the earlier ones were changed for RTL languages. But
>> > this one is always line-oriented. Is the point to help bottom-to-top
>> > languages? I can buy that, though it feels like that would be something
>> > that the terminal would deal with (because even with this, you're still
>> > getting the "error:" line printed separately, for example).
>> >
>> > I don't think what this is doing is wrong (at first I wondered about the
>> > "hint:" lines, but because advise() looks for embedded newlines, we're
>> > OK). But if the translation doesn't need to reorder things across lines,
>> > this extra format-into-a-strbuf step doesn't seem necessary. We can just
>> > call advise() directly in show_ambiguous_object(), as before.
>> >
>> > If it is necessary, then note that you leak "sb" here.
>> 
>> I'll keep that bit as-is, it's not strictly necessary, but it gives
>> translators a bit more context.
>
> If it's just for the context, wouldn't this do the same thing:
>
>   /*
>    * TRANSLATORS: This is followed by the list of ambiguous
>    * objects composed in show_ambiguous_object(). See its
>    * "TRANSLATORS" comments for details.
>    */
>   advise(_("The candidates are:"));
>   ...
>   if (oid_array_for_each(&collect, show_ambiguous_object, &ds))
>      ...
>
> I.e., leave the code as-is, and just add the extra comment. There is no
> need for the extra struct or any change of ordering between this
> advise() and the others.
>
> I would think it is worthwhile if we are de-lego-ing a message that is
> made in chunks, but in this case the we have to construct an opaque "%s"
> to represent the individual lines for each object, because we don't know
> how many of them there will be.
>
> -Peff
>
> PS In my "something like this" commit message, I indicated that the
>    "candidates" message was getting translated, but it actually is
>    already translated in the pre-image. So I think we would not need to
>    touch that line at all.

Yes you're right. You've got me, I guess :)

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.

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".

Anyway, now that we're here I'm not sure what the best way forward is. One is to just address the pointed-out bugs and keep that accumulate/print pattern I instituded, which would help that subsequent series. But I agree that while I think it is a bit better to translate the "foo:\n\n%s" message (it gives a bit more context about what sort of message it is), it's not really worth it just in the context of this patch.

What do you think? That we could let this pass for now, or we should drop this and I can try to re-visit it as part of some larger topic? That meaningful improvement to advise() depends on this + another series of advise fixes I submitted in parallel at [1].

1. https://lore.kernel.org/git/cover-0.5-00000000000-20211004T015432Z-avarab@gmail.com/T/#t