Re: [PATCH 3/3] cat-file: add mailmap support

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Siddharth

On 30/06/2022 15:24, Siddharth Asthana wrote:
> git cat-file is not a plumbing command anymore, especially as it gained
> more and more high level features like its `--batch-command` mode. 

cat-file is definitely a plumbing command as it is intended to be used 
by scripts. It has a number of features that are used by porcelain 
commands but that does not make cat-file itself porcelain.

> So
> tools do use it to get commit and tag contents that are then displayed
> to users. This content which has author, committer or tagger
> information, could benefit from passing through the mailmap mechanism,
> before being sent or displayed.
> 
> This patch adds --[no-]use-mailmap command line option to the git
> cat-file command. It also adds --[no-]mailmap option as an alias to
> --[no-]use-mailmap.

I don't think we need an alias for this option, it'll just end up 
confusing people.

> At this time, this patch only adds a command line
> option, but perhaps a `cat-file.mailmap` config option could be added as
> well in the same way as for `git log`.

As cat-file is a plumbing command that is used by scripts we should not 
add a config option for this as it would potentially break those scripts.

I like the idea of adding mailmap support to cat-file and I think this 
patch is definitely going in the right direction.

> +char *replace_idents_using_mailmap(char *object_buf, size_t *size)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	strbuf_attach(&sb, object_buf, *size, *size + 1);

I'm worried by this as I don't think we really own the buffer returned 
by read_object_file(). git maintains a cache of objects it has loaded 
and if this strbuf grows when the author is rewritten then the pointer 
stored in the cache will become invalid. If you look at the code in 
revision.c you'll see that commit_rewrite_person() is called on a copy 
of the original object.

> +	rewrite_ident_line(&sb, "\nauthor ", &mailmap);
> +	rewrite_ident_line(&sb, "\ncommitter ", &mailmap);
> +	rewrite_ident_line(&sb, "\ntagger ", &mailmap);
> +	*size = sb.len;
> +	return strbuf_detach(&sb, NULL);
> +}
> [...]
> +test_expect_success '--no-use-mailmap disables mailmap in cat-file' '
> +	test_when_finished "rm .mailmap" &&
> +	cat >.mailmap <<-EOF &&
> +	A U Thor <author@example.com> Orig <orig@example.com>
> +	EOF
> +	cat >expect <<-EOF &&
> +	author Orig <orig@example.com>
> +	EOF
> +	git cat-file --no-use-mailmap commit HEAD >log &&
> +	grep author log >actual &&
> +	sed -e "/^author/q" actual >log &&

This line does not have any effect on the contents of log

> +	sed -e "s/ [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$//" log >actual &&

I think you can simplify this series of commands to do
	git cat-file ... >log
	sed -n "/^author /s/\([^>]*>\).*/\1/p" log >actual

Best Wishes

Phillip