[RFC/PATCH] tests: add initial bash completion tests

[RFC/PATCH] tests: add initial bash completion tests

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

Something is better than nothing.

 t/t9902-completion.sh |  198 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)
 create mode 100755 t/t9902-completion.sh

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
new file mode 100755
index 0000000..a037dff
--- /dev/null
+++ b/t/t9902-completion.sh
@@ -0,0 +1,198 @@
+#!/bin/bash
+#
+# Copyright (c) 2012 Felipe Contreras
+#
+
+test_description='test bash completion'
+
+. ./test-lib.sh
+
+complete ()
+{
+	# do nothing
+	return 0
+}
+
+. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+
+print_comp ()
+{
+	local IFS=$'\n'
+	echo "${COMPREPLY[*]}" > out
+}
+
+print_words ()
+{
+	local w=(foo foo. --foo=bar)
+	local IFS=$'\n'
+	echo "${w[*]}"
+}
+
+_get_comp_words_by_ref ()
+{
+	while [ $# -gt 0 ]; do
+		case "$1" in
+		cur)
+			cur=${_words[_cword]}
+			;;
+		prev)
+			prev=${_words[_cword-1]}
+			;;
+		words)
+			words=("${_words[@]}")
+			;;
+		cword)
+			cword=$_cword
+			;;
+		esac
+		shift
+	done
+}
+
+test_completion ()
+{
+	local -a COMPREPLY _words
+	local _cword
+	_words=( $1 )
+	(( _cword = ${#_words[@]} - 1 ))
+	_git && print_comp &&
+	test_cmp expected out
+}
+
+test_expect_success 'git' '
+	cat >expected <<-\EOF &&
+	help 
+	add 
+	gc 
+	reflog 
+	get-tar-commit-id 
+	relink 
+	grep 
+	relink.perl 
+	am 
+	remote 
+	am.sh 
+	help 
+	annotate 
+	apply 
+	archimport.perl 
+	imap-send 
+	archive 
+	bisect 
+	init 
+	repack 
+	bisect.sh 
+	instaweb 
+	repack.sh 
+	blame 
+	instaweb.sh 
+	replace 
+	branch 
+	log 
+	bundle 
+	request-pull 
+	lost-found.sh 
+	request-pull.sh 
+	reset 
+	checkout 
+	cherry 
+	revert 
+	cherry-pick 
+	merge 
+	rm 
+	clean 
+	send-email 
+	clone 
+	send-email.perl 
+	commit 
+	config 
+	shortlog 
+	credential-cache 
+	show 
+	show-branch 
+	credential-store 
+	cvsexportcommit.perl 
+	stage 
+	stash 
+	cvsimport.perl 
+	stash.sh 
+	mergetool 
+	status 
+	cvsserver.perl 
+	mergetool.sh 
+	submodule 
+	describe 
+	submodule.sh 
+	diff 
+	mv 
+	svn 
+	name-rev 
+	svn.perl 
+	notes 
+	tag 
+	difftool 
+	difftool.perl 
+	fetch 
+	pull 
+	pull.sh 
+	filter-branch 
+	push 
+	filter-branch.sh 
+	quiltimport.sh 
+	format-patch 
+	rebase 
+	fsck 
+	rebase.sh 
+	whatchanged 
+	cccmd 
+	proxy 
+	send-pull 
+	EOF
+	test_completion "git" &&
+
+	cat >expected <<-\EOF &&
+	help 
+	help 
+	EOF
+	test_completion "git he"
+
+	cat >expected <<-\EOF &&
+	fetch 
+	filter-branch 
+	filter-branch.sh 
+	format-patch 
+	fsck 
+	EOF
+	test_completion "git f"
+'
+
+test_expect_success 'git (double dash)' '
+	cat >expected <<-\EOF &&
+	--paginate 
+	--no-pager 
+	--git-dir=
+	--bare 
+	--version 
+	--exec-path 
+	--html-path 
+	--work-tree=
+	--namespace=
+	--help 
+	EOF
+	test_completion "git --"
+
+	cat >expected <<-\EOF &&
+	--quiet 
+	--ours 
+	--theirs 
+	--track 
+	--no-track 
+	--merge 
+	--conflict=
+	--orphan 
+	--patch 
+	EOF
+	test_completion "git checkout --"
+'
+
+test_done
-- 
1.7.9.6

Re: [RFC/PATCH] tests: add initial bash completion tests

[Jeff King]

On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:

> Something is better than nothing.

Yes, but...

> --- /dev/null
> +++ b/t/t9902-completion.sh
> @@ -0,0 +1,198 @@
> +#!/bin/bash

Won't this test break "make test" on systems without bash (or with bash
elsewhere)?

I think you need to start with something like:

  #!/bin/sh

  if ! type bash; then
          echo '1..0 # SKIP skipping bash tests, bash not available'
          exit 0
  fi

  bash <<\END_OF_BASH_SCRIPT

  test_description='test bash completion'
  . ./test-lib.sh

  [... actual tests ...]

  test_done

  END_OF_BASH_SCRIPT

You could also run the main harness in the outer sh script, and just run
bash inside each test, but I suspect that would end up cumbersome. I
don't really care strongly which, as long as it gracefully skips on
non-bash systems.

-Peff

Re: [RFC/PATCH] tests: add initial bash completion tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:
>
>> Something is better than nothing.
>
> Yes, but...

;-)

This is a good example that sometimes something is worse than nothing,
unless watched carefully by a competent reviewer.

Your suggestion makes sense to me.

Re: [RFC/PATCH] tests: add initial bash completion tests

[Felipe Contreras]

On Fri, Apr 6, 2012 at 11:19 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:

> I think you need to start with something like:
>
>  #!/bin/sh

That is irrelevant, even if it's '#!/bin/foobar', it wouldn't make any
difference since the actual command would be something like
'$(SHELL_PATH) t9902-completion.sh'. Plus /bin/sh does not always
point to bash, even when bash is available (see debian).

-- 
Felipe Contreras

Re: [RFC/PATCH] tests: add initial bash completion tests

[Felipe Contreras]

On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:
>>
>>> Something is better than nothing.
>>
>> Yes, but...
>
> ;-)
>
> This is a good example that sometimes something is worse than nothing,
> unless watched carefully by a competent reviewer.

And this is a good example of why you shouldn't blindly trust what a
'competent reviewer' says, as I'm pretty sure the comment is wrong.

But hey, if you prefer nothing, fine, drop this patch; let's continue
to blindly modify the completion and fix regressions as they come. I
guess I should drop my other tests as well.

I was planning to do a bit of reorganization to the bash completion
after having some basic tests, but I'll just jump straight away to the
actual patches then.

Cheers.

-- 
Felipe Contreras

Re: [RFC/PATCH] tests: add initial bash completion tests

[Jeff King]

On Sat, Apr 07, 2012 at 12:21:35AM +0300, Felipe Contreras wrote:

> On Fri, Apr 6, 2012 at 11:19 PM, Jeff King <peff@peff.net> wrote:
> > On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:
> 
> > I think you need to start with something like:
> >
> >  #!/bin/sh
> 
> That is irrelevant, even if it's '#!/bin/foobar', it wouldn't make any
> difference since the actual command would be something like
> '$(SHELL_PATH) t9902-completion.sh'.

True; I thought "prove" would run them directly, but even it uses
$(SHELL_PATH) to run the tests. However, doesn't that mean your test
will fail completely when $(SHELL_PATH) isn't bash?

So yes, the #! isn't relevant to "make test" (though marking it as
#!/bin/sh does serve as documentation for what you expect, and does let
people with a sane /bin/sh more easily run the test directly).

But my point still stands that you cannot assume that you are running
bash, and you need to either find bash, or gracefully exit the test
script if it is not available. Anything else will cause "make test" to
fail on some systems (and indeed, applying and running your test, it
breaks "make test" on my debian box with dash as /bin/sh).

>Plus /bin/sh does not always point to bash, even when bash is available
>(see debian).

Yes, that was my point, and why the example code I showed executed bash
explicitly instead of relying on $(SHELL_PATH) to be set to it.

-Peff

Re: [RFC/PATCH] tests: add initial bash completion tests

[Jeff King]

On Sat, Apr 07, 2012 at 12:28:05AM +0300, Felipe Contreras wrote:

> On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Jeff King <peff@peff.net> writes:
> >
> >> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:
> >>
> >>> Something is better than nothing.
> >>
> >> Yes, but...
> >
> > ;-)
> >
> > This is a good example that sometimes something is worse than nothing,
> > unless watched carefully by a competent reviewer.
> 
> And this is a good example of why you shouldn't blindly trust what a
> 'competent reviewer' says, as I'm pretty sure the comment is wrong.
> 
> But hey, if you prefer nothing, fine, drop this patch; let's continue
> to blindly modify the completion and fix regressions as they come. I
> guess I should drop my other tests as well.

Sorry, but I think you are wrong, and this is a perfect example of why
you are sometimes frustrating to work with. Your patch is definitely a
move in the right direction, and we would love to have something like it
as part of git. And I'm sure it runs fine under _your_ setup. But the
git community is much larger than just your setup, and your patch is a
regression for other people, as it breaks "make test".

Did I say "let's throw away this patch"? No. I said "here is a problem
with your patch", and I even sketched out a fix.

And nor do I think Junio was saying "let's throw it out". I think he was
responding specifically to "something is better than nothing". It's
_not_ better if it regresses other cases. So the patch as-is is not
acceptable, but it could be made so.

But instead of taking the constructive criticism and iterating on your
patch, you are ready to withdraw your patch. That seems silly when you
have already done the hard part, and the fix to make the patch
acceptable is the easy part.

But maybe I am wrong. Maybe there is no problem at all with your patch,
and my analysis is wrong, and yours is right. I am willing to admit that
as a possibility. But let's discuss it in the other part of the thread
and find out, shall we?

-Peff

Re: [RFC/PATCH] tests: add initial bash completion tests

[Junio C Hamano]

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

> On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:
>>>
>>>> Something is better than nothing.
>>>
>>> Yes, but...
>>
>> ;-)
>>
>> This is a good example that sometimes something is worse than nothing,
>> unless watched carefully by a competent reviewer.
>
> And this is a good example of why you shouldn't blindly trust what a
> 'competent reviewer' says, as I'm pretty sure the comment is wrong.

We run the tests under whatever is configured as SHELL_PATH, so whatever
you have on #! line does not matter much (except as a documentation).

But it would not make any sense to running the bash completion tests if
the shell that is running the test script is *not* bash, would it?  That
is the point Peff made---the primary thing his message cared about is not
to cause "make test" fail when your build does not use bash to run tests.

And that seems to have escaped you.

> But hey, if you prefer nothing, fine, drop this patch;...

Adding a test for bash completion is a good thing, and blindly modifying
the completion script without having good tests is a bad idea.

Just add the tests in the right way. Don't break tests for non-bash users.

Re: [RFC/PATCH] tests: add initial bash completion tests

[Felipe Contreras]

On Sat, Apr 7, 2012 at 12:34 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Apr 07, 2012 at 12:21:35AM +0300, Felipe Contreras wrote:
>
>> On Fri, Apr 6, 2012 at 11:19 PM, Jeff King <peff@peff.net> wrote:
>> > On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:
>>
>> > I think you need to start with something like:
>> >
>> >  #!/bin/sh
>>
>> That is irrelevant, even if it's '#!/bin/foobar', it wouldn't make any
>> difference since the actual command would be something like
>> '$(SHELL_PATH) t9902-completion.sh'.
>
> True; I thought "prove" would run them directly, but even it uses
> $(SHELL_PATH) to run the tests. However, doesn't that mean your test
> will fail completely when $(SHELL_PATH) isn't bash?

Yes.

> So yes, the #! isn't relevant to "make test" (though marking it as
> #!/bin/sh does serve as documentation for what you expect,

No, it's the opposite; #!/bin/sh would imply that you can run this
with any POSIX shell; you can't. For example, if /bin/sh points to
dash (which is the case in debian AFAIK), the test would fail in the
middle of it. #!/bin/bash would be more proper; it would properly
document that you need bash to run this. Sure, maybe bash is not in
/bin (I have never seen that), but that would not affect 'make tests',
only the people that want to run this directly, which I suspect are
very, *very* few, and they would immediately see a clear error:
'./blah: /bin/bash: bad interpreter: No such file or directory', and
then they could easily try 'bash ./blah'.

> But my point still stands that you cannot assume that you are running
> bash, and you need to either find bash, or gracefully exit the test
> script if it is not available. Anything else will cause "make test" to
> fail on some systems (and indeed, applying and running your test, it
> breaks "make test" on my debian box with dash as /bin/sh).

So? 'make test' fails on my Arch Linux box which doesn't have
/usr/bin/python, which is presumably why there is NO_PYTHON. Instead
of doing some nasty hacks such as 'bash <<\END_OF_BASH_SCRIPT', it
would be much easier to have a NO_BASH option. And in fact, when zsh
completion tests are available, NO_ZSH (probably on by default).

But you clearly prefer the status quo, so I'm not going to bother.

-- 
Felipe Contreras

Re: [RFC/PATCH] tests: add initial bash completion tests

[Felipe Contreras]

On Sat, Apr 7, 2012 at 12:42 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Apr 07, 2012 at 12:28:05AM +0300, Felipe Contreras wrote:
>
>> On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Jeff King <peff@peff.net> writes:
>> >
>> >> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:
>> >>
>> >>> Something is better than nothing.
>> >>
>> >> Yes, but...
>> >
>> > ;-)
>> >
>> > This is a good example that sometimes something is worse than nothing,
>> > unless watched carefully by a competent reviewer.
>>
>> And this is a good example of why you shouldn't blindly trust what a
>> 'competent reviewer' says, as I'm pretty sure the comment is wrong.
>>
>> But hey, if you prefer nothing, fine, drop this patch; let's continue
>> to blindly modify the completion and fix regressions as they come. I
>> guess I should drop my other tests as well.
>
> Sorry, but I think you are wrong, and this is a perfect example of why
> you are sometimes frustrating to work with. Your patch is definitely a
> move in the right direction, and we would love to have something like it
> as part of git. And I'm sure it runs fine under _your_ setup. But the
> git community is much larger than just your setup, and your patch is a
> regression for other people, as it breaks "make test".
>
> Did I say "let's throw away this patch"? No. I said "here is a problem
> with your patch", and I even sketched out a fix.

My comment was to Junio, not to you; Junio said "sometimes something
is worse than nothing"; if that's not preferring nothing, I don't know
what it is.

I just sent an email where I assumed you preferred the status quo,
sorry if I misinterpreted you, but my last patches to add some tests
were never applied because you advocated the status quo; no tests.

> And nor do I think Junio was saying "let's throw it out". I think he was
> responding specifically to "something is better than nothing". It's
> _not_ better if it regresses other cases. So the patch as-is is not
> acceptable, but it could be made so.

Did you notice the "RFC" part of it? This patch was not meant to be
applied as-is.

> But instead of taking the constructive criticism and iterating on your
> patch, you are ready to withdraw your patch. That seems silly when you
> have already done the hard part, and the fix to make the patch
> acceptable is the easy part.

I have already done the "hard part" in the past and yet my patches
were dropped because of commit message wording (or whatever), and we
ended up with the status quo; no tests. So excuse me if I'm not eager
to engage in a discussion in which you provide some "constructive
criticism", Junio agrees, I disagree, and the patch never goes
anywhere.

> But maybe I am wrong. Maybe there is no problem at all with your patch,
> and my analysis is wrong, and yours is right. I am willing to admit that
> as a possibility. But let's discuss it in the other part of the thread
> and find out, shall we?

I didn't say there was no problem with the patch, I said the
*particular* problem that you pointed out was incorrect (the shebang).

But OK, I will try once more to provide a more proper solution, let's
see how it goes.

Cheers.

-- 
Felipe Contreras

Re: [RFC/PATCH] tests: add initial bash completion tests

[Junio C Hamano]

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

> On Sat, Apr 7, 2012 at 12:34 AM, Jeff King <peff@peff.net> wrote:
>
>> But my point still stands that you cannot assume that you are running
>> bash, and you need to either find bash, or gracefully exit the test
>> script if it is not available. Anything else will cause "make test" to
>> fail on some systems (and indeed, applying and running your test, it
>> breaks "make test" on my debian box with dash as /bin/sh).
>
> So? 'make test' fails on my Arch Linux box which doesn't have
> /usr/bin/python, which is presumably why there is NO_PYTHON.

If you "git grep NO_PYTHON", you would notice that t/test-lib.sh does have
a provision to set PYTHON prerequisite, so fixing it presumably is just
the matter of marking appropriate tests with the prerequisite, no?  Which
ones are broken for you?  Complaining about it in a thread that does not
directly related to that "on a box without python some tests are broken"
issue is a very good way to leave it unfixed.

> Instead
> of doing some nasty hacks such as 'bash <<\END_OF_BASH_SCRIPT', it
> would be much easier to have a NO_BASH option. And in fact, when zsh
> completion tests are available, NO_ZSH (probably on by default).

"The box does not have bash" and "The builder does not wish to let the
scripts run with bash" are two separate things.  SHELL_PATH is often set
to /bin/dash even on a box that has /bin/bash because it is much faster to
run scripted Porcelains with, but the end user of the resulting build may
still want to use bash in her interactive session.

For this "the rest of the script is all meant to test stuff written for
bash" test, I think the right approach is to explicitly detect the
presense of bash (i.e. "Can the end user run bash for her interactive
session?  Otherwise we cannot test and there is no point testing"), and
feed these tests explicitly to bash.  Perhaps by putting your bash
specific test in t/t9902/bash-completion-test.bash and calling it from the
t9902-completionl.sh script that is meant to be run with $SHELL_PATH, like
this:

== t/t9902-completion.sh ==
#!/bin/sh

. ./test-lib.sh

if bash is available ;# do *not* check if we are running under bash!!
then
	bash "$TEST_DIRECTORY/t9902/bash-comletion-test.bash"
fi

if zsh is available ;# do *not* check if we are running under zsh!!
then
	zsh "$TEST_DIRECTORY/t9902/zsh-comletion-test.zsh"
fi

test_done

Re: [RFC/PATCH] tests: add initial bash completion tests

[Felipe Contreras]

On Fri, Apr 6, 2012 at 11:19 PM, Jeff King <peff@peff.net> wrote:

> I think you need to start with something like:
>
>  #!/bin/sh
>
>  if ! type bash; then
>          echo '1..0 # SKIP skipping bash tests, bash not available'
>          exit 0
>  fi

What about this instead?

---
#!/bin/sh

if type bash >/dev/null 2>&1
then
	# execute inside bash
	[ -z "$BASH" ] && exec bash $0
else
	echo '1..0 #SKIP skipping bash completion tests; bash not available'
	exit 0
fi
---

-- 
Felipe Contreras

Re: [RFC/PATCH] tests: add initial bash completion tests

[Junio C Hamano]

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

> What about this instead?
>
> #!/bin/sh
>
> if type bash >/dev/null 2>&1
> then
> 	# execute inside bash
> 	[ -z "$BASH" ] && exec bash $0
> else
> 	echo '1..0 #SKIP skipping bash completion tests; bash not available'
> 	exit 0
> fi

Please follow the CodingGuidelines and use

	test -z "$BASH" && ...

instead.

Other than that, it is very much simple and straight-forward.  I like it.

Re: [RFC/PATCH] tests: add initial bash completion tests

[Felipe Contreras]

On Sat, Apr 7, 2012 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sat, Apr 7, 2012 at 12:34 AM, Jeff King <peff@peff.net> wrote:
>>
>>> But my point still stands that you cannot assume that you are running
>>> bash, and you need to either find bash, or gracefully exit the test
>>> script if it is not available. Anything else will cause "make test" to
>>> fail on some systems (and indeed, applying and running your test, it
>>> breaks "make test" on my debian box with dash as /bin/sh).
>>
>> So? 'make test' fails on my Arch Linux box which doesn't have
>> /usr/bin/python, which is presumably why there is NO_PYTHON.
>
> If you "git grep NO_PYTHON", you would notice that t/test-lib.sh does have
> a provision to set PYTHON prerequisite,

Yes, but you have to specify NO_PYTHON, otherwise it would just assume
python is installed.

> so fixing it presumably is just
> the matter of marking appropriate tests with the prerequisite, no?

You would still have to specify NO_PYTHON.

> Which ones are broken for you?

In fact, I do have python, but the program name is python2, but if I do this:

 export PYTHON_PATH=/usr/bin/python2

The tests fail on remote-helpers.

> Complaining about it in a thread that does not
> directly related to that "on a box without python some tests are broken"
> issue is a very good way to leave it unfixed.

My point is that the argument "this is worst than nothing because
tests would break on X systems" seems rather weak to me considering
that tests are already broken on Y systems.

>> Instead
>> of doing some nasty hacks such as 'bash <<\END_OF_BASH_SCRIPT', it
>> would be much easier to have a NO_BASH option. And in fact, when zsh
>> completion tests are available, NO_ZSH (probably on by default).
>
> "The box does not have bash" and "The builder does not wish to let the
> scripts run with bash" are two separate things.  SHELL_PATH is often set
> to /bin/dash even on a box that has /bin/bash because it is much faster to
> run scripted Porcelains with, but the end user of the resulting build may
> still want to use bash in her interactive session.

So? SHELL_PATH would not interfere with NO_BASH/NO_ZSH. But in any
case, I've sent a patch that detects bash presence, similarly to
Jeff's patch.

Cheers.

-- 
Felipe Contreras

Re: [RFC/PATCH] tests: add initial bash completion tests

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> What about this instead?
>>
>> #!/bin/sh
>>
>> if type bash >/dev/null 2>&1
>> then
>> 	# execute inside bash
>> 	[ -z "$BASH" ] && exec bash $0
>> else
>> 	echo '1..0 #SKIP skipping bash completion tests; bash not available'
>> 	exit 0
>> fi
> ...
> Other than that, it is very much simple and straight-forward.  I like it.

It probably is even simpler to do it like this.

if test -z "$BASH" && ! exec bash "$0" "$@"
then
	echo '1..0 #SKIP...'
        exit 0
fi

... rest of the test starting with ". ./test-lib.sh" ...

Re: [RFC/PATCH] tests: add initial bash completion tests

[Jeff King]

On Sat, Apr 07, 2012 at 01:05:51AM +0300, Felipe Contreras wrote:

> > So yes, the #! isn't relevant to "make test" (though marking it as
> > #!/bin/sh does serve as documentation for what you expect,
> 
> No, it's the opposite; #!/bin/sh would imply that you can run this
> with any POSIX shell; you can't.

I think you are conflating the test _harness_ with the contents of the
tests themselves.  The harness must run in a POSIX shell, because it
needs to run everywhere and at least say "sorry, I must skip these tests
because I don't have bash".

This issue is made more confusing by the fact that the tool you are
testing is itself a shell. But let us imagine for a moment that you want
to test some tool "foo" that may or may not exist on the system. Then
you would execute the harness via the POSIX shell. Then you would first
check whether "foo" is available (either using "type", or by checking a
build option like NO_FOO). If not, then you would skip all tests, and if
so, you would continue to perform tests using "foo".

So we want the same effect here; you must be able to run the bare
minimum of harness to get to the "skip or run" decision with a POSIX
shell. So you could write it as a pure-sh harness that runs:

  bash -c '. git-completion.bash &&
           some_actual_test'

in each test (or some equivalent). But because the tool you are testing
is in fact a POSIX-compatible shell, you have chosen to instead run all
of the post-skip-decision parts directly inside the harness. Which is
fine by me, but I think is what makes the issue more confusing.

So what is the appropriate shebang line? The first part _must_ be POSIX
shell, but the latter part need not be. I would argue for the lowest
common denominator, as that is what you need for the script to get to
the decision point (and possibly not just exit, but actually run bash).

> For example, if /bin/sh points to
> dash (which is the case in debian AFAIK), the test would fail in the
> middle of it. #!/bin/bash would be more proper; it would properly
> document that you need bash to run this. Sure, maybe bash is not in
> /bin (I have never seen that),

You see this on systems where bash is not part of the base system (e.g.,
Solaris). I think it is also the case on FreeBSD (because bash comes
from ports and gets installed in /usr/local), though it has been years
since I have run FreeBSD, so that may no longer be the case.

> but that would not affect 'make tests',

The shebang line does not affect people who run "make test", but the
fact that the script does not run under a POSIX shell does (and that is
primarily what I was complaining about).

> only the people that want to run this directly, which I suspect are
> very, *very* few, and they would immediately see a clear error:
> './blah: /bin/bash: bad interpreter: No such file or directory', and
> then they could easily try 'bash ./blah'.

Yes, but why make them do that? The script must be able to run and at
least exit gracefully under a POSIX shell (or hopefully find and exec
bash itself) in order for "make test" to work. So since it must contain
code to handle this already, why not have #!/bin/sh in the shebang line
and simply let the code run as it would under "make test"?

> So? 'make test' fails on my Arch Linux box which doesn't have
> /usr/bin/python, which is presumably why there is NO_PYTHON.

We design our test suite to succeed everywhere, and Junio does not push
out master unless it passes (or skips) all tests. If it doesn't, then
either there is a build option (like NO_PYTHON) that you should set, or
there is a bug which should be fixed.  But that is not an excuse to
blatantly violate the existing property of the suite with a new test.

> Instead of doing some nasty hacks such as 'bash
> <<\END_OF_BASH_SCRIPT', it would be much easier to have a NO_BASH
> option. And in fact, when zsh completion tests are available, NO_ZSH
> (probably on by default).

Yeah, we could do that. Though I think it is nice to run the bash tests
even when $SHELL_PATH does not point to bash, since it gets test
coverage on systems that would otherwise not have it (e.g., on debian
systems where /bin/sh is dash). So I would prefer to find and run bash
if it exists, and save NO_BASH for the case of "yes, I have bash, but it
is old or broken, so do not bother to run the tests".

> But you clearly prefer the status quo, so I'm not going to bother.

I'm not even sure what to make of this. I've already said I like the
concept of your patch, that it does not meet the requirements of the
test suite, and tried to work out a solution for meeting that
requirement so we can apply it. How in the world does that equate to the
status quo?

-Peff

Re: [RFC/PATCH] tests: add initial bash completion tests

[Jeff King]

On Sat, Apr 07, 2012 at 02:12:22AM +0300, Felipe Contreras wrote:

> On Fri, Apr 6, 2012 at 11:19 PM, Jeff King <peff@peff.net> wrote:
> 
> > I think you need to start with something like:
> >
> >  #!/bin/sh
> >
> >  if ! type bash; then
> >          echo '1..0 # SKIP skipping bash tests, bash not available'
> >          exit 0
> >  fi
> 
> What about this instead?
> 
> ---
> #!/bin/sh
> 
> if type bash >/dev/null 2>&1
> then
> 	# execute inside bash
> 	[ -z "$BASH" ] && exec bash $0
> else
> 	echo '1..0 #SKIP skipping bash completion tests; bash not available'
> 	exit 0
> fi
> ---

Yes, that's fine. It should be functionally equivalent to what I posted,
but without having to invoke an extra bash when we are already running
it (and avoiding the giant here-doc, though that is simply ugly and not
a functional problem).

One minor fix. I think it should be:

  exec bash $0 "$@"

to pass arguments (e.g., "-v -i") to the bash sub-process.

-Peff

Re: [RFC/PATCH] tests: add initial bash completion tests

[Felipe Contreras]

On Sat, Apr 7, 2012 at 2:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> What about this instead?
>>>
>>> #!/bin/sh
>>>
>>> if type bash >/dev/null 2>&1
>>> then
>>>      # execute inside bash
>>>      [ -z "$BASH" ] && exec bash $0
>>> else
>>>      echo '1..0 #SKIP skipping bash completion tests; bash not available'
>>>      exit 0
>>> fi
>> ...
>> Other than that, it is very much simple and straight-forward.  I like it.
>
> It probably is even simpler to do it like this.
>
> if test -z "$BASH" && ! exec bash "$0" "$@"

That would exit immediately with an error:

t9902-completion.sh: 6: exec: bash: not found
make: *** [t9902-completion.sh] Error 127

-- 
Felipe Contreras