threads / rfc / 32689

[RFC] Instruct git-completion.bash that we are in test mode

Subject: [RFC] Instruct git-completion.bash that we are in test mode

## tl;dr

15 messages between Jan 21, 2013 and Jan 25, 2013.

replies: 14people: 4as markdown or json

Jean-Noël AVILA· Jan 21, 2013, 22:30 UTC · lore
In test mode, git completion should only propose core commands.
Signed-off-by: Jean-Noel Avila <jn.avila@free.fr>
---

I reworked the patch so that the test argument is only evaluated when sourcing the file and there is no environment clutter.

At least, "it works for me".
 contrib/completion/git-completion.bash | 8 +++++++-
 t/t9902-completion.sh                  | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 14dd5e7..ac9fa65 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,10 +531,16 @@ __git_complete_strategy ()
 	return 1
 }
 
+if [ "x$1" != "xTEST" ]; then
+	__git_cmdlist () { git help -a|egrep '^  [a-zA-Z0-9]'; }
+else
+	__git_cmdlist () { git help -a| egrep -m 1 -B1000 PATH | egrep '^  [a-zA-Z0-9]'; }
+fi
+
 __git_list_all_commands ()
 {
 	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
+	for i in $(__git_cmdlist)
 	do
 		case $i in
 		*--*)             : helper pattern;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..51463b2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -13,7 +13,7 @@ complete ()
 	return 0
 }
 
-. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" TEST
 
 # We don't need this function to actually join words or do anything special.
 # Also, it's cleaner to avoid touching bash's internal completion variables.
-- 
1.8.1.1.271.g02f55e6
Junio C Hamano· Jan 21, 2013, 23:32 UTC · re: Jean-Noël AVILA · lore

Re: [RFC] Instruct git-completion.bash that we are in test mode

"Jean-Noël AVILA" <jn.avila@free.fr> writes:
> At least, "it works for me".

I suspect that your approach will still not fix the case in which you build a branch with a new command git-check-ignore, and then check out another branch that does not yet have that command without first running "make clean".

Does the following really pass with your patch?
	git checkout origin/next
        make
        git checkout origin/maint
	git apply your_patch.mbox
        make
        cd t && sh ./t9902-completion.sh
> +	__git_cmdlist () { git help -a| egrep -m 1 -B1000 PATH | egrep '^  [a-zA-Z0-9]'; }

'egrep' is not even in POSIX in the first place but grep -E ought to be a replacement for it, so I'll let it pass, but "-m1 -B1000"? Please stay within portable options.

    git help -a |
    sed -n -e '/^  [a-z]/p' -e '/^git commands available from elsewhere/q'/'

might be a good enough substitute, I think, if we were to take your approach, but I suspect it needs a lot more to limit the output in the test mode.

Jeff King· Jan 22, 2013, 00:39 UTC · re: Junio C Hamano · lore

Re: [RFC] Instruct git-completion.bash that we are in test mode

On Mon, Jan 21, 2013 at 03:32:29PM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> "Jean-Noël AVILA" <jn.avila@free.fr> writes:
> 
> > At least, "it works for me".
> 
> I suspect that your approach will still not fix the case in which
> you build a branch with a new command git-check-ignore, and then
> check out another branch that does not yet have that command without
> first running "make clean".
> 
> Does the following really pass with your patch?
> 
> 	git checkout origin/next
>         make
>         git checkout origin/maint
> 	git apply your_patch.mbox
>         make
>         cd t && sh ./t9902-completion.sh
I really hate to suggest this, but should it be more like:
  if test -z "$FAKE_COMMAND_LIST"; then
          __git_cmdlist() {
                  git help -a | egrep '^  [a-zA-Z0-9]'
          }
  else
          __git_cmdlist() {
                  printf '%s' "$FAKE_COMMAND_LIST"
          }
  fi

That gives us a nice predictable starting point for actually testing the completion code. The downside is that it doesn't let us test that we remain compatible with the output of "help -a". But we could potentially add a single, more liberal test (without $FAKE_COMMAND_LIST, but ready to expect extra output) that checks that.

Show 5 quoted lines
> > +	__git_cmdlist () { git help -a| egrep -m 1 -B1000 PATH | egrep '^  [a-zA-Z0-9]'; }
> 
> 'egrep' is not even in POSIX in the first place but grep -E ought to
> be a replacement for it, so I'll let it pass, but "-m1 -B1000"?
> Please stay within portable options.

If I recall correctly, egrep is actually more portable than "grep -E" (and it is already in use, so I think we are OK). I agree on the rest, though. :)

-Peff
Junio C Hamano· Jan 22, 2013, 04:31 UTC · re: Jeff King · lore

Re: [RFC] Instruct git-completion.bash that we are in test mode

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> I really hate to suggest this, but should it be more like:
>
>   if test -z "$FAKE_COMMAND_LIST"; then
>           __git_cmdlist() {
>                   git help -a | egrep '^  [a-zA-Z0-9]'
>           }
>   else
>           __git_cmdlist() {
>                   printf '%s' "$FAKE_COMMAND_LIST"
>           }
>   fi
>
> That gives us a nice predictable starting point for actually testing the
> completion code. The downside is that it  doesn't let us test that we
> remain compatible with the output of "help -a".

Yeah, I think this is simpler and more to the point for the test in t9902. If we really want to test something that is the same as, or at least any closer than this approach (or my "help --standard"), to what the real users use, the test has to become inherently flaky, so I think we should go for the simplicity of this patch shows.

Jean-Noël Avila· Jan 22, 2013, 08:04 UTC · re: Junio C Hamano · lore

Re: [RFC] Instruct git-completion.bash that we are in test mode

Le 22/01/2013 05:31, Junio C Hamano a écrit :
> Jeff King <peff@peff.net>  writes:
 >
 >> I really hate to suggest this, but should it be more like:
 >>
 >> if test -z "$FAKE_COMMAND_LIST"; then __git_cmdlist() { git help -a
 >> | egrep '^ [a-zA-Z0-9]' } else __git_cmdlist() { printf '%s'
 >> "$FAKE_COMMAND_LIST" } fi
 >>
 >> That gives us a nice predictable starting point for actually
 >> testing the completion code. The downside is that it doesn't let
 >> us test that we remain compatible with the output of "help -a".
 >
 > Yeah, I think this is simpler and more to the point for the test in
 > t9902. If we really want to test something that is the same as, or
 > at least any closer than this approach (or my "help --standard"), to
 > what the real users use, the test has to become inherently flaky, so
 > I think we should go for the simplicity of this patch shows.

Instead of imposing the list of command, we could use the command list argument to filter the ouput of git help -a. This would ensure that the completions we want to test are still present in the installation while still restricting them to the test case.

Junio C Hamano· Jan 22, 2013, 16:31 UTC · re: Jean-Noël Avila · lore

Re: [RFC] Instruct git-completion.bash that we are in test mode

Jean-Noël Avila <avila.jn@gmail.com> writes:
Show 17 quoted lines
> Le 22/01/2013 05:31, Junio C Hamano a écrit :
>> Jeff King <peff@peff.net>  writes:
>>
>>> I really hate to suggest this, but should it be more like:
>>>
>>> if test -z "$FAKE_COMMAND_LIST"; then __git_cmdlist() { git help -a
>>> | egrep '^ [a-zA-Z0-9]' } else __git_cmdlist() { printf '%s'
>>> "$FAKE_COMMAND_LIST" } fi
>>>
>>> That gives us a nice predictable starting point for actually
>>> testing the completion code. The downside is that it doesn't let
>>> us test that we remain compatible with the output of "help -a".
> ...
> Instead of imposing the list of command, we could use the command
> list argument to filter the ouput of git help -a. This would ensure that the
> completions we want to test are still present in the installation while
> still restricting them to the test case.

In order to "filter the output", you still need to know how output from "git help -a" looks like, and adjust the code to filter when the shape of the output changes. The effort to do so is pretty similar to the amount of effort needed to maintain FAKE_COMMAND_LIST to look like the output from "git help -a". It is of dubious value compared to the simplicity of "printf" FAKE_COMMAND_LIST approach, I think.

Junio C Hamano· Jan 24, 2013, 23:07 UTC · re: Jeff King · lore

[PATCH] t9902: protect test from stray build artifacts

When you have random build artifacts in your build directory, left behind by running "make" while on another branch, the "git help -a" command run by __git_list_all_commands in the completion script that is being tested does not have a way to know that they are not part of the subcommands this build will ship. Such extra subcommands may come from the user's $PATH. They will interfere with the tests that expect a certain prefix to uniquely expand to a known completion.

Instrument the completion script and give it a way for us to tell what (subset of) subcommands we are going to ship.

Also add a test to "git --help <prefix><TAB>" expansion. It needs to show not just commands but some selected documentation pages.

Based on an idea by Jeff King.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/completion/git-completion.bash | 11 ++++++++++-
 t/t9902-completion.sh                  | 25 ++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a4c48e1..6139b50 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,10 +531,19 @@ __git_complete_strategy ()
 	return 1
 }
 
+__git_commands () {
+	if test -n "${GIT_TESTING_COMMAND_COMPLETION:-}"
+	then
+		printf "%s" "${GIT_TESTING_COMMAND_COMPLETION}"
+	else
+		git help -a|egrep '^  [a-zA-Z0-9]'
+	fi
+}
+
 __git_list_all_commands ()
 {
 	local i IFS=" "$'\n'
-	for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
+	for i in $(__git_commands)
 	do
 		case $i in
 		*--*)             : helper pattern;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..adc1372 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -13,6 +13,25 @@ complete ()
 	return 0
 }
 
+# Be careful when updating this list:
+#
+# (1) The build tree may have build artifact from different branch, or
+#     the user's $PATH may have a random executable that may begin
+#     with "git-check" that are not part of the subcommands this build
+#     will ship, e.g.  "check-ignore".  The tests for completion for
+#     subcommand names tests how "check" is expanded; we limit the
+#     possible candidates to "checkout" and "check-attr" to make sure
+#     "check-attr", which is known by the filter function as a
+#     subcommand to be thrown out, while excluding other random files
+#     that happen to begin with "check" to avoid letting them get in
+#     the way.
+#
+# (2) A test makes sure that common subcommands are included in the
+#     completion for "git <TAB>", and a plumbing is excluded.  "add",
+#     "filter-branch" and "ls-files" are listed for this.
+
+GIT_TESTING_COMMAND_COMPLETION='add checkout check-attr filter-branch ls-files'
+
 . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
 
 # We don't need this function to actually join words or do anything special.
@@ -196,7 +215,6 @@ test_expect_success 'general options plus command' '
 	test_completion "git --paginate check" "checkout " &&
 	test_completion "git --git-dir=foo check" "checkout " &&
 	test_completion "git --bare check" "checkout " &&
-	test_completion "git --help des" "describe " &&
 	test_completion "git --exec-path=foo check" "checkout " &&
 	test_completion "git --html-path check" "checkout " &&
 	test_completion "git --no-pager check" "checkout " &&
@@ -207,6 +225,11 @@ test_expect_success 'general options plus command' '
 	test_completion "git --no-replace-objects check" "checkout "
 '
 
+test_expect_success 'git --help completion' '
+	test_completion "git --help ad" "add " &&
+	test_completion "git --help core" "core-tutorial "
+'
+
 test_expect_success 'setup for ref completion' '
 	echo content >file1 &&
 	echo more >file2 &&
Jeff King· Jan 25, 2013, 01:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] t9902: protect test from stray build artifacts

On Thu, Jan 24, 2013 at 03:07:42PM -0800, Junio C Hamano wrote:
Show 21 quoted lines
> When you have random build artifacts in your build directory, left
> behind by running "make" while on another branch, the "git help -a"
> command run by __git_list_all_commands in the completion script that
> is being tested does not have a way to know that they are not part
> of the subcommands this build will ship.  Such extra subcommands may
> come from the user's $PATH.  They will interfere with the tests that
> expect a certain prefix to uniquely expand to a known completion.
> 
> Instrument the completion script and give it a way for us to tell
> what (subset of) subcommands we are going to ship.
> 
> Also add a test to "git --help <prefix><TAB>" expansion.  It needs
> to show not just commands but some selected documentation pages.
> 
> Based on an idea by Jeff King.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  contrib/completion/git-completion.bash | 11 ++++++++++-
>  t/t9902-completion.sh                  | 25 ++++++++++++++++++++++++-
>  2 files changed, 34 insertions(+), 2 deletions(-)
This looks good to me.

The only thing I might add is a test just to double-check that "git help -a" is parsed correctly. Like:

  test_expect_success 'command completion works without test harness' '
           GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
           grep "^bundle\$" out
  '

(we know we are running bash here, so the one-shot variable is OK to be used with a function).

-Peff
Jonathan Nieder· Jan 25, 2013, 02:59 UTC · re: Jeff King · lore

Re: [PATCH] t9902: protect test from stray build artifacts

Jeff King wrote:
> On Thu, Jan 24, 2013 at 03:07:42PM -0800, Junio C Hamano wrote[1]:
>> Instrument the completion script and give it a way for us to tell
>> what (subset of) subcommands we are going to ship.
[...]
Show 7 quoted lines
> The only thing I might add is a test just to double-check that "git help
> -a" is parsed correctly. Like:
>
>   test_expect_success 'command completion works without test harness' '
>            GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
>            grep "^bundle\$" out
>   '

Yes. Since there are no other 'git help -a' tests, I think we need this.

Aside from that, the fix looks good to me.
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/214167/focus=214469
Junio C Hamano· Jan 25, 2013, 04:11 UTC · re: Jeff King · lore

Re: [PATCH] t9902: protect test from stray build artifacts

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> This looks good to me.
>
> The only thing I might add is a test just to double-check that "git help
> -a" is parsed correctly. Like:
>
>   test_expect_success 'command completion works without test harness' '
>            GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
>            grep "^bundle\$" out
>   '
>
> (we know we are running bash here, so the one-shot variable is OK to be
> used with a function).

I think you meant "^bundle $" there, but don't we have the same problem when there is an end-user subcommand "git bunny"?

Ahh, ok, we show one element per line and just make sure "bundle" is there, and we do not care what other buns appear in the output.

Will squash in, then.
Jeff King· Jan 25, 2013, 04:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] t9902: protect test from stray build artifacts

On Thu, Jan 24, 2013 at 08:11:07PM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > This looks good to me.
> >
> > The only thing I might add is a test just to double-check that "git help
> > -a" is parsed correctly. Like:
> >
> >   test_expect_success 'command completion works without test harness' '
> >            GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
> >            grep "^bundle\$" out
> >   '
> >
> > (we know we are running bash here, so the one-shot variable is OK to be
> > used with a function).
> 
> I think you meant "^bundle $" there, but don't we have the same
> problem when there is an end-user subcommand "git bunny"?
> 
> Ahh, ok, we show one element per line and just make sure "bundle"
> is there, and we do not care what other buns appear in the output.
Exactly. At least that was the intent; I typed it straight into my MUA. :)
-Peff
Junio C Hamano· Jan 25, 2013, 04:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] t9902: protect test from stray build artifacts

Junio C Hamano <gitster@pobox.com> writes:
Show 22 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> This looks good to me.
>>
>> The only thing I might add is a test just to double-check that "git help
>> -a" is parsed correctly. Like:
>>
>>   test_expect_success 'command completion works without test harness' '
>>            GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
>>            grep "^bundle\$" out
>>   '
>>
>> (we know we are running bash here, so the one-shot variable is OK to be
>> used with a function).
>
> I think you meant "^bundle $" there, but don't we have the same
> problem when there is an end-user subcommand "git bunny"?
>
> Ahh, ok, we show one element per line and just make sure "bundle"
> is there, and we do not care what other buns appear in the output.
>
> Will squash in, then.

Not so quick, though. The lower level "read from help -a" is only run once and its output kept in a two-level cache hierarchy; we need to reset both.

It starts to look a bit too intimately tied to the implementation of what is being tested for my taste, though.

 t/t9902-completion.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index adc1372..bb6ee1a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -286,4 +286,14 @@ test_expect_success 'send-email' '
 	test_completion "git send-email ma" "master "
 '
 
+# This is better to be at the end, as it resets the state by tweaking
+# the internals.
+test_expect_success 'help -a read correctly by command list generator' '
+	__git_all_commands= &&
+	__git_porcelain_commands= &&
+	GIT_TESTING_COMMAND_COMPLETION= &&
+	run_completion "git bun" &&
+	grep "^bundle $" out
+'
+
 test_done
-- 
1.8.1.1.525.gdace530
Jeff King· Jan 25, 2013, 04:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] t9902: protect test from stray build artifacts

On Thu, Jan 24, 2013 at 08:19:30PM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> > Ahh, ok, we show one element per line and just make sure "bundle"
> > is there, and we do not care what other buns appear in the output.
> >
> Not so quick, though.  The lower level "read from help -a" is only
> run once and its output kept in a two-level cache hierarchy; we need
> to reset both.
Ugh, I didn't even think about that.

I wonder if it would be simpler if the completion tests actually ran a new bash for each test. That would be slower, but it somehow seems cleaner.

Show 10 quoted lines
> It starts to look a bit too intimately tied to the implementation of
> what is being tested for my taste, though.
> [...]
> +test_expect_success 'help -a read correctly by command list generator' '
> +	__git_all_commands= &&
> +	__git_porcelain_commands= &&
> +	GIT_TESTING_COMMAND_COMPLETION= &&
> +	run_completion "git bun" &&
> +	grep "^bundle $" out
> +'

Agreed. I could take or leave it at this point. It's nice to check that changes to "help -a" will not break it, but ultimately it feels a bit too contrived to catch anything useful.

-Peff
Junio C Hamano· Jan 25, 2013, 18:34 UTC · re: Jeff King · lore

Re: [PATCH] t9902: protect test from stray build artifacts

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> On Thu, Jan 24, 2013 at 08:19:30PM -0800, Junio C Hamano wrote:
>
>> > Ahh, ok, we show one element per line and just make sure "bundle"
>> > is there, and we do not care what other buns appear in the output.
>> >
>> Not so quick, though.  The lower level "read from help -a" is only
>> run once and its output kept in a two-level cache hierarchy; we need
>> to reset both.
>
> Ugh, I didn't even think about that.
>
> I wonder if it would be simpler if the completion tests actually ran a
> new bash for each test. That would be slower, but it somehow seems
> cleaner.

I agree 100% with that. Let's leave this fix as-is, at least as a tentative fix while "git check-ignore" graduates into the upcoming release, and let somebody who is interested work on an update to this test script to do so as an independent topic.

Show 17 quoted lines
>
>> It starts to look a bit too intimately tied to the implementation of
>> what is being tested for my taste, though.
>> [...]
>> +test_expect_success 'help -a read correctly by command list generator' '
>> +	__git_all_commands= &&
>> +	__git_porcelain_commands= &&
>> +	GIT_TESTING_COMMAND_COMPLETION= &&
>> +	run_completion "git bun" &&
>> +	grep "^bundle $" out
>> +'
>
> Agreed. I could take or leave it at this point. It's nice to check that
> changes to "help -a" will not break it, but ultimately it feels a bit
> too contrived to catch anything useful.
>
> -Peff
Jeff King· Jan 25, 2013, 22:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] t9902: protect test from stray build artifacts

On Fri, Jan 25, 2013 at 10:34:56AM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> >> Not so quick, though.  The lower level "read from help -a" is only
> >> run once and its output kept in a two-level cache hierarchy; we need
> >> to reset both.
> >
> > Ugh, I didn't even think about that.
> >
> > I wonder if it would be simpler if the completion tests actually ran a
> > new bash for each test. That would be slower, but it somehow seems
> > cleaner.
> 
> I agree 100% with that.  Let's leave this fix as-is, at least as a
> tentative fix while "git check-ignore" graduates into the upcoming
> release, and let somebody who is interested work on an update to
> this test script to do so as an independent topic.
Sounds good to me. Thanks.
-Peff

← back to recent threads