git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Pratik Karki <predatoramigo@gmail.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
Date: Wed, 14 Mar 2018 03:30:11 -0400
Message-ID: <CAPig+cRPzyw525ODC4=-E7w=zbpbhVN2eqxSYDSLij5wfW8S_A@mail.gmail.com> (raw)
In-Reply-To: <20180313201945.8409-1-predatoramigo@gmail.com>

Thanks for the patch. See comments below...

On Tue, Mar 13, 2018 at 4:19 PM, Pratik Karki <predatoramigo@gmail.com> wrote:
> This patch removes the necessity of pipes in git related commands for test suite.
>
> Exit code of the upstream in a pipe is ignored so, it's use should be avoided. The fix for this is to write the output of the git command to a file and test the exit codes of both the commands being linked by pipe.

Please wrap commit messages to fit in about 72 columns; this one is
far too wide.

On the Git project, commit messages are written in imperative mood, as
if telling the codebase to "do something". So, instead of writing
"This patch removes...", you could word it "Remove..." or "Avoid...".

It's misleading to say that the patch "removes the _necessity_ of
pipes" since pipes were not used out of necessity; they were probably
just a convenience and seemed reasonable at the time, but later
experience has shown that they can be problematic for the reason you
give in the second paragraph.

Taking these observations into consideration, perhaps you could
rewrite the commit message something like this:

    Avoid using pipes downstream of Git commands since the exit codes
    of commands upstream of pipes get swallowed, thus potentially
    hiding failure of those commands. Instead, capture Git command
    output to a file apply the downstream command(s) to that file.

More comments below...

> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> @@ -116,10 +116,10 @@ test_expect_success \
>  test_expect_success \
>      'checking the commit' \
> -    'git diff-tree -r -M --name-status  HEAD^ HEAD | \
> -     grep "^R100..*path0/COPYING..*path2/COPYING" &&
> -     git diff-tree -r -M --name-status  HEAD^ HEAD | \
> -     grep "^R100..*path0/README..*path2/README"'
> +    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
> +     grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
> +     git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
> +     grep "^R100..*path0/README..*path2/README" actual'

Although this "mechanical" transformation is technically correct, it
is nevertheless wasteful. The exact same "git diff-tree ..." command
is run twice, and both times output is captured to file 'actual',
which makes the second invocation superfluous. Instead, a better
transformation would be:

    git diff-tree ... >actual &&
    grep ... actual &&
    grep ... actual

The same observation applies to other transformations in this patch.

> diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
> @@ -204,8 +204,8 @@ test_expect_success "follow-parent is atomic" '
>  test_expect_success "track multi-parent paths" '
>         svn_cmd cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
>         git svn multi-fetch &&
> -       test $(git cat-file commit refs/remotes/glob | \
> -              grep "^parent " | wc -l) -eq 2
> +       test $(git cat-file commit refs/remotes/glob >actual &&
> +              grep "^parent " actual | wc -l) -eq 2
>         '

This is not a great transformation. If "git cat-file" fails, then
neither 'grep' nor 'wc' will run, and the result will be as if 'test'
was called without an argument before "-eq". For example:

    % test $(false >actual && grep "^parent " actual | wc -l) -eq 2
    test: -eq: unary operator expected

It would be better to run "git cat-file" outside of "test $(...)". For instance:

    git cat-file ... >actual &&
    test $(grep ... actual | wc -l) -eq 2

Alternately, you could take advantage of the test_line_count() helper function:

    git cat-file ... >actual &&
    grep ... actual >actual2 &&
    test_line_count = 2 actual2

> diff --git a/t/t9110-git-svn-use-svm-props.sh b/t/t9110-git-svn-use-svm-props.sh
> @@ -21,32 +21,32 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
>  test_expect_success 'verify metadata for /bar' "
> -       git cat-file commit refs/remotes/bar | \
> -          grep '^git-svn-id: $bar_url@12 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~1 | \
> -          grep '^git-svn-id: $bar_url@11 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~2 | \
> -          grep '^git-svn-id: $bar_url@10 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~3 | \
> -          grep '^git-svn-id: $bar_url@9 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~4 | \
> -          grep '^git-svn-id: $bar_url@6 $uuid$' &&
> -       git cat-file commit refs/remotes/bar~5 | \
> -          grep '^git-svn-id: $bar_url@1 $uuid$'
> +       git cat-file commit refs/remotes/bar >actual &&
> +          grep '^git-svn-id: $bar_url@12 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~1 >actual &&
> +          grep '^git-svn-id: $bar_url@11 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~2 >actual &&
> +          grep '^git-svn-id: $bar_url@10 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~3 >actual &&
> +          grep '^git-svn-id: $bar_url@9 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~4 >actual &&
> +          grep '^git-svn-id: $bar_url@6 $uuid$' actual &&
> +       git cat-file commit refs/remotes/bar~5 >actual &&
> +          grep '^git-svn-id: $bar_url@1 $uuid$' actual
>         "

An indented line in the original shows that it is a continuation of
the preceding line. However, in the revised code, that is not so, thus
it probably makes sense to drop the indentation.

The same comment applies to several additional cases snipped from this reply.

> diff --git a/t/t9114-git-svn-dcommit-merge.sh b/t/t9114-git-svn-dcommit-merge.sh
> index 50bca62de..c945c3758 100755
> --- a/t/t9114-git-svn-dcommit-merge.sh
> +++ b/t/t9114-git-svn-dcommit-merge.sh
> @@ -68,7 +68,7 @@ test_debug 'gitk --all & sleep 1'
>  test_expect_success 'verify pre-merge ancestry' "
>         test x\$(git rev-parse --verify refs/heads/svn^2) = \
>              x\$(git rev-parse --verify refs/heads/merge) &&
> -       git cat-file commit refs/heads/svn^ | grep '^friend$'
> +       git cat-file commit refs/heads/svn^ >actual && grep '^friend$' actual
>         "

Style: split the line at the '&&'...

    git cat-file ... >actual &&
    grep ... actual

The same comment applies to another test snipped from this reply.

Aside: The current patch wants to solve the problem of exit code being
swallowed down pipes, however, this and other tests are afflicted by a
similar problem with $(...) also swallowing the exit code. A failure
of "git rev-parse" could potentially go unnoticed inside "test
x$(...)". Fixing that is outside the scope of the current patch,
however, a follow-on patch to fix that problem (if you feel so
inclined) might transform it something like this:

    git rev-parse ... >rev1 &&
    git rev-parse ... >rev2 &&
    test_cmp rev1 rev2 &&

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-13 20:19 Pratik Karki
2018-03-14  7:30 ` Eric Sunshine [this message]
2018-03-14  9:57   ` Ævar Arnfjörð Bjarmason
2018-03-14 18:22     ` Eric Sunshine
2018-03-15 17:04       ` Junio C Hamano
2018-03-19 17:32         ` [GSoC][PATCH] " Pratik Karki
2018-03-21 11:02           ` Eric Sunshine
2018-03-21 15:23             ` [GSoC][PATCH v3] test: avoid pipes in git related commands for test Pratik Karki
2018-03-21 18:11               ` Junio C Hamano
2018-03-21 18:45                 ` Eric Sunshine
2018-03-21 18:58                 ` Eric Sunshine
2018-03-23 15:01                   ` [GSoC][PATCH v4] " Pratik Karki
2018-03-25  8:37                     ` Eric Sunshine
2018-03-27 17:31                       ` [GSoC][PATCH v5] " Pratik Karki
2018-03-30 21:45                         ` Eric Sunshine
2018-03-30 22:08                           ` Junio C Hamano

Reply instructions:

You may reply publically 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='CAPig+cRPzyw525ODC4=-E7w=zbpbhVN2eqxSYDSLij5wfW8S_A@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=predatoramigo@gmail.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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox