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

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

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

quoted

t9109-git-svn-props.sh: split up several pipes

Similar to my comment about 5/6, this title talks about the mechanical
changes made by the patch but not the intent. Perhaps reword it like
this:

    t9109: avoid swallowing Git exit code upstream of a pipe

Here is my new commit description:

    t9109: don't swallow Git errors upstream of pipe

    'git ... | foo' will mask any errors or crashes in git, so split up such
    pipes.

    One testcase uses several separate pipe sequences in a row which are
    awkward to split up. Wrap the split-up pipe in a function so the
    awkwardness is not repeated.

    Signed-off-by: Matthew DeVore [off-list ref]

quoted

diff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh

@@ -190,16 +190,21 @@ EOF
+# Note we avoid using pipes in order to ensure that git exits with 0.

This new comment doesn't really add value for someone reading the
patch without knowing the history leading up to the point the comment
was added. It should probably be dropped. (The actual text of the
comment is rather confusing anyhow since avoiding pipes has nothing to
do with ensuring that git exits with 0, thus another reason why this
comment ought to be dropped.)

Yes, this comment was worded quite poorly. I've removed it since I
agree it doesn't add a lot of value.

quoted

 test_expect_success 'test propget' "
-       git svn propget svn:ignore . | cmp - prop.expect &&
+       test_propget () {
+               git svn propget $1 $2 >observed

The &&-chain is broken here, which means you're losing the exit status
from the Git command anyhow (despite the point of the patch being to
avoid losing it).

Fixed.

Also, for consistency, how about calling this "actual" rather than "observed"?

Done.

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. My
understanding is based soley on this page:
http://svnbook.red-bean.com/en/1.6/svn.ref.svn.c.propget.html

quoted

+       } &&
+       test_propget svn:ignore . prop.expect &&
        cd deeply &&
-       git svn propget svn:ignore . | cmp - ../prop.expect &&
-       git svn propget svn:entry:committed-rev nested/directory/.keep \
-         | cmp - ../prop2.expect &&
-       git svn propget svn:ignore .. | cmp - ../prop.expect &&
-       git svn propget svn:ignore nested/ | cmp - ../prop.expect &&
-       git svn propget svn:ignore ./nested | cmp - ../prop.expect &&
-       git svn propget svn:ignore .././deeply/nested | cmp - ../prop.expect
+       test_propget svn:ignore . ../prop.expect &&
+       test_propget svn:entry:committed-rev nested/directory/.keep \
+               ../prop2.expect &&
+       test_propget svn:ignore .. ../prop.expect &&
+       test_propget svn:ignore nested/ ../prop.expect &&
+       test_propget svn:ignore ./nested ../prop.expect &&
+       test_propget svn:ignore .././deeply/nested ../prop.expect
        "

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
        } &&
        test_propget svn:ignore . prop.expect &&
        cd deeply &&
        test_propget svn:ignore . ../prop.expect &&
        test_propget svn:entry:committed-rev nested/directory/.keep \
                ../prop2.expect &&
        test_propget svn:ignore .. ../prop.expect &&
        test_propget svn:ignore nested/ ../prop.expect &&
        test_propget svn:ignore ./nested ../prop.expect &&
        test_propget svn:ignore .././deeply/nested ../prop.expect
        "

I confirmed that git's exit code is being checked by putting "!" in
front of git and making sure the test failed. I made sure that
"actual" was actually being compared and the exit code of "cmp" was
checked by adding "echo foo >> actual &&" before cmp, and again making
sure the test failed. This test should be well-formed now.