[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 <pranit.bauva@xxxxxxxxx> wrote: > On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: >> 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? > > 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." > >>> 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. > > 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? > >>> +! 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. > >>> 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 > >>> 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