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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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?

> Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxxx>
> ---
> 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

>  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
>  '
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]