From: Matthew DeVore <hidden> Date: 2018-09-17 22:25:16
A test 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 <redacted>
---
t/t9101-git-svn-props.sh | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
@@ -190,16 +190,21 @@ EOF# This test can be improved: since all the svn:ignore contain the same# pattern, it can pass even though the propget did not execute on the# right directory.+# Note we avoid using pipes in order to ensure that git exits with 0. test_expect_success'test propget'"-gitsvnpropgetsvn:ignore.|cmp-prop.expect&&+test_propget(){+gitsvnpropget$1$2>observed+cmp-$3+}&&+test_propgetsvn:ignore.prop.expect&&cddeeply&&-gitsvnpropgetsvn:ignore.|cmp-../prop.expect&&-gitsvnpropgetsvn:entry:committed-revnested/directory/.keep\-|cmp-../prop2.expect&&-gitsvnpropgetsvn:ignore..|cmp-../prop.expect&&-gitsvnpropgetsvn:ignorenested/|cmp-../prop.expect&&-gitsvnpropgetsvn:ignore./nested|cmp-../prop.expect&&-gitsvnpropgetsvn:ignore.././deeply/nested|cmp-../prop.expect+test_propgetsvn:ignore.../prop.expect&&+test_propgetsvn:entry:committed-revnested/directory/.keep\+../prop2.expect&&+test_propgetsvn:ignore..../prop.expect&&+test_propgetsvn:ignorenested/../prop.expect&&+test_propgetsvn:ignore./nested../prop.expect&&+test_propgetsvn:ignore.././deeply/nested../prop.expect" cat>prop.expect<<\EOF
From: Eric Sunshine <hidden> Date: 2018-09-18 01:57:10
On Mon, Sep 17, 2018 at 6:25 PM Matthew DeVore [off-list ref] 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
quoted hunk
A test 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 <redacted>
---
@@ -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.)
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).
Also, for consistency, how about calling this "actual" rather than "observed"?
+ 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.
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).
From: Matthew DeVore <hidden> Date: 2018-09-19 02:56:44
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]
@@ -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.
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.
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.
From: Eric Sunshine <hidden> Date: 2018-09-19 12:50:35
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.
From: Matthew DeVore <hidden> Date: 2018-09-19 18:43:20
On Wed, Sep 19, 2018 at 5:50 AM Eric Sunshine [off-list ref] wrote:
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.
All the other testcases in this file use "cmp" and I would prefer to
maintain the consistency in this file. Note that the non-stylistic
fixes (breaking up pipes) in this test were originally not the goal of
this patchset, so changing this entire file to use test_cmp (or even
just a single testcase) seems like it's just getting even farther away
from the original goal of making pipe placement consistent in the code
base.
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.
Done. I thought of that earlier and thought that I should use double
quotes for consistency with the surrounding code, but then I saw that
there were testcases farther away in the same file that used single
quotes.
Here is the new commit message:
t9109: don't swallow Git errors upstream of pipes
'git ... | foo' will mask any errors or crashes in git, so split up such
pipes in this file.
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. Also change that testcase's surrounding
quotes from double to single to avoid premature string interpolation.
Signed-off-by: Matthew DeVore <redacted>