Re: [PATCH v3 1/1] cat-file: add mailmap subcommand to --batch-command

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

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

> git-cat-file(1)'s --batch-command works with the --use-mailmap option,
> but this option needs to be set when the process is created. This means
> we cannot change this option mid-operation.
>
> At GitLab, Gitaly keeps interacting with a long-lived git-cat-file
> process and it would be useful if --batch-command supported toggling
> mailmap dynamically on an existing process.
>
> Add a `mailmap` subcommand to --batch-command that takes a boolean
> argument. The command now uses `git_parse_maybe_bool()` and supports all
> standard Git boolean values. Mailmap data is loaded lazily and kept in

I do not think you want to say "now uses `git_parse_maybe_bool()`".
Nobody is interested in the difference relative to what you did in
the previous iteration.

    ... that takes a boolean argument (usual ways you can specify a
    boolean value like 'yes', 'true', etc., are supported).

> +static void load_mailmap(void)
> +{
> +	if (mailmap.strdup_strings)
> +		return;
> +
> +	read_mailmap(the_repository, &mailmap);
> +}

This, especially the early return condition, may deserve a bit of
in-code comment, as "a used string_list has the .strdup_strings bit
set" is not a generally applicable rule.

    /*
     * The mailmap is initialized with .strdup_strings set to 0,
     * but read_mailmap() sets the bit to 1 (this is true even when
     * not a single mailmap entry is read), so it can be used for
     * lazy loading.
     */

or something, perhaps?

> @@ -692,6 +700,21 @@ static void parse_cmd_info(struct batch_options *opt,
>  	batch_one_object(line, output, opt, data);
>  }
>  
> +static void parse_cmd_mailmap(struct batch_options *opt UNUSED,
> +			      const char *line,
> +			      struct strbuf *output UNUSED,
> +			      struct expand_data *data UNUSED)
> +{
> +	int value = git_parse_maybe_bool(line);

As "line" is never NULL, one standard way to spell a boolean True is
not available to the callers, namely, "mailmap<EOL>" (like how a
configuration file entry "[core] bare" means "[core] bare = true"),
but that is probably OK. "mailmap<SP><EOL>" may be interpreted as
feeding an empty string as an argument, which is "false" to the
git_parse_maybe_bool() function.  That might be surprising.

Nothing actionable in the above comment (other than perhaps as a
hint for documentation update).

> +	if (value < 0)
> +		die(_("mailmap: invalid boolean '%s'"), line);
> +
> +	if (value > 0)
> +		load_mailmap();
> +	use_mailmap = value;
> +}

Hmph, why not use use_mailmap from the beginning of the function
without introducing the local variable "value"?  Nothing in
load_mailmap() pays attention to the current value of use_mailmap
so I do not see much point in preserving the current status until
the last minute.

Thanks.