From: Siddharth Asthana Date: Tue, 31 Mar 2026 01:42:36 GMT Subject: Re: [PATCH v2 1/1] cat-file: add mailmap subcommand to --batch-command Message-ID: <49662ba7-3685-4745-8f23-7a9ba44c7966@gmail.com> In-Reply-To: On 30/03/26 15:14, Karthik Nayak wrote: > Siddharth Asthana 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 >> Signed-off-by: Siddharth Asthana >> --- >> 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 :: >> + 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. Good point, agreed. I will simplify the wording in v3 and avoid listing specific commands there. > > 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? Yes, makes sense. I will switch that to bool in v3. > > 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 Orig >> + 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 > + size_mailmap=$(wc -c > + 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 Orig >> + 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 > + size_mailmap=$(wc -c > + 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 Orig >> + 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 > + size_mailmap=$(wc -c > + 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'? Agreed, that coverage is missing. I will add tests for startup option + runtime toggle interaction in v3. > >> test_expect_success 'git cat-file --mailmap works with different author and committer' ' >> test_when_finished "rm .mailmap" && >> cat >.mailmap <<-\EOF && >> -- >> 2.51.0