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

[forwarding this to the list since Pranit (presumably) accidentally
replied only to me but it's relevant to the ongoing discussion]

On Sun, Mar 27, 2016 at 1:42 AM, Pranit Bauva [off-list ref] wrote:

On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva [off-list ref] wrote:

quoted

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?

Sorry for not writing commit messages properly. It is a part I am working on.
How does this paragraph sound to you in addition to the previous commit message?
"A subsequent commit will introduce a scenario where it is important
to be able to exactly determine how many diffs were present."

quoted

quoted

Signed-off-by: Pranit Bauva <redacted>
---

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.

quoted

+! 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.

I wasn't aware about '-f' option. It is important to remove the file.
I initially tried without removing the file and it caused problems.
This is because let's say the previous test had 1 diff which was
stored in out. Now, if in the next test, it is expected to have 0
diffs then grep would fail to execute and nothing will be re-written
to out and out will contain the 1 diff from previous test. An
explanation for this should be mentioned in the commit message?

quoted

quoted

+! 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

The reason of two greps in that if in some test it fails to find any
diffs, the editor will return an error. Of course we could test this
scenario by using test_must_fail as in previous patches, but I think
it would be more clearer if we could test for the existence of out
which is done in patch 3/3. I will explanation for this in commit
message.

quoted

quoted

 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

Then you mentioned that you were talking[1] about some other review.
So I thought you mean to keep as it is. I guess I misinterpreted it.
And I did not test that before. Now, I have tested that it works if we
remove chmod.
[1] : http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289832

quoted

quoted

 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
 '