Re: [PATCH 2/5] completion: fix args of run_completion() test helper

Jeff King [off-list ref] writes:

On Fri, Sep 28, 2012 at 12:23:47PM -0700, Junio C Hamano wrote:

quoted

quoted

quoted

quoted

@@ -57,7 +57,7 @@ run_completion ()
 test_completion ()
 {
 	test $# -gt 1 && echo "$2" > expected
-	run_completion "$@" &&
+	run_completion $1 &&
 	test_cmp expected out
 }

I can understand the other three hunks, but this one is fishy.
Shouldn't "$1" be inside a pair of dq?  I.e.

	+	run_completion "$1" &&

No.  $1 holds all words on the command line.  If it was between a pair
of dq, then the whole command line would be passed to the completion
script as a single word.

And these "words" can be split at $IFS boundaries without any
issues?  IOW, nobody would ever want to make words array in the
run_completion function to ['git' 'foo bar' 'baz']?

It might be simpler to just convert test_completion into the
test_completion_long I added in my series; the latter takes the expected
output on stdin, leaving the actual arguments free to represent the real
command-line. E.g., your example would become:

  test_completion git "foo bar" baz <<-\EOF
  ... expected output ...
  EOF

I realize that the way my question was stated was misleading.  It
was not meant as a rhetorical "You would never be able to pass
['git' 'foo bar' 'baz'] with that interface, and the patch sucks."
but was meant as a pure question "Do we want to pass such word
list?".  "test_completion is almost always used to test completion
with inputs without any $IFS letters in it, so not being able to
test such an input via this interface is fine. If needed, we can
give another less often used interface to let you pass such an
input" is perfectly fine by me.

But I suspect that the real reason test_completion requires the
caller to express the list of inputs to run_completion as $IFS
separate list is because it needs to also get expected from the
command line:

quoted

quoted

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

I wonder if doing something like this would be a far simpler
solution:

	test_completion ()
        {
		case "$1" in
                '')
			;;
		*)
			echo "$1" >expect &&
	                shift
                        ;;
		esac &&
                run_completion "$@" &&
                test_cmp expect output
	}

On Fri, Sep 28, 2012 at 12:49:25PM -0700, Junio C Hamano wrote:

quoted

quoted

And these "words" can be split at $IFS boundaries without any
issues?  IOW, nobody would ever want to make words array in the
run_completion function to ['git' 'foo bar' 'baz']?

It might be simpler to just convert test_completion into the
test_completion_long I added in my series; the latter takes the expected
output on stdin, leaving the actual arguments free to represent the real
command-line. E.g., your example would become:

  test_completion git "foo bar" baz <<-\EOF
  ... expected output ...
  EOF

I realize that the way my question was stated was misleading.  It
was not meant as a rhetorical "You would never be able to pass
['git' 'foo bar' 'baz'] with that interface, and the patch sucks."
but was meant as a pure question "Do we want to pass such word
list?".  "test_completion is almost always used to test completion
with inputs without any $IFS letters in it, so not being able to
test such an input via this interface is fine. If needed, we can
give another less often used interface to let you pass such an
input" is perfectly fine by me.

I think we may eventually want to pass arguments with IFS into the
function, just to make sure it works (the tests I added checked for IFS
in the completion list rather than the input, but we should probably
check both).

I'm OK if it needs to be an alternate interface (right now you could do
it by calling run_completion yourself).

But I suspect that the real reason test_completion requires the
caller to express the list of inputs to run_completion as $IFS
separate list is because it needs to also get expected from the
command line:

Right, that's why I suggested bumping that to stdin for the function.

quoted

quoted

quoted

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

I wonder if doing something like this would be a far simpler
solution:

	test_completion ()
        {
		case "$1" in
                '')
			;;
		*)
			echo "$1" >expect &&
	                shift
                        ;;
		esac &&
                run_completion "$@" &&
                test_cmp expect output
	}

That would also work. I mainly suggested the stdin thing because we need
it anyway for output that generates multiple answers (well, you don't
_need_ it; you can call run_completion yourself, but it saves a few
lines at each call site).

-Peff