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

From: Matthew DeVore <matvore@google.com>
Date: Sep 19, 2018, 02:56 UTC
Message-ID: <CAMfpvhJ3ye_7LWaQ1abXKtMB=O1sfOz6xYN=7acrVdLOksq9eA@mail.gmail.com>
In-Reply-To: <CAPig+cT5BLu2onbuTBbZ_mMzNMkEuPk5-g2d5YKw4V6Z42Y3aQ@mail.gmail.com>

On Mon, Sep 17, 2018 at 6:57 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 10 quoted lines

>
> On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore <matvore@google.com> wrote:
> > 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 <matvore@google.com>

Show 10 quoted lines

> > 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.

Show 9 quoted lines

>
> >  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.

Show 10 quoted lines

>
> > +               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

Show 26 quoted lines

>
> > +       } &&
> > +       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.