{"thread":{"id":"29527","subject":"[PATCH] t0300-credentials: Word around a solaris /bin/sh bug","startedAt":"2012-02-02T19:32:15Z","lastAt":"2012-02-04T07:00:09Z","messageCount":22,"participants":["Ben Walton","Frans Klaver","Jeff King","Jonathan Nieder","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"183638","messageId":"1328211135-25217-1-git-send-email-bwalton@artsci.utoronto.ca","threadId":"29527","inReplyTo":null,"subject":"[PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-02-02T19:32:15Z","receivedAt":"2012-02-02T19:32:15Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Solaris' /bin/sh was making the IFS setting permanent instead of\ntemporary when using it to slurp in credentials in the generated\n'dump' script of the 'setup helper scripts' test in t0300-credentials.\n\nThe stderr file that was being compared to expected-stderr contained the\nfollowing stray line from the credential helper run:\n\nwarning: invalid credential line: username foo\n\nTo avoid this bug, capture the original IFS and force it to be reset\nafter its use is no longer required. For now, this is lighter weight\nthan altering which shell these scripts use as their shebang.\n\nSigned-off-by: Ben Walton \n---\n t/t0300-credentials.sh | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 885af8f..1be3fe2 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -8,10 +8,12 @@ test_expect_success 'setup helper scripts' '\n \tcat >dump <<-\\EOF &&\n \twhoami=`echo $0 | sed s/.*git-credential-//`\n \techo >&2 \"$whoami: $*\"\n+\tOIFS=$IFS\n \twhile IFS== read key value; do\n \t\techo >&2 \"$whoami: $key=$value\"\n \t\teval \"$key=$value\"\n \tdone\n+\tIFS=$OIFS\n \tEOF\n \n \tcat >git-credential-useless <<-\\EOF &&\n-- \n1.7.8.3\n"},{"id":"183640","messageId":"op.v82g3ura0aolir@keputer","threadId":"29527","inReplyTo":"1328211135-25217-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Frans Klaver","fromEmail":"fransklaver@gmail.com","sentAt":"2012-02-02T19:44:08Z","receivedAt":"2012-02-02T19:44:08Z","isPatch":true,"sender":{"key":"fransklaver@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1876483?v=4"},"body":"Wor_k_ around ...\n\n\nOn Thu, 02 Feb 2012 20:32:15 +0100, Ben Walton \n wrote:\n\n> Solaris' /bin/sh was making the IFS setting permanent instead of\n> temporary when using it to slurp in credentials in the generated\n> 'dump' script of the 'setup helper scripts' test in t0300-credentials.\n>\n> The stderr file that was being compared to expected-stderr contained the\n> following stray line from the credential helper run:\n>\n> warning: invalid credential line: username foo\n>\n> To avoid this bug, capture the original IFS and force it to be reset\n> after its use is no longer required. For now, this is lighter weight\n> than altering which shell these scripts use as their shebang.\n>\n> Signed-off-by: Ben Walton \n> ---\n> t/t0300-credentials.sh | 2 ++\n> 1 files changed, 2 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> index 885af8f..1be3fe2 100755\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -8,10 +8,12 @@ test_expect_success 'setup helper scripts' '\n> \tcat >dump <<-\\EOF &&\n> \twhoami=`echo $0 | sed s/.*git-credential-//`\n> \techo >&2 \"$whoami: $*\"\n> +\tOIFS=$IFS\n> \twhile IFS== read key value; do\n> \t\techo >&2 \"$whoami: $key=$value\"\n> \t\teval \"$key=$value\"\n> \tdone\n> +\tIFS=$OIFS\n> \tEOF\n> \tcat >git-credential-useless <<-\\EOF &&\n\n\n-- \nUsing Opera's revolutionary e-mail client: http://www.opera.com/mail/\n"},{"id":"183642","messageId":"1328212038-sup-9896@pinkfloyd.chass.utoronto.ca","threadId":"29527","inReplyTo":"op.v82g3ura0aolir@keputer","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-02-02T19:48:29Z","receivedAt":"2012-02-02T19:48:29Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Frans Klaver's message of Thu Feb 02 14:44:08 -0500 2012:\n\n> Wor_k_ around ...\n\n*face*palm*\n\nThanks for catching that.\n\nJunio, can you make that tweak if the patch is ok or would you prefer\na new mail?\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"183647","messageId":"20120202200240.GC9246@sigill.intra.peff.net","threadId":"29527","inReplyTo":"1328211135-25217-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-02T20:02:40Z","receivedAt":"2012-02-02T20:02:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 02, 2012 at 02:32:15PM -0500, Ben Walton wrote:\n\n> Solaris' /bin/sh was making the IFS setting permanent instead of\n> temporary when using it to slurp in credentials in the generated\n> 'dump' script of the 'setup helper scripts' test in t0300-credentials.\n\nHmm. Presumably you are setting SHELL_PATH, as Solaris /bin/sh would be\nuseless for running the rest of the tests. Usually scripts inside the\ntests use #!$SHELL_PATH, but I often don't bother if it's a simple \"even\nSolaris /bin/sh could run this\" script. But in this case I either\nunderestimated the complexity of my script or overestimated the quality\nof the Solaris /bin/sh.\n\nI wonder if a better solution is to use a known-good shell instead of\ntrying to work around problems in a bogus shell. Does the patch below\nfix it for you?\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 885af8f..edf6547 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -14,15 +14,15 @@ test_expect_success 'setup helper scripts' '\n \tdone\n \tEOF\n \n-\tcat >git-credential-useless <<-\\EOF &&\n-\t#!/bin/sh\n+\tcat >git-credential-useless <<-EOF &&\n+\t#!$SHELL_PATH\n \t. ./dump\n \texit 0\n \tEOF\n \tchmod +x git-credential-useless &&\n \n-\tcat >git-credential-verbatim <<-\\EOF &&\n-\t#!/bin/sh\n+\techo \"#!$SHELL_PATH\" >git-credential-verbatim &&\n+\tcat >>git-credential-verbatim <<-\\EOF &&\n \tuser=$1; shift\n \tpass=$1; shift\n \t. ./dump\n"},{"id":"183656","messageId":"20120202201629.GA20200@burratino","threadId":"29527","inReplyTo":"1328211135-25217-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T20:16:29Z","receivedAt":"2012-02-02T20:16:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ben Walton wrote:\n\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -8,10 +8,12 @@ test_expect_success 'setup helper scripts' '\n> \tcat >dump <<-\\EOF &&\n> \twhoami=`echo $0 | sed s/.*git-credential-//`\n> \techo >&2 \"$whoami: $*\"\n> +\tOIFS=$IFS\n> \twhile IFS== read key value; do\n> \t\techo >&2 \"$whoami: $key=$value\"\n> \t\teval \"$key=$value\"\n> \tdone\n> +\tIFS=$OIFS\n\nOh, good catch. Technically \"read\" is not a special builtin so POSIX shells\nare not supposed to do this (and Jeff's patch definitely looks right), but in\nany case temporary variable settings while running a builtin are close\nenough to the assignment-during-special-builtin-or-function case to\nmake me shiver a little. ;-)\n\nWould something like\n\n\t(\n\t\tIFS==\n\t\twhile read key value\n\t\tdo\n\t\t\t...\n\t\tdone\n\t)\n\nmake sense?\n"},{"id":"183661","messageId":"vpq62fp3r15.fsf@bauges.imag.fr","threadId":"29527","inReplyTo":"20120202201629.GA20200@burratino","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2012-02-02T20:43:18Z","receivedAt":"2012-02-02T20:43:18Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jonathan Nieder writes:\n\n> Would something like\n>\n> \t(\n> \t\tIFS==\n> \t\twhile read key value\n> \t\tdo\n> \t\t\t...\n> \t\tdone\n> \t)\n>\n> make sense?\n\nI don't think so since the \"...\" contains\n\n eval \"$key=$value\"\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"183667","messageId":"20120202211146.GC19520@burratino","threadId":"29527","inReplyTo":"vpq62fp3r15.fsf@bauges.imag.fr","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T21:11:46Z","receivedAt":"2012-02-02T21:11:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Matthieu Moy wrote:\n> Jonathan Nieder writes:\n\n>> \t(\n>> \t\tIFS==\n>> \t\twhile read key value\n>> \t\tdo\n>> \t\t\t...\n>> \t\tdone\n>> \t)\n[...]\n> I don't think so since the \"...\" contains\n>\n> eval \"$key=$value\"\n\nOh, whoops. Thanks for noticing.\n\nHere's an updated patch, for amusement value. No functional change\nintended. I don't think it's actually worth applying unless people\nactively working on this file find the result easier to work with.\n\n-- >8 --\nSubject: t0300 (credentials): shell scripting style cleanups\n\nAs Ben noticed, the helper used by this test script assigns a\ntemporary value to IFS while calling the \"read\" builtin, which in\nancient shells causes the value to leak into the environment and\naffect later code in the same script. Explicitly save and restore IFS\nto avoid rekindling old memories.\n\nWhile at it, put the \"do\" associated to a \"while\" statement on its own\nline to match the house style and define helper scripts in the test\ndata section above all test assertions so the \"setup\" test itself is\nless cluttered and we can worry a little less about quoting issues.\n\nInspired-by: Ben Walton \nSigned-off-by: Jonathan Nieder \n---\n t/t0300-credentials.sh | 36 ++++++++++++++++++++----------------\n 1 files changed, 20 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex edf65478..780d5dcb 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -4,33 +4,37 @@ test_description='basic credential helper tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n \n-test_expect_success 'setup helper scripts' '\n-\tcat >dump <<-\\EOF &&\n+cat >dump <<-\\EOF\n \twhoami=`echo $0 | sed s/.*git-credential-//`\n \techo >&2 \"$whoami: $*\"\n-\twhile IFS== read key value; do\n+\tsave_IFS=$IFS\n+\tIFS==\n+\twhile read key value\n+\tdo\n \t\techo >&2 \"$whoami: $key=$value\"\n \t\teval \"$key=$value\"\n \tdone\n-\tEOF\n+\tIFS=$save_IFS\n+EOF\n \n-\tcat >git-credential-useless <<-EOF &&\n+cat >git-credential-useless <<-EOF\n \t#!$SHELL_PATH\n \t. ./dump\n \texit 0\n-\tEOF\n+EOF\n+\n+cat >git-credential-verbatim <<-EOF\n+\t#!$SHELL_PATH\n+\tuser=\\$1; shift\n+\tpass=\\$1; shift\n+\t. ./dump\n+\ttest -z \"\\$user\" || echo username=\\$user\n+\ttest -z \"\\$pass\" || echo password=\\$pass\n+EOF\n+\n+test_expect_success setup '\n \tchmod +x git-credential-useless &&\n-\n-\techo \"#!$SHELL_PATH\" >git-credential-verbatim &&\n-\tcat >>git-credential-verbatim <<-\\EOF &&\n-\tuser=$1; shift\n-\tpass=$1; shift\n-\t. ./dump\n-\ttest -z \"$user\" || echo username=$user\n-\ttest -z \"$pass\" || echo password=$pass\n-\tEOF\n \tchmod +x git-credential-verbatim &&\n-\n \tPATH=\"$PWD:$PATH\"\n '\n \n-- \n1.7.9\n"},{"id":"183684","messageId":"7vr4ycu3ty.fsf@alter.siamese.dyndns.org","threadId":"29527","inReplyTo":"20120202200240.GC9246@sigill.intra.peff.net","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-03T01:02:17Z","receivedAt":"2012-02-03T01:02:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wonder if a better solution is to use a known-good shell instead of\n> trying to work around problems in a bogus shell.\n\nYeah, I think that is a better approach.\n\nWhat prevents us from doing 's|^#! */bin/sh|$#$SHELL_PATH|' on everything\nin t/ directory (I am not suggesting to do this. I just want to know if\nthere is a reason we want hardcoded \"#!/bin/sh\" for some instances).\n"},{"id":"183708","messageId":"20120203120657.GB31441@sigill.intra.peff.net","threadId":"29527","inReplyTo":"7vr4ycu3ty.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-03T12:06:57Z","receivedAt":"2012-02-03T12:06:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 02, 2012 at 05:02:17PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I wonder if a better solution is to use a known-good shell instead of\n> > trying to work around problems in a bogus shell.\n> \n> Yeah, I think that is a better approach.\n> \n> What prevents us from doing 's|^#! */bin/sh|$#$SHELL_PATH|' on everything\n> in t/ directory (I am not suggesting to do this. I just want to know if\n> there is a reason we want hardcoded \"#!/bin/sh\" for some instances).\n\nThe quoting is more annoying, because you usually don't want\ninterpolation on the rest of the lines of your embedded script. So:\n\n cat >foo.sh <<\\EOF\n #!/bin/sh\n echo my arguments are \"$@\"\n EOF\n\ncannot have the mechanical replace you mentioned above. It would need:\n\n cat >foo.sh <foo.sh\n\nWhen I have hard-coded \"#!/bin/sh\", my thinking is usually \"this is less\ncumbersome to type and to read, and this script-let is so small that\neven Solaris will get it right\".\n\n-Peff\n"},{"id":"183730","messageId":"1328276601-sup-5887@pinkfloyd.chass.utoronto.ca","threadId":"29527","inReplyTo":"20120203120657.GB31441@sigill.intra.peff.net","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-02-03T13:45:07Z","receivedAt":"2012-02-03T13:45:07Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Jeff King's message of Fri Feb 03 07:06:57 -0500 2012:\n\n> When I have hard-coded \"#!/bin/sh\", my thinking is usually \"this is\n> less cumbersome to type and to read, and this script-let is so small\n> that even Solaris will get it right\".\n\nThis is why I opted to stick with /bin/sh and just avoid the damage.\nOverall, using a sane shell is a better option...It is harder to read\nthough.\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"183758","messageId":"7v7h03odyo.fsf@alter.siamese.dyndns.org","threadId":"29527","inReplyTo":"20120203120657.GB31441@sigill.intra.peff.net","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-03T20:32:15Z","receivedAt":"2012-02-03T20:32:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> cat >foo.sh <<\\EOF\n> #!/bin/sh\n> echo my arguments are \"$@\"\n> EOF\n>\n> cannot have the mechanical replace you mentioned above. It would need:\n>\n> cat >foo.sh < #!$SHELL_PATH\n> echo my arguments are \"\\$@\"\n> EOF\n>\n> or:\n>\n> {\n> echo \"#!$SHELL_PATH\" &&\n> cat < echo my arguments are \"$@\"\n> EOF\n> } >foo.sh\n>\n> When I have hard-coded \"#!/bin/sh\", my thinking is usually \"this is less\n> cumbersome to type and to read, and this script-let is so small that\n> even Solaris will get it right\".\n\nI am toying with the pros-and-cons of\n\n\twrite_script () {\n\t\techo \"#!$1\"\n\t\tshift\n cat\n\t}\n\nso that the above can become\n\n\twrite_script \"$SHELL_PATH\" >foo.sh <<-EOF\n echo my arguments are \"\\$@\"\n\tEOF\n\nwithout requiring the brain-cycle to waste on the \"Is this simple enough\nfor even Solaris to grok?\" guess game. This should also be reusable for\nother stuff like $PERL_PATH, I would think.\n"},{"id":"183761","messageId":"20120203212604.GA1890@sigill.intra.peff.net","threadId":"29527","inReplyTo":"7v7h03odyo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-03T21:26:04Z","receivedAt":"2012-02-03T21:26:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 03, 2012 at 12:32:15PM -0800, Junio C Hamano wrote:\n\n> I am toying with the pros-and-cons of\n> \n> \twrite_script () {\n> \t\techo \"#!$1\"\n> \t\tshift\n> cat\n> \t}\n> \n> so that the above can become\n> \n> \twrite_script \"$SHELL_PATH\" >foo.sh <<-EOF\n> echo my arguments are \"\\$@\"\n> \tEOF\n> \n> without requiring the brain-cycle to waste on the \"Is this simple enough\n> for even Solaris to grok?\" guess game. This should also be reusable for\n> other stuff like $PERL_PATH, I would think.\n\nI like it. Even better would be:\n\n write_script() {\n echo \"#!$2\" >\"$1\" &&\n cat >>\"$1\" &&\n chmod +x \"$1\"\n }\n\n write_script foo.sh \"$SHELL_PATH\" <<-\\EOF\n echo my arguments are \"$@\"\n EOF\n\n-Peff\n"},{"id":"183767","messageId":"7vr4ybmvrq.fsf@alter.siamese.dyndns.org","threadId":"29527","inReplyTo":"20120203212604.GA1890@sigill.intra.peff.net","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-03T21:50:33Z","receivedAt":"2012-02-03T21:50:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> without requiring the brain-cycle to waste on the \"Is this simple enough\n>> for even Solaris to grok?\" guess game. This should also be reusable for\n>> other stuff like $PERL_PATH, I would think.\n>\n> I like it. Even better would be:\n>\n> write_script() {\n> echo \"#!$2\" >\"$1\" &&\n> cat >>\"$1\" &&\n> chmod +x \"$1\"\n> }\n>\n> write_script foo.sh \"$SHELL_PATH\" <<-\\EOF\n> echo my arguments are \"$@\"\n> EOF\n\nI first thought that the order of parameters were unusual, but with that\norder, you could even go something fancier like:\n\n\twrite_script () {\n\t\tcase \"$#\" in\n\t\t1)\tcase \"$1\" in\n\t\t\t*.perl | *.pl) echo \"#!$PERL_PATH\" ;;\n\t\t\t*) echo \"#!$SHELL_PATH\" ;;\n\t\t\tesac\n 2)\techo \"#!$2\" ;;\n\t\t*)\tBUG ;;\n esac >\"$1\" &&\n cat >>\"$1\" &&\n chmod +x \"$1\"\n\t}\n\n\twrite_script foo.sh\n write_script bar.perl\n write_script pre-receive /no/frobnication/today\n\nThe tongue-in-cheek comment aside, I think ${2-\"$SHELL_PATH\"} or some form\nof fallback would be a good idea in any case, as 99% of the time what we\nwrite in the test scripts is a shell script.\n\nAlso \"chmod +x\" is a very good idea.\n\n \n"},{"id":"183770","messageId":"20120203215507.GB3472@sigill.intra.peff.net","threadId":"29527","inReplyTo":"7vr4ybmvrq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-03T21:55:07Z","receivedAt":"2012-02-03T21:55:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 03, 2012 at 01:50:33PM -0800, Junio C Hamano wrote:\n\n> > write_script foo.sh \"$SHELL_PATH\" <<-\\EOF\n> > echo my arguments are \"$@\"\n> > EOF\n> \n> I first thought that the order of parameters were unusual, but with that\n> order, you could even go something fancier like:\n> \n> \twrite_script () {\n> \t\tcase \"$#\" in\n> \t\t1)\tcase \"$1\" in\n> \t\t\t*.perl | *.pl) echo \"#!$PERL_PATH\" ;;\n> \t\t\t*) echo \"#!$SHELL_PATH\" ;;\n> \t\t\tesac\n> 2)\techo \"#!$2\" ;;\n> \t\t*)\tBUG ;;\n> esac >\"$1\" &&\n> cat >>\"$1\" &&\n> chmod +x \"$1\"\n> \t}\n> \n\nNice. I was going to suggest a wrapper like \"write_sh_script\" so you\ndidn't have to spell out $SHELL_PATH, but I think the auto-detection\nmakes sense (and falling back to shell makes even more sense, as that\ncovers 99% of the cases anyway).\n\n-Peff\n"},{"id":"183771","messageId":"1328306228-sup-8799@pinkfloyd.chass.utoronto.ca","threadId":"29527","inReplyTo":"20120203215507.GB3472@sigill.intra.peff.net","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-02-03T22:00:34Z","receivedAt":"2012-02-03T22:00:34Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Jeff King's message of Fri Feb 03 16:55:07 -0500 2012:\n\n> > write_script () {\n> > case \"$#\" in\n> > 1) case \"$1\" in\n> > *.perl | *.pl) echo \"#!$PERL_PATH\" ;;\n> > *) echo \"#!$SHELL_PATH\" ;;\n> > esac\n> > 2) echo \"#!$2\" ;;\n> > *) BUG ;;\n> > esac >\"$1\" &&\n> > cat >>\"$1\" &&\n> > chmod +x \"$1\"\n> > }\n> > \n> \n> Nice. I was going to suggest a wrapper like \"write_sh_script\" so you\n> didn't have to spell out $SHELL_PATH, but I think the auto-detection\n> makes sense (and falling back to shell makes even more sense, as that\n> covers 99% of the cases anyway).\n\nThis looks like a very nice, general purpose, solution to the problem.\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"183775","messageId":"7vipjnmt8a.fsf@alter.siamese.dyndns.org","threadId":"29527","inReplyTo":"20120203215507.GB3472@sigill.intra.peff.net","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-03T22:45:25Z","receivedAt":"2012-02-03T22:45:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> 2)\techo \"#!$2\" ;;\n>> \t\t*)\tBUG ;;\n>> esac >\"$1\" &&\n>> cat >>\"$1\" &&\n>> chmod +x \"$1\"\n>> \t}\n>> \n>\n> Nice. I was going to suggest a wrapper like \"write_sh_script\" so you\n> didn't have to spell out $SHELL_PATH, but I think the auto-detection\n> makes sense (and falling back to shell makes even more sense, as that\n> covers 99% of the cases anyway).\n\nLet's not over-engineer this and stick to the simple-stupid-sufficient.\n\nSomething like this?\n\n t/test-lib.sh | 9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex bdd9513..1b9c461 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -379,6 +379,15 @@ test_config () {\n \tgit config \"$@\"\n }\n \n+# Prepare a script to be used in the test\n+write_script () {\n+\t{\n+\t\techo \"#!${2-\"$SHELL_PATH\"}\"\n+\t\tcat\n+\t} >\"$1\" &&\n+\tchmod +x \"$1\"\n+}\n+\n # Use test_set_prereq to tell that a particular prerequisite is available.\n # The prerequisite can later be checked for in two ways:\n #\n"},{"id":"183787","messageId":"20120203232755.GA11953@sigill.intra.peff.net","threadId":"29527","inReplyTo":"7vipjnmt8a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-03T23:27:55Z","receivedAt":"2012-02-03T23:27:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 03, 2012 at 02:45:25PM -0800, Junio C Hamano wrote:\n\n> Let's not over-engineer this and stick to the simple-stupid-sufficient.\n\nFair enough.\n\n> Something like this?\n> [...]\n> +# Prepare a script to be used in the test\n> +write_script () {\n> +\t{\n> +\t\techo \"#!${2-\"$SHELL_PATH\"}\"\n> +\t\tcat\n> +\t} >\"$1\" &&\n> +\tchmod +x \"$1\"\n> +}\n\nLooks good to me (it probably doesn't matter, but you may want to\nconnect the echo and cat via &&).\n\n-Peff\n"},{"id":"183806","messageId":"20120204062712.GA20076@sigill.intra.peff.net","threadId":"29527","inReplyTo":"7vipjnmt8a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-04T06:27:12Z","receivedAt":"2012-02-04T06:27:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 03, 2012 at 02:45:25PM -0800, Junio C Hamano wrote:\n\n> > Nice. I was going to suggest a wrapper like \"write_sh_script\" so you\n> > didn't have to spell out $SHELL_PATH, but I think the auto-detection\n> > makes sense (and falling back to shell makes even more sense, as that\n> > covers 99% of the cases anyway).\n> \n> Let's not over-engineer this and stick to the simple-stupid-sufficient.\n> \n> Something like this?\n\nHere it is as patches with commit messages. I don't think it's worth\ndoing a mechanical conversion of the whole test suite to write_script.\n\n [1/2]: tests: add write_script helper function\n [2/2]: t0300: use write_script helper\n\n-Peff\n"},{"id":"183807","messageId":"20120204062901.GA21559@sigill.intra.peff.net","threadId":"29527","inReplyTo":"20120204062712.GA20076@sigill.intra.peff.net","subject":"[PATCH 1/2] tests: add write_script helper function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-04T06:29:01Z","receivedAt":"2012-02-04T06:29:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Junio C Hamano \n\nMany of the scripts in the test suite write small helper\nshell scripts to disk. It's best if these shell scripts\nstart with \"#!$SHELL_PATH\" rather than \"#!/bin/sh\", because\n/bin/sh on some platforms is too buggy to be used.\n\nHowever, it can be cumbersome to expand $SHELL_PATH, because\nthe usual recipe for writing a script is:\n\n\tcat >foo.sh <<-\\EOF\n\t#!/bin/sh\n\techo my arguments are \"$@\"\n\tEOF\n\nTo expand $SHELL_PATH, you have to either interpolate the\nhere-doc (which would require quoting \"\\$@\"), or split the\ncreation into two commands (interpolating the $SHELL_PATH\nline, but not the rest of the script). Let's provide a\nhelper function that makes that less syntactically painful.\n\nWhile we're at it, this helper can also take care of the\n\"chmod +x\" that typically comes after the creation of such a\nscript, saving the caller a line.\n\nSigned-off-by: Jeff King \n---\nI suspect you already have this in your repo, but maybe the commit\nmessage is useful.\n\n t/test-lib.sh | 8 ++++++++\n 1 files changed, 8 insertions(+), 0 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex b22bee7..254849e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -400,6 +400,14 @@ test_config_global () {\n \tgit config --global \"$@\"\n }\n \n+write_script () {\n+\t{\n+\t\techo \"#!${2-\"$SHELL_PATH\"}\" &&\n+\t\tcat\n+\t} >\"$1\" &&\n+\tchmod +x \"$1\"\n+}\n+\n # Use test_set_prereq to tell that a particular prerequisite is available.\n # The prerequisite can later be checked for in two ways:\n #\n-- \n1.7.9.rc1.28.gf4be5\n"},{"id":"183808","messageId":"20120204063018.GB21559@sigill.intra.peff.net","threadId":"29527","inReplyTo":"20120204062712.GA20076@sigill.intra.peff.net","subject":"[PATCH 2/2] t0300: use write_script helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-04T06:30:18Z","receivedAt":"2012-02-04T06:30:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"t0300 creates some helper shell scripts, and marks them with\n\"!/bin/sh\". Even though the scripts are fairly simple, they\ncan fail on broken shells (specifically, Solaris /bin/sh\nwill persist a temporary assignment to IFS in a \"read\"\ncommand).\n\nRather than work around the problem for Solaris /bin/sh,\nusing write_script will make sure we point to a known-good\nshell that the user has given us.\n\nSigned-off-by: Jeff King \n---\nThis works fine on my Linux box, but just to sanity check that I didn't\nscrew anything up in the whopping 5 lines of changes, can you confirm\nthis fixes the issue for you, Ben?\n\n t/t0300-credentials.sh | 6 ++----\n 1 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 885af8f..0b46248 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -14,14 +14,13 @@ test_expect_success 'setup helper scripts' '\n \tdone\n \tEOF\n \n-\tcat >git-credential-useless <<-\\EOF &&\n+\twrite_script git-credential-useless <<-\\EOF &&\n \t#!/bin/sh\n \t. ./dump\n \texit 0\n \tEOF\n-\tchmod +x git-credential-useless &&\n \n-\tcat >git-credential-verbatim <<-\\EOF &&\n+\twrite_script git-credential-verbatim <<-\\EOF &&\n \t#!/bin/sh\n \tuser=$1; shift\n \tpass=$1; shift\n@@ -29,7 +28,6 @@ test_expect_success 'setup helper scripts' '\n \ttest -z \"$user\" || echo username=$user\n \ttest -z \"$pass\" || echo password=$pass\n \tEOF\n-\tchmod +x git-credential-verbatim &&\n \n \tPATH=\"$PWD:$PATH\"\n '\n-- \n1.7.9.rc1.28.gf4be5\n"},{"id":"183811","messageId":"7vd39vjda9.fsf@alter.siamese.dyndns.org","threadId":"29527","inReplyTo":"20120204063018.GB21559@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] t0300: use write_script helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-04T06:58:06Z","receivedAt":"2012-02-04T06:58:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> t0300 creates some helper shell scripts, and marks them with\n> \"!/bin/sh\". Even though the scripts are fairly simple, they\n> can fail on broken shells (specifically, Solaris /bin/sh\n> will persist a temporary assignment to IFS in a \"read\"\n> command).\n>\n> Rather than work around the problem for Solaris /bin/sh,\n> using write_script will make sure we point to a known-good\n> shell that the user has given us.\n>\n> Signed-off-by: Jeff King \n> ---\n> This works fine on my Linux box, but just to sanity check that I didn't\n> screw anything up in the whopping 5 lines of changes, can you confirm\n> this fixes the issue for you, Ben?\n>\n> t/t0300-credentials.sh | 6 ++----\n> 1 files changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> index 885af8f..0b46248 100755\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -14,14 +14,13 @@ test_expect_success 'setup helper scripts' '\n> \tdone\n> \tEOF\n> \n> -\tcat >git-credential-useless <<-\\EOF &&\n> +\twrite_script git-credential-useless <<-\\EOF &&\n> \t#!/bin/sh\n\nAn innocuous facepalm I'd be glad to remove myself ;-)\n\n> \t. ./dump\n> \texit 0\n> \tEOF\n> -\tchmod +x git-credential-useless &&\n> \n> -\tcat >git-credential-verbatim <<-\\EOF &&\n> +\twrite_script git-credential-verbatim <<-\\EOF &&\n> \t#!/bin/sh\n\nBut other than that, looks good.\n\n> \tuser=$1; shift\n> \tpass=$1; shift\n> @@ -29,7 +28,6 @@ test_expect_success 'setup helper scripts' '\n> \ttest -z \"$user\" || echo username=$user\n> \ttest -z \"$pass\" || echo password=$pass\n> \tEOF\n> -\tchmod +x git-credential-verbatim &&\n> \n> \tPATH=\"$PWD:$PATH\"\n> '\n"},{"id":"183812","messageId":"20120204070009.GA22188@sigill.intra.peff.net","threadId":"29527","inReplyTo":"7vd39vjda9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] t0300: use write_script helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-04T07:00:09Z","receivedAt":"2012-02-04T07:00:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 03, 2012 at 10:58:06PM -0800, Junio C Hamano wrote:\n\n> > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> > index 885af8f..0b46248 100755\n> > --- a/t/t0300-credentials.sh\n> > +++ b/t/t0300-credentials.sh\n> > @@ -14,14 +14,13 @@ test_expect_success 'setup helper scripts' '\n> > \tdone\n> > \tEOF\n> > \n> > -\tcat >git-credential-useless <<-\\EOF &&\n> > +\twrite_script git-credential-useless <<-\\EOF &&\n> > \t#!/bin/sh\n> \n> An innocuous facepalm I'd be glad to remove myself ;-)\n\nHeh, it took me a second to notice it, even after you mentioned it. And\nit's even right there in the context. At least the line written by\nwrite_script takes precedence. :)\n\nThanks.\n\n-Peff\n"}]}