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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 21, 2022, 11:27 UTC
Message-ID: <221121.867czoczdr.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAP8UFD1CB90eoWpQmGJbfxK7uHX0-u4BuSE-v=mD1yuW+nnAxA@mail.gmail.com>

On Mon, Nov 21 2022, Christian Couder wrote:

Show 29 quoted lines

> On Mon, Nov 21, 2022 at 8:27 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
>>
>> > +test_expect_success 'git cat-file -s 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
>> > +     git cat-file commit HEAD | wc -c >expect &&
>> > +     git cat-file --use-mailmap commit HEAD | wc -c >>expect &&
>> > +     git cat-file -s HEAD >actual &&
>> > +     git cat-file --use-mailmap -s HEAD >>actual &&
>>
>> Doesn't this break under macOS where wc output tends to be padded
>> with SP on the right?  We used to often see test breakage when a
>> carelessly written test like
>>
>>         test "$(wc -l <outout)" = 2
>>
>> which expects the output file to have exactly two files (the
>> solution in this sample case is to lose the double quotes around the
>> command substitution).
>
> I guess that's the reason why `wc -c | sed -e 's/^ *//'` is used in
> the strlen() function in t1006-cat-file.sh. There are a number of
> places in the tests where wc -c or wc -l are used without piping the
> result into sed -e 's/^ *//' though. So it's not easy to understand
> why it's sometimes needed.

It's because in "t1006-cat-file.sh" we're assigning the "wc -c" to a variable, because it's used to "test_cmp" the number of bytes in some free-form text.

It would be nicer to split "test_line_count" into some utility function that knew how to parse out "wc -l", "wc -c" etc. for a given input file, and return that as a string.

In that case the "sed" isn't needed, and we're just (ab)using it to do
things we can do with whitespace managent + shell built-ins. E.g. this
works too (the "echo; echo; echo" showing that we're stripping out
whitespace "wc -c" might emit:
	
	diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
	index 23b8942edba..9ae4b534421 100755
	--- a/t/t1006-cat-file.sh
	+++ b/t/t1006-cat-file.sh
	@@ -106,7 +106,7 @@ echo_without_newline_nul () {
	 }
	 
	 strlen () {
	-    echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
	+    printf "%s" $(printf "%s" "$1" | (echo ; echo ; echo ; wc -c))
	 }
	 
	 maybe_remove_timestamp () {