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

> 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 caches git-cat-file processes and it would be useful
>> if --batch-command supported toggling mailmap dynamically with existing
>> processes.
>>
>> Add a `mailmap` subcommand to --batch-command that takes a single
>> argument: `yes` to enable mailmap and `no` to disable it. When enabled,
>> mailmap data is loaded from disk on first use and kept in memory so that
>> toggling back on does not require reloading.
>>
>> Suggested-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
>> ---
>> CI: https://gitlab.com/gitlab-org/git/-/pipelines/2416081861
>>
>>   Documentation/git-cat-file.adoc |  7 +++++
>>   builtin/cat-file.c              | 30 ++++++++++++++++++---
>>   t/t4203-mailmap.sh              | 48 +++++++++++++++++++++++++++++++++
>>   3 files changed, 81 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/git-cat-file.adoc b/Documentation/git-cat-file.adoc
>> index c139f55a16..af32e929a8 100644
>> --- a/Documentation/git-cat-file.adoc
>> +++ b/Documentation/git-cat-file.adoc
>> @@ -174,6 +174,13 @@ flush::
>>   	since the beginning or since the last flush was issued. When `--buffer`
>>   	is used, no output will come until a `flush` is issued. When `--buffer`
>>   	is not used, commands are flushed each time without issuing `flush`.
>> +
>> +mailmap <yes|no>::
>> +	Enable or disable mailmap for subsequent `contents` and `info`
>> +	commands. When `yes` is given, mailmap data is loaded from disk on
> 
> Are there any commands that the mailmap wouldn't apply to? Would it make
> sense to simply say
> 
>        Enable or disable mailmap for subsequent commands.

> 
> also we can s/is given//.
> 
>> +	first use and kept in memory; passing `yes` again does not reload it.
>> +	When `no` is given, mailmap is disabled but the data stays in memory
>> +	so that a later `mailmap yes` does not need to reload it from disk.
> 
> I think the first sentense here jumps directly into the the caching
> mechanism on using `yes`. It's more important for users to know what
> `yes` implies. So perhaps:
> 
>      When `yes` mailmap data is used and disabled on `no`. The first
>      `yes` caches the mailmap data until the command exits.
> 
>>   --
>>   +
>>
>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
>> index b6f12f41d6..a53926d2bb 100644
>> --- a/builtin/cat-file.c
>> +++ b/builtin/cat-file.c
>> @@ -54,6 +54,7 @@ static const char *force_path;
>>
>>   static struct string_list mailmap = STRING_LIST_INIT_NODUP;
>>   static int use_mailmap;
>> +static int mailmap_loaded;
>>
> 
> Nit: should we use a 'bool' here?

> 
> So we use a variable and not simple rely on checking `mailmap.nr`
> because it is possible that we do load the mailmap but there are no
> entries. I assume we could rely on `maimap.cmp` being non-NULL, but
> that's getting into implementation details.
> 
>>   static char *replace_idents_using_mailmap(char *, size_t *);
>>
>> @@ -692,6 +693,24 @@ 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)
>> +{
>> +	if (!strcmp(line, "yes")) {
>> +		if (!mailmap_loaded) {
>> +			read_mailmap(the_repository, &mailmap);
>> +			mailmap_loaded = 1;
>> +		}
>> +		use_mailmap = 1;
>> +	} else if (!strcmp(line, "no")) {
>> +		use_mailmap = 0;
>> +	} else {
>> +		die(_("mailmap: unknown argument '%s', expected 'yes' or 'no'"), line);
>> +	}
>> +}
>> +
>>   static void dispatch_calls(struct batch_options *opt,
>>   		struct strbuf *output,
>>   		struct expand_data *data,
>> @@ -725,9 +744,10 @@ static const struct parse_cmd {
>>   	parse_cmd_fn_t fn;
>>   	unsigned takes_args;
>>   } commands[] = {
>> -	{ "contents", parse_cmd_contents, 1},
>> -	{ "info", parse_cmd_info, 1},
>> -	{ "flush", NULL, 0},
>> +	{ "contents", parse_cmd_contents, 1 },
>> +	{ "info", parse_cmd_info, 1 },
>> +	{ "flush", NULL, 0 },
>> +	{ "mailmap", parse_cmd_mailmap, 1 },
>>   };
>>
>>   static void batch_objects_command(struct batch_options *opt,
>> @@ -1127,8 +1147,10 @@ int cmd_cat_file(int argc,
>>   	opt_cw = (opt == 'c' || opt == 'w');
>>   	opt_epts = (opt == 'e' || opt == 'p' || opt == 't' || opt == 's');
>>
>> -	if (use_mailmap)
>> +	if (use_mailmap) {
>>   		read_mailmap(the_repository, &mailmap);
>> +		mailmap_loaded = 1;
>> +	}
>>
> 
> The rest of the code looks good.
> 
>>   	switch (batch.objects_filter.choice) {
>>   	case LOFC_DISABLED:
>> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
>> index 74b7ddccb2..f66637cd86 100755
>> --- a/t/t4203-mailmap.sh
>> +++ b/t/t4203-mailmap.sh
>> @@ -1133,6 +1133,54 @@ test_expect_success 'git cat-file --batch-command returns correct size with --us
>>   	test_cmp expect actual
>>   '
>>
>> +test_expect_success 'git cat-file --batch-command mailmap yes enables mailmap mid-stream' '
>> +	test_when_finished "rm .mailmap" &&
>> +	cat >.mailmap <<-\EOF &&
>> +	C O Mitter <committer@example.com> Orig <orig@example.com>
>> +	EOF
>> +	commit_sha=$(git rev-parse HEAD) &&
>> +	git cat-file commit HEAD >commit_no_mailmap.out &&
>> +	git cat-file --use-mailmap commit HEAD >commit_mailmap.out &&
>> +	size_no_mailmap=$(wc -c <commit_no_mailmap.out) &&
>> +	size_mailmap=$(wc -c <commit_mailmap.out) &&
>> +	printf "info HEAD\nmailmap yes\ninfo HEAD\n" | git cat-file --batch-command >actual &&
>> +	echo $commit_sha commit $size_no_mailmap >expect &&
>> +	echo $commit_sha commit $size_mailmap >>expect &&
>> +	test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'git cat-file --batch-command mailmap no disables mailmap mid-stream' '
>> +	test_when_finished "rm .mailmap" &&
>> +	cat >.mailmap <<-\EOF &&
>> +	C O Mitter <committer@example.com> Orig <orig@example.com>
>> +	EOF
>> +	commit_sha=$(git rev-parse HEAD) &&
>> +	git cat-file commit HEAD >commit_no_mailmap.out &&
>> +	git cat-file --use-mailmap commit HEAD >commit_mailmap.out &&
>> +	size_no_mailmap=$(wc -c <commit_no_mailmap.out) &&
>> +	size_mailmap=$(wc -c <commit_mailmap.out) &&
>> +	printf "mailmap yes\ninfo HEAD\nmailmap no\ninfo HEAD\n" | git cat-file --batch-command >actual &&
>> +	echo $commit_sha commit $size_mailmap >expect &&
>> +	echo $commit_sha commit $size_no_mailmap >>expect &&
>> +	test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'git cat-file --batch-command mailmap works in --buffer mode' '
>> +	test_when_finished "rm .mailmap" &&
>> +	cat >.mailmap <<-\EOF &&
>> +	C O Mitter <committer@example.com> Orig <orig@example.com>
>> +	EOF
>> +	commit_sha=$(git rev-parse HEAD) &&
>> +	git cat-file commit HEAD >commit_no_mailmap.out &&
>> +	git cat-file --use-mailmap commit HEAD >commit_mailmap.out &&
>> +	size_no_mailmap=$(wc -c <commit_no_mailmap.out) &&
>> +	size_mailmap=$(wc -c <commit_mailmap.out) &&
>> +	printf "mailmap yes\ninfo HEAD\nmailmap no\ninfo HEAD\nflush\n" | git cat-file --batch-command --buffer >actual &&
>> +	echo $commit_sha commit $size_mailmap >expect &&
>> +	echo $commit_sha commit $size_no_mailmap >>expect &&
>> +	test_cmp expect actual
>> +'
> 
> Shouldn't we also add tests for how this interacts with '--mailmap' and
> '--no-mailmap'?

> 
>>   test_expect_success 'git cat-file --mailmap works with different author and committer' '
>>   	test_when_finished "rm .mailmap" &&
>>   	cat >.mailmap <<-\EOF &&
>> --
>> 2.51.0