{"thread":{"id":"53473","subject":"[PATCH] t0300: workaround bug in FreeBSD < 10 sh","startedAt":"2020-05-14T21:05:30Z","lastAt":"2020-05-14T23:43:07Z","messageCount":7,"participants":["Carlo Marcelo Arenas Belón","Jeff King","Eric Sunshine","brian m. carlson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"397826","messageId":"20200514210518.56101-1-carenas@gmail.com","threadId":"53473","inReplyTo":null,"subject":"[PATCH] t0300: workaround bug in FreeBSD < 10 sh","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-14T21:05:18Z","receivedAt":"2020-05-14T21:05:30Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"4c5971e18a (credential: treat \"?\" and \"#\" in URLs as end of host,\n2020-04-14) introduces check_host_and_path to t0300 and some tests that\nuse it, but fail in at least FreeBSD 9.3.\n\nThe variables in the here-doc fail to be expanded until they are used as\npart of the eval in check(), resulting in (ex: url=fill) instead of what\nwas expected.\n\nTransfer the 3 parameters to local variables which will be used instead\nand that will be in scope without conflicting names.\n\nWhile at it, make sure all of the parameters which potentially sensitive\ncharacters (ex: ?#), are quote protected.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n t/t0300-credentials.sh | 38 ++++++++++++++++++++++----------------\n 1 file changed, 22 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex bc2d74098f..e0b7c001f1 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -622,37 +622,43 @@ test_expect_success 'credential system refuses to work with missing protocol' '\n \n # usage: check_host_and_path <url> <expected-host> <expected-path>\n check_host_and_path () {\n+\tlocal url=$1\n+\tlocal host=$2\n+\tlocal path=$3\n+\n \t# we always parse the path component, but we need this to make sure it\n \t# is passed to the helper\n \ttest_config credential.useHTTPPath true &&\n-\tcheck fill \"verbatim user pass\" <<-EOF\n-\turl=$1\n+\tcheck fill 'verbatim user pass' <<-EOF\n+\turl=$url\n \t--\n \tprotocol=https\n-\thost=$2\n-\tpath=$3\n+\thost=$host\n+\tpath=$path\n \tusername=user\n \tpassword=pass\n \t--\n \tverbatim: get\n \tverbatim: protocol=https\n-\tverbatim: host=$2\n-\tverbatim: path=$3\n+\tverbatim: host=$host\n+\tverbatim: path=$path\n \tEOF\n }\n \n-test_expect_success 'url parser handles bare query marker' '\n-\tcheck_host_and_path https://example.com?foo.git example.com ?foo.git\n-'\n+test_expect_success 'url parser handles bare query marker' \"\n+\tcheck_host_and_path 'https://example.com?foo.git' \\\n+\t\texample.com '?foo.git'\n+\"\n \n-test_expect_success 'url parser handles bare fragment marker' '\n-\tcheck_host_and_path https://example.com#foo.git example.com \"#foo.git\"\n-'\n+test_expect_success 'url parser handles bare fragment marker' \"\n+\tcheck_host_and_path 'https://example.com#foo.git' \\\n+\t\texample.com '#foo.git'\n+\"\n \n-test_expect_success 'url parser not confused by encoded markers' '\n-\tcheck_host_and_path https://example.com%23%3f%2f/foo.git \\\n-\t\t\"example.com#?/\" foo.git\n-'\n+test_expect_success 'url parser not confused by encoded markers' \"\n+\tcheck_host_and_path 'https://example.com%23%3f%2f/foo.git' \\\n+\t\t'example.com#?/' foo.git\n+\"\n \n test_expect_success 'credential config with partial URLs' '\n \techo \"echo password=yep\" | write_script git-credential-yep &&\n-- \n2.26.2.812.g046d49d455\n\n"},{"id":"397836","messageId":"20200514220346.GA3074610@coredump.intra.peff.net","threadId":"53473","inReplyTo":"20200514210518.56101-1-carenas@gmail.com","subject":"Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-14T22:03:46Z","receivedAt":"2020-05-14T22:03:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 14, 2020 at 02:05:18PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> 4c5971e18a (credential: treat \"?\" and \"#\" in URLs as end of host,\n> 2020-04-14) introduces check_host_and_path to t0300 and some tests that\n> use it, but fail in at least FreeBSD 9.3.\n> \n> The variables in the here-doc fail to be expanded until they are used as\n> part of the eval in check(), resulting in (ex: url=fill) instead of what\n> was expected.\n\nWow, that's very surprising.\n\nJust to be clear, if you run:\n\nfoo() {\n  for i in \"$@\"; do\n    echo \"arg:$i\"\n  done\n  sed s/^/stdin:/\n}\nset -- outer\nfoo inner <<EOF\n$1\nEOF\n\ndo you get:\n\narg:inner\nstdin:inner\n\n? (on dash and bash, I get stdin:outer as expected). I don't think the\nfact that check() uses eval() should matter, because we'd be\ninterpreting that here-doc earlier as part of read_chunk().\n\n> While at it, make sure all of the parameters which potentially sensitive\n> characters (ex: ?#), are quote protected.\n\nI don't mind more quoting, but...\n\n> -test_expect_success 'url parser handles bare query marker' '\n> -\tcheck_host_and_path https://example.com?foo.git example.com ?foo.git\n> -'\n> +test_expect_success 'url parser handles bare query marker' \"\n> +\tcheck_host_and_path 'https://example.com?foo.git' \\\n> +\t\texample.com '?foo.git'\n> +\"\n\n...please don't invert the double and single quotes. In either case the\nmetacharacter is subject to double-quote expansion, and by putting\nthe double-quotes on the outside, you've left a trap for somebody adding\nmore lines to the test (the shell snippet will now be interpolated\nbefore being eval'd, which is contrary to how most of our tests run).\n\n-Peff\n"},{"id":"397841","messageId":"CAPig+cTEO--BCE_hMsWNowx7R9pP-oag2_ugEPDTPAizqOMDsQ@mail.gmail.com","threadId":"53473","inReplyTo":"20200514220346.GA3074610@coredump.intra.peff.net","subject":"Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-14T22:44:42Z","receivedAt":"2020-05-14T22:44:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 14, 2020 at 6:03 PM Jeff King <peff@peff.net> wrote:\n> Just to be clear, if you run:\n>\n> foo() {\n>   for i in \"$@\"; do\n>     echo \"arg:$i\"\n>   done\n>   sed s/^/stdin:/\n> }\n> set -- outer\n> foo inner <<EOF\n> $1\n> EOF\n>\n> do you get:\n>\n> arg:inner\n> stdin:inner\n\nThat is indeed the result with the FreeBSD 9.3 shell. Seems buggy.\nAssigning $1, $2, ... to variables (local or not) as Carlo's patch\ndoes seems to work around the buggy behavior.\n"},{"id":"397843","messageId":"20200514225514.GA6362@camp.crustytoothpaste.net","threadId":"53473","inReplyTo":"20200514210518.56101-1-carenas@gmail.com","subject":"Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-14T22:55:14Z","receivedAt":"2020-05-14T22:55:21Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-14 at 21:05:18, Carlo Marcelo Arenas Belón wrote:\n> 4c5971e18a (credential: treat \"?\" and \"#\" in URLs as end of host,\n> 2020-04-14) introduces check_host_and_path to t0300 and some tests that\n> use it, but fail in at least FreeBSD 9.3.\n\nFrom FreeBSD's website, it looks like the production releases are 11.3\nand 12.1.  9.3 is EOL and has been since 2019.  Since FreeBSD is not\nsupporting this release with security updates, nobody should be using\nit.  In light of that, do we need this patch?\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"397844","messageId":"xmqq5zcyp188.fsf@gitster.c.googlers.com","threadId":"53473","inReplyTo":"20200514225514.GA6362@camp.crustytoothpaste.net","subject":"Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-14T23:04:39Z","receivedAt":"2020-05-14T23:04:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2020-05-14 at 21:05:18, Carlo Marcelo Arenas Belón wrote:\n>> 4c5971e18a (credential: treat \"?\" and \"#\" in URLs as end of host,\n>> 2020-04-14) introduces check_host_and_path to t0300 and some tests that\n>> use it, but fail in at least FreeBSD 9.3.\n>\n> From FreeBSD's website, it looks like the production releases are 11.3\n> and 12.1.  9.3 is EOL and has been since 2019.  Since FreeBSD is not\n> supporting this release with security updates, nobody should be using\n> it.  In light of that, do we need this patch?\n\nPerfect comment given on the day I am cutting -rc0---the fewer\npatches I need to worry about, the better ;-).\n"},{"id":"397848","messageId":"20200514233051.GB38566@Carlos-MBP","threadId":"53473","inReplyTo":"20200514220346.GA3074610@coredump.intra.peff.net","subject":"Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-14T23:30:51Z","receivedAt":"2020-05-14T23:30:57Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, May 14, 2020 at 06:03:46PM -0400, Jeff King wrote:\n> On Thu, May 14, 2020 at 02:05:18PM -0700, Carlo Marcelo Arenas Belón wrote:\n> \n> > 4c5971e18a (credential: treat \"?\" and \"#\" in URLs as end of host,\n> > 2020-04-14) introduces check_host_and_path to t0300 and some tests that\n> > use it, but fail in at least FreeBSD 9.3.\n> > \n> > The variables in the here-doc fail to be expanded until they are used as\n> > part of the eval in check(), resulting in (ex: url=fill) instead of what\n> > was expected.\n> \n> Wow, that's very surprising.\n\nAnd luckily, also a problem that is no longer present with newer versions of\nthe shell and neither NetBSD's or OpenBSD's AFAIK.\n\n> Just to be clear, if you run:\n> \n> foo() {\n>   for i in \"$@\"; do\n>     echo \"arg:$i\"\n>   done\n>   sed s/^/stdin:/\n> }\n> set -- outer\n> foo inner <<EOF\n> $1\n> EOF\n> \n> do you get:\n> \n> arg:inner\n> stdin:inner\n\ncorrect, so the problem was in the here-doc, not the eval; thanks for your\nexplanation and reproduction, will update it in the reroll.\n\n$ foo() {\n>   for i in \"$@\"; do\n>     echo \"arg:$i\"\n>   done\n>   sed s/^/stdin:/\n> }\n$ set -- outer\n$ foo inner <<EOF\n> $1\n> EOF\narg:inner\nstdin:inner\n\n> ? (on dash and bash, I get stdin:outer as expected). I don't think the\n> fact that check() uses eval() should matter, because we'd be\n> interpreting that here-doc earlier as part of read_chunk().\n\nindeed, apologies for the extra quoting which was part of my original\ndebugging and wasn't really needed.\n\nwill keep it without the inversion, only because I think it looks also\nclearer with the added indentantion.\n\nCarlo\n"},{"id":"397849","messageId":"20200514234301.GC38566@Carlos-MBP","threadId":"53473","inReplyTo":"xmqq5zcyp188.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-14T23:43:01Z","receivedAt":"2020-05-14T23:43:07Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, May 14, 2020 at 04:04:39PM -0700, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > On 2020-05-14 at 21:05:18, Carlo Marcelo Arenas Belón wrote:\n> >> 4c5971e18a (credential: treat \"?\" and \"#\" in URLs as end of host,\n> >> 2020-04-14) introduces check_host_and_path to t0300 and some tests that\n> >> use it, but fail in at least FreeBSD 9.3.\n> >\n> > From FreeBSD's website, it looks like the production releases are 11.3\n> > and 12.1.  9.3 is EOL and has been since 2019.  Since FreeBSD is not\n> > supporting this release with security updates, nobody should be using\n> > it.  In light of that, do we need this patch?\n\nNot urgently, and my rationale was that it might get reverted and dropped\nin the near future, like we did for those workarounds for defunct versions\nof dash.\n\nWhich is why it was clearly marked as a bug and the version it affected.\n\nFWIW there are some people still building git in macOS 10.5 (the last with\npowerpc support) that was EOL in 2011.\n\n> Perfect comment given on the day I am cutting -rc0---the fewer\n> patches I need to worry about, the better ;-).\n\nI was considering it will be worth pu (in case someone might need to pull\nit), but definitely not needed in master for this release.\n\nCarlo\n"}]}