{"thread":{"id":"30171","subject":"[RFC/PATCH] tests: add initial bash completion tests","startedAt":"2012-04-06T19:28:39Z","lastAt":"2012-04-07T00:02:11Z","messageCount":18,"participants":["Felipe Contreras","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188680","messageId":"1333740519-3792-1-git-send-email-felipe.contreras@gmail.com","threadId":"30171","inReplyTo":null,"subject":"[RFC/PATCH] tests: add initial bash completion tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-04-06T19:28:39Z","receivedAt":"2012-04-06T19:28:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n\nSomething is better than nothing.\n\n t/t9902-completion.sh | 198 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 198 insertions(+)\n create mode 100755 t/t9902-completion.sh\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nnew file mode 100755\nindex 0000000..a037dff\n--- /dev/null\n+++ b/t/t9902-completion.sh\n@@ -0,0 +1,198 @@\n+#!/bin/bash\n+#\n+# Copyright (c) 2012 Felipe Contreras\n+#\n+\n+test_description='test bash completion'\n+\n+. ./test-lib.sh\n+\n+complete ()\n+{\n+\t# do nothing\n+\treturn 0\n+}\n+\n+. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\"\n+\n+print_comp ()\n+{\n+\tlocal IFS=$'\\n'\n+\techo \"${COMPREPLY[*]}\" > out\n+}\n+\n+print_words ()\n+{\n+\tlocal w=(foo foo. --foo=bar)\n+\tlocal IFS=$'\\n'\n+\techo \"${w[*]}\"\n+}\n+\n+_get_comp_words_by_ref ()\n+{\n+\twhile [ $# -gt 0 ]; do\n+\t\tcase \"$1\" in\n+\t\tcur)\n+\t\t\tcur=${_words[_cword]}\n+\t\t\t;;\n+\t\tprev)\n+\t\t\tprev=${_words[_cword-1]}\n+\t\t\t;;\n+\t\twords)\n+\t\t\twords=(\"${_words[@]}\")\n+\t\t\t;;\n+\t\tcword)\n+\t\t\tcword=$_cword\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone\n+}\n+\n+test_completion ()\n+{\n+\tlocal -a COMPREPLY _words\n+\tlocal _cword\n+\t_words=( $1 )\n+\t(( _cword = ${#_words[@]} - 1 ))\n+\t_git && print_comp &&\n+\ttest_cmp expected out\n+}\n+\n+test_expect_success 'git' '\n+\tcat >expected <<-\\EOF &&\n+\thelp \n+\tadd \n+\tgc \n+\treflog \n+\tget-tar-commit-id \n+\trelink \n+\tgrep \n+\trelink.perl \n+\tam \n+\tremote \n+\tam.sh \n+\thelp \n+\tannotate \n+\tapply \n+\tarchimport.perl \n+\timap-send \n+\tarchive \n+\tbisect \n+\tinit \n+\trepack \n+\tbisect.sh \n+\tinstaweb \n+\trepack.sh \n+\tblame \n+\tinstaweb.sh \n+\treplace \n+\tbranch \n+\tlog \n+\tbundle \n+\trequest-pull \n+\tlost-found.sh \n+\trequest-pull.sh \n+\treset \n+\tcheckout \n+\tcherry \n+\trevert \n+\tcherry-pick \n+\tmerge \n+\trm \n+\tclean \n+\tsend-email \n+\tclone \n+\tsend-email.perl \n+\tcommit \n+\tconfig \n+\tshortlog \n+\tcredential-cache \n+\tshow \n+\tshow-branch \n+\tcredential-store \n+\tcvsexportcommit.perl \n+\tstage \n+\tstash \n+\tcvsimport.perl \n+\tstash.sh \n+\tmergetool \n+\tstatus \n+\tcvsserver.perl \n+\tmergetool.sh \n+\tsubmodule \n+\tdescribe \n+\tsubmodule.sh \n+\tdiff \n+\tmv \n+\tsvn \n+\tname-rev \n+\tsvn.perl \n+\tnotes \n+\ttag \n+\tdifftool \n+\tdifftool.perl \n+\tfetch \n+\tpull \n+\tpull.sh \n+\tfilter-branch \n+\tpush \n+\tfilter-branch.sh \n+\tquiltimport.sh \n+\tformat-patch \n+\trebase \n+\tfsck \n+\trebase.sh \n+\twhatchanged \n+\tcccmd \n+\tproxy \n+\tsend-pull \n+\tEOF\n+\ttest_completion \"git\" &&\n+\n+\tcat >expected <<-\\EOF &&\n+\thelp \n+\thelp \n+\tEOF\n+\ttest_completion \"git he\"\n+\n+\tcat >expected <<-\\EOF &&\n+\tfetch \n+\tfilter-branch \n+\tfilter-branch.sh \n+\tformat-patch \n+\tfsck \n+\tEOF\n+\ttest_completion \"git f\"\n+'\n+\n+test_expect_success 'git (double dash)' '\n+\tcat >expected <<-\\EOF &&\n+\t--paginate \n+\t--no-pager \n+\t--git-dir=\n+\t--bare \n+\t--version \n+\t--exec-path \n+\t--html-path \n+\t--work-tree=\n+\t--namespace=\n+\t--help \n+\tEOF\n+\ttest_completion \"git --\"\n+\n+\tcat >expected <<-\\EOF &&\n+\t--quiet \n+\t--ours \n+\t--theirs \n+\t--track \n+\t--no-track \n+\t--merge \n+\t--conflict=\n+\t--orphan \n+\t--patch \n+\tEOF\n+\ttest_completion \"git checkout --\"\n+'\n+\n+test_done\n-- \n1.7.9.6\n"},{"id":"188685","messageId":"20120406201926.GA1677@sigill.intra.peff.net","threadId":"30171","inReplyTo":"1333740519-3792-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-06T20:19:26Z","receivedAt":"2012-04-06T20:19:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:\n\n> Something is better than nothing.\n\nYes, but...\n\n> --- /dev/null\n> +++ b/t/t9902-completion.sh\n> @@ -0,0 +1,198 @@\n> +#!/bin/bash\n\nWon't this test break \"make test\" on systems without bash (or with bash\nelsewhere)?\n\nI think you need to start with something like:\n\n #!/bin/sh\n\n if ! type bash; then\n echo '1..0 # SKIP skipping bash tests, bash not available'\n exit 0\n fi\n\n bash <<\\END_OF_BASH_SCRIPT\n\n test_description='test bash completion'\n . ./test-lib.sh\n\n [... actual tests ...]\n\n test_done\n\n END_OF_BASH_SCRIPT\n\nYou could also run the main harness in the outer sh script, and just run\nbash inside each test, but I suspect that would end up cumbersome. I\ndon't really care strongly which, as long as it gracefully skips on\nnon-bash systems.\n\n-Peff\n"},{"id":"188687","messageId":"7v7gxs1ud4.fsf@alter.siamese.dyndns.org","threadId":"30171","inReplyTo":"20120406201926.GA1677@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-06T20:24:39Z","receivedAt":"2012-04-06T20:24:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:\n>\n>> Something is better than nothing.\n>\n> Yes, but...\n\n;-)\n\nThis is a good example that sometimes something is worse than nothing,\nunless watched carefully by a competent reviewer.\n\nYour suggestion makes sense to me.\n"},{"id":"188691","messageId":"CAMP44s0n+dyp-QNOPtiKtGX1VNOsM8GJOLLCksqwPArR+FS8ng@mail.gmail.com","threadId":"30171","inReplyTo":"20120406201926.GA1677@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-04-06T21:21:35Z","receivedAt":"2012-04-06T21:21:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Apr 6, 2012 at 11:19 PM, Jeff King wrote:\n> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:\n\n> I think you need to start with something like:\n>\n>  #!/bin/sh\n\nThat is irrelevant, even if it's '#!/bin/foobar', it wouldn't make any\ndifference since the actual command would be something like\n'$(SHELL_PATH) t9902-completion.sh'. Plus /bin/sh does not always\npoint to bash, even when bash is available (see debian).\n\n-- \nFelipe Contreras\n"},{"id":"188692","messageId":"CAMP44s0xAvc9aTeBP81tXhX6Q67+7cQ-51C1AWrKPN7bc7=JXQ@mail.gmail.com","threadId":"30171","inReplyTo":"7v7gxs1ud4.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-04-06T21:28:05Z","receivedAt":"2012-04-06T21:28:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:\n>>\n>>> Something is better than nothing.\n>>\n>> Yes, but...\n>\n> ;-)\n>\n> This is a good example that sometimes something is worse than nothing,\n> unless watched carefully by a competent reviewer.\n\nAnd this is a good example of why you shouldn't blindly trust what a\n'competent reviewer' says, as I'm pretty sure the comment is wrong.\n\nBut hey, if you prefer nothing, fine, drop this patch; let's continue\nto blindly modify the completion and fix regressions as they come. I\nguess I should drop my other tests as well.\n\nI was planning to do a bit of reorganization to the bash completion\nafter having some basic tests, but I'll just jump straight away to the\nactual patches then.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"188694","messageId":"20120406213448.GA5436@sigill.intra.peff.net","threadId":"30171","inReplyTo":"CAMP44s0n+dyp-QNOPtiKtGX1VNOsM8GJOLLCksqwPArR+FS8ng@mail.gmail.com","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-06T21:34:48Z","receivedAt":"2012-04-06T21:34:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 07, 2012 at 12:21:35AM +0300, Felipe Contreras wrote:\n\n> On Fri, Apr 6, 2012 at 11:19 PM, Jeff King wrote:\n> > On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:\n> \n> > I think you need to start with something like:\n> >\n> >  #!/bin/sh\n> \n> That is irrelevant, even if it's '#!/bin/foobar', it wouldn't make any\n> difference since the actual command would be something like\n> '$(SHELL_PATH) t9902-completion.sh'.\n\nTrue; I thought \"prove\" would run them directly, but even it uses\n$(SHELL_PATH) to run the tests. However, doesn't that mean your test\nwill fail completely when $(SHELL_PATH) isn't bash?\n\nSo yes, the #! isn't relevant to \"make test\" (though marking it as\n#!/bin/sh does serve as documentation for what you expect, and does let\npeople with a sane /bin/sh more easily run the test directly).\n\nBut my point still stands that you cannot assume that you are running\nbash, and you need to either find bash, or gracefully exit the test\nscript if it is not available. Anything else will cause \"make test\" to\nfail on some systems (and indeed, applying and running your test, it\nbreaks \"make test\" on my debian box with dash as /bin/sh).\n\n>Plus /bin/sh does not always point to bash, even when bash is available\n>(see debian).\n\nYes, that was my point, and why the example code I showed executed bash\nexplicitly instead of relying on $(SHELL_PATH) to be set to it.\n\n-Peff\n"},{"id":"188695","messageId":"20120406214219.GB5436@sigill.intra.peff.net","threadId":"30171","inReplyTo":"CAMP44s0xAvc9aTeBP81tXhX6Q67+7cQ-51C1AWrKPN7bc7=JXQ@mail.gmail.com","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-06T21:42:19Z","receivedAt":"2012-04-06T21:42:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 07, 2012 at 12:28:05AM +0300, Felipe Contreras wrote:\n\n> On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano wrote:\n> > Jeff King writes:\n> >\n> >> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:\n> >>\n> >>> Something is better than nothing.\n> >>\n> >> Yes, but...\n> >\n> > ;-)\n> >\n> > This is a good example that sometimes something is worse than nothing,\n> > unless watched carefully by a competent reviewer.\n> \n> And this is a good example of why you shouldn't blindly trust what a\n> 'competent reviewer' says, as I'm pretty sure the comment is wrong.\n> \n> But hey, if you prefer nothing, fine, drop this patch; let's continue\n> to blindly modify the completion and fix regressions as they come. I\n> guess I should drop my other tests as well.\n\nSorry, but I think you are wrong, and this is a perfect example of why\nyou are sometimes frustrating to work with. Your patch is definitely a\nmove in the right direction, and we would love to have something like it\nas part of git. And I'm sure it runs fine under _your_ setup. But the\ngit community is much larger than just your setup, and your patch is a\nregression for other people, as it breaks \"make test\".\n\nDid I say \"let's throw away this patch\"? No. I said \"here is a problem\nwith your patch\", and I even sketched out a fix.\n\nAnd nor do I think Junio was saying \"let's throw it out\". I think he was\nresponding specifically to \"something is better than nothing\". It's\n_not_ better if it regresses other cases. So the patch as-is is not\nacceptable, but it could be made so.\n\nBut instead of taking the constructive criticism and iterating on your\npatch, you are ready to withdraw your patch. That seems silly when you\nhave already done the hard part, and the fix to make the patch\nacceptable is the easy part.\n\nBut maybe I am wrong. Maybe there is no problem at all with your patch,\nand my analysis is wrong, and yours is right. I am willing to admit that\nas a possibility. But let's discuss it in the other part of the thread\nand find out, shall we?\n\n-Peff\n"},{"id":"188696","messageId":"7vsjggzgd9.fsf@alter.siamese.dyndns.org","threadId":"30171","inReplyTo":"CAMP44s0xAvc9aTeBP81tXhX6Q67+7cQ-51C1AWrKPN7bc7=JXQ@mail.gmail.com","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-06T21:42:58Z","receivedAt":"2012-04-06T21:42:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano wrote:\n>> Jeff King writes:\n>>\n>>> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:\n>>>\n>>>> Something is better than nothing.\n>>>\n>>> Yes, but...\n>>\n>> ;-)\n>>\n>> This is a good example that sometimes something is worse than nothing,\n>> unless watched carefully by a competent reviewer.\n>\n> And this is a good example of why you shouldn't blindly trust what a\n> 'competent reviewer' says, as I'm pretty sure the comment is wrong.\n\nWe run the tests under whatever is configured as SHELL_PATH, so whatever\nyou have on #! line does not matter much (except as a documentation).\n\nBut it would not make any sense to running the bash completion tests if\nthe shell that is running the test script is *not* bash, would it? That\nis the point Peff made---the primary thing his message cared about is not\nto cause \"make test\" fail when your build does not use bash to run tests.\n\nAnd that seems to have escaped you.\n\n> But hey, if you prefer nothing, fine, drop this patch;...\n\nAdding a test for bash completion is a good thing, and blindly modifying\nthe completion script without having good tests is a bad idea.\n\nJust add the tests in the right way. Don't break tests for non-bash users.\n"},{"id":"188700","messageId":"CAMP44s0VWWuM6eeij_nys9OAqu6Jr87Wv4K56mbbqhuMbVXKaQ@mail.gmail.com","threadId":"30171","inReplyTo":"20120406213448.GA5436@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-04-06T22:05:51Z","receivedAt":"2012-04-06T22:05:51Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Apr 7, 2012 at 12:34 AM, Jeff King wrote:\n> On Sat, Apr 07, 2012 at 12:21:35AM +0300, Felipe Contreras wrote:\n>\n>> On Fri, Apr 6, 2012 at 11:19 PM, Jeff King wrote:\n>> > On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:\n>>\n>> > I think you need to start with something like:\n>> >\n>> >  #!/bin/sh\n>>\n>> That is irrelevant, even if it's '#!/bin/foobar', it wouldn't make any\n>> difference since the actual command would be something like\n>> '$(SHELL_PATH) t9902-completion.sh'.\n>\n> True; I thought \"prove\" would run them directly, but even it uses\n> $(SHELL_PATH) to run the tests. However, doesn't that mean your test\n> will fail completely when $(SHELL_PATH) isn't bash?\n\nYes.\n\n> So yes, the #! isn't relevant to \"make test\" (though marking it as\n> #!/bin/sh does serve as documentation for what you expect,\n\nNo, it's the opposite; #!/bin/sh would imply that you can run this\nwith any POSIX shell; you can't. For example, if /bin/sh points to\ndash (which is the case in debian AFAIK), the test would fail in the\nmiddle of it. #!/bin/bash would be more proper; it would properly\ndocument that you need bash to run this. Sure, maybe bash is not in\n/bin (I have never seen that), but that would not affect 'make tests',\nonly the people that want to run this directly, which I suspect are\nvery, *very* few, and they would immediately see a clear error:\n'./blah: /bin/bash: bad interpreter: No such file or directory', and\nthen they could easily try 'bash ./blah'.\n\n> But my point still stands that you cannot assume that you are running\n> bash, and you need to either find bash, or gracefully exit the test\n> script if it is not available. Anything else will cause \"make test\" to\n> fail on some systems (and indeed, applying and running your test, it\n> breaks \"make test\" on my debian box with dash as /bin/sh).\n\nSo? 'make test' fails on my Arch Linux box which doesn't have\n/usr/bin/python, which is presumably why there is NO_PYTHON. Instead\nof doing some nasty hacks such as 'bash <<\\END_OF_BASH_SCRIPT', it\nwould be much easier to have a NO_BASH option. And in fact, when zsh\ncompletion tests are available, NO_ZSH (probably on by default).\n\nBut you clearly prefer the status quo, so I'm not going to bother.\n\n-- \nFelipe Contreras\n"},{"id":"188701","messageId":"CAMP44s07JOVQp2xNNdPew5L5_ibyk713m7UoJ6MrKOyKEmTkGw@mail.gmail.com","threadId":"30171","inReplyTo":"20120406214219.GB5436@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-04-06T22:22:12Z","receivedAt":"2012-04-06T22:22:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Apr 7, 2012 at 12:42 AM, Jeff King wrote:\n> On Sat, Apr 07, 2012 at 12:28:05AM +0300, Felipe Contreras wrote:\n>\n>> On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano wrote:\n>> > Jeff King writes:\n>> >\n>> >> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:\n>> >>\n>> >>> Something is better than nothing.\n>> >>\n>> >> Yes, but...\n>> >\n>> > ;-)\n>> >\n>> > This is a good example that sometimes something is worse than nothing,\n>> > unless watched carefully by a competent reviewer.\n>>\n>> And this is a good example of why you shouldn't blindly trust what a\n>> 'competent reviewer' says, as I'm pretty sure the comment is wrong.\n>>\n>> But hey, if you prefer nothing, fine, drop this patch; let's continue\n>> to blindly modify the completion and fix regressions as they come. I\n>> guess I should drop my other tests as well.\n>\n> Sorry, but I think you are wrong, and this is a perfect example of why\n> you are sometimes frustrating to work with. Your patch is definitely a\n> move in the right direction, and we would love to have something like it\n> as part of git. And I'm sure it runs fine under _your_ setup. But the\n> git community is much larger than just your setup, and your patch is a\n> regression for other people, as it breaks \"make test\".\n>\n> Did I say \"let's throw away this patch\"? No. I said \"here is a problem\n> with your patch\", and I even sketched out a fix.\n\nMy comment was to Junio, not to you; Junio said \"sometimes something\nis worse than nothing\"; if that's not preferring nothing, I don't know\nwhat it is.\n\nI just sent an email where I assumed you preferred the status quo,\nsorry if I misinterpreted you, but my last patches to add some tests\nwere never applied because you advocated the status quo; no tests.\n\n> And nor do I think Junio was saying \"let's throw it out\". I think he was\n> responding specifically to \"something is better than nothing\". It's\n> _not_ better if it regresses other cases. So the patch as-is is not\n> acceptable, but it could be made so.\n\nDid you notice the \"RFC\" part of it? This patch was not meant to be\napplied as-is.\n\n> But instead of taking the constructive criticism and iterating on your\n> patch, you are ready to withdraw your patch. That seems silly when you\n> have already done the hard part, and the fix to make the patch\n> acceptable is the easy part.\n\nI have already done the \"hard part\" in the past and yet my patches\nwere dropped because of commit message wording (or whatever), and we\nended up with the status quo; no tests. So excuse me if I'm not eager\nto engage in a discussion in which you provide some \"constructive\ncriticism\", Junio agrees, I disagree, and the patch never goes\nanywhere.\n\n> But maybe I am wrong. Maybe there is no problem at all with your patch,\n> and my analysis is wrong, and yours is right. I am willing to admit that\n> as a possibility. But let's discuss it in the other part of the thread\n> and find out, shall we?\n\nI didn't say there was no problem with the patch, I said the\n*particular* problem that you pointed out was incorrect (the shebang).\n\nBut OK, I will try once more to provide a more proper solution, let's\nsee how it goes.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"188703","messageId":"7vk41szdev.fsf@alter.siamese.dyndns.org","threadId":"30171","inReplyTo":"CAMP44s0VWWuM6eeij_nys9OAqu6Jr87Wv4K56mbbqhuMbVXKaQ@mail.gmail.com","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-06T22:46:48Z","receivedAt":"2012-04-06T22:46:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Sat, Apr 7, 2012 at 12:34 AM, Jeff King wrote:\n>\n>> But my point still stands that you cannot assume that you are running\n>> bash, and you need to either find bash, or gracefully exit the test\n>> script if it is not available. Anything else will cause \"make test\" to\n>> fail on some systems (and indeed, applying and running your test, it\n>> breaks \"make test\" on my debian box with dash as /bin/sh).\n>\n> So? 'make test' fails on my Arch Linux box which doesn't have\n> /usr/bin/python, which is presumably why there is NO_PYTHON.\n\nIf you \"git grep NO_PYTHON\", you would notice that t/test-lib.sh does have\na provision to set PYTHON prerequisite, so fixing it presumably is just\nthe matter of marking appropriate tests with the prerequisite, no? Which\nones are broken for you? Complaining about it in a thread that does not\ndirectly related to that \"on a box without python some tests are broken\"\nissue is a very good way to leave it unfixed.\n\n> Instead\n> of doing some nasty hacks such as 'bash <<\\END_OF_BASH_SCRIPT', it\n> would be much easier to have a NO_BASH option. And in fact, when zsh\n> completion tests are available, NO_ZSH (probably on by default).\n\n\"The box does not have bash\" and \"The builder does not wish to let the\nscripts run with bash\" are two separate things. SHELL_PATH is often set\nto /bin/dash even on a box that has /bin/bash because it is much faster to\nrun scripted Porcelains with, but the end user of the resulting build may\nstill want to use bash in her interactive session.\n\nFor this \"the rest of the script is all meant to test stuff written for\nbash\" test, I think the right approach is to explicitly detect the\npresense of bash (i.e. \"Can the end user run bash for her interactive\nsession? Otherwise we cannot test and there is no point testing\"), and\nfeed these tests explicitly to bash. Perhaps by putting your bash\nspecific test in t/t9902/bash-completion-test.bash and calling it from the\nt9902-completionl.sh script that is meant to be run with $SHELL_PATH, like\nthis:\n\n== t/t9902-completion.sh ==\n#!/bin/sh\n\n. ./test-lib.sh\n\nif bash is available ;# do *not* check if we are running under bash!!\nthen\n\tbash \"$TEST_DIRECTORY/t9902/bash-comletion-test.bash\"\nfi\n\nif zsh is available ;# do *not* check if we are running under zsh!!\nthen\n\tzsh \"$TEST_DIRECTORY/t9902/zsh-comletion-test.zsh\"\nfi\n\ntest_done\n"},{"id":"188704","messageId":"CAMP44s3-Ji33oG=Os_O9KpxawWzZzmiZ04QMTokQ+K6_x-31MA@mail.gmail.com","threadId":"30171","inReplyTo":"20120406201926.GA1677@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-04-06T23:12:22Z","receivedAt":"2012-04-06T23:12:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Apr 6, 2012 at 11:19 PM, Jeff King wrote:\n\n> I think you need to start with something like:\n>\n>  #!/bin/sh\n>\n>  if ! type bash; then\n>          echo '1..0 # SKIP skipping bash tests, bash not available'\n>          exit 0\n>  fi\n\nWhat about this instead?\n\n---\n#!/bin/sh\n\nif type bash >/dev/null 2>&1\nthen\n\t# execute inside bash\n\t[ -z \"$BASH\" ] && exec bash $0\nelse\n\techo '1..0 #SKIP skipping bash completion tests; bash not available'\n\texit 0\nfi\n---\n\n-- \nFelipe Contreras\n"},{"id":"188705","messageId":"7vfwcgzbrd.fsf@alter.siamese.dyndns.org","threadId":"30171","inReplyTo":"CAMP44s3-Ji33oG=Os_O9KpxawWzZzmiZ04QMTokQ+K6_x-31MA@mail.gmail.com","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-06T23:22:30Z","receivedAt":"2012-04-06T23:22:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> What about this instead?\n>\n> #!/bin/sh\n>\n> if type bash >/dev/null 2>&1\n> then\n> \t# execute inside bash\n> \t[ -z \"$BASH\" ] && exec bash $0\n> else\n> \techo '1..0 #SKIP skipping bash completion tests; bash not available'\n> \texit 0\n> fi\n\nPlease follow the CodingGuidelines and use\n\n\ttest -z \"$BASH\" && ...\n\ninstead.\n\nOther than that, it is very much simple and straight-forward. I like it.\n"},{"id":"188706","messageId":"CAMP44s3CA9e06QoJ_-QWoEevfY==CgqZrUu1EKn3zeVDdAyvOA@mail.gmail.com","threadId":"30171","inReplyTo":"7vk41szdev.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-04-06T23:30:07Z","receivedAt":"2012-04-06T23:30:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Apr 7, 2012 at 1:46 AM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Sat, Apr 7, 2012 at 12:34 AM, Jeff King wrote:\n>>\n>>> But my point still stands that you cannot assume that you are running\n>>> bash, and you need to either find bash, or gracefully exit the test\n>>> script if it is not available. Anything else will cause \"make test\" to\n>>> fail on some systems (and indeed, applying and running your test, it\n>>> breaks \"make test\" on my debian box with dash as /bin/sh).\n>>\n>> So? 'make test' fails on my Arch Linux box which doesn't have\n>> /usr/bin/python, which is presumably why there is NO_PYTHON.\n>\n> If you \"git grep NO_PYTHON\", you would notice that t/test-lib.sh does have\n> a provision to set PYTHON prerequisite,\n\nYes, but you have to specify NO_PYTHON, otherwise it would just assume\npython is installed.\n\n> so fixing it presumably is just\n> the matter of marking appropriate tests with the prerequisite, no?\n\nYou would still have to specify NO_PYTHON.\n\n> Which ones are broken for you?\n\nIn fact, I do have python, but the program name is python2, but if I do this:\n\n export PYTHON_PATH=/usr/bin/python2\n\nThe tests fail on remote-helpers.\n\n> Complaining about it in a thread that does not\n> directly related to that \"on a box without python some tests are broken\"\n> issue is a very good way to leave it unfixed.\n\nMy point is that the argument \"this is worst than nothing because\ntests would break on X systems\" seems rather weak to me considering\nthat tests are already broken on Y systems.\n\n>> Instead\n>> of doing some nasty hacks such as 'bash <<\\END_OF_BASH_SCRIPT', it\n>> would be much easier to have a NO_BASH option. And in fact, when zsh\n>> completion tests are available, NO_ZSH (probably on by default).\n>\n> \"The box does not have bash\" and \"The builder does not wish to let the\n> scripts run with bash\" are two separate things.  SHELL_PATH is often set\n> to /bin/dash even on a box that has /bin/bash because it is much faster to\n> run scripted Porcelains with, but the end user of the resulting build may\n> still want to use bash in her interactive session.\n\nSo? SHELL_PATH would not interfere with NO_BASH/NO_ZSH. But in any\ncase, I've sent a patch that detects bash presence, similarly to\nJeff's patch.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"188707","messageId":"7vbon4zb89.fsf@alter.siamese.dyndns.org","threadId":"30171","inReplyTo":"7vfwcgzbrd.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-06T23:33:58Z","receivedAt":"2012-04-06T23:33:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Felipe Contreras writes:\n>\n>> What about this instead?\n>>\n>> #!/bin/sh\n>>\n>> if type bash >/dev/null 2>&1\n>> then\n>> \t# execute inside bash\n>> \t[ -z \"$BASH\" ] && exec bash $0\n>> else\n>> \techo '1..0 #SKIP skipping bash completion tests; bash not available'\n>> \texit 0\n>> fi\n> ...\n> Other than that, it is very much simple and straight-forward. I like it.\n\nIt probably is even simpler to do it like this.\n\nif test -z \"$BASH\" && ! exec bash \"$0\" \"$@\"\nthen\n\techo '1..0 #SKIP...'\n exit 0\nfi\n\n... rest of the test starting with \". ./test-lib.sh\" ...\n"},{"id":"188708","messageId":"20120406234545.GA31750@sigill.intra.peff.net","threadId":"30171","inReplyTo":"CAMP44s0VWWuM6eeij_nys9OAqu6Jr87Wv4K56mbbqhuMbVXKaQ@mail.gmail.com","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-06T23:45:45Z","receivedAt":"2012-04-06T23:45:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 07, 2012 at 01:05:51AM +0300, Felipe Contreras wrote:\n\n> > So yes, the #! isn't relevant to \"make test\" (though marking it as\n> > #!/bin/sh does serve as documentation for what you expect,\n> \n> No, it's the opposite; #!/bin/sh would imply that you can run this\n> with any POSIX shell; you can't.\n\nI think you are conflating the test _harness_ with the contents of the\ntests themselves. The harness must run in a POSIX shell, because it\nneeds to run everywhere and at least say \"sorry, I must skip these tests\nbecause I don't have bash\".\n\nThis issue is made more confusing by the fact that the tool you are\ntesting is itself a shell. But let us imagine for a moment that you want\nto test some tool \"foo\" that may or may not exist on the system. Then\nyou would execute the harness via the POSIX shell. Then you would first\ncheck whether \"foo\" is available (either using \"type\", or by checking a\nbuild option like NO_FOO). If not, then you would skip all tests, and if\nso, you would continue to perform tests using \"foo\".\n\nSo we want the same effect here; you must be able to run the bare\nminimum of harness to get to the \"skip or run\" decision with a POSIX\nshell. So you could write it as a pure-sh harness that runs:\n\n bash -c '. git-completion.bash &&\n some_actual_test'\n\nin each test (or some equivalent). But because the tool you are testing\nis in fact a POSIX-compatible shell, you have chosen to instead run all\nof the post-skip-decision parts directly inside the harness. Which is\nfine by me, but I think is what makes the issue more confusing.\n\nSo what is the appropriate shebang line? The first part _must_ be POSIX\nshell, but the latter part need not be. I would argue for the lowest\ncommon denominator, as that is what you need for the script to get to\nthe decision point (and possibly not just exit, but actually run bash).\n\n> For example, if /bin/sh points to\n> dash (which is the case in debian AFAIK), the test would fail in the\n> middle of it. #!/bin/bash would be more proper; it would properly\n> document that you need bash to run this. Sure, maybe bash is not in\n> /bin (I have never seen that),\n\nYou see this on systems where bash is not part of the base system (e.g.,\nSolaris). I think it is also the case on FreeBSD (because bash comes\nfrom ports and gets installed in /usr/local), though it has been years\nsince I have run FreeBSD, so that may no longer be the case.\n\n> but that would not affect 'make tests',\n\nThe shebang line does not affect people who run \"make test\", but the\nfact that the script does not run under a POSIX shell does (and that is\nprimarily what I was complaining about).\n\n> only the people that want to run this directly, which I suspect are\n> very, *very* few, and they would immediately see a clear error:\n> './blah: /bin/bash: bad interpreter: No such file or directory', and\n> then they could easily try 'bash ./blah'.\n\nYes, but why make them do that? The script must be able to run and at\nleast exit gracefully under a POSIX shell (or hopefully find and exec\nbash itself) in order for \"make test\" to work. So since it must contain\ncode to handle this already, why not have #!/bin/sh in the shebang line\nand simply let the code run as it would under \"make test\"?\n\n> So? 'make test' fails on my Arch Linux box which doesn't have\n> /usr/bin/python, which is presumably why there is NO_PYTHON.\n\nWe design our test suite to succeed everywhere, and Junio does not push\nout master unless it passes (or skips) all tests. If it doesn't, then\neither there is a build option (like NO_PYTHON) that you should set, or\nthere is a bug which should be fixed. But that is not an excuse to\nblatantly violate the existing property of the suite with a new test.\n\n> Instead of doing some nasty hacks such as 'bash\n> <<\\END_OF_BASH_SCRIPT', it would be much easier to have a NO_BASH\n> option. And in fact, when zsh completion tests are available, NO_ZSH\n> (probably on by default).\n\nYeah, we could do that. Though I think it is nice to run the bash tests\neven when $SHELL_PATH does not point to bash, since it gets test\ncoverage on systems that would otherwise not have it (e.g., on debian\nsystems where /bin/sh is dash). So I would prefer to find and run bash\nif it exists, and save NO_BASH for the case of \"yes, I have bash, but it\nis old or broken, so do not bother to run the tests\".\n\n> But you clearly prefer the status quo, so I'm not going to bother.\n\nI'm not even sure what to make of this. I've already said I like the\nconcept of your patch, that it does not meet the requirements of the\ntest suite, and tried to work out a solution for meeting that\nrequirement so we can apply it. How in the world does that equate to the\nstatus quo?\n\n-Peff\n"},{"id":"188709","messageId":"20120406235209.GB31750@sigill.intra.peff.net","threadId":"30171","inReplyTo":"CAMP44s3-Ji33oG=Os_O9KpxawWzZzmiZ04QMTokQ+K6_x-31MA@mail.gmail.com","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-06T23:52:09Z","receivedAt":"2012-04-06T23:52:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 07, 2012 at 02:12:22AM +0300, Felipe Contreras wrote:\n\n> On Fri, Apr 6, 2012 at 11:19 PM, Jeff King wrote:\n> \n> > I think you need to start with something like:\n> >\n> >  #!/bin/sh\n> >\n> >  if ! type bash; then\n> >          echo '1..0 # SKIP skipping bash tests, bash not available'\n> >          exit 0\n> >  fi\n> \n> What about this instead?\n> \n> ---\n> #!/bin/sh\n> \n> if type bash >/dev/null 2>&1\n> then\n> \t# execute inside bash\n> \t[ -z \"$BASH\" ] && exec bash $0\n> else\n> \techo '1..0 #SKIP skipping bash completion tests; bash not available'\n> \texit 0\n> fi\n> ---\n\nYes, that's fine. It should be functionally equivalent to what I posted,\nbut without having to invoke an extra bash when we are already running\nit (and avoiding the giant here-doc, though that is simply ugly and not\na functional problem).\n\nOne minor fix. I think it should be:\n\n exec bash $0 \"$@\"\n\nto pass arguments (e.g., \"-v -i\") to the bash sub-process.\n\n-Peff\n"},{"id":"188710","messageId":"CAMP44s2KjEYsHeufyon8aKj=tL+uuJiVBMnrzbYdePOJs1iZ-A@mail.gmail.com","threadId":"30171","inReplyTo":"7vbon4zb89.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] tests: add initial bash completion tests","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-04-07T00:02:11Z","receivedAt":"2012-04-07T00:02:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Apr 7, 2012 at 2:33 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Felipe Contreras writes:\n>>\n>>> What about this instead?\n>>>\n>>> #!/bin/sh\n>>>\n>>> if type bash >/dev/null 2>&1\n>>> then\n>>>      # execute inside bash\n>>>      [ -z \"$BASH\" ] && exec bash $0\n>>> else\n>>>      echo '1..0 #SKIP skipping bash completion tests; bash not available'\n>>>      exit 0\n>>> fi\n>> ...\n>> Other than that, it is very much simple and straight-forward.  I like it.\n>\n> It probably is even simpler to do it like this.\n>\n> if test -z \"$BASH\" && ! exec bash \"$0\" \"$@\"\n\nThat would exit immediately with an error:\n\nt9902-completion.sh: 6: exec: bash: not found\nmake: *** [t9902-completion.sh] Error 127\n\n-- \nFelipe Contreras\n"}]}