{"thread":{"id":"32364","subject":"$PATH pollution and t9902-completion.sh","startedAt":"2012-12-17T01:05:38Z","lastAt":"2012-12-20T21:04:47Z","messageCount":10,"participants":["Adam Spiers","Jeff King","Torsten Bögershausen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"204995","messageId":"20121217010538.GC3673@gmail.com","threadId":"32364","inReplyTo":null,"subject":"$PATH pollution and t9902-completion.sh","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2012-12-17T01:05:38Z","receivedAt":"2012-12-17T01:05:38Z","isPatch":false,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"t/t9902-completion.sh is currently failing for me because I happen to\nhave a custom shell-script called git-check-email in ~/bin, which is\non my $PATH. This is different to a similar-looking case reported\nrecently, which was due to an unclean working tree:\n\n http://thread.gmane.org/gmane.comp.version-control.git/208085\n\nIt's not unthinkable that in the future other tests could break for\nsimilar reasons. Therefore it would be good to sanitize $PATH in the\ntest framework so that it cannot destabilize tests, although I am\nstruggling to think of a good way of doing this. Naively stripping\ndirectories under $HOME would not protect against git \"plugins\" such\nas the above being installed into places like /usr/bin. Thoughts?\n"},{"id":"205257","messageId":"20121220145519.GB27211@sigill.intra.peff.net","threadId":"32364","inReplyTo":"20121217010538.GC3673@gmail.com","subject":"Re: $PATH pollution and t9902-completion.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-20T14:55:19Z","receivedAt":"2012-12-20T14:55:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 17, 2012 at 01:05:38AM +0000, Adam Spiers wrote:\n\n> t/t9902-completion.sh is currently failing for me because I happen to\n> have a custom shell-script called git-check-email in ~/bin, which is\n> on my $PATH. This is different to a similar-looking case reported\n> recently, which was due to an unclean working tree:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/208085\n> \n> It's not unthinkable that in the future other tests could break for\n> similar reasons. Therefore it would be good to sanitize $PATH in the\n> test framework so that it cannot destabilize tests, although I am\n> struggling to think of a good way of doing this. Naively stripping\n> directories under $HOME would not protect against git \"plugins\" such\n> as the above being installed into places like /usr/bin. Thoughts?\n\nI've run into this, too. I think sanitizing $PATH is the wrong approach.\nThe real problem is that the test is overly picky. Right now it is\nfailing because you happen to have \"check-email\" in your $PATH, but it\nwill also need to be adjusted when a true \"check-email\" command is added\nto git.\n\nI can think of two other options:\n\n 1. Make the test input more specific (e.g., looking for \"checkou\").\n This doesn't eliminate the problem, but makes it less likely\n to occur.\n\n 2. Loosen the test to look for the presence of \"checkout\", but not\n fail when other items are present. Bonus points if it makes sure\n that everything returned starts with \"check\".\n\nI think (2) is the ideal solution in terms of behavior, but writing it\nmay be more of a pain.\n\n-Peff\n"},{"id":"205261","messageId":"CAOkDyE-J7sTJ-GefhteP1wy7WorqTRnj5nn0k6hd1dp0VJz5iQ@mail.gmail.com","threadId":"32364","inReplyTo":"20121220145519.GB27211@sigill.intra.peff.net","subject":"Re: $PATH pollution and t9902-completion.sh","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2012-12-20T15:13:27Z","receivedAt":"2012-12-20T15:13:27Z","isPatch":false,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Thu, Dec 20, 2012 at 2:55 PM, Jeff King wrote:\n> On Mon, Dec 17, 2012 at 01:05:38AM +0000, Adam Spiers wrote:\n>> t/t9902-completion.sh is currently failing for me because I happen to\n>> have a custom shell-script called git-check-email in ~/bin, which is\n>> on my $PATH. This is different to a similar-looking case reported\n>> recently, which was due to an unclean working tree:\n>>\n>> http://thread.gmane.org/gmane.comp.version-control.git/208085\n>>\n>> It's not unthinkable that in the future other tests could break for\n>> similar reasons. Therefore it would be good to sanitize $PATH in the\n>> test framework so that it cannot destabilize tests, although I am\n>> struggling to think of a good way of doing this. Naively stripping\n>> directories under $HOME would not protect against git \"plugins\" such\n>> as the above being installed into places like /usr/bin. Thoughts?\n>\n> I've run into this, too. I think sanitizing $PATH is the wrong approach.\n> The real problem is that the test is overly picky. Right now it is\n> failing because you happen to have \"check-email\" in your $PATH, but it\n> will also need to be adjusted when a true \"check-email\" command is added\n> to git.\n>\n> I can think of two other options:\n>\n> 1. Make the test input more specific (e.g., looking for \"checkou\").\n> This doesn't eliminate the problem, but makes it less likely\n> to occur.\n>\n> 2. Loosen the test to look for the presence of \"checkout\", but not\n> fail when other items are present. Bonus points if it makes sure\n> that everything returned starts with \"check\".\n>\n> I think (2) is the ideal solution in terms of behavior, but writing it\n> may be more of a pain.\n\nI agree with all your points. Thanks for the suggestions.\n"},{"id":"205271","messageId":"50D349F9.9030100@web.de","threadId":"32364","inReplyTo":"CAOkDyE-J7sTJ-GefhteP1wy7WorqTRnj5nn0k6hd1dp0VJz5iQ@mail.gmail.com","subject":"Re: $PATH pollution and t9902-completion.sh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2012-12-20T17:25:13Z","receivedAt":"2012-12-20T17:25:13Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 20.12.12 16:13, Adam Spiers wrote:\n> On Thu, Dec 20, 2012 at 2:55 PM, Jeff King wrote:\n>> On Mon, Dec 17, 2012 at 01:05:38AM +0000, Adam Spiers wrote:\n>>> t/t9902-completion.sh is currently failing for me because I happen to\n>>> have a custom shell-script called git-check-email in ~/bin, which is\n>>> on my $PATH. This is different to a similar-looking case reported\n>>> recently, which was due to an unclean working tree:\n>>>\n>>> http://thread.gmane.org/gmane.comp.version-control.git/208085\n>>>\n>>> It's not unthinkable that in the future other tests could break for\n>>> similar reasons. Therefore it would be good to sanitize $PATH in the\n>>> test framework so that it cannot destabilize tests, although I am\n>>> struggling to think of a good way of doing this. Naively stripping\n>>> directories under $HOME would not protect against git \"plugins\" such\n>>> as the above being installed into places like /usr/bin. Thoughts?\n>>\n>> I've run into this, too. I think sanitizing $PATH is the wrong approach.\n>> The real problem is that the test is overly picky. Right now it is\n>> failing because you happen to have \"check-email\" in your $PATH, but it\n>> will also need to be adjusted when a true \"check-email\" command is added\n>> to git.\n>>\n>> I can think of two other options:\n>>\n>> 1. Make the test input more specific (e.g., looking for \"checkou\").\n>> This doesn't eliminate the problem, but makes it less likely\n>> to occur.\n>>\n>> 2. Loosen the test to look for the presence of \"checkout\", but not\n>> fail when other items are present. Bonus points if it makes sure\n>> that everything returned starts with \"check\".\n>>\n>> I think (2) is the ideal solution in terms of behavior, but writing it\n>> may be more of a pain.\n> \n> I agree with all your points. Thanks for the suggestions.\nI volonteer for 1) (and we got for 2) later)\n"},{"id":"205280","messageId":"7vk3sc606f.fsf@alter.siamese.dyndns.org","threadId":"32364","inReplyTo":"20121220145519.GB27211@sigill.intra.peff.net","subject":"Re: $PATH pollution and t9902-completion.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-20T18:36:08Z","receivedAt":"2012-12-20T18:36:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 2. Loosen the test to look for the presence of \"checkout\", but not\n> fail when other items are present. Bonus points if it makes sure\n> that everything returned starts with \"check\".\n>\n> I think (2) is the ideal solution in terms of behavior, but writing it\n> may be more of a pain.\n\nYeah, I think (2) is the way to go.\n"},{"id":"205286","messageId":"7vobho4hxa.fsf@alter.siamese.dyndns.org","threadId":"32364","inReplyTo":"7vk3sc606f.fsf@alter.siamese.dyndns.org","subject":"Re: $PATH pollution and t9902-completion.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-20T19:55:45Z","receivedAt":"2012-12-20T19:55:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> 2. Loosen the test to look for the presence of \"checkout\", but not\n>> fail when other items are present. Bonus points if it makes sure\n>> that everything returned starts with \"check\".\n>>\n>> I think (2) is the ideal solution in terms of behavior, but writing it\n>> may be more of a pain.\n>\n> Yeah, I think (2) is the way to go.\n\nThe beginning of such a change may look like the attached patch.\n\nIf we want to go for the bonus points, we would either add another\nparameter \"prefix\" to the test_completion function, or introduce the\ntest_complete_command function that takes that prefix parameter, and\nin addition to making sure lines from \"expect\" is fully contained in\nthe \"actual\", make sure that output from\n\n\tcomm -13 expect.sorted actual.sorted\n\nall begin with that \"prefix\" string, perhaps with\n\n\tgrep -v \"^$prefix\"\n\nor something. The test_fully_contains function needs to be renamed\nif somebody goes that additional step.\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 3cd53f8..5fab389 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -54,10 +54,16 @@ run_completion ()\n \t__git_wrap__git_main && print_comp\n }\n \n+test_fully_contains () {\n+\tsort \"$1\" >expect.sorted &&\n+\tsort \"$2\" >actual.sorted &&\n+\ttest $(comm -23 expect.sorted actual.sorted | wc -l) = 0\n+}\n+\n # Test high-level completion\n # Arguments are:\n # 1: typed text so far (cur)\n-# 2: expected completion\n+# 2: expected completion (if missing, this is read from stdin)\n test_completion ()\n {\n \tif test $# -gt 1\n@@ -67,7 +73,7 @@ test_completion ()\n \t\tsed -e 's/Z$//' >expected\n \tfi &&\n \trun_completion \"$1\" &&\n-\ttest_cmp expected out\n+\ttest_fully_contains expected out\n }\n \n # Test __gitcomp.\n"},{"id":"205288","messageId":"20121220200109.GC21785@sigill.intra.peff.net","threadId":"32364","inReplyTo":"7vobho4hxa.fsf@alter.siamese.dyndns.org","subject":"Re: $PATH pollution and t9902-completion.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-20T20:01:09Z","receivedAt":"2012-12-20T20:01:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 20, 2012 at 11:55:45AM -0800, Junio C Hamano wrote:\n\n> The beginning of such a change may look like the attached patch.\n> [...]\n> +test_fully_contains () {\n> +\tsort \"$1\" >expect.sorted &&\n> +\tsort \"$2\" >actual.sorted &&\n> +\ttest $(comm -23 expect.sorted actual.sorted | wc -l) = 0\n> +}\n\nI like the direction. I suspect test_fully_contains could be used in a\nlot of other places, too.\n\n-Peff\n"},{"id":"205291","messageId":"50D37AB2.1040508@web.de","threadId":"32364","inReplyTo":"20121220200109.GC21785@sigill.intra.peff.net","subject":"Re: $PATH pollution and t9902-completion.sh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2012-12-20T20:53:06Z","receivedAt":"2012-12-20T20:53:06Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 20.12.12 21:01, Jeff King wrote:\n> +test_fully_contains () {\n>> +\tsort \"$1\" >expect.sorted &&\n>> +\tsort \"$2\" >actual.sorted &&\n>> +\ttest $(comm -23 expect.sorted actual.sorted | wc -l) = 0\n>> +}\n\n(Good to learn about the comm command, thanks )\nWhat do we think about this:\n\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 3cd53f8..82eeba7 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -62,12 +62,16 @@ test_completion ()\n {\n \tif test $# -gt 1\n \tthen\n-\t\tprintf '%s\\n' \"$2\" >expected\n+\t\tprintf '%s\\n' \"$2\" | sort >expected.sorted\n \telse\n-\t\tsed -e 's/Z$//' >expected\n+\t\tsed -e 's/Z$//' | sort >expected.sorted\n \tfi &&\n \trun_completion \"$1\" &&\n-\ttest_cmp expected out\n+\tsort actual.sorted &&\n+\t>empty &&\n+\tcomm -23 expected.sorted actual.sorted >actual &&\n+\ttest_cmp empty actual &&\n+\trm empty actual\n }\n \n # Test __gitcomp.\n"},{"id":"205293","messageId":"7v7goc4eu1.fsf@alter.siamese.dyndns.org","threadId":"32364","inReplyTo":"50D37AB2.1040508@web.de","subject":"Re: $PATH pollution and t9902-completion.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-20T21:02:30Z","receivedAt":"2012-12-20T21:02:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On 20.12.12 21:01, Jeff King wrote:\n>> +test_fully_contains () {\n>>> +\tsort \"$1\" >expect.sorted &&\n>>> +\tsort \"$2\" >actual.sorted &&\n>>> +\ttest $(comm -23 expect.sorted actual.sorted | wc -l) = 0\n>>> +}\n>\n> (Good to learn about the comm command, thanks )\n> What do we think about this:\n\nThree points to consider.\n\n * Do _all_ tests that use test_completion not care about the actual\n order of choices that come out of the completion machinery?\n\n * Do _all_ tests that use test_completion not care about having\n extra choice, or is the command name completion the only odd\n case?\n\n * Is this still as easy to extend as the original to conditionally\n verify that all extra output share the same prefix?\n\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 3cd53f8..82eeba7 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -62,12 +62,16 @@ test_completion ()\n> {\n> \tif test $# -gt 1\n> \tthen\n> -\t\tprintf '%s\\n' \"$2\" >expected\n> +\t\tprintf '%s\\n' \"$2\" | sort >expected.sorted\n> \telse\n> -\t\tsed -e 's/Z$//' >expected\n> +\t\tsed -e 's/Z$//' | sort >expected.sorted\n> \tfi &&\n> \trun_completion \"$1\" &&\n> -\ttest_cmp expected out\n> +\tsort actual.sorted &&\n> +\t>empty &&\n> +\tcomm -23 expected.sorted actual.sorted >actual &&\n> +\ttest_cmp empty actual &&\n> +\trm empty actual\n> }\n> \n> # Test __gitcomp.\n"},{"id":"205295","messageId":"20121220210447.GA32032@sigill.intra.peff.net","threadId":"32364","inReplyTo":"50D37AB2.1040508@web.de","subject":"Re: $PATH pollution and t9902-completion.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-12-20T21:04:47Z","receivedAt":"2012-12-20T21:04:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 20, 2012 at 09:53:06PM +0100, Torsten Bögershausen wrote:\n\n> (Good to learn about the comm command, thanks )\n> What do we think about this:\n> \n> \n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 3cd53f8..82eeba7 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -62,12 +62,16 @@ test_completion ()\n> {\n> \tif test $# -gt 1\n> \tthen\n> -\t\tprintf '%s\\n' \"$2\" >expected\n> +\t\tprintf '%s\\n' \"$2\" | sort >expected.sorted\n> \telse\n> -\t\tsed -e 's/Z$//' >expected\n> +\t\tsed -e 's/Z$//' | sort >expected.sorted\n> \tfi &&\n> \trun_completion \"$1\" &&\n> -\ttest_cmp expected out\n> +\tsort actual.sorted &&\n> +\t>empty &&\n> +\tcomm -23 expected.sorted actual.sorted >actual &&\n> +\ttest_cmp empty actual &&\n> +\trm empty actual\n\nI like test_* helpers that tell you what happened on error, but this\noutput will be kind of a weird diff (it is a diff showing things you\nexpected to have but did not get as additions, and no mention of things\nyou expected and got).\n\nThe output is human readable, so I think it is perfectly OK to print a\nmessage about what is going on (we do this in test_expect_code, for\nexample). Something like:\n\n test_fully_contains() {\n sort \"$1\" >expect.sorted &&\n sort \"$2\" >actual.sorted &&\n comm -23 expect.sorted actual.sorted >missing\n test -f missing -a ! -s missing &&\n rm -f expect.sorted actual.sorted missing &&\n return 0\n\n {\n echo \"test_fully_contains: some lines were missing\"\n echo \"expected:\"\n sed 's/^/ /' <\"$1\"\n echo \"actual:\"\n sed 's/^/ /' <\"$2\"\n echo \"missing:\"\n sed 's/^/ /' &2\n return 1\n }\n\nThough come to think of it, just showing the diff between expect.sorted\nand actual.sorted would probably be enough. It would show the missing\nelements as deletions, the found elements as common lines, and the\n\"extra\" elements as additions (which are not an error, but when you are\ndebugging, might be useful to know about).\n\n-Peff\n"}]}