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

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

Junio C Hamano <gitster@pobox.com> writes:

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

The thread went dark since this message.

I do not want to see energy spent on a viable topic so far disappear
into a void without achieving anything.  Let's push the topic over
the finish line with futher work.

Thanks.