Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 27, 2016, 03:07 UTC
Message-ID: <CAPig+cRQ5hOLi3539Emq=YexHRXqdesf+Yo2cY+NkiPO_9Vhyg@mail.gmail.com>
In-Reply-To: <01020153b478cf9b-1417ac08-005c-4d72-91f1-0b07f00a24d5-000000@eu-west-1.amazonses.com>

On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

> So that we can see how many diffs were contained in the message and use
> them in individual tests where ever it is required. Also use
> write_script() to create the fake "editor".

It is important to explain *why* you want to be able to count how many diffs were in the editor message. In particular, you will be adding new tests in a subsequent patch which will expect a specific number of diffs (rather than any number of diffs)

Also, you need to explain why you're changing the existing tests to count the number of diffs. Is it simply "because you can" or is it because you suspect that a change you're making in a subsequent patch might accidentally cause too many diffs to show up in the existing test cases?

Show 8 quoted lines

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> @@ -3,9 +3,11 @@
> -cat >check-for-diff <<EOF
> -#!$SHELL_PATH
> -exec grep '^diff --git' "\$1"
> +write_script "check-for-diff" <<'EOF' &&

The 'EOF' is more commonly written as \EOF in Git test scripts.

> +! test -s out ||
> +rm out &&

Why not just "rm -f out"? But, more importantly, why do you need to remove the file at all? The '>' redirection operator (used below) will overwrite the file, so no need to remove it beforehand.

> +! grep '^diff --git' "$1" ||
> +grep '^diff --git' "$1" >out

Um, what? Why two greps? I would have expected you to simply re-use the existing grep (minus the backslash) while adding the redirection:

    -exec grep '^diff --git' "\$1"
    +exec grep '^diff --git' "$1" >out

Am I missing something obvious?

>  EOF
>  chmod +x check-for-diff

Mentioned previously[1]: Drop the 'chmod' since write_script() does it for you.

[1]: http://article.gmane.org/gmane.comp.version-control.git/289832

Show 25 quoted lines

>  test_set_editor "$PWD/check-for-diff"
> @@ -23,7 +25,8 @@ test_expect_success 'setup' '
>  test_expect_success 'initial commit shows verbose diff' '
> -       git commit --amend -v
> +       git commit --amend -v &&
> +       test_line_count = 1 out
>  '
>
>  test_expect_success 'second commit' '
> @@ -39,13 +42,15 @@ check_message() {
>
>  test_expect_success 'verbose diff is stripped out' '
>         git commit --amend -v &&
> -       check_message message
> +       check_message message &&
> +       test_line_count = 1 out
>  '
>
>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
>         git config diff.mnemonicprefix true &&
>         git commit --amend -v &&
> -       check_message message
> +       check_message message &&
> +       test_line_count = 1 out
>  '