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

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7486 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]