$PATH pollution and t9902-completion.sh

$PATH pollution and t9902-completion.sh

[Adam Spiers]

t/t9902-completion.sh is currently failing for me because I happen to
have a custom shell-script called git-check-email in ~/bin, which is
on my $PATH.  This is different to a similar-looking case reported
recently, which was due to an unclean working tree:

  http://thread.gmane.org/gmane.comp.version-control.git/208085

It's not unthinkable that in the future other tests could break for
similar reasons.  Therefore it would be good to sanitize $PATH in the
test framework so that it cannot destabilize tests, although I am
struggling to think of a good way of doing this.  Naively stripping
directories under $HOME would not protect against git "plugins" such
as the above being installed into places like /usr/bin.  Thoughts?

Re: $PATH pollution and t9902-completion.sh

[Jeff King]

On Mon, Dec 17, 2012 at 01:05:38AM +0000, Adam Spiers wrote:

> t/t9902-completion.sh is currently failing for me because I happen to
> have a custom shell-script called git-check-email in ~/bin, which is
> on my $PATH.  This is different to a similar-looking case reported
> recently, which was due to an unclean working tree:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/208085
> 
> It's not unthinkable that in the future other tests could break for
> similar reasons.  Therefore it would be good to sanitize $PATH in the
> test framework so that it cannot destabilize tests, although I am
> struggling to think of a good way of doing this.  Naively stripping
> directories under $HOME would not protect against git "plugins" such
> as the above being installed into places like /usr/bin.  Thoughts?

I've run into this, too. I think sanitizing $PATH is the wrong approach.
The real problem is that the test is overly picky. Right now it is
failing because you happen to have "check-email" in your $PATH, but it
will also need to be adjusted when a true "check-email" command is added
to git.

I can think of two other options:

  1. Make the test input more specific (e.g., looking for "checkou").
     This doesn't eliminate the problem, but makes it less likely
     to occur.

  2. Loosen the test to look for the presence of "checkout", but not
     fail when other items are present. Bonus points if it makes sure
     that everything returned starts with "check".

I think (2) is the ideal solution in terms of behavior, but writing it
may be more of a pain.

-Peff

Re: $PATH pollution and t9902-completion.sh

[Adam Spiers]

On Thu, Dec 20, 2012 at 2:55 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 17, 2012 at 01:05:38AM +0000, Adam Spiers wrote:
>> t/t9902-completion.sh is currently failing for me because I happen to
>> have a custom shell-script called git-check-email in ~/bin, which is
>> on my $PATH.  This is different to a similar-looking case reported
>> recently, which was due to an unclean working tree:
>>
>>   http://thread.gmane.org/gmane.comp.version-control.git/208085
>>
>> It's not unthinkable that in the future other tests could break for
>> similar reasons.  Therefore it would be good to sanitize $PATH in the
>> test framework so that it cannot destabilize tests, although I am
>> struggling to think of a good way of doing this.  Naively stripping
>> directories under $HOME would not protect against git "plugins" such
>> as the above being installed into places like /usr/bin.  Thoughts?
>
> I've run into this, too. I think sanitizing $PATH is the wrong approach.
> The real problem is that the test is overly picky. Right now it is
> failing because you happen to have "check-email" in your $PATH, but it
> will also need to be adjusted when a true "check-email" command is added
> to git.
>
> I can think of two other options:
>
>   1. Make the test input more specific (e.g., looking for "checkou").
>      This doesn't eliminate the problem, but makes it less likely
>      to occur.
>
>   2. Loosen the test to look for the presence of "checkout", but not
>      fail when other items are present. Bonus points if it makes sure
>      that everything returned starts with "check".
>
> I think (2) is the ideal solution in terms of behavior, but writing it
> may be more of a pain.

I agree with all your points.  Thanks for the suggestions.

Re: $PATH pollution and t9902-completion.sh

[Torsten Bögershausen]

On 20.12.12 16:13, Adam Spiers wrote:
> On Thu, Dec 20, 2012 at 2:55 PM, Jeff King <peff@peff.net> wrote:
>> On Mon, Dec 17, 2012 at 01:05:38AM +0000, Adam Spiers wrote:
>>> t/t9902-completion.sh is currently failing for me because I happen to
>>> have a custom shell-script called git-check-email in ~/bin, which is
>>> on my $PATH.  This is different to a similar-looking case reported
>>> recently, which was due to an unclean working tree:
>>>
>>>   http://thread.gmane.org/gmane.comp.version-control.git/208085
>>>
>>> It's not unthinkable that in the future other tests could break for
>>> similar reasons.  Therefore it would be good to sanitize $PATH in the
>>> test framework so that it cannot destabilize tests, although I am
>>> struggling to think of a good way of doing this.  Naively stripping
>>> directories under $HOME would not protect against git "plugins" such
>>> as the above being installed into places like /usr/bin.  Thoughts?
>>
>> I've run into this, too. I think sanitizing $PATH is the wrong approach.
>> The real problem is that the test is overly picky. Right now it is
>> failing because you happen to have "check-email" in your $PATH, but it
>> will also need to be adjusted when a true "check-email" command is added
>> to git.
>>
>> I can think of two other options:
>>
>>   1. Make the test input more specific (e.g., looking for "checkou").
>>      This doesn't eliminate the problem, but makes it less likely
>>      to occur.
>>
>>   2. Loosen the test to look for the presence of "checkout", but not
>>      fail when other items are present. Bonus points if it makes sure
>>      that everything returned starts with "check".
>>
>> I think (2) is the ideal solution in terms of behavior, but writing it
>> may be more of a pain.
> 
> I agree with all your points.  Thanks for the suggestions.
I volonteer for 1) (and we got for 2) later)

Re: $PATH pollution and t9902-completion.sh

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   2. Loosen the test to look for the presence of "checkout", but not
>      fail when other items are present. Bonus points if it makes sure
>      that everything returned starts with "check".
>
> I think (2) is the ideal solution in terms of behavior, but writing it
> may be more of a pain.

Yeah, I think (2) is the way to go.

Re: $PATH pollution and t9902-completion.sh

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>>   2. Loosen the test to look for the presence of "checkout", but not
>>      fail when other items are present. Bonus points if it makes sure
>>      that everything returned starts with "check".
>>
>> I think (2) is the ideal solution in terms of behavior, but writing it
>> may be more of a pain.
>
> Yeah, I think (2) is the way to go.

The beginning of such a change may look like the attached patch.

If we want to go for the bonus points, we would either add another
parameter "prefix" to the test_completion function, or introduce the
test_complete_command function that takes that prefix parameter, and
in addition to making sure lines from "expect" is fully contained in
the "actual", make sure that output from

	comm -13 expect.sorted actual.sorted

all begin with that "prefix" string, perhaps with

	grep -v "^$prefix"

or something.  The test_fully_contains function needs to be renamed
if somebody goes that additional step.

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..5fab389 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -54,10 +54,16 @@ run_completion ()
 	__git_wrap__git_main && print_comp
 }
 
+test_fully_contains () {
+	sort "$1" >expect.sorted &&
+	sort "$2" >actual.sorted &&
+	test $(comm -23 expect.sorted actual.sorted | wc -l) = 0
+}
+
 # Test high-level completion
 # Arguments are:
 # 1: typed text so far (cur)
-# 2: expected completion
+# 2: expected completion (if missing, this is read from stdin)
 test_completion ()
 {
 	if test $# -gt 1
@@ -67,7 +73,7 @@ test_completion ()
 		sed -e 's/Z$//' >expected
 	fi &&
 	run_completion "$1" &&
-	test_cmp expected out
+	test_fully_contains expected out
 }
 
 # Test __gitcomp.

Re: $PATH pollution and t9902-completion.sh

[Jeff King]

On Thu, Dec 20, 2012 at 11:55:45AM -0800, Junio C Hamano wrote:

> The beginning of such a change may look like the attached patch.
> [...]
> +test_fully_contains () {
> +	sort "$1" >expect.sorted &&
> +	sort "$2" >actual.sorted &&
> +	test $(comm -23 expect.sorted actual.sorted | wc -l) = 0
> +}

I like the direction. I suspect test_fully_contains could be used in a
lot of other places, too.

-Peff

Re: $PATH pollution and t9902-completion.sh

[Torsten Bögershausen]

On 20.12.12 21:01, Jeff King wrote:
> +test_fully_contains () {
>> +	sort "$1" >expect.sorted &&
>> +	sort "$2" >actual.sorted &&
>> +	test $(comm -23 expect.sorted actual.sorted | wc -l) = 0
>> +}

(Good to learn about the comm command, thanks )
What do we think about this:


diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..82eeba7 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -62,12 +62,16 @@ test_completion ()
 {
 	if test $# -gt 1
 	then
-		printf '%s\n' "$2" >expected
+		printf '%s\n' "$2" | sort >expected.sorted
 	else
-		sed -e 's/Z$//' >expected
+		sed -e 's/Z$//' | sort >expected.sorted
 	fi &&
 	run_completion "$1" &&
-	test_cmp expected out
+	sort <out >actual.sorted &&
+	>empty &&
+	comm -23 expected.sorted actual.sorted >actual &&
+	test_cmp empty actual &&
+	rm empty actual
 }
 
 # Test __gitcomp.

Re: $PATH pollution and t9902-completion.sh

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On 20.12.12 21:01, Jeff King wrote:
>> +test_fully_contains () {
>>> +	sort "$1" >expect.sorted &&
>>> +	sort "$2" >actual.sorted &&
>>> +	test $(comm -23 expect.sorted actual.sorted | wc -l) = 0
>>> +}
>
> (Good to learn about the comm command, thanks )
> What do we think about this:

Three points to consider.

 * Do _all_ tests that use test_completion not care about the actual
   order of choices that come out of the completion machinery?

 * Do _all_ tests that use test_completion not care about having
   extra choice, or is the command name completion the only odd
   case?

 * Is this still as easy to extend as the original to conditionally
   verify that all extra output share the same prefix?

> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 3cd53f8..82eeba7 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -62,12 +62,16 @@ test_completion ()
>  {
>  	if test $# -gt 1
>  	then
> -		printf '%s\n' "$2" >expected
> +		printf '%s\n' "$2" | sort >expected.sorted
>  	else
> -		sed -e 's/Z$//' >expected
> +		sed -e 's/Z$//' | sort >expected.sorted
>  	fi &&
>  	run_completion "$1" &&
> -	test_cmp expected out
> +	sort <out >actual.sorted &&
> +	>empty &&
> +	comm -23 expected.sorted actual.sorted >actual &&
> +	test_cmp empty actual &&
> +	rm empty actual
>  }
>  
>  # Test __gitcomp.

Re: $PATH pollution and t9902-completion.sh

[Jeff King]

On Thu, Dec 20, 2012 at 09:53:06PM +0100, Torsten Bögershausen wrote:

> (Good to learn about the comm command, thanks )
> What do we think about this:
> 
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 3cd53f8..82eeba7 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -62,12 +62,16 @@ test_completion ()
>  {
>  	if test $# -gt 1
>  	then
> -		printf '%s\n' "$2" >expected
> +		printf '%s\n' "$2" | sort >expected.sorted
>  	else
> -		sed -e 's/Z$//' >expected
> +		sed -e 's/Z$//' | sort >expected.sorted
>  	fi &&
>  	run_completion "$1" &&
> -	test_cmp expected out
> +	sort <out >actual.sorted &&
> +	>empty &&
> +	comm -23 expected.sorted actual.sorted >actual &&
> +	test_cmp empty actual &&
> +	rm empty actual

I like test_* helpers that tell you what happened on error, but this
output will be kind of a weird diff (it is a diff showing things you
expected to have but did not get as additions, and no mention of things
you expected and got).

The output is human readable, so I think it is perfectly OK to print a
message about what is going on (we do this in test_expect_code, for
example). Something like:

  test_fully_contains() {
    sort "$1" >expect.sorted &&
    sort "$2" >actual.sorted &&
    comm -23 expect.sorted actual.sorted >missing
    test -f missing -a ! -s missing &&
    rm -f expect.sorted actual.sorted missing &&
    return 0

    {
      echo "test_fully_contains: some lines were missing"
      echo "expected:"
      sed 's/^/  /' <"$1"
      echo "actual:"
      sed 's/^/  /' <"$2"
      echo "missing:"
      sed 's/^/  /' <missing
    } >&2
    return 1
  }

Though come to think of it, just showing the diff between expect.sorted
and actual.sorted would probably be enough. It would show the missing
elements as deletions, the found elements as common lines, and the
"extra" elements as additions (which are not an error, but when you are
debugging, might be useful to know about).

-Peff