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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 30, 2022, 15:50 UTC
Message-ID: <61074b4c-c48f-da89-5d03-b280b9c4fedf@gmail.com>
In-Reply-To: <20220630142444.651948-4-siddharthasthana31@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.

Show 9 quoted lines

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

Show 18 quoted lines

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