Re: [PATCH v2 6/6] t9109-git-svn-props.sh: split up several pipes

On Tue, Sep 18, 2018 at 10:56 PM Matthew DeVore [off-list ref] wrote:

On Mon, Sep 17, 2018 at 6:57 PM Eric Sunshine [off-list ref] wrote:

quoted

On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore [off-list ref] wrote:

quoted

+               cmp - $3

This is just wrong. The "-" argument to 'cmp' says to read from
standard input, but there is nothing being passed to 'cmp' on standard
input anymore now that you're removed the pipe. I'm guessing that you
really meant to use "observed" here (and reverse the order of
arguments to be consistent with the expect-then-actual idiom).
Finally, since these (apparently) might be binary, you can use
test_cmp_bin() instead.

Fixed, except for the test_cmp_bin part. My understanding is that git
svn propget is supposed to be printing human-readable strings.

If so, then please use test_cmp() rather than raw 'cmp' since
test_cmp() will show the actual difference between the expected and
actual files, which can be helpful when diagnosing a failing test.

quoted

After this patch, the test is even more broken than appears at first
glance since the test body is inside double-quotes. This means that
the $1, $2, $3 inside the test_propget() function are getting expanded
_before_ the function itself is ever defined, to whatever bogus values
$1, $2, $3 hold at that point. I can't see how this could ever have
worked (except only appearing to work by pure accident).

Fixed, and here is the new test:

test_expect_success 'test propget' "
        test_propget () {
                git svn propget \$1 \$2 >actual &&
                cmp \$3 actual
        } &&

Rather than escaping "$" with backslash, a cleaner fix would be to
change the double quotes around the test body to single quotes. Those
double quotes weren't needed anyhow since there are no variable
interpolations in the body. Single quotes would make that obvious at a
glance in addition to avoiding unexpected behavior in the future (like
$1, $2, etc. being interpolated at the wrong time). Single quotes
would also make the test more idiomatic and consistent with the bulk
of other tests in the suite. If you do go the route of swapping
quotes, please be sure to mention the change in the commit message.

Thanks.