Re: [PATCH 1/3] fast-export: allow dumping the refname mapping

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 19, 2020, 18:00 UTC
Message-ID: <CAPig+cQxoz1AcS51qhtBr6oaTs9KTDamofCyN3qy_EARUi7oFQ@mail.gmail.com>
In-Reply-To: <20200619174551.GA2123813@coredump.intra.peff.net>

On Fri, Jun 19, 2020 at 1:45 PM Jeff King <peff@peff.net> wrote:

Show 23 quoted lines

> On Fri, Jun 19, 2020 at 12:18:16PM -0400, Eric Sunshine wrote:
> > Hmph, that shouldn't have failed. Did you quote the $(wc -l refs)
> > invocation? Quoting it would cause it to fail.
>
> Nope (and indeed, I was wary of the issue and made sure I didn't use
> quotes). My original was:
>
> test_expect_success 'refname mapping can be dumped' '
>        [...]
>        expected_count=$(git for-each-ref | wc -l) &&
>        expected_count=$((expected_count - 1)) &&
>        test_line_count = "$expected_count" refs.out &&
>
> So I guess I did quote the variable later.  It works fine on Linux, but
> one of the osx ci jobs failed:
>
> ++ expected_count='       7'
> ++ test_line_count = '       7' refs.out
> ++ test 7 = '       7'
> ++ echo 'test_line_count: line count for refs.out !=        7'
>
> so the whitespace is eaten not when "wc" is run, but rather when the
> variable is expanded.

Not something that should be done by this series (more a left-over-bitty thing, perhaps), but this almost suggests that test_line_count() deserves a tweak to make it more robust against that sort of thing:

    test_line_count () {
        if test $# != 3
        then
            BUG "not 3 parameters to test_line_count"
        elif ! test $(wc -l <"$3") "$1" "$2"
        then
            echo "test_line_count: line count for $3 !$1 $2"
            cat "$3"
            return 1
        fi
    }

If we drop the quotes around $2 from the 'test':

    elif ! test $(wc -l <"$3") "$1" $2

then your code would have worked as expected.

My only worry about that is that a poorly written caller would get a weird and unhelpful error message:

    test_line_count = 4 4
    --> sh: test: too many arguments