Re: [PATCH v2 1/2] rev-list: add --missing-info to print missing object path

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Jan 10, 2025 at 6:38 AM Justin Tobler <jltobler@gmail.com> wrote:
>
>> +--missing-info::
>> +       Only useful with `--missing=print`; prints any additional information
>> +       about the missing object inferred from its containing object. The
>> +       information is all printed on the same line with the missing object ID
>> +       in the form: `?<oid> [<token>=<value>]...`. Additional attributes are
>> +       each separated by a SP.
>
> Nit: I'd rather say "The `<token>=<value>` pairs containing additional
> information are separated from each other by a SP." to avoid
> introducing "attributes" which might not be very clear.

Excellent.

>> Any value containing a SP or special character
>> +       is enclosed in double-quotes in the C style as needed. Each
>> +       `<token>=<value>` may be one of the following:
>
> It might be a bit better to decide for each token-value pair how the
> value is encoded, instead of deciding in advance for all of them.

I strongly advise against it.

Declaring the syntax we will use for forseeable future for new
attributes upfront would allow the receiving end, the scripts that
interpret our output, to be written in a futureproof way.

An alternative is "value is encoded in token specific fashion, but
one thing that is common across these future encodings is that SP or
LF contained in value will be represented in such a way that the
resulting encoded value will not have either of these two
problematic bytes".

>> +       if (entry->path && *entry->path) {
>> +               strbuf_addstr(&sb, " path=");
>> +
>> +               if (quote_c_style(entry->path, NULL, NULL, 0))
>> +                       quote_c_style(entry->path, &sb, NULL, 0);
>> +               else if (strchr(entry->path, ' '))
>> +                       strbuf_addf(&sb, "\"%s\"", entry->path);
>> +               else
>> +                       strbuf_addstr(&sb, entry->path);
>
> I think the above code paragraph could be simplified to just:
>
>             quote_c_style(entry->path, &sb, NULL, 0);

Hmph, you two may be both wrong ;-)  It is unfortunate that you
cannot easily configure what is considered "must be quoted" bytes
per invocation of the quote_c_style() function.  Most problematic
is that in the cq_lookup[] table, SP and "!" are treated the same
way, i.e., <a b c> does not need to be quoted.  This comes from the
fact that quote_c_style() was written for the sole purpose of
showing the pathnames on the header lines of "git diff" output.

We may be able to (ab)use quote_path() with QUOTE_PATH_QUOTE_SP
bit set for this purpose, though, to work around the above.

cq_must_quote() also is affected by core.quotepath, which is not a
desirable feature here---we want our machine readable program output
stable regardless of end-user preference.

By the way, perhaps we should propose to make the default value for
core.quotepath to "no" at Git 3.0 boundary?

Thanks.