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

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

[Jean-Noël AVILA]

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

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

[Junio C Hamano]

"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.

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

[Jeff King]

On Mon, Jan 21, 2013 at 03:32:29PM -0800, Junio C Hamano wrote:

> "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.

> > +	__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

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

[Junio C Hamano]

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.

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

[Jean-Noël Avila]

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.

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

[Junio C Hamano]

Jean-Noël Avila <avila.jn@gmail.com> writes:

> 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.

[PATCH] t9902: protect test from stray build artifacts

[Junio C Hamano]

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 &&

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

[Jeff King]

On Thu, Jan 24, 2013 at 03:07:42PM -0800, Junio C Hamano wrote:

> 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

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

[Jonathan Nieder]

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.
[...]
> 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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Thu, Jan 24, 2013 at 08:11:07PM -0800, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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

> 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

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

[Jeff King]

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.

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.


>
>> 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

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

[Jeff King]

On Fri, Jan 25, 2013 at 10:34:56AM -0800, Junio C Hamano wrote:

> >> 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