[PATCH 0/3] Trivial patches

[PATCH 0/3] Trivial patches

[Felipe Contreras]

Felipe Contreras (3):
  sequencer: trivial fix
  test: improve rebase -q test
  submodule: remove unnecessary check

 sequencer.c       | 7 +++++--
 submodule.c       | 5 ++---
 t/t3400-rebase.sh | 1 +
 3 files changed, 8 insertions(+), 5 deletions(-)

-- 
1.8.3.698.g079b096

[PATCH 1/3] sequencer: trivial fix

[Felipe Contreras]

We should free objects before leaving.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sequencer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ab6f8a7..7eeae2f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -626,12 +626,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		rerere(opts->allow_rerere_auto);
 	} else {
 		int allow = allow_empty(opts, commit);
-		if (allow < 0)
-			return allow;
+		if (allow < 0) {
+			res = allow;
+			goto leave;
+		}
 		if (!opts->no_commit)
 			res = run_git_commit(defmsg, opts, allow);
 	}
 
+leave:
 	free_message(&msg);
 	free(defmsg);
 
-- 
1.8.3.698.g079b096

Re: [PATCH 1/3] sequencer: trivial fix

[Junio C Hamano]

Thanks.

I thought the conclusion was that combination of c8d1351 and 706728a
we already queued was the right change.  Is this meant to replace
them?

Re: [PATCH 1/3] sequencer: trivial fix

[Felipe Contreras]

On Fri, Jun 7, 2013 at 5:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I thought the conclusion was that combination of c8d1351 and 706728a
> we already queued was the right change.  Is this meant to replace
> them?

Yes, those would do, but I'm not going to work on that series any more.

-- 
Felipe Contreras

[PATCH 2/3] test: improve rebase -q test

[Felipe Contreras]

Let's show the output so it's clear why it failed.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t3400-rebase.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b58fa1a..fb39531 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' '
 test_expect_success 'rebase -q is quiet' '
 	git checkout -b quiet topic &&
 	git rebase -q master >output.out 2>&1 &&
+	cat output.out &&
 	test ! -s output.out
 '
 
-- 
1.8.3.698.g079b096

Re: [PATCH 2/3] test: improve rebase -q test

[Duy Nguyen]

On Sat, Jun 8, 2013 at 3:32 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Let's show the output so it's clear why it failed.

I think you can always look into trash-directory.t3400 and figure why.
But if you show this, I think you should use test_cmp to make it clear
what is not wanted. Something like

: >expected &&
test_cmp expected output.out

>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t3400-rebase.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index b58fa1a..fb39531 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -185,6 +185,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' '
>  test_expect_success 'rebase -q is quiet' '
>         git checkout -b quiet topic &&
>         git rebase -q master >output.out 2>&1 &&
> +       cat output.out &&
>         test ! -s output.out
>  '
>
> --
> 1.8.3.698.g079b096
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
Duy

Re: [PATCH 2/3] test: improve rebase -q test

[Felipe Contreras]

On Fri, Jun 7, 2013 at 9:44 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Jun 8, 2013 at 3:32 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Let's show the output so it's clear why it failed.
>
> I think you can always look into trash-directory.t3400 and figure why.
> But if you show this, I think you should use test_cmp to make it clear
> what is not wanted. Something like
>
> : >expected &&
> test_cmp expected output.out

Feel free to send that patch. I'm done with this one.

-- 
Felipe Contreras

Re: [PATCH 2/3] test: improve rebase -q test

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Jun 8, 2013 at 3:32 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Let's show the output so it's clear why it failed.
>
> I think you can always look into trash-directory.t3400 and figure why.
> But if you show this, I think you should use test_cmp to make it clear
> what is not wanted. Something like
>
> : >expected &&
> test_cmp expected output.out

There are quite many places that do this "output must be empty" in
the test suite, so it may deserve a special case perhaps like this
one.

-- >8 --
Subject: [PATCH] test: test_output_must_be_empty helper

There are quite a lot places where an output file is expected to be
empty, and we fail the test when it is not.  The output from running
the test script with -i -v can be helped if we showed the unexpected
contents at that point.

We could of course do

    >expected.empty && test_cmp expected.empty actual

but this is commmon enough to be done with a dedicated helper.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0040-parse-options.sh      | 46 +++++++++++++++++++++----------------------
 t/t3400-rebase.sh             |  2 +-
 t/t3903-stash.sh              | 10 +++++-----
 t/t5521-pull-options.sh       | 26 ++++++++++++------------
 t/t5702-clone-options.sh      |  2 +-
 t/t7102-reset.sh              |  2 +-
 t/t7400-submodule-basic.sh    |  6 +++---
 t/t9402-git-cvsserver-refs.sh | 12 +++++------
 t/test-lib-functions.sh       | 12 +++++++++++
 9 files changed, 65 insertions(+), 53 deletions(-)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 244a43c..e2f5be0 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -50,7 +50,7 @@ EOF
 
 test_expect_success 'test help' '
 	test_must_fail test-parse-options -h > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_i18ncmp expect output
 '
 
@@ -75,7 +75,7 @@ check() {
 	shift &&
 	sed "s/^$what .*/$what $expect/" <expect.template >expect &&
 	test-parse-options $* >output 2>output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 }
 
@@ -86,7 +86,7 @@ check_i18n() {
 	shift &&
 	sed "s/^$what .*/$what $expect/" <expect.template >expect &&
 	test-parse-options $* >output 2>output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_i18ncmp expect output
 }
 
@@ -99,7 +99,7 @@ check_unknown() {
 	esac &&
 	cat expect.err >>expect &&
 	test_must_fail test-parse-options $* >output 2>output.err &&
-	test ! -s output &&
+	test_output_must_be_empty output &&
 	test_cmp expect output.err
 }
 
@@ -112,7 +112,7 @@ check_unknown_i18n() {
 	esac &&
 	cat expect.err >>expect &&
 	test_must_fail test-parse-options $* >output 2>output.err &&
-	test ! -s output &&
+	test_output_must_be_empty output &&
 	test_i18ncmp expect output.err
 }
 
@@ -149,7 +149,7 @@ test_expect_success 'short options' '
 	test-parse-options -s123 -b -i 1729 -b -vv -n -F my.file \
 	> output 2> output.err &&
 	test_cmp expect output &&
-	test ! -s output.err
+	test_output_must_be_empty output.err
 '
 
 cat > expect << EOF
@@ -168,7 +168,7 @@ test_expect_success 'long options' '
 	test-parse-options --boolean --integer 1729 --boolean --string2=321 \
 		--verbose --verbose --no-dry-run --abbrev=10 --file fi.le\
 		--obsolete > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
@@ -199,7 +199,7 @@ EOF
 test_expect_success 'intermingled arguments' '
 	test-parse-options a1 --string 123 b1 --boolean -j 13 -- --boolean \
 		> output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
@@ -217,13 +217,13 @@ EOF
 
 test_expect_success 'unambiguously abbreviated option' '
 	test-parse-options --int 2 --boolean --no-bo > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
 	test-parse-options --int=2 > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
@@ -246,7 +246,7 @@ EOF
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
 	test-parse-options --st 123 > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
@@ -256,7 +256,7 @@ EOF
 
 test_expect_success 'detect possible typos' '
 	test_must_fail test-parse-options -boolean > output 2> output.err &&
-	test ! -s output &&
+	test_output_must_be_empty output &&
 	test_cmp typo.err output.err
 '
 
@@ -266,7 +266,7 @@ EOF
 
 test_expect_success 'detect possible typos' '
 	test_must_fail test-parse-options -ambiguous > output 2> output.err &&
-	test ! -s output &&
+	test_output_must_be_empty output &&
 	test_cmp typo.err output.err
 '
 
@@ -285,7 +285,7 @@ EOF
 
 test_expect_success 'keep some options as arguments' '
 	test-parse-options --quux > output 2> output.err &&
-        test ! -s output.err &&
+	test_output_must_be_empty output.err &&
         test_cmp expect output
 '
 
@@ -305,7 +305,7 @@ EOF
 test_expect_success 'OPT_DATE() and OPT_SET_PTR() work' '
 	test-parse-options -t "1970-01-01 00:00:01 +0000" --default-string \
 		foo -q > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
@@ -324,7 +324,7 @@ EOF
 
 test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 	test-parse-options --length=four -b -4 > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
@@ -352,13 +352,13 @@ EOF
 
 test_expect_success 'OPT_BIT() and OPT_SET_INT() work' '
 	test-parse-options --set23 -bbbbb --no-or4 > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '
 	test-parse-options --set23 -bbbbb --neg-or4 > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
@@ -376,19 +376,19 @@ EOF
 
 test_expect_success 'OPT_BIT() works' '
 	test-parse-options -bb --or4 > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'OPT_NEGBIT() works' '
 	test-parse-options -bb --no-neg-or4 > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '
 	test-parse-options + + + + + + > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
@@ -406,7 +406,7 @@ EOF
 
 test_expect_success 'OPT_NUMBER_CALLBACK() works' '
 	test-parse-options -12345 > output 2> output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
@@ -424,7 +424,7 @@ EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
 	test-parse-options --no-ambig >output 2>output.err &&
-	test ! -s output.err &&
+	test_output_must_be_empty output.err &&
 	test_cmp expect output
 '
 
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 1de0ebd..e198419 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -179,7 +179,7 @@ test_expect_success 'default to @{upstream} when upstream arg is missing' '
 test_expect_success 'rebase -q is quiet' '
 	git checkout -b quiet topic &&
 	git rebase -q master >output.out 2>&1 &&
-	test ! -s output.out
+	test_output_must_be_empty output.out
 '
 
 test_expect_success 'Rebase a commit that sprinkles CRs in' '
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5dfbda7..ebd838a 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -200,17 +200,17 @@ test_expect_success 'apply -q is quiet' '
 	echo foo > file &&
 	git stash &&
 	git stash apply -q > output.out 2>&1 &&
-	test ! -s output.out
+	test_output_must_be_empty output.out
 '
 
 test_expect_success 'save -q is quiet' '
 	git stash save --quiet > output.out 2>&1 &&
-	test ! -s output.out
+	test_output_must_be_empty output.out
 '
 
 test_expect_success 'pop -q is quiet' '
 	git stash pop -q > output.out 2>&1 &&
-	test ! -s output.out
+	test_output_must_be_empty output.out
 '
 
 test_expect_success 'pop -q --index works and is quiet' '
@@ -219,13 +219,13 @@ test_expect_success 'pop -q --index works and is quiet' '
 	git stash save --quiet &&
 	git stash pop -q --index > output.out 2>&1 &&
 	test foo = "$(git show :file)" &&
-	test ! -s output.out
+	test_output_must_be_empty output.out
 '
 
 test_expect_success 'drop -q is quiet' '
 	git stash &&
 	git stash drop -q > output.out 2>&1 &&
-	test ! -s output.out
+	test_output_must_be_empty output.out
 '
 
 test_expect_success 'stash -k' '
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index aa31abe..f913166 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -15,19 +15,19 @@ test_expect_success 'git pull -q' '
 	mkdir clonedq &&
 	(cd clonedq && git init &&
 	git pull -q "../parent" >out 2>err &&
-	test ! -s err &&
-	test ! -s out)
+	test_output_must_be_empty err &&
+	test_output_must_be_empty out)
 '
 
 test_expect_success 'git pull -q --rebase' '
 	mkdir clonedqrb &&
 	(cd clonedqrb && git init &&
 	git pull -q --rebase "../parent" >out 2>err &&
-	test ! -s err &&
-	test ! -s out &&
+	test_output_must_be_empty err &&
+	test_output_must_be_empty out &&
 	git pull -q --rebase "../parent" >out 2>err &&
-	test ! -s err &&
-	test ! -s out)
+	test_output_must_be_empty err &&
+	test_output_must_be_empty out)
 '
 
 test_expect_success 'git pull' '
@@ -35,7 +35,7 @@ test_expect_success 'git pull' '
 	(cd cloned && git init &&
 	git pull "../parent" >out 2>err &&
 	test -s err &&
-	test ! -s out)
+	test_output_must_be_empty out)
 '
 
 test_expect_success 'git pull --rebase' '
@@ -43,7 +43,7 @@ test_expect_success 'git pull --rebase' '
 	(cd clonedrb && git init &&
 	git pull --rebase "../parent" >out 2>err &&
 	test -s err &&
-	test ! -s out)
+	test_output_must_be_empty out)
 '
 
 test_expect_success 'git pull -v' '
@@ -51,7 +51,7 @@ test_expect_success 'git pull -v' '
 	(cd clonedv && git init &&
 	git pull -v "../parent" >out 2>err &&
 	test -s err &&
-	test ! -s out)
+	test_output_must_be_empty out)
 '
 
 test_expect_success 'git pull -v --rebase' '
@@ -59,22 +59,22 @@ test_expect_success 'git pull -v --rebase' '
 	(cd clonedvrb && git init &&
 	git pull -v --rebase "../parent" >out 2>err &&
 	test -s err &&
-	test ! -s out)
+	test_output_must_be_empty out)
 '
 
 test_expect_success 'git pull -v -q' '
 	mkdir clonedvq &&
 	(cd clonedvq && git init &&
 	git pull -v -q "../parent" >out 2>err &&
-	test ! -s out &&
-	test ! -s err)
+	test_output_must_be_empty out &&
+	test_output_must_be_empty err)
 '
 
 test_expect_success 'git pull -q -v' '
 	mkdir clonedqv &&
 	(cd clonedqv && git init &&
 	git pull -q -v "../parent" >out 2>err &&
-	test ! -s out &&
+	test_output_must_be_empty out &&
 	test -s err)
 '
 
diff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh
index 02cb024..d486c9b 100755
--- a/t/t5702-clone-options.sh
+++ b/t/t5702-clone-options.sh
@@ -22,7 +22,7 @@ test_expect_success 'clone -o' '
 test_expect_success 'redirected clone' '
 
 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&
-	test ! -s err
+	test_output_must_be_empty err
 
 '
 test_expect_success 'redirected clone -v' '
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index df82ec9..622feff 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -457,7 +457,7 @@ test_expect_success 'disambiguation (1)' '
 	test_must_fail git diff --quiet -- secondfile &&
 	test -z "$(git diff --cached --name-only)" &&
 	test -f secondfile &&
-	test ! -s secondfile
+	test_output_must_be_empty secondfile
 
 '
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2683cba..2779b7e 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -78,7 +78,7 @@ test_expect_success 'submodule add' '
 	(
 		cd addtest &&
 		git submodule add -q "$submodurl" submod >actual &&
-		test ! -s actual &&
+		test_output_must_be_empty actual &&
 		echo "gitdir: ../.git/modules/submod" >expect &&
 		test_cmp expect submod/.git &&
 		(
@@ -308,7 +308,7 @@ test_expect_success 'update should work when path is an empty dir' '
 
 	mkdir init &&
 	git submodule update -q >update.out &&
-	test ! -s update.out &&
+	test_output_must_be_empty update.out &&
 
 	inspect init &&
 	test_cmp expect head-sha1
@@ -696,7 +696,7 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano
 		rm -rf repo &&
 		git rm repo &&
 		git submodule add -q --name repo_new "$submodurl/bare.git" repo >actual &&
-		test ! -s actual &&
+		test_output_must_be_empty actual &&
 		echo "gitdir: ../.git/modules/submod" >expect &&
 		test_cmp expect submod/.git &&
 		(
diff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh
index 735a018..84a4309 100755
--- a/t/t9402-git-cvsserver-refs.sh
+++ b/t/t9402-git-cvsserver-refs.sh
@@ -330,7 +330,7 @@ test_expect_success 'validate result of edits [cvswork2]' '
 
 test_expect_success 'validate basic diffs saved during above cvswork2 edits' '
 	test $(grep Index: cvsEdit1.diff | wc -l) = 1 &&
-	test ! -s cvsEdit2-empty.diff &&
+	test_output_must_be_empty cvsEdit2-empty.diff &&
 	test $(grep Index: cvsEdit2-N.diff | wc -l) = 1 &&
 	test $(grep Index: cvsEdit3.diff | wc -l) = 3 &&
 	rm -rf diffSandbox &&
@@ -456,20 +456,20 @@ test_expect_success 'cvs up -r $(git rev-parse v1)' '
 
 test_expect_success 'cvs diff -r v1 -u' '
 	( cd cvswork && cvs -f diff -r v1 -u ) >cvsDiff.out 2>cvs.log &&
-	test ! -s cvsDiff.out &&
-	test ! -s cvs.log
+	test_output_must_be_empty cvsDiff.out &&
+	test_output_must_be_empty cvs.log
 '
 
 test_expect_success 'cvs diff -N -r v2 -u' '
 	( cd cvswork && ! cvs -f diff -N -r v2 -u ) >cvsDiff.out 2>cvs.log &&
-	test ! -s cvs.log &&
+	test_output_must_be_empty cvs.log &&
 	test -s cvsDiff.out &&
 	check_diff cvsDiff.out v2 v1 >check_diff.out 2>&1
 '
 
 test_expect_success 'cvs diff -N -r v2 -r v1.2' '
 	( cd cvswork && ! cvs -f diff -N -r v2 -r v1.2 -u ) >cvsDiff.out 2>cvs.log &&
-	test ! -s cvs.log &&
+	test_output_must_be_empty cvs.log &&
 	test -s cvsDiff.out &&
 	check_diff cvsDiff.out v2 v1.2 >check_diff.out 2>&1
 '
@@ -488,7 +488,7 @@ test_expect_success 'apply early [cvswork3] diff to b3' '
 
 test_expect_success 'check [cvswork3] diff' '
 	( cd cvswork3 && ! cvs -f diff -N -u ) >"$WORKDIR/cvsDiff.out" 2>cvs.log &&
-	test ! -s cvs.log &&
+	test_output_must_be_empty cvs.log &&
 	test -s cvsDiff.out &&
 	test $(grep Index: cvsDiff.out | wc -l) = 3 &&
 	test_cmp cvsDiff.out cvswork3edit.diff &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3fc9cc9..a88e652 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -606,6 +606,18 @@ test_cmp() {
 	$GIT_TEST_CMP "$@"
 }
 
+# Check if the file expected to be empty is indeed empty, and barfs
+# otherwise.
+
+test_output_must_be_empty () {
+	if test -s "$1"
+	then
+		echo "Bad: test_output_must_be_empty '$1', but has the following."
+		cat "$1"
+		return 1
+	fi
+}
+
 # Tests that its two parameters refer to the same revision
 test_cmp_rev () {
 	git rev-parse --verify "$1" >expect.rev &&
-- 
1.8.3-477-gc2fede3

Re: [PATCH 2/3] test: improve rebase -q test

[Felipe Contreras]

On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:

> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -606,6 +606,18 @@ test_cmp() {
>         $GIT_TEST_CMP "$@"
>  }
>
> +# Check if the file expected to be empty is indeed empty, and barfs
> +# otherwise.
> +
> +test_output_must_be_empty () {

Why such a big name? test_empty() does the trick.

> +       if test -s "$1"
> +       then
> +               echo "Bad: test_output_must_be_empty '$1', but has the following."

echo "'$1' is not empty, it contains:"

Cheers.

-- 
Felipe Contreras

Re: [PATCH 2/3] test: improve rebase -q test

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -606,6 +606,18 @@ test_cmp() {
>>         $GIT_TEST_CMP "$@"
>>  }
>>
>> +# Check if the file expected to be empty is indeed empty, and barfs
>> +# otherwise.
>> +
>> +test_output_must_be_empty () {
>
> Why such a big name? test_empty() does the trick.

Primarily in order to avoid that exact name "test_empty" that others
may want to use for a helper to check that the contents of a string
variable is empty.  test-file-must-be-empty is a bit shorter and
also good for that purpose; I do not think we want to go any shorter
than that.

Re: [PATCH 2/3] test: improve rebase -q test

[Felipe Contreras]

On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> --- a/t/test-lib-functions.sh
>>> +++ b/t/test-lib-functions.sh
>>> @@ -606,6 +606,18 @@ test_cmp() {
>>>         $GIT_TEST_CMP "$@"
>>>  }
>>>
>>> +# Check if the file expected to be empty is indeed empty, and barfs
>>> +# otherwise.
>>> +
>>> +test_output_must_be_empty () {
>>
>> Why such a big name? test_empty() does the trick.
>
> Primarily in order to avoid that exact name "test_empty" that others
> may want to use for a helper to check that the contents of a string
> variable is empty.

Which is never going to happen.

-- 
Felipe Contreras

Re: [PATCH 2/3] test: improve rebase -q test

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>>> --- a/t/test-lib-functions.sh
>>>> +++ b/t/test-lib-functions.sh
>>>> @@ -606,6 +606,18 @@ test_cmp() {
>>>>         $GIT_TEST_CMP "$@"
>>>>  }
>>>>
>>>> +# Check if the file expected to be empty is indeed empty, and barfs
>>>> +# otherwise.
>>>> +
>>>> +test_output_must_be_empty () {
>>>
>>> Why such a big name? test_empty() does the trick.
>>
>> Primarily in order to avoid that exact name "test_empty" that others
>> may want to use for a helper to check that the contents of a string
>> variable is empty.
>
> Which is never going to happen.

For anything, a failure from

	test -z "$mustbeemptystring"

in the test suite is much harder to diagnose because there is
nothing left in the trash directory to inspect, as opposed to

	test ! -s "$mustbeemptyfile"

where you can just go there and inspect yourself.

Re: [PATCH 2/3] test: improve rebase -q test

[Felipe Contreras]

On Sun, Jun 9, 2013 at 3:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>
>>>>> --- a/t/test-lib-functions.sh
>>>>> +++ b/t/test-lib-functions.sh
>>>>> @@ -606,6 +606,18 @@ test_cmp() {
>>>>>         $GIT_TEST_CMP "$@"
>>>>>  }
>>>>>
>>>>> +# Check if the file expected to be empty is indeed empty, and barfs
>>>>> +# otherwise.
>>>>> +
>>>>> +test_output_must_be_empty () {
>>>>
>>>> Why such a big name? test_empty() does the trick.
>>>
>>> Primarily in order to avoid that exact name "test_empty" that others
>>> may want to use for a helper to check that the contents of a string
>>> variable is empty.
>>
>> Which is never going to happen.
>
> For anything, a failure from
>
>         test -z "$mustbeemptystring"
>
> in the test suite is much harder to diagnose because there is
> nothing left in the trash directory to inspect, as opposed to
>
>         test ! -s "$mustbeemptyfile"
>
> where you can just go there and inspect yourself.

Except that it's usually gone. And I challenge you to find a instance
where there's a test -z "$mustbeemptystring" that throws a test
failure. It will take you time to find it (if there's any).

Moreover, by that rationale, we should call test_cmp, test_file_cmp,
but there's no need, because that's rarely needed (if at all). There
will not be a need for test_string_must_be_empty() just like there's
no need for test_string_cmp().

-- 
Felipe Contreras

Re: [PATCH 2/3] test: improve rebase -q test

[SZEDER Gábor]

On Sun, Jun 09, 2013 at 03:41:54PM -0500, Felipe Contreras wrote:
> There
> will not be a need for test_string_must_be_empty() just like there's
> no need for test_string_cmp().

Actually, if there were a test_string_cmp(), that would be the test
helper function I used most often.

Re: [PATCH 2/3] test: improve rebase -q test

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> On Sun, Jun 09, 2013 at 03:41:54PM -0500, Felipe Contreras wrote:
>> There
>> will not be a need for test_string_must_be_empty() just like there's
>> no need for test_string_cmp().
>
> Actually, if there were a test_string_cmp(), that would be the test
> helper function I used most often.

Hmm, there indeed are quite a many "At this point, the variable's
value must be this" in the test scripts.  With things like this:

    t/t0002-gitfile.sh:     test "$SHA" = "$(git rev-list HEAD)"

we can go to the trash directory upon seeing a failure to run the
command used on the RHS, but the value in $SHA is cumbersome to find
out (either running it under sh -x or insert an extra echo before
it), so such a helper function may be useful.

Do you really need a general comparison ("does A sort before B") or
just equality?  If the latter, test_string_equal (or even
string_equal) might be a better name for it.

By the way, test_cmp() is a replacement for the "cmp" command and
that is why it does not have "file" in its name.

Re: [PATCH 2/3] test: improve rebase -q test

[Felipe Contreras]

On Mon, Jun 10, 2013 at 10:56 AM, Junio C Hamano <gitster@pobox.com> wrote:

> By the way, test_cmp() is a replacement for the "cmp" command and
> that is why it does not have "file" in its name.

That's an irrelevant implementation detail. But if you want to be
driven the the implementation, call it test_zero().

-- 
Felipe Contreras

Re: [PATCH 2/3] test: improve rebase -q test

[SZEDER Gábor]

On Mon, Jun 10, 2013 at 08:56:58AM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > On Sun, Jun 09, 2013 at 03:41:54PM -0500, Felipe Contreras wrote:
> >> There
> >> will not be a need for test_string_must_be_empty() just like there's
> >> no need for test_string_cmp().
> >
> > Actually, if there were a test_string_cmp(), that would be the test
> > helper function I used most often.
> 
> Hmm, there indeed are quite a many "At this point, the variable's
> value must be this" in the test scripts.  With things like this:
> 
>     t/t0002-gitfile.sh:     test "$SHA" = "$(git rev-list HEAD)"
> 
> we can go to the trash directory upon seeing a failure to run the
> command used on the RHS, but the value in $SHA is cumbersome to find
> out (either running it under sh -x or insert an extra echo before
> it), so such a helper function may be useful.
> 
> Do you really need a general comparison ("does A sort before B") or
> just equality?  If the latter, test_string_equal (or even
> string_equal) might be a better name for it.

Yeah, I need only equality.  Or at least it would be nice to have.

My main motivation is that, like in your example, in the bash prompt
tests I only have to check a single line of output, but because of
debuggability I always did:

  echo "(master)" >expected
  __git_ps1 >actual
  test_cmp expected actual

With such a helper function this could be reduced to a single line:

  test_string_equal "(master)" "$(__git_ps1)"

without loss of functionality or debuggability, because in case of a
failure it would output something like this (bikesheddable, of
course):

  Error:
    expected: "(master)"
    got: "((deadbeef...))"

And perhaps with a description as an optional third argument to help
identify the failed check if multiple such checks are done in a single
test, e.g. the test_rev_parse() helper in t/t1501-worktree.sh, 'setup:
helper for testing rev-parse', which could be shortened as:

  test_string_equal "$1" "$(git rev-parse --is-bare-repository)" "bare"
  test_string_equal "$2" "$(git rev-parse --is-inside-git-dir)" "gitdir"
  test_string_equal "$3" "$(git rev-parse --is-inside-work-tree)" "worktree"

and if something goes wrong we'd get:

  Error: worktree
    expected: "true"
    got: "false"

Perhaps I could find some time in the days ahead to give it a go.


Gábor

Re: [PATCH 2/3] test: improve rebase -q test

[Johannes Sixt]

Am 10.06.2013 19:27, schrieb SZEDER Gábor:
> My main motivation is that, like in your example, in the bash prompt
> tests I only have to check a single line of output, but because of
> debuggability I always did:
> 
>   echo "(master)" >expected
>   __git_ps1 >actual
>   test_cmp expected actual

Chained by &&, I presume.

> With such a helper function this could be reduced to a single line:
> 
>   test_string_equal "(master)" "$(__git_ps1)"
> 
> without loss of functionality

Not quite: A non-zero exit code of the $(__git_ps1) is lost. (It
probably doesn't matter here, but it certainly does if the command is
$(git rev-parse foo) or similar.)

-- Hannes

Re: [PATCH 2/3] test: improve rebase -q test

[SZEDER Gábor]

On Mon, Jun 10, 2013 at 09:07:06PM +0200, Johannes Sixt wrote:
> Am 10.06.2013 19:27, schrieb SZEDER Gábor:
> > My main motivation is that, like in your example, in the bash prompt
> > tests I only have to check a single line of output, but because of
> > debuggability I always did:
> > 
> >   echo "(master)" >expected
> >   __git_ps1 >actual
> >   test_cmp expected actual
> 
> Chained by &&, I presume.

Sure.

> > With such a helper function this could be reduced to a single line:
> > 
> >   test_string_equal "(master)" "$(__git_ps1)"
> > 
> > without loss of functionality
> 
> Not quite: A non-zero exit code of the $(__git_ps1) is lost. (It
> probably doesn't matter here, but it certainly does if the command is
> $(git rev-parse foo) or similar.)

Ouch, indeed.  Yeah, the exit code doesn't matter for the prompt tests
(I mean for __git_ps1() tests, but maybe it does matter for some
__gitdir() tests), but I suppose it does matter everywhere else where
the same construct is used.  We could still do

  actual="$(git foo)" &&
  test_string_equal "good" "$actual"

to preserve and check the exit code, and this is still one line
shorter, but overall not that appealing anymore.

However.  The git command's exit code is lost the same way in 'test
good = $(git foo)' constructs, too, and plenty of such constructs are
all over the test suite.  Shouldn't we avoid using such constucts
then?


Gábor

Re: [PATCH 2/3] test: improve rebase -q test

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> With such a helper function this could be reduced to a single line:
>
>   test_string_equal "(master)" "$(__git_ps1)"
>
> without loss of functionality or debuggability, because in case of a
> failure it would output something like this (bikesheddable, of
> course):
>
>   Error:
>     expected: "(master)"
>     got: "((deadbeef...))"

Yeah, that looks sensible.

Re: [PATCH 2/3] test: improve rebase -q test

[Philip Oakley]

From: "Felipe Contreras" <felipe.contreras@gmail.com>
Sent: Sunday, June 09, 2013 8:33 PM
> On Sun, Jun 9, 2013 at 2:20 PM, Junio C Hamano <gitster@pobox.com> 
> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> On Sun, Jun 9, 2013 at 1:30 PM, Junio C Hamano <gitster@pobox.com> 
>>> wrote:
>>>
>>>> --- a/t/test-lib-functions.sh
>>>> +++ b/t/test-lib-functions.sh
>>>> @@ -606,6 +606,18 @@ test_cmp() {
>>>>         $GIT_TEST_CMP "$@"
>>>>  }
>>>>
>>>> +# Check if the file expected to be empty is indeed empty, and 
>>>> barfs
>>>> +# otherwise.
>>>> +
>>>> +test_output_must_be_empty () {
>>>
>>> Why such a big name? test_empty() does the trick.
>>
>> Primarily in order to avoid that exact name "test_empty" that others
>> may want to use for a helper to check that the contents of a string
>> variable is empty.
>
> Which is never going to happen.
>

While folks do use such simplistic names, given that the patch had many 
call sites, I do think Filipe's short name would quickly become the 
accepted test name and not cause any great difficulties.

regards
Philip 

Re: [PATCH 2/3] test: improve rebase -q test

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> While folks do use such simplistic names, given that the patch had
> many call sites, I do think Filipe's short name would quickly become
> the accepted test name and not cause any great difficulties.

OK.

Re: [PATCH 2/3] test: improve rebase -q test

[Jeff King]

On Sun, Jun 09, 2013 at 11:30:01AM -0700, Junio C Hamano wrote:

> -- >8 --
> Subject: [PATCH] test: test_output_must_be_empty helper
> 
> There are quite a lot places where an output file is expected to be
> empty, and we fail the test when it is not.  The output from running
> the test script with -i -v can be helped if we showed the unexpected
> contents at that point.
> 
> We could of course do
> 
>     >expected.empty && test_cmp expected.empty actual
> 
> but this is commmon enough to be done with a dedicated helper.

Thanks, I think this improves the readability of the test suite (and its
output when there are failures).

You can also do:

  test_cmp /dev/null actual

for the same effect, but I guess the diff is not all that interesting
(by definition, it would consist only of added lines, and you are
already showing them, so it would not be an improvement).

-Peff

[PATCH 3/3] submodule: remove unnecessary check

[Felipe Contreras]

read_cache() already does that check.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 submodule.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index ad476ce..8685424 100644
--- a/submodule.c
+++ b/submodule.c
@@ -603,9 +603,8 @@ int fetch_populated_submodules(const struct argv_array *options,
 	if (!work_tree)
 		goto out;
 
-	if (!the_index.initialized)
-		if (read_cache() < 0)
-			die("index file corrupt");
+	if (read_cache() < 0)
+		die("index file corrupt");
 
 	argv_array_push(&argv, "fetch");
 	for (i = 0; i < options->argc; i++)
-- 
1.8.3.698.g079b096