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

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 10, 2025, 08:47 UTC
Message-ID: <CAP8UFD2Cfriv4puPe8agaTZOpLHr-=4CkK-yrzw8fH-k5mPkAA@mail.gmail.com>
In-Reply-To: <20250110053417.2602109-3-jltobler@gmail.com>

On Fri, Jan 10, 2025 at 6:38 AM Justin Tobler <jltobler@gmail.com> wrote:

Show 6 quoted lines

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

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

> ++
> +The `path=<path>` shows the path of the missing object inferred from a
> +containing object.

For example for the path, I think it might be easier to always enclose it in double-quotes in the C style rather than checking first if it contains spaces or other special characters, see below.

Show 11 quoted lines

> +static void print_missing_object(struct missing_objects_map_entry *entry,
> +                                int print_missing_info)
> +{
> +       struct strbuf sb;
> +
> +       if (!print_missing_info) {
> +               printf("?%s\n", oid_to_hex(&entry->entry.oid));
> +               return;
> +       }
> +
> +       strbuf_init(&sb, 0);

I am not sure it's worth initializing the sb separately from its declaration above. Using "struct strbuf sb = STRBUF_INIT;" is more standard in the code base and I think most compilers these days are likely to be able to optimize away the initialization in the "!print_missing_info" case.

Show 9 quoted lines

> +       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);

if we decided to always quote the path. The decoding part would likely be simplified too.

Show 5 quoted lines

> +       }
> +
> +       printf("?%s%s\n", oid_to_hex(&entry->entry.oid), sb.buf);
> +       strbuf_release(&sb);
> +}

Show 8 quoted lines

> @@ -656,6 +703,15 @@ int cmd_rev_list(int argc,
>                 if (skip_prefix(arg, "--missing=", &arg))
>                         continue; /* already handled above */
>
> +               if (!strcmp(arg, "--missing-info")) {
> +                       if (arg_missing_action != MA_PRINT)
> +                               die(_("the option '%s' requires '%s'"),
> +                                   "--missing-info", "--missing=print");

It seems to me that this check should be performed outside the arg parsing loop so that this check passes if the user passes "--missing-info" before "--missing=print" on the command line.

> +                       print_missing_info = 1;
> +                       continue;
> +               }
> +