On Fri, Jan 10, 2025 at 6:38 AM Justin Tobler <jltobler@xxxxxxxxx> 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.
> 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.
> +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.
> + 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.
> + }
> +
> + printf("?%s%s\n", oid_to_hex(&entry->entry.oid), sb.buf);
> + strbuf_release(&sb);
> +}
> @@ -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;
> + }
> +
