Felipe ContrerasFelipe 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
Felipe Contreras[PATCH 1/3] sequencer: trivial fix
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
Junio C HamanoRe: [PATCH 1/3] sequencer: trivial fix
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?
Felipe ContrerasRe: [PATCH 1/3] sequencer: trivial fix
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
Felipe Contreras[PATCH 2/3] test: improve rebase -q test
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
Duy NguyenRe: [PATCH 2/3] test: improve rebase -q test
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
Felipe ContrerasRe: [PATCH 2/3] test: improve rebase -q test
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
Junio C HamanoRe: [PATCH 2/3] test: improve rebase -q test
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(-)
t/t0040-parse-options.sh, t/t3400-rebase.sh, t/t3903-stash.sh, t/t5521-pull-options.sh, t/t5702-clone-options.sh, t/t7102-reset.sh, t/t7400-submodule-basic.sh, t/t9402-git-cvsserver-refs.sh, t/test-lib-functions.sh
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
Felipe ContrerasRe: [PATCH 2/3] test: improve rebase -q test
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
Junio C HamanoRe: [PATCH 2/3] test: improve rebase -q test
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.
Felipe ContrerasRe: [PATCH 2/3] test: improve rebase -q test
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
Junio C HamanoRe: [PATCH 2/3] test: improve rebase -q test
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.
Felipe ContrerasRe: [PATCH 2/3] test: improve rebase -q test
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
SZEDER GáborRe: [PATCH 2/3] test: improve rebase -q test
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.
Junio C HamanoRe: [PATCH 2/3] test: improve rebase -q test
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.
Felipe ContrerasRe: [PATCH 2/3] test: improve rebase -q test
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
SZEDER GáborRe: [PATCH 2/3] test: improve rebase -q test
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
Johannes SixtRe: [PATCH 2/3] test: improve rebase -q test
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
SZEDER GáborRe: [PATCH 2/3] test: improve rebase -q test
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
Junio C HamanoRe: [PATCH 2/3] test: improve rebase -q test
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.
Philip OakleyRe: [PATCH 2/3] test: improve rebase -q test
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
Junio C HamanoRe: [PATCH 2/3] test: improve rebase -q test
"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.
Jeff KingRe: [PATCH 2/3] test: improve rebase -q test
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
Felipe Contreras[PATCH 3/3] submodule: remove unnecessary check
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