From: Matthew DeVore <matvore@google.com>
To: sunshine@sunshineco.com
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
Jonathan Tan <jonathantanmy@google.com>,
Junio C Hamano <gitster@pobox.com>,
Jonathan Nieder <jrn@google.com>
Subject: Re: [PATCH v2 6/6] t9109-git-svn-props.sh: split up several pipes
Date: Tue, 18 Sep 2018 19:56:29 -0700 [thread overview]
Message-ID: <CAMfpvhJ3ye_7LWaQ1abXKtMB=O1sfOz6xYN=7acrVdLOksq9eA@mail.gmail.com> (raw)
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:
>
> 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>
> > 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.
>
> > 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.
>
> > + 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
>
> > + } &&
> > + 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.
next prev parent reply other threads:[~2018-09-19 2:56 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-15 0:02 [PATCH v1 0/2] Cleanup tests for test_cmp argument ordering and "|" placement Matthew DeVore
2018-09-15 0:02 ` [PATCH v1 1/2] t/*: fix pipe placement and remove \'s Matthew DeVore
2018-09-17 16:31 ` Jonathan Nieder
2018-09-17 21:47 ` Matthew DeVore
2018-09-15 0:02 ` [PATCH v1 2/2] t/*: fix ordering of expected/observed arguments Matthew DeVore
2018-09-17 12:56 ` Matthew DeVore
2018-09-15 15:55 ` [PATCH v1 0/2] Cleanup tests for test_cmp argument ordering and "|" placement Junio C Hamano
2018-09-17 22:24 ` [PATCH v2 0/6] Clean up tests for test_cmp arg ordering and pipe placement Matthew DeVore
2018-09-17 22:24 ` [PATCH v2 4/6] tests: Add linter check for pipe placement style Matthew DeVore
2018-09-18 0:34 ` Eric Sunshine
2018-09-19 17:39 ` Matthew DeVore
[not found] ` <cover.1537223021.git.matvore@google.com>
2018-09-17 22:24 ` [PATCH v2 1/6] CodingGuidelines: add shell piping guidelines Matthew DeVore
2018-09-18 0:16 ` Eric Sunshine
2018-09-19 2:11 ` Matthew DeVore
2018-09-19 12:36 ` Eric Sunshine
2018-09-19 14:24 ` Junio C Hamano
2018-09-19 20:06 ` Matthew DeVore
2018-09-17 22:24 ` [PATCH v2 2/6] tests: standardize pipe placement Matthew DeVore
2018-09-17 22:24 ` [PATCH v2 3/6] t/*: fix ordering of expected/observed arguments Matthew DeVore
2018-09-17 22:24 ` [PATCH v2 4/6] tests: add linter check for pipe placement style Matthew DeVore
2018-09-17 22:24 ` [PATCH v2 5/6] tests: split up pipes Matthew DeVore
2018-09-18 1:30 ` Eric Sunshine
2018-09-19 2:33 ` Matthew DeVore
2018-09-17 22:24 ` [PATCH v2 6/6] t9109-git-svn-props.sh: split up several pipes Matthew DeVore
2018-09-18 1:56 ` Eric Sunshine
2018-09-19 2:56 ` Matthew DeVore [this message]
2018-09-19 12:50 ` Eric Sunshine
2018-09-19 18:43 ` Matthew DeVore
2018-09-21 1:43 ` [PATCH v3 0/5] Clean up tests for test_cmp arg ordering and pipe placement Matthew DeVore
2018-09-21 1:43 ` [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines Matthew DeVore
2018-09-21 2:06 ` Eric Sunshine
2018-09-21 21:03 ` Matthew DeVore
2018-09-24 21:03 ` SZEDER Gábor
2018-09-25 21:58 ` Matthew DeVore
2018-09-27 21:18 ` SZEDER Gábor
2018-10-01 23:07 ` Matthew DeVore
2018-09-21 1:43 ` [PATCH v3 2/5] tests: standardize pipe placement Matthew DeVore
2018-09-21 1:43 ` [PATCH v3 3/5] t/*: fix ordering of expected/observed arguments Matthew DeVore
2018-09-21 1:43 ` [PATCH v3 4/5] tests: don't swallow Git errors upstream of pipes Matthew DeVore
2018-09-21 1:43 ` [PATCH v3 5/5] t9109: " Matthew DeVore
2018-10-03 16:25 ` [PATCH v4 0/7] Clean up tests for test_cmp arg ordering and pipe placement Matthew DeVore
2018-10-03 16:25 ` [PATCH v4 1/7] t/README: reformat Do, Don't, Keep in mind lists Matthew DeVore
2018-10-05 6:15 ` Junio C Hamano
2018-10-05 14:57 ` Matthew DeVore
2018-10-03 16:26 ` [PATCH v4 2/7] Documentation: add shell guidelines Matthew DeVore
2018-10-05 16:48 ` Junio C Hamano
2018-10-05 18:21 ` Matthew DeVore
2018-10-03 16:26 ` [PATCH v4 3/7] tests: standardize pipe placement Matthew DeVore
2018-10-03 16:26 ` [PATCH v4 4/7] t/*: fix ordering of expected/observed arguments Matthew DeVore
2018-10-03 16:26 ` [PATCH v4 5/7] tests: don't swallow Git errors upstream of pipes Matthew DeVore
2018-10-05 16:48 ` Junio C Hamano
2018-10-05 17:54 ` Matthew DeVore
2018-10-05 18:12 ` Matthew DeVore
2018-10-03 16:26 ` [PATCH v4 6/7] t9109: " Matthew DeVore
2018-10-03 16:26 ` [PATCH v4 7/7] tests: order arguments to git-rev-list properly Matthew DeVore
2018-10-03 16:33 ` Matthew DeVore
2018-10-05 8:16 ` Junio C Hamano
2018-10-05 21:54 ` [PATCH v5 0/7] subject: Clean up tests for test_cmp arg ordering and pipe placement Matthew DeVore
2018-10-05 21:54 ` [PATCH v5 1/7] t/README: reformat Do, Don't, Keep in mind lists Matthew DeVore
2018-10-05 21:54 ` [PATCH v5 2/7] Documentation: add shell guidelines Matthew DeVore
2018-10-05 21:54 ` [PATCH v5 3/7] tests: standardize pipe placement Matthew DeVore
2018-10-05 21:54 ` [PATCH v5 4/7] t/*: fix ordering of expected/observed arguments Matthew DeVore
2018-10-05 21:54 ` [PATCH v5 5/7] tests: don't swallow Git errors upstream of pipes Matthew DeVore
2018-10-05 21:54 ` [PATCH v5 6/7] t9109: " Matthew DeVore
2018-10-05 21:54 ` [PATCH v5 7/7] tests: order arguments to git-rev-list properly Matthew DeVore
2018-10-06 23:53 ` [PATCH v5 0/7] subject: Clean up tests for test_cmp arg ordering and pipe placement Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMfpvhJ3ye_7LWaQ1abXKtMB=O1sfOz6xYN=7acrVdLOksq9eA@mail.gmail.com' \
--to=matvore@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=jrn@google.com \
--cc=peff@peff.net \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).