Re: [PATCH v2 1/2] cat-file: add mailmap support to -s option

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Sep 26 2022, Siddharth Asthana wrote:

> Even though the cat-file command with `-s` option does not complain when
> `--use-mailmap` option is given, the latter option is ignored. Compute
> the size of the object after replacing the idents and report it instead.
>
> In order to make `-s` option honour the mailmap mechanism we have to
> read the contents of the commit/tag object. Make use of the call to
> `oid_object_info_extended()` to get the contents of the object and store
> in `buf`. `buf` is later freed in the function.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: John Cai <johncai86@gmail.com>
> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
> ---
>  Documentation/git-cat-file.txt |  4 +++-
>  builtin/cat-file.c             | 13 +++++++++++++
>  t/t4203-mailmap.sh             | 10 ++++++++++
>  3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> index ec30b5c574..594b6f2dfd 100644
> --- a/Documentation/git-cat-file.txt
> +++ b/Documentation/git-cat-file.txt
> @@ -45,7 +45,9 @@ OPTIONS
>  
>  -s::
>  	Instead of the content, show the object size identified by
> -	`<object>`.
> +	`<object>`. If used with `--use-mailmap` option, will show the
> +	size of updated object after replacing idents using the mailmap
> +	mechanism.
>  
>  -e::
>  	Exit with zero status if `<object>` exists and is a valid
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 989eee0bb4..9942b93867 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -132,8 +132,21 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
>  
>  	case 's':
>  		oi.sizep = &size;
> +
> +		if (use_mailmap) {
> +			oi.typep = &type;
> +			oi.contentp = (void**)&buf;
> +		}
> +
>  		if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
>  			die("git cat-file: could not get object info");
> +
> +		if (use_mailmap && (type == OBJ_COMMIT || type == OBJ_TAG)) {

Just following along here: We want to handle both tag printing and size
computations. I.e. we happily search-replace the author in tag objects:
	
	$ git -P diff -- .mailmap
	diff --git a/.mailmap b/.mailmap
	index 07db36a9bb9..cace49e462b 100644
	--- a/.mailmap
	+++ b/.mailmap
	@@ -125,7 +125,7 @@ Jonathan del Strother <jon.delStrother@bestbefore.tv> <maillist@steelskies.com>
	 Josh Triplett <josh@joshtriplett.org> <josh@freedesktop.org>
	 Josh Triplett <josh@joshtriplett.org> <josht@us.ibm.com>
	 Julian Phillips <julian@quantumfyre.co.uk> <jp3@quantumfyre.co.uk>
	-Junio C Hamano <gitster@pobox.com> <gitster@pobox.com>
	+Foo <bar@baz.blah> Junio C Hamano <gitster@pobox.com>
	 Junio C Hamano <gitster@pobox.com> <junio@hera.kernel.org>
	 Junio C Hamano <gitster@pobox.com> <junio@kernel.org>
	 Junio C Hamano <gitster@pobox.com> <junio@pobox.com>
	$ ./git cat-file --use-mailmap tag v2.37.0 | head -n 4
	object e4a4b31577c7419497ac30cebe30d755b97752c5
	type commit
	tag v2.37.0
	tagger Foo <bar@baz.blah> 1656346695 -0700

And we want the "-s" to match, okey, but... (continued below)

> +			size_t s = size;
> +			buf = replace_idents_using_mailmap(buf, &s);
> +			size = cast_size_t_to_ulong(s);
> +		}
> +
>  		printf("%"PRIuMAX"\n", (uintmax_t)size);
>  		ret = 0;
>  		goto cleanup;
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index cd1cab3e54..59513e7c57 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -1022,4 +1022,14 @@ test_expect_success '--mailmap enables mailmap in cat-file for annotated tag obj
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'git cat-file -s returns correct size with --use-mailmap' '
> +	test_when_finished "rm .mailmap" &&
> +	cat >.mailmap <<-EOF &&

nit: use \ before EOF, no variables here.

> +	C O Mitter <committer@example.com> Orig <orig@example.com>
> +	EOF
> +	echo "220" >expect &&

nit: no need for "" quotes.

> +	git cat-file --use-mailmap -s HEAD >actual &&

I'd find this a bit easier to follow if acter setting up .mailmap we did
something like (I didn't look up what the actual "234" value is):

	>actual &&
	git cat-file -s HEAD >actual &&
	git cat-file -s --use-mailmap HEAD >>actual &&
	cat >expect <<-\EOF
        234
        220
	EOF

We surely test that somewhere else, but it would be a bit more
self-documenting, as the difference in sizes would correspond to the
size of the address (or a multiple thereof, if it's used replaced N
times).

> +	test_cmp expect actual
> +'

...our test only checks the commit handling. Let's be a bit more
defensive here & test both potential paths through that new "if".