| Message ID | e01b719de662f0b150f78b5a6ab6ccfce9c675fa.1537223021.git.matvore@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] CodingGuidelines: add shell piping guidelines | expand |
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 > 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 <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.) > 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). 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. > + } && > + 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).
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.
On Tue, Sep 18, 2018 at 10:56 PM Matthew DeVore <matvore@google.com> wrote: > 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: > > > + 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. > > 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.
On Wed, Sep 19, 2018 at 5:50 AM Eric Sunshine <sunshine@sunshineco.com> 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 <matvore@google.com> > > Thanks. Thank you!
diff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh index 8cba331fc..952bd4814 100755 --- a/t/t9101-git-svn-props.sh +++ b/t/t9101-git-svn-props.sh @@ -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' " - git svn propget svn:ignore . | cmp - prop.expect && + test_propget () { + git svn propget $1 $2 >observed + cmp - $3 + } && + 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 " cat >prop.expect <<\EOF
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 <matvore@google.com> --- t/t9101-git-svn-props.sh | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)