From: Felipe Contreras <hidden> Date: 2016-06-15 22:53:33
Signed-off-by: Felipe Contreras <redacted>
---
Since v1:
* Check if we are running bash in posix mode
* Don't check for all git porcelain commands
t/t9902-completion.sh | 115 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 115 insertions(+)
create mode 100755 t/t9902-completion.sh
From: SZEDER Gábor <hidden> Date: 2016-06-15 22:53:34
Hi,
On Thu, Apr 12, 2012 at 12:57:03AM +0300, Felipe Contreras wrote:
quoted hunk
Signed-off-by: Felipe Contreras <redacted>
---
Since v1:
* Check if we are running bash in posix mode
* Don't check for all git porcelain commands
t/t9902-completion.sh | 115 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 115 insertions(+)
create mode 100755 t/t9902-completion.sh
@@ -0,0 +1,115 @@+#!/bin/sh+#+# Copyright (c) 2012 Felipe Contreras+#++iftest-n"$BASH"&&test-z"$POSIXLY_CORRECT";then+# we are in full-on bash mode+true+eliftypebash>/dev/null2>&1;then+# execute in full-on bash mode+unsetPOSIXLY_CORRECT+execbash"$0""$@"+else+echo'1..0 #SKIP skipping bash completion tests; bash not available'+exit0+fi++test_description='test bash completion'++../test-lib.sh++complete()+{+# do nothing+return0+}++."$GIT_BUILD_DIR/contrib/completion/git-completion.bash"++_get_comp_words_by_ref()+{+while[$#-gt0];do+case"$1"in+cur)+cur=${_words[_cword]}+;;+prev)+prev=${_words[_cword-1]}+;;+words)+words=("${_words[@]}")+;;+cword)+cword=$_cword+;;+esac+shift+done+}
Git's completion script already implements this function. Why
override it here?
grep is not a git command, so I'm not sure, but shouldn't these use
'test_must_fail grep' instead of '! grep'?
Anyway, thanks for pushing this forward. I have a bunch of tests for
my __git_ps1() optimizations, but, being a bash function, I could
never figure out how to integrate it with the test framework.
Best,
Gábor
Git's completion script already implements this function. Why
override it here?
Ah, ok, I think I got it.
Of course, the words on the command line must be specified somehow to
test completion functions. But the two implementations of
_get_comp_words_by_ref() for bash and zsh in the completion script
take the words on the command line from different variables, so we
need a common implementation to test completion functions both on bash
and zsh. Hence the _get_comp_words_by_ref() above, which takes the
words on the command line and their count from $_words and $_cword,
respectively, and run_completion() below, which fills those variables
with its arguments.
quoted
+print_comp ()
+{
+ local IFS=$'\n'
+ echo "${COMPREPLY[*]}" > out
+}
+
+run_completion ()
+{
+ local -a COMPREPLY _words
+ local _cword
+ _words=( $1 )
+ (( _cword = ${#_words[@]} - 1 ))
+ _git && print_comp
+}
Git's completion script already implements this function. Why
override it here?
Ah, ok, I think I got it.
Of course, the words on the command line must be specified somehow to
test completion functions. But the two implementations of
_get_comp_words_by_ref() for bash and zsh in the completion script
take the words on the command line from different variables, so we
need a common implementation to test completion functions both on bash
and zsh. Hence the _get_comp_words_by_ref() above, which takes the
words on the command line and their count from $_words and $_cword,
respectively, and run_completion() below, which fills those variables
with its arguments.
Well, yeah, that's one reason, but also I don't see the point in
trying to fill the internal bash completion variables, maybe there
would be some conflicts? Plus, the bash version of
_get_comp_words_by_ref is rather complicated, so I decided to start
with something simple that I could understand and see exactly what's
going on. And for zsh I would definitely prefer to override
_get_comp_words_by_ref than to mess with the internal variables,
although I haven't found a way to test completion for zsh.
Cheers.
--
Felipe Contreras
grep is not a git command, so I'm not sure, but shouldn't these use
'test_must_fail grep' instead of '! grep'?
I'm not sure. Junio has already queued this, maybe you should send a
patch on top of that.
It seems that both are used in the test suite, but '! grep' is more
common, so perhaps it's good as it is.
$ git grep '! grep' -- t |wc -l
136
$ git grep 'test_must_fail grep' -- t |wc -l
17
Git's completion script already implements this function. Why
override it here?
Ah, ok, I think I got it.
Of course, the words on the command line must be specified somehow to
test completion functions. But the two implementations of
_get_comp_words_by_ref() for bash and zsh in the completion script
take the words on the command line from different variables, so we
need a common implementation to test completion functions both on bash
and zsh. Hence the _get_comp_words_by_ref() above, which takes the
words on the command line and their count from $_words and $_cword,
respectively, and run_completion() below, which fills those variables
with its arguments.
Well, yeah, that's one reason, but also I don't see the point in
trying to fill the internal bash completion variables, maybe there
would be some conflicts? Plus, the bash version of
_get_comp_words_by_ref is rather complicated, so I decided to start
with something simple that I could understand and see exactly what's
going on. And for zsh I would definitely prefer to override
_get_comp_words_by_ref than to mess with the internal variables,
although I haven't found a way to test completion for zsh.
The tests are run in a non-interactive shell, which by default doesn't
load bash completion with its complicated _get_comp_words_by_ref().
So these tests use _get_comp_words_by_ref() from git's completion
script.
Anyway, out of curiosity I quickly tried this on top of b8574ba7 (i.e.
your patch from today's pu):
i.e. to set COMP_WORDS and COMP_CWORD in run_completion() and it
worked. However, I agree that it feels iffy to mess with a
shell-specific variable, and I'm afraid that this just happened to
work on my system, but it might be broken in previous or future bash
versions.
Best,
Gábor
From: Felipe Contreras <hidden> Date: 2016-06-15 22:53:34
2012/4/13 SZEDER Gábor [off-list ref]:
i.e. to set COMP_WORDS and COMP_CWORD in run_completion() and it
worked. However, I agree that it feels iffy to mess with a
shell-specific variable, and I'm afraid that this just happened to
work on my system, but it might be broken in previous or future bash
versions.
Yeah, we could explore that possibility later, as
_get_comp_words_by_ref is part of the completion, and should be tested
as well, otherwise we might be missing some bugs.
However, I wonder if _get_comp_words_by_ref is needed at all. From
what I can see it has to do with '--foo=bar' and 'foo:bar'
completions, which in fact don't work correctly in zsh (I have patches
for zsh to fix this though), but by modifying the code that checks for
'--*=*' stuff we might be able to get rid of it, or at least the call
to __git_reassemble_comp_words_by_ref. Right?
Cheers.
--
Felipe Contreras
From: SZEDER Gábor <hidden> Date: 2016-06-15 22:53:36
Hi,
I picked up Stephen Boyd's two-patch series[1] to use parse-options to
generate options for git commands, and the following test promply
failed (taken from 5c293a6b (tests: add initial bash completion tests,
2012-04-12)):
test_expect_success 'double dash "git checkout"' '
sed -e "s/Z$//" >expected <<-\EOF &&
--quiet Z
--ours Z
--theirs Z
--track Z
--no-track Z
--merge Z
--conflict=
--orphan Z
--patch Z
EOF
test_completion "git checkout --"
'
Not surprising, the completion script doesn't know about many 'git
checkout' long options. So whenever 'git checkout' learns a new long
option, this list must be updated. This won't be more work than the
update of the completion script, so this is probably OK.
But it got me thinking about what do we actually want to test here?
Whether the completion script returns the right long options in a
specific order upon 'git checkout --<TAB>'? Or whether _git() works
properly and invokes the right command-specific completion function?
Or whether regular options get a trailing space while options
expecting an argument don't? Or is this sort of an integration test
and basically all of the above?
[1] - http://thread.gmane.org/gmane.comp.version-control.git/195158/focus=195158
Best,
Gábor
From: Felipe Contreras <hidden> Date: 2016-06-15 22:53:36
On Tue, Apr 17, 2012 at 3:31 AM, SZEDER Gábor [off-list ref] wrote:
Hi,
I picked up Stephen Boyd's two-patch series[1] to use parse-options to
generate options for git commands, and the following test promply
failed (taken from 5c293a6b (tests: add initial bash completion tests,
2012-04-12)):
test_expect_success 'double dash "git checkout"' '
sed -e "s/Z$//" >expected <<-\EOF &&
--quiet Z
--ours Z
--theirs Z
--track Z
--no-track Z
--merge Z
--conflict=
--orphan Z
--patch Z
EOF
test_completion "git checkout --"
'
Not surprising, the completion script doesn't know about many 'git
checkout' long options. So whenever 'git checkout' learns a new long
option, this list must be updated. This won't be more work than the
update of the completion script, so this is probably OK.
But it got me thinking about what do we actually want to test here?
Whether the completion script returns the right long options in a
specific order upon 'git checkout --<TAB>'? Or whether _git() works
properly and invokes the right command-specific completion function?
Or whether regular options get a trailing space while options
expecting an argument don't? Or is this sort of an integration test
and basically all of the above?
I don't think the order is relevant, just that all the options are
there, and the ones with arguments have a = in there, and the ones
that don't, a space.
--
Felipe Contreras
From: SZEDER Gábor <hidden> Date: 2016-06-15 22:53:37
On Tue, Apr 17, 2012 at 09:32:29AM +0300, Felipe Contreras wrote:
On Tue, Apr 17, 2012 at 3:31 AM, SZEDER Gábor [off-list ref] wrote:
quoted
Hi,
I picked up Stephen Boyd's two-patch series[1] to use parse-options to
generate options for git commands, and the following test promply
failed (taken from 5c293a6b (tests: add initial bash completion tests,
2012-04-12)):
test_expect_success 'double dash "git checkout"' '
sed -e "s/Z$//" >expected <<-\EOF &&
--quiet Z
--ours Z
--theirs Z
--track Z
--no-track Z
--merge Z
--conflict=
--orphan Z
--patch Z
EOF
test_completion "git checkout --"
'
Not surprising, the completion script doesn't know about many 'git
checkout' long options. So whenever 'git checkout' learns a new long
option, this list must be updated. This won't be more work than the
update of the completion script, so this is probably OK.
But it got me thinking about what do we actually want to test here?
Whether the completion script returns the right long options in a
specific order upon 'git checkout --<TAB>'? Or whether _git() works
properly and invokes the right command-specific completion function?
Or whether regular options get a trailing space while options
expecting an argument don't? Or is this sort of an integration test
and basically all of the above?
I don't think the order is relevant, just that all the options are
there,
The order of options is not relevant in the completion script, because
Bash will sort them alphabetically anyway. But it is relevant in the
test: it fails if the order is changed either in the completion script
or in the test.
and the ones with arguments have a = in there, and the ones
that don't, a space.
Couldn't we check that better with a test or two for __gitcomp()?
If a test for __gitcomp() fails, we would immediately have a fairly
good idea where to look for the cause of the breakage. However, if
this 'double dash "git checkout"' test fails, there are a bunch of
other things that can possibly cause the failure.
Patch comes in a minute.
Best,
Gábor