From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 21 Nov 2022 07:38:43 GMT
Subject: Re: [PATCH v5 2/2] cat-file: add mailmap support to --batch-check option
Message-ID: <xmqqbkp0wyd8.fsf@gitster.g>
In-Reply-To: <20221120074852.121346-3-siddharthasthana31@gmail.com>

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 87b77fc5c9..21ba6bc278 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -1047,4 +1047,36 @@ test_expect_success 'git cat-file -s returns correct size with --use-mailmap for
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'git cat-file --batch-check returns correct size with --use-mailmap' '
> +	test_when_finished "rm .mailmap" &&
> +	cat >.mailmap <<-\EOF &&
> +	C O Mitter <committer@example.com> Orig <orig@example.com>
> +	EOF
> +	commit_size=`git cat-file commit HEAD | wc -c` &&

We prefer $(command substitution) over `command substitution`.  

When "cat-file" segfaults and dumps core, having it upstream of a
pipe would mean its crashing will be hidden.

Note that some implementations of "wc" pads its output with SP.  The
implication will be seen in a few paragraphs below.

> +	commit_sha=`git log --pretty=format:'%H' -n 1` &&

These single quotes are not doing what you think they are doing.
The body of the test is inside a pair of single quotes, and the one
after format: makes you step outside the single quote, take two
bytes %H literally, and the other single quote opens a new singly
quoted string segment.  Which is not wrong per-se, because there is
no special meaning attached to the sequence %H in the shell language,
but then you'd be better off writing format:%H without any quotes,
as that is more direct way to write what you are writing.

Also, --pretty=format:<something> is almost always a mistake.
Unless you have a good reason to use it, you'd most likely want to
use --format=<something> instead.

In any case, don't abuse "log" when you mean

    commit_object_name=$(git rev-parse HEAD) &&

> +	echo "$commit_sha commit $commit_size" >expect &&

As $commit_size here may have extra and unwanted SP before it, this
may break with the implementation of "wc" on certain platforms.  In
this particular instance, losing quoting, i.e.

	echo $commit_sha commit $commit_size >expect

may be a good workaround.

> +	commit_size=`git cat-file --use-mailmap commit HEAD | wc -c` &&

Exactly the same set of comments as above apply to this side, too.

> +	echo "$commit_sha commit $commit_size" >>expect &&
> +	echo "HEAD" >in &&
> +	git cat-file --batch-check <in >actual &&
> +	git cat-file --use-mailmap --batch-check <in >>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git cat-file --batch-command returns correct size with --use-mailmap' '
> +	test_when_finished "rm .mailmap" &&
> +	cat >.mailmap <<-\EOF &&
> +	C O Mitter <committer@example.com> Orig <orig@example.com>
> +	EOF
> +	commit_size=`git cat-file commit HEAD | wc -c` &&
> +	commit_sha=`git log --pretty=format:'%H' -n 1` &&
> +	echo "$commit_sha commit $commit_size" >expect &&
> +	commit_size=`git cat-file --use-mailmap commit HEAD | wc -c` &&
> +	echo "$commit_sha commit $commit_size" >>expect &&
> +	echo "info HEAD" >in &&
> +	git cat-file --batch-command <in >actual &&
> +	git cat-file --use-mailmap --batch-command <in >>actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done