From: Christian Couder <christian.couder@gmail.com>
Date: Mon, 21 Nov 2022 09:40:31 GMT
Subject: Re: [PATCH v5 1/2] cat-file: add mailmap support to -s option
Message-ID: <CAP8UFD1CB90eoWpQmGJbfxK7uHX0-u4BuSE-v=mD1yuW+nnAxA@mail.gmail.com>
In-Reply-To: <xmqqy1s4wyvf.fsf@gitster.g>

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.

> Besides, having "cat-file" on the upstream side of a pipe is a bad
> practice.

Yeah, right. So I would suggest the following:

     git cat-file commit HEAD >commit.out &&
     wc -c <commit.out | sed -e 's/^ *//' >expect &&
     git cat-file --use-mailmap commit HEAD >commit.out &&
     wc -c <commit.out | sed -e 's/^ *//' >>expect &&

Thanks!