[PATCH v2 0/6] completion: test consolidations

[PATCH v2 0/6] completion: test consolidations

[Felipe Contreras]

These started from a discussion with SZEDER, but then I realized there were
many improvements possible.

Changes since v1:

 * A lot more cleanups

Felipe Contreras (6):
  completion: add comment for test_completion()
  completion: standardize final space marker in tests
  completion: simplify tests using test_completion_long()
  completion: consolidate test_completion*() tests
  completion: refactor __gitcomp related tests
  completion: simplify __gitcomp() test helper

 t/t9902-completion.sh | 133 +++++++++++++++++++-------------------------------
 1 file changed, 51 insertions(+), 82 deletions(-)

-- 
1.8.0

[PATCH v2 1/6] completion: add comment for test_completion()

[Felipe Contreras]

So that it's easier to understand what it does.

Also, make sure we pass only the first argument for completion.
Shouldn't cause any functional changes because run_completion only
checks $1.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cbd0fb6..5c06709 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -54,10 +54,14 @@ run_completion ()
 	__git_wrap__git_main && print_comp
 }
 
+# Test high-level completion
+# Arguments are:
+# 1: typed text so far (cur)
+# 2: expected completion
 test_completion ()
 {
 	test $# -gt 1 && echo "$2" > expected
-	run_completion "$@" &&
+	run_completion "$1" &&
 	test_cmp expected out
 }
 
-- 
1.8.0

Re: [PATCH v2 1/6] completion: add comment for test_completion()

[SZEDER Gábor]

On Sun, Nov 11, 2012 at 03:35:53PM +0100, Felipe Contreras wrote:
> So that it's easier to understand what it does.
> 
> Also, make sure we pass only the first argument for completion.
> Shouldn't cause any functional changes because run_completion only
> checks $1.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index cbd0fb6..5c06709 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -54,10 +54,14 @@ run_completion ()
>  	__git_wrap__git_main && print_comp
>  }
>  
> +# Test high-level completion
> +# Arguments are:
> +# 1: typed text so far (cur)

Bash manuals calls this the current command line or words in the
current command line.  I'm not sure what you mean with '(cur)' here.
The variable $cur in the completion script (or in bash-completion in
general) is something completely different.

> +# 2: expected completion
>  test_completion ()
>  {
>  	test $# -gt 1 && echo "$2" > expected
> -	run_completion "$@" &&
> +	run_completion "$1" &&
>  	test_cmp expected out
>  }
>  
> -- 
> 1.8.0

Re: [PATCH v2 1/6] completion: add comment for test_completion()

[Felipe Contreras]

On Fri, Nov 16, 2012 at 9:54 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sun, Nov 11, 2012 at 03:35:53PM +0100, Felipe Contreras wrote:
>> So that it's easier to understand what it does.
>>
>> Also, make sure we pass only the first argument for completion.
>> Shouldn't cause any functional changes because run_completion only
>> checks $1.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index cbd0fb6..5c06709 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -54,10 +54,14 @@ run_completion ()
>>       __git_wrap__git_main && print_comp
>>  }
>>
>> +# Test high-level completion
>> +# Arguments are:
>> +# 1: typed text so far (cur)
>
> Bash manuals calls this the current command line or words in the
> current command line.  I'm not sure what you mean with '(cur)' here.

The current _word_ text typed so far.

> The variable $cur in the completion script (or in bash-completion in
> general) is something completely different.

I believe bash's completion, this test, and the whole git completion
stuff uses the same definition of 'cur'.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 1/6] completion: add comment for test_completion()

[Felipe Contreras]

On Fri, Nov 16, 2012 at 10:06 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Fri, Nov 16, 2012 at 9:54 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> On Sun, Nov 11, 2012 at 03:35:53PM +0100, Felipe Contreras wrote:
>>> So that it's easier to understand what it does.
>>>
>>> Also, make sure we pass only the first argument for completion.
>>> Shouldn't cause any functional changes because run_completion only
>>> checks $1.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> ---
>>>  t/t9902-completion.sh | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>>> index cbd0fb6..5c06709 100755
>>> --- a/t/t9902-completion.sh
>>> +++ b/t/t9902-completion.sh
>>> @@ -54,10 +54,14 @@ run_completion ()
>>>       __git_wrap__git_main && print_comp
>>>  }
>>>
>>> +# Test high-level completion
>>> +# Arguments are:
>>> +# 1: typed text so far (cur)
>>
>> Bash manuals calls this the current command line or words in the
>> current command line.  I'm not sure what you mean with '(cur)' here.
>
> The current _word_ text typed so far.
>
>> The variable $cur in the completion script (or in bash-completion in
>> general) is something completely different.
>
> I believe bash's completion, this test, and the whole git completion
> stuff uses the same definition of 'cur'.

Oops, actually in this particular helper this is not actually 'cur'. I
think 'command line' might be more appropriate.

-- 
Felipe Contreras

[PATCH v2 2/6] completion: standardize final space marker in tests

[Felipe Contreras]

The rest of the code uses ' Z$'. Lets use that for
test_completion_long() as well.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5c06709..aff7e44 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -66,11 +66,10 @@ test_completion ()
 }
 
 # Like test_completion, but reads expectation from stdin,
-# which is convenient when it is multiline. We also process "_" into
-# spaces to make test vectors more readable.
+# which is convenient when it is multiline.
 test_completion_long ()
 {
-	tr _ " " >expected &&
+	sed -e 's/Z$//' > expected &&
 	test_completion "$1"
 }
 
@@ -252,24 +251,24 @@ test_expect_success 'setup for ref completion' '
 
 test_expect_success 'checkout completes ref names' '
 	test_completion_long "git checkout m" <<-\EOF
-	master_
-	mybranch_
-	mytag_
+	master Z
+	mybranch Z
+	mytag Z
 	EOF
 '
 
 test_expect_success 'show completes all refs' '
 	test_completion_long "git show m" <<-\EOF
-	master_
-	mybranch_
-	mytag_
+	master Z
+	mybranch Z
+	mytag Z
 	EOF
 '
 
 test_expect_success '<ref>: completes paths' '
 	test_completion_long "git show mytag:f" <<-\EOF
-	file1_
-	file2_
+	file1 Z
+	file2 Z
 	EOF
 '
 
@@ -278,7 +277,7 @@ test_expect_success 'complete tree filename with spaces' '
 	git add . &&
 	git commit -m spaces &&
 	test_completion_long "git show HEAD:nam" <<-\EOF
-	name with spaces_
+	name with spaces Z
 	EOF
 '
 
@@ -287,8 +286,8 @@ test_expect_failure 'complete tree filename with metacharacters' '
 	git add . &&
 	git commit -m meta &&
 	test_completion_long "git show HEAD:nam" <<-\EOF
-	name with ${meta}_
-	name with spaces_
+	name with ${meta} Z
+	name with spaces Z
 	EOF
 '
 
-- 
1.8.0

[PATCH v2 3/6] completion: simplify tests using test_completion_long()

[Felipe Contreras]

No need to duplicate that functionality.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index aff7e44..204c92a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -172,7 +172,7 @@ test_expect_success 'basic' '
 '
 
 test_expect_success 'double dash "git" itself' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_completion_long "git --" <<-\EOF
 	--paginate Z
 	--no-pager Z
 	--git-dir=
@@ -187,11 +187,10 @@ test_expect_success 'double dash "git" itself' '
 	--no-replace-objects Z
 	--help Z
 	EOF
-	test_completion "git --"
 '
 
 test_expect_success 'double dash "git checkout"' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_completion_long "git checkout --" <<-\EOF
 	--quiet Z
 	--ours Z
 	--theirs Z
@@ -202,17 +201,15 @@ test_expect_success 'double dash "git checkout"' '
 	--orphan Z
 	--patch Z
 	EOF
-	test_completion "git checkout --"
 '
 
 test_expect_success 'general options' '
 	test_completion "git --ver" "--version " &&
 	test_completion "git --hel" "--help " &&
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_completion_long "git --exe" <<-\EOF &&
 	--exec-path Z
 	--exec-path=
 	EOF
-	test_completion "git --exe" &&
 	test_completion "git --htm" "--html-path " &&
 	test_completion "git --pag" "--paginate " &&
 	test_completion "git --no-p" "--no-pager " &&
-- 
1.8.0

[PATCH v2 4/6] completion: consolidate test_completion*() tests

[Felipe Contreras]

No need to have two versions; if a second argument is specified, use
that, otherwise use stdin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 204c92a..59cdbfd 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -60,19 +60,15 @@ run_completion ()
 # 2: expected completion
 test_completion ()
 {
-	test $# -gt 1 && echo "$2" > expected
+	if [ $# -gt 1 ]; then
+		echo "$2" > expected
+	else
+		sed -e 's/Z$//' > expected
+	fi &&
 	run_completion "$1" &&
 	test_cmp expected out
 }
 
-# Like test_completion, but reads expectation from stdin,
-# which is convenient when it is multiline.
-test_completion_long ()
-{
-	sed -e 's/Z$//' > expected &&
-	test_completion "$1"
-}
-
 newline=$'\n'
 
 test_expect_success '__gitcomp - trailing space - options' '
@@ -172,7 +168,7 @@ test_expect_success 'basic' '
 '
 
 test_expect_success 'double dash "git" itself' '
-	test_completion_long "git --" <<-\EOF
+	test_completion "git --" <<-\EOF
 	--paginate Z
 	--no-pager Z
 	--git-dir=
@@ -190,7 +186,7 @@ test_expect_success 'double dash "git" itself' '
 '
 
 test_expect_success 'double dash "git checkout"' '
-	test_completion_long "git checkout --" <<-\EOF
+	test_completion "git checkout --" <<-\EOF
 	--quiet Z
 	--ours Z
 	--theirs Z
@@ -206,7 +202,7 @@ test_expect_success 'double dash "git checkout"' '
 test_expect_success 'general options' '
 	test_completion "git --ver" "--version " &&
 	test_completion "git --hel" "--help " &&
-	test_completion_long "git --exe" <<-\EOF &&
+	test_completion "git --exe" <<-\EOF &&
 	--exec-path Z
 	--exec-path=
 	EOF
@@ -247,7 +243,7 @@ test_expect_success 'setup for ref completion' '
 '
 
 test_expect_success 'checkout completes ref names' '
-	test_completion_long "git checkout m" <<-\EOF
+	test_completion "git checkout m" <<-\EOF
 	master Z
 	mybranch Z
 	mytag Z
@@ -255,7 +251,7 @@ test_expect_success 'checkout completes ref names' '
 '
 
 test_expect_success 'show completes all refs' '
-	test_completion_long "git show m" <<-\EOF
+	test_completion "git show m" <<-\EOF
 	master Z
 	mybranch Z
 	mytag Z
@@ -263,7 +259,7 @@ test_expect_success 'show completes all refs' '
 '
 
 test_expect_success '<ref>: completes paths' '
-	test_completion_long "git show mytag:f" <<-\EOF
+	test_completion "git show mytag:f" <<-\EOF
 	file1 Z
 	file2 Z
 	EOF
@@ -273,7 +269,7 @@ test_expect_success 'complete tree filename with spaces' '
 	echo content >"name with spaces" &&
 	git add . &&
 	git commit -m spaces &&
-	test_completion_long "git show HEAD:nam" <<-\EOF
+	test_completion "git show HEAD:nam" <<-\EOF
 	name with spaces Z
 	EOF
 '
@@ -282,7 +278,7 @@ test_expect_failure 'complete tree filename with metacharacters' '
 	echo content >"name with \${meta}" &&
 	git add . &&
 	git commit -m meta &&
-	test_completion_long "git show HEAD:nam" <<-\EOF
+	test_completion "git show HEAD:nam" <<-\EOF
 	name with ${meta} Z
 	name with spaces Z
 	EOF
-- 
1.8.0

Re: [PATCH v2 4/6] completion: consolidate test_completion*() tests

[Junio C Hamano]

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

> No need to have two versions; if a second argument is specified, use
> that, otherwise use stdin.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 204c92a..59cdbfd 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -60,19 +60,15 @@ run_completion ()
>  # 2: expected completion
>  test_completion ()
>  {
> -	test $# -gt 1 && echo "$2" > expected
> +	if [ $# -gt 1 ]; then
> +		echo "$2" > expected
> +	else
> +		sed -e 's/Z$//' > expected
> +	fi &&

As "$2" could begin with dash, end with \c, etc. that possibly can
be misinterpred by echo, I'd rewrite this as

		printf '%s\n' "$2" >expected

Otherwise looked fine; thanks.

Re: [PATCH v2 4/6] completion: consolidate test_completion*() tests

[Felipe Contreras]

On Sat, Nov 17, 2012 at 12:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> No need to have two versions; if a second argument is specified, use
>> that, otherwise use stdin.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 30 +++++++++++++-----------------
>>  1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 204c92a..59cdbfd 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -60,19 +60,15 @@ run_completion ()
>>  # 2: expected completion
>>  test_completion ()
>>  {
>> -     test $# -gt 1 && echo "$2" > expected
>> +     if [ $# -gt 1 ]; then
>> +             echo "$2" > expected
>> +     else
>> +             sed -e 's/Z$//' > expected
>> +     fi &&
>
> As "$2" could begin with dash, end with \c, etc. that possibly can
> be misinterpred by echo, I'd rewrite this as
>
>                 printf '%s\n' "$2" >expected
>
> Otherwise looked fine; thanks.

But that was the case before. I would do that in a separate patch.

Cheers.

-- 
Felipe Contreras

[PATCH v2 5/6] completion: refactor __gitcomp related tests

[Felipe Contreras]

Lots of duplicated code!

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 76 ++++++++++++++++++---------------------------------
 1 file changed, 27 insertions(+), 49 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 59cdbfd..66c7af6 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -71,87 +71,65 @@ test_completion ()
 
 newline=$'\n'
 
-test_expect_success '__gitcomp - trailing space - options' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
-	--reuse-message=Z
-	--reedit-message=Z
-	--reset-author Z
-	EOF
+# Test __gitcomp.
+# Arguments are:
+# 1: typed text so far (cur)
+# *: arguments to pass to __gitcomp
+test_gitcomp ()
+{
+	sed -e 's/Z$//' > expected &&
 	(
 		local -a COMPREPLY &&
-		cur="--re" &&
-		__gitcomp "--dry-run --reuse-message= --reedit-message=
-				--reset-author" &&
+		cur="$1" &&
+		shift &&
+		__gitcomp "$@" &&
 		IFS="$newline" &&
 		echo "${COMPREPLY[*]}" > out
 	) &&
 	test_cmp expected out
+}
+
+test_expect_success '__gitcomp - trailing space - options' '
+	test_gitcomp "--re" "--dry-run --reuse-message= --reedit-message=
+		--reset-author" <<-EOF
+	--reuse-message=Z
+	--reedit-message=Z
+	--reset-author Z
+	EOF
 '
 
 test_expect_success '__gitcomp - trailing space - config keys' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp "br" "branch. branch.autosetupmerge
+		branch.autosetuprebase browser." <<-\EOF
 	branch.Z
 	branch.autosetupmerge Z
 	branch.autosetuprebase Z
 	browser.Z
 	EOF
-	(
-		local -a COMPREPLY &&
-		cur="br" &&
-		__gitcomp "branch. branch.autosetupmerge
-				branch.autosetuprebase browser." &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
-	test_cmp expected out
 '
 
 test_expect_success '__gitcomp - option parameter' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp "--strategy=re" "octopus ours recursive resolve subtree" \
+		"" "re" <<-\EOF
 	recursive Z
 	resolve Z
 	EOF
-	(
-		local -a COMPREPLY &&
-		cur="--strategy=re" &&
-		__gitcomp "octopus ours recursive resolve subtree
-			" "" "re" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
-	test_cmp expected out
 '
 
 test_expect_success '__gitcomp - prefix' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp "branch.me" "remote merge mergeoptions rebase" \
+		"branch.maint." "me" <<-\EOF
 	branch.maint.merge Z
 	branch.maint.mergeoptions Z
 	EOF
-	(
-		local -a COMPREPLY &&
-		cur="branch.me" &&
-		__gitcomp "remote merge mergeoptions rebase
-			" "branch.maint." "me" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
-	test_cmp expected out
 '
 
 test_expect_success '__gitcomp - suffix' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp "branch.me" "master maint next pu" "branch." \
+		"ma" "." <<-\EOF
 	branch.master.Z
 	branch.maint.Z
 	EOF
-	(
-		local -a COMPREPLY &&
-		cur="branch.me" &&
-		__gitcomp "master maint next pu
-			" "branch." "ma" "." &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
-	test_cmp expected out
 '
 
 test_expect_success 'basic' '
-- 
1.8.0

Re: [PATCH v2 5/6] completion: refactor __gitcomp related tests

[Junio C Hamano]

Not asking for a re-roll but am asking for clarification so that I
can locally update before queuing.

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

> Lots of duplicated code!

... removed, you mean?

> No functional changes.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 76 ++++++++++++++++++---------------------------------
>  1 file changed, 27 insertions(+), 49 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 59cdbfd..66c7af6 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -71,87 +71,65 @@ test_completion ()
>  
>  newline=$'\n'
>  
> -test_expect_success '__gitcomp - trailing space - options' '
> -	sed -e "s/Z$//" >expected <<-\EOF &&
> -	--reuse-message=Z
> -	--reedit-message=Z
> -	--reset-author Z
> -	EOF
> +# Test __gitcomp.
> +# Arguments are:
> +# 1: typed text so far (cur)
> +# *: arguments to pass to __gitcomp

s/\*/remainder/, perhaps?  I think you shift $1 out and do not pass
it to __gitcomp.

And expected output is from the standard input just like
test_completion?

> +test_gitcomp ()
> +{
> +	sed -e 's/Z$//' > expected &&
>  	(
>  		local -a COMPREPLY &&
> -		cur="--re" &&
> -		__gitcomp "--dry-run --reuse-message= --reedit-message=
> -				--reset-author" &&
> +		cur="$1" &&
> +		shift &&
> +		__gitcomp "$@" &&
>  		IFS="$newline" &&
>  		echo "${COMPREPLY[*]}" > out
>  	) &&
>  	test_cmp expected out
> +}
> +
> +test_expect_success '__gitcomp - trailing space - options' '
> +	test_gitcomp "--re" "--dry-run --reuse-message= --reedit-message=
> +		--reset-author" <<-EOF
> +	--reuse-message=Z
> +	--reedit-message=Z
> +	--reset-author Z
> +	EOF
>  '

Nice shrinkage.

Re: [PATCH v2 5/6] completion: refactor __gitcomp related tests

[Felipe Contreras]

On Fri, Nov 16, 2012 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Not asking for a re-roll but am asking for clarification so that I
> can locally update before queuing.
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Lots of duplicated code!
>
> ... removed, you mean?

Yes.

>> No functional changes.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 76 ++++++++++++++++++---------------------------------
>>  1 file changed, 27 insertions(+), 49 deletions(-)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 59cdbfd..66c7af6 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -71,87 +71,65 @@ test_completion ()
>>
>>  newline=$'\n'
>>
>> -test_expect_success '__gitcomp - trailing space - options' '
>> -     sed -e "s/Z$//" >expected <<-\EOF &&
>> -     --reuse-message=Z
>> -     --reedit-message=Z
>> -     --reset-author Z
>> -     EOF
>> +# Test __gitcomp.
>> +# Arguments are:
>> +# 1: typed text so far (cur)
>> +# *: arguments to pass to __gitcomp
>
> s/\*/remainder/, perhaps?  I think you shift $1 out and do not pass
> it to __gitcomp.

Right, by * I meant the rest.

> And expected output is from the standard input just like
> test_completion?

Correct.

>> +test_gitcomp ()
>> +{
>> +     sed -e 's/Z$//' > expected &&
>>       (
>>               local -a COMPREPLY &&
>> -             cur="--re" &&
>> -             __gitcomp "--dry-run --reuse-message= --reedit-message=
>> -                             --reset-author" &&
>> +             cur="$1" &&
>> +             shift &&
>> +             __gitcomp "$@" &&
>>               IFS="$newline" &&
>>               echo "${COMPREPLY[*]}" > out
>>       ) &&
>>       test_cmp expected out
>> +}
>> +
>> +test_expect_success '__gitcomp - trailing space - options' '
>> +     test_gitcomp "--re" "--dry-run --reuse-message= --reedit-message=
>> +             --reset-author" <<-EOF
>> +     --reuse-message=Z
>> +     --reedit-message=Z
>> +     --reset-author Z
>> +     EOF
>>  '
>
> Nice shrinkage.

That's a comment about the whole patch series I hope :)

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 5/6] completion: refactor __gitcomp related tests

[SZEDER Gábor]

On Sun, Nov 11, 2012 at 03:35:57PM +0100, Felipe Contreras wrote:
> Lots of duplicated code!
> 
> No functional changes.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 76 ++++++++++++++++++---------------------------------
>  1 file changed, 27 insertions(+), 49 deletions(-)

Despite the impressive numbers, these tests are more useful without
this cleanup.

> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 59cdbfd..66c7af6 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -71,87 +71,65 @@ test_completion ()
>  
>  newline=$'\n'
>  
> -test_expect_success '__gitcomp - trailing space - options' '
> -	sed -e "s/Z$//" >expected <<-\EOF &&
> -	--reuse-message=Z
> -	--reedit-message=Z
> -	--reset-author Z
> -	EOF
> +# Test __gitcomp.
> +# Arguments are:
> +# 1: typed text so far (cur)

The first argument is not the typed text so far, but the word
currently containing the cursor position.

> +# *: arguments to pass to __gitcomp
> +test_gitcomp ()
> +{
> +	sed -e 's/Z$//' > expected &&
>  	(
>  		local -a COMPREPLY &&
> -		cur="--re" &&
> -		__gitcomp "--dry-run --reuse-message= --reedit-message=
> -				--reset-author" &&
> +		cur="$1" &&
> +		shift &&
> +		__gitcomp "$@" &&
>  		IFS="$newline" &&
>  		echo "${COMPREPLY[*]}" > out
>  	) &&
>  	test_cmp expected out
> +}
> +
> +test_expect_success '__gitcomp - trailing space - options' '
> +	test_gitcomp "--re" "--dry-run --reuse-message= --reedit-message=
> +		--reset-author" <<-EOF
> +	--reuse-message=Z
> +	--reedit-message=Z
> +	--reset-author Z
> +	EOF
>  '
>  
>  test_expect_success '__gitcomp - trailing space - config keys' '
> -	sed -e "s/Z$//" >expected <<-\EOF &&
> +	test_gitcomp "br" "branch. branch.autosetupmerge
> +		branch.autosetuprebase browser." <<-\EOF
>  	branch.Z
>  	branch.autosetupmerge Z
>  	branch.autosetuprebase Z
>  	browser.Z
>  	EOF
> -	(
> -		local -a COMPREPLY &&
> -		cur="br" &&
> -		__gitcomp "branch. branch.autosetupmerge
> -				branch.autosetuprebase browser." &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out
> -	) &&
> -	test_cmp expected out
>  '
>  
>  test_expect_success '__gitcomp - option parameter' '
> -	sed -e "s/Z$//" >expected <<-\EOF &&
> +	test_gitcomp "--strategy=re" "octopus ours recursive resolve subtree" \
> +		"" "re" <<-\EOF
>  	recursive Z
>  	resolve Z
>  	EOF
> -	(
> -		local -a COMPREPLY &&
> -		cur="--strategy=re" &&
> -		__gitcomp "octopus ours recursive resolve subtree
> -			" "" "re" &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out
> -	) &&
> -	test_cmp expected out
>  '
>  
>  test_expect_success '__gitcomp - prefix' '
> -	sed -e "s/Z$//" >expected <<-\EOF &&
> +	test_gitcomp "branch.me" "remote merge mergeoptions rebase" \
> +		"branch.maint." "me" <<-\EOF
>  	branch.maint.merge Z
>  	branch.maint.mergeoptions Z
>  	EOF
> -	(
> -		local -a COMPREPLY &&
> -		cur="branch.me" &&
> -		__gitcomp "remote merge mergeoptions rebase
> -			" "branch.maint." "me" &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out
> -	) &&
> -	test_cmp expected out
>  '
>  
>  test_expect_success '__gitcomp - suffix' '
> -	sed -e "s/Z$//" >expected <<-\EOF &&
> +	test_gitcomp "branch.me" "master maint next pu" "branch." \
> +		"ma" "." <<-\EOF
>  	branch.master.Z
>  	branch.maint.Z
>  	EOF
> -	(
> -		local -a COMPREPLY &&
> -		cur="branch.me" &&
> -		__gitcomp "master maint next pu
> -			" "branch." "ma" "." &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out
> -	) &&
> -	test_cmp expected out
>  '
>  
>  test_expect_success 'basic' '
> -- 
> 1.8.0
> 

Re: [PATCH v2 5/6] completion: refactor __gitcomp related tests

[Junio C Hamano]

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

> On Sun, Nov 11, 2012 at 03:35:57PM +0100, Felipe Contreras wrote:
>> Lots of duplicated code!
>> 
>> No functional changes.
>> 
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 76 ++++++++++++++++++---------------------------------
>>  1 file changed, 27 insertions(+), 49 deletions(-)
>
> Despite the impressive numbers, these tests are more useful without
> this cleanup.

Is this because consolidation of the duplicated part of the tests
into a single helper makes it harder to instrument one test you are
interested in (or developing) for debugging?

It indeed is a problem, and cutting and pasting the same code to
multiple tests is one way to solve the problem (you can easily
instrument the copy in the test you are interested in while leaving
others intact), but I do not think that is a good solution.  A bugfix
or enhancement to the shared (or duplicated) part can be done by
touching one place only after this change, while with the current
code you have to repeat the same fix to all places, no?

>> +# Test __gitcomp.
>> +# Arguments are:
>> +# 1: typed text so far (cur)
>
> The first argument is not the typed text so far, but the word
> currently containing the cursor position.

Care to update this with a follow-up patch, so that I do not have to
keep track of minute details ;-)?

[PATCH v2 6/6] completion: simplify __gitcomp() test helper

[Felipe Contreras]

By using print_comp as suggested by SZEDER Gábor.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 66c7af6..9b38b69 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -69,23 +69,18 @@ test_completion ()
 	test_cmp expected out
 }
 
-newline=$'\n'
-
 # Test __gitcomp.
 # Arguments are:
 # 1: typed text so far (cur)
 # *: arguments to pass to __gitcomp
 test_gitcomp ()
 {
+	local -a COMPREPLY &&
 	sed -e 's/Z$//' > expected &&
-	(
-		local -a COMPREPLY &&
-		cur="$1" &&
-		shift &&
-		__gitcomp "$@" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
+	cur="$1" &&
+	shift &&
+	__gitcomp "$@" &&
+	print_comp &&
 	test_cmp expected out
 }
 
-- 
1.8.0