git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v1 1/2] t/*: fix pipe placement and remove \'s

From
Matthew DeVore <matvore@google.com>
Date
Sep 17, 2018, 21:47 UTC
Message-ID
<CAMfpvhKTk0VJicAKO_ASxKMU99GXEAbZ7e5K35g8D3uM9tqv2g@mail.gmail.com>
In-Reply-To
<20180917163137.GB89942@aiede.svl.corp.google.com>
On Mon, Sep 17, 2018 at 9:31 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 38 quoted lines
>
> Matthew DeVore wrote:
>
> > Subject: t/*: fix pipe placement and remove \'s
> >
> > Where ever there was code in the tests like this:
> >
> >       foo \
> >               | bar
>
> Language nits:
> - s/Where ever/Wherever/
> - Git's commit messages use the present tense to describe the existing
>   previous state of the codebase, as though reporting a bug.
>
> Maybe something like
>
>         tests: standardize pipe placement
>
>         Instead of using a line-continuation and pipe on the second
>         line, take advantage of the shell's implicit line continuation
>         after a pipe character.  So for example, instead of
>
>                 some long line \
>                         | next line
>
>         use
>
>                 some long line |
>                 next line
>
> At this point, it would be useful to say something about rationale ---
> for example,
>
>         This better matches the coding style documented in
>         Documentation/CodingGuidelines and used in shell scripts
>         elsewhere in Git.
>
Done.
> Except: is this documented in Documentation/CodingGuidelines?  Or,
> better, is there a linter that we can run in the test-lint target of
> t/Makefile to ensure we keep sticking to this style?
It's not documented there, so I've created a new commit at the start
of this patchset which addresses that. I also added a commit which
adds a lint test, but it uses a questionable heuristic in order to
avoid false positives (it's hard to distinguish graphs generated with
git log --oneline since they often have "\ [newline] [tab or spaces]
|"  ). Let me know if you think it looks promising. I'd be happy to
just drop it.
Show 22 quoted lines
>
> [...]
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -57,8 +57,8 @@ then
> >               echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
> >                       --passphrase-fd 0 --pinentry-mode loopback \
> >                       --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
> > -             gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K \
> > -                     | grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
> > +             gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
> > +             grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
> >                       ${GNUPGHOME}/trustlist.txt &&
>
> I think this would be more readable with one item from the pipeline
> per line:
>
>                 gpgsm --homedir ... |
>                 grep ... |
>                 cut ... |
>                 tr ... >... &&
>
Done.
Show 15 quoted lines
> [...]
> > --- a/t/t1006-cat-file.sh
> > +++ b/t/t1006-cat-file.sh
> > @@ -218,8 +218,8 @@ test_expect_success "--batch-check for a non-existent hash" '
> >      test "0000000000000000000000000000000000000042 missing
> >  0000000000000000000000000000000000000084 missing" = \
> >      "$( ( echo 0000000000000000000000000000000000000042;
> > -         echo_without_newline 0000000000000000000000000000000000000084; ) \
> > -       | git cat-file --batch-check)"
> > +         echo_without_newline 0000000000000000000000000000000000000084; ) |
> > +       git cat-file --batch-check)"
>
> This test is problematic in a lot of ways.  Most importantly, it ignores
> the exist status from git cat-file.
>
[...]
> but unless there's a linter that we're helping support, it's probably
> better to skip this file and use a dedicated patch to modernize its
> style more generally.

Yes, the cat-file.sh test is kind of funky, and I like the style of your suggestions much better, but in this case I think that perfect is the enemy of good. Fixing everything wrong with these lines would necessitate fixing the surrounding couple of tests that also swallow up the exit code of git cat-file. This may in turn necessitate other fixes for consistency that may even spread to other files... I am basing my argument here on what's in Documentation/CodingGuidelines, which indicates that minor stylistic nits that result in code churn are not recommended, and that we must be consistent with the surrounding code. The surrounding code here looks for the most part like:

 test "asdf" = $(echo "asdf" | git foo-bar)

Which I think is satisfactory in its own context. You asked me to fix other test files later on, which I did, since they didn't seem to have such a contrarian style, so the fixes were very localized, and I was already editing many lines in those files already.

Show 17 quoted lines
>
> [...]
> > --- a/t/t5317-pack-objects-filter-objects.sh
> > +++ b/t/t5317-pack-objects-filter-objects.sh
> > @@ -20,17 +20,20 @@ test_expect_success 'setup r1' '
> >  '
> >
> >  test_expect_success 'verify blob count in normal packfile' '
> > -     git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
> > -             | awk -f print_2.awk \
> > -             | sort >expected &&
> > +     git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
> > +     awk -f print_2.awk |
> > +     sort >expected &&
>
> This loses the exit status from git, so we should make it write to a
> temporary file instead (as a separate patch).
Fixed.
Show 10 quoted lines
>
> [...]
> > -     git -C r1 verify-pack -v ../all.pack \
> > -             | grep blob \
> > -             | awk -f print_1.awk \
> > -             | sort >observed &&
> > +
> > +     git -C r1 verify-pack -v ../all.pack |
>
> Likewise (and likewise for the rest in this file).
Fixed this file's exit code issues in a separate patch.
Show 22 quoted lines
>
> [...]
> > --- a/t/t5500-fetch-pack.sh
> > +++ b/t/t5500-fetch-pack.sh
> > @@ -50,8 +50,9 @@ pull_to_client () {
> >                       case "$heads" in *B*)
> >                           git update-ref refs/heads/B "$BTIP";;
> >                       esac &&
> > -                     git symbolic-ref HEAD refs/heads/$(echo $heads \
> > -                             | sed -e "s/^\(.\).*$/\1/") &&
> > +
> > +                     git symbolic-ref HEAD refs/heads/$(echo $heads |
> > +                     sed -e "s/^\(.\).*$/\1/") &&
>
> It would be better to use a temporary variable.  If we're just
> changing line wrapping, then this would be
>
>                         git symbolic-ref HAD refs/heads/$(
>                                 echo $heads |
>                                 sed ...
>                         ) &&
>
Fixed using your suggestion (only fixed the line wrapping)
Show 16 quoted lines
> [...]
> > --- a/t/t5616-partial-clone.sh
> > +++ b/t/t5616-partial-clone.sh
> > @@ -34,10 +34,12 @@ test_expect_success 'setup bare clone for server' '
> >  # confirm partial clone was registered in the local config.
> >  test_expect_success 'do partial clone 1' '
> >       git clone --no-checkout --filter=blob:none "file://$(pwd)/srv.bare" pc1 &&
> > -     git -C pc1 rev-list HEAD --quiet --objects --missing=print \
> > -             | awk -f print_1.awk \
> > -             | sed "s/?//" \
> > -             | sort >observed.oids &&
> > +
> > +     git -C pc1 rev-list HEAD --quiet --objects --missing=print |
>
> Also needs to write to a temporary to avoid losing the exist status
> (and likewise for the rest of this file).

Done in a separate patch, although I didn't do this for pipes inside of $( ) and for a trivial "git rev-parse HEAD".

Show 20 quoted lines
>
> [...]
> > --- a/t/t6112-rev-list-filters-objects.sh
> > +++ b/t/t6112-rev-list-filters-objects.sh
> > @@ -20,24 +20,28 @@ test_expect_success 'setup r1' '
> >  '
> >
> >  test_expect_success 'verify blob:none omits all 5 blobs' '
> > -     git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \
> > -             | awk -f print_2.awk \
> > -             | sort >expected &&
> > -     git -C r1 rev-list HEAD --quiet --objects --filter-print-omitted --filter=blob:none \
> > -             | awk -f print_1.awk \
> > -             | sed "s/~//" \
> > -             | sort >observed &&
> > +     git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 |
> > +     awk -f print_2.awk |
> > +     sort >expected &&
>
> Likewise.
Fixed this file.
Show 14 quoted lines
>
> [...]
> > --- a/t/t9101-git-svn-props.sh
> > +++ b/t/t9101-git-svn-props.sh
> > @@ -193,8 +193,8 @@ test_expect_success 'test propget' "
> >       git svn propget svn:ignore . | cmp - 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:entry:committed-rev nested/directory/.keep |
> > +     cmp - ../prop2.expect &&
>
> Likewise.

Fixed this section, including the one earlier in this file. This section is not a trivial change, so I put it in a different commit.

>
> Thanks and hope that helps,
> Jonathan
I will send an updated patchset shortly.
Previous: Jonathan NiederNext: Matthew DeVore
Message 4 of 66 in “Cleanup tests for test_cmp argument ordering and "|" placement”
  1. 0/2 Cleanup tests for test_cmp argument ordering and "|" placementMatthew DeVore, Sep 15, 2018
  2. 1/2 t/*: fix pipe placement and remove \'sMatthew DeVore, Sep 15, 2018
  3. Jonathan NiederSep 17, 2018
  4. Matthew DeVoreSep 17, 2018
  5. 2/2 t/*: fix ordering of expected/observed argumentsMatthew DeVore, Sep 15, 2018
  6. Matthew DeVoreSep 17, 2018
  7. Junio C HamanoSep 15, 2018
  8. 0/6 Clean up tests for test_cmp arg ordering and pipe placementMatthew DeVore, Sep 17, 2018
  9. 4/6 tests: Add linter check for pipe placement styleMatthew DeVore, Sep 17, 2018
  10. Eric SunshineSep 18, 2018
  11. Matthew DeVoreSep 19, 2018
  12. 0/5 Clean up tests for test_cmp arg ordering and pipe placementMatthew DeVore, Sep 21, 2018
  13. 1/5 CodingGuidelines: add shell piping guidelinesMatthew DeVore, Sep 21, 2018
  14. Eric SunshineSep 21, 2018
  15. Matthew DeVoreSep 21, 2018
  16. SZEDER GáborSep 24, 2018
  17. Matthew DeVoreSep 25, 2018
  18. SZEDER GáborSep 27, 2018
  19. Matthew DeVoreOct 1, 2018
  20. 2/5 tests: standardize pipe placementMatthew DeVore, Sep 21, 2018
  21. 3/5 t/*: fix ordering of expected/observed argumentsMatthew DeVore, Sep 21, 2018
  22. 4/5 tests: don't swallow Git errors upstream of pipesMatthew DeVore, Sep 21, 2018
  23. 5/5 t9109: don't swallow Git errors upstream of pipesMatthew DeVore, Sep 21, 2018
  24. 0/7 Clean up tests for test_cmp arg ordering and pipe placementMatthew DeVore, Oct 3, 2018
  25. 1/7 t/README: reformat Do, Don't, Keep in mind listsMatthew DeVore, Oct 3, 2018
  26. Junio C HamanoOct 5, 2018
  27. Matthew DeVoreOct 5, 2018
  28. 2/7 Documentation: add shell guidelinesMatthew DeVore, Oct 3, 2018
  29. Junio C HamanoOct 5, 2018
  30. Matthew DeVoreOct 5, 2018
  31. 3/7 tests: standardize pipe placementMatthew DeVore, Oct 3, 2018
  32. 4/7 t/*: fix ordering of expected/observed argumentsMatthew DeVore, Oct 3, 2018
  33. 5/7 tests: don't swallow Git errors upstream of pipesMatthew DeVore, Oct 3, 2018
  34. Junio C HamanoOct 5, 2018
  35. Matthew DeVoreOct 5, 2018
  36. Matthew DeVoreOct 5, 2018
  37. 6/7 t9109: don't swallow Git errors upstream of pipesMatthew DeVore, Oct 3, 2018
  38. 7/7 tests: order arguments to git-rev-list properlyMatthew DeVore, Oct 3, 2018
  39. Matthew DeVoreOct 3, 2018
  40. Junio C HamanoOct 5, 2018
  41. 0/7 subject: Clean up tests for test_cmp arg ordering and pipe placementMatthew DeVore, Oct 5, 2018
  42. 1/7 t/README: reformat Do, Don't, Keep in mind listsMatthew DeVore, Oct 5, 2018
  43. 2/7 Documentation: add shell guidelinesMatthew DeVore, Oct 5, 2018
  44. 3/7 tests: standardize pipe placementMatthew DeVore, Oct 5, 2018
  45. 4/7 t/*: fix ordering of expected/observed argumentsMatthew DeVore, Oct 5, 2018
  46. 5/7 tests: don't swallow Git errors upstream of pipesMatthew DeVore, Oct 5, 2018
  47. 6/7 t9109: don't swallow Git errors upstream of pipesMatthew DeVore, Oct 5, 2018
  48. 7/7 tests: order arguments to git-rev-list properlyMatthew DeVore, Oct 5, 2018
  49. Junio C HamanoOct 6, 2018
  50. 1/6 CodingGuidelines: add shell piping guidelinesMatthew DeVore, Sep 17, 2018
  51. Eric SunshineSep 18, 2018
  52. Matthew DeVoreSep 19, 2018
  53. Eric SunshineSep 19, 2018
  54. Junio C HamanoSep 19, 2018
  55. Matthew DeVoreSep 19, 2018
  56. 2/6 tests: standardize pipe placementMatthew DeVore, Sep 17, 2018
  57. 3/6 t/*: fix ordering of expected/observed argumentsMatthew DeVore, Sep 17, 2018
  58. 4/6 tests: add linter check for pipe placement styleMatthew DeVore, Sep 17, 2018
  59. 5/6 tests: split up pipesMatthew DeVore, Sep 17, 2018
  60. Eric SunshineSep 18, 2018
  61. Matthew DeVoreSep 19, 2018
  62. 6/6 t9109-git-svn-props.sh: split up several pipesMatthew DeVore, Sep 17, 2018
  63. Eric SunshineSep 18, 2018
  64. Matthew DeVoreSep 19, 2018
  65. Eric SunshineSep 19, 2018
  66. Matthew DeVoreSep 19, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.