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>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [GSoC][PATCH v4] test: avoid pipes in git related commands for test
Date: Sun, 25 Mar 2018 04:37:54 -0400
Message-ID: <CAPig+cS3GjYo+5C_W6WqzK3RP=W+918E6Cz=FSvHky6EWCEZPA@mail.gmail.com> (raw)
In-Reply-To: <20180323150150.31186-1-predatoramigo@gmail.com>

On Fri, Mar 23, 2018 at 11:01 AM, Pratik Karki <predatoramigo@gmail.com> wrote:
> I hope this follow-on patch[1] is ready for merge.

This iteration appears to address review comments from the last few
rounds, however, see below for a few new ones...

> 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 and apply the downstream command(s) to that file.
>
> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -840,8 +840,8 @@ test_expect_success C_LOCALE_OUTPUT 'fetch aligned output' '
>         test_commit looooooooooooong-tag &&
>         (
>                 cd full-output &&
> -               git -c fetch.output=full fetch origin 2>&1 | \
> -                       grep -e "->" | cut -c 22- >../actual
> +               git -c fetch.output=full fetch origin >actual2 2>&1 &&
> +               grep -e "->" actual2 | cut -c 22- >../actual

The file "actual2" is clearly distinct from the file "../actual", so
inventing a name ("actual2") isn't particularly helping; you could
just as easily also name it "actual" without hurting comprehension.
(Not necessarily worth a re-roll.)

>         ) &&
> @@ -855,8 +855,8 @@ test_expect_success C_LOCALE_OUTPUT 'fetch compact output' '
>         test_commit extraaa &&
>         (
>                 cd compact &&
> -               git -c fetch.output=compact fetch origin 2>&1 | \
> -                       grep -e "->" | cut -c 22- >../actual
> +               git -c fetch.output=compact fetch origin >actual2 2>&1 &&
> +               grep -e "->" actual2 | cut -c 22- >../actual

Same comment.

>         ) &&
> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> @@ -74,11 +74,12 @@ test_expect_success 'iso-8859-1' '
>         git commit -s -m den file &&
> -       git fast-export wer^..wer |
> -               sed "s/wer/i18n/" |
> +       git fast-export wer^..wer >actual &&
> +       sed "s/wer/i18n/" actual |
>                 (cd new &&
>                  git fast-import &&
> -                git cat-file commit i18n | grep "Áéí óú")
> +                git cat-file commit i18n >actual &&
> +                grep "Áéí óú" actual)

It was a bit surprising to see a new "actual" file created inside the
subshell even as 'sed' is processing a file named "actual" outside the
subshell, and, as a reader, I was concerned about bad interaction
between the operations. However, the file in the subshell is really
"new/actual", thus is distinct from the other "actual", so it's okay.

This is one of those cases, however, in which it might make sense to
give the files different names to make the code easier to grok, so
future readers don't stumble over this as well. For instance, the
outer file could be named "iso8859-1.fi" (or something), and the file
in the subshell can remain "actual". Not itself worth a re-roll, but
probably a good idea.

(This differs in couple ways from my comment above about t5510 tests
naming files "actual2" and "../actual". In that case, it was quite
clear that, within the cd'd subshell, file "../actual" was distinct
from the file created within the cd'd directory, so no confusion.
Moreover, those files were not being accessed at the same time,
whereas in this t9350 test, the 'sed' is reading from the a file at
the same time as 'git cat-file' is outputting to a similarly named
file, which is potentially confusing and requires extra brain cycles
to sort out.)

>  '
> @@ -87,18 +88,16 @@ test_expect_success 'import/export-marks' '
>         test_line_count = 3 tmp-marks &&
> -       test $(
> -               git fast-export --import-marks=tmp-marks\
> -               --export-marks=tmp-marks HEAD |
> -               grep ^commit |
> +       git fast-export --import-marks=tmp-marks \
> +               --export-marks=tmp-marks HEAD >actual &&
> +       test $(grep ^commit actual |
>                 wc -l) \
>         -eq 0 &&

Since the git-fast-export invocation has been pulled out of the
$(...), the entire 'test' expression is now short enough to fit easily
on one line. Making such a change would improve readability
considering how hard it is to read split over three lines like that
(with inconsistent indentation, moreover):

    test $(grep ^commit actual | wc -l) -eq 0 &&

>         echo change > file &&
>         git commit -m "last commit" file &&
> -       test $(
> -               git fast-export --import-marks=tmp-marks \
> -               --export-marks=tmp-marks HEAD |
> -               grep ^commit\  |
> +       git fast-export --import-marks=tmp-marks \
> +               --export-marks=tmp-marks HEAD >actual &&
> +       test $(grep ^commit\  actual |
>                 wc -l) \
>         -eq 1 &&

Same comment.

>         test_line_count = 4 tmp-marks
> @@ -500,13 +501,13 @@ test_expect_success 'refs are updated even if no commits need to be exported' '
>         git fast-export --import-marks=tmp-marks \
>                 --export-marks=tmp-marks master > /dev/null &&
>         git fast-export --import-marks=tmp-marks \
> -               --export-marks=tmp-marks master > actual &&
> +               --export-marks=tmp-marks master >actual &&

This change is unrelated to the purpose of this patch, thus is noise
which distracts reviewers from real changes. Fixing style problems in
code you're touching is fine (and usually recommended), however, this
code is outside the scope of what the patch should be touching (there
is no piping output of a git command here). Moreover, it doesn't make
sense to fix only "> actual" but not "> /dev/null" just above it.
Consequently, this change should be dropped from the patch.

>         test_cmp expected actual
>  '

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-13 20:19 [GSoC] [PATCH] test: avoid pipes in git related commands for test suite Pratik Karki
2018-03-14  7:30 ` Eric Sunshine
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 [this message]
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+cS3GjYo+5C_W6WqzK3RP=W+918E6Cz=FSvHky6EWCEZPA@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --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