[PATCH] t0300: workaround bug in FreeBSD < 10 sh

[PATCH] t0300: workaround bug in FreeBSD < 10 sh

[Carlo Marcelo Arenas Belón]

4c5971e18a (credential: treat "?" and "#" in URLs as end of host,
2020-04-14) introduces check_host_and_path to t0300 and some tests that
use it, but fail in at least FreeBSD 9.3.

The variables in the here-doc fail to be expanded until they are used as
part of the eval in check(), resulting in (ex: url=fill) instead of what
was expected.

Transfer the 3 parameters to local variables which will be used instead
and that will be in scope without conflicting names.

While at it, make sure all of the parameters which potentially sensitive
characters (ex: ?#), are quote protected.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t0300-credentials.sh | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index bc2d74098f..e0b7c001f1 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -622,37 +622,43 @@ test_expect_success 'credential system refuses to work with missing protocol' '
 
 # usage: check_host_and_path <url> <expected-host> <expected-path>
 check_host_and_path () {
+	local url=$1
+	local host=$2
+	local path=$3
+
 	# we always parse the path component, but we need this to make sure it
 	# is passed to the helper
 	test_config credential.useHTTPPath true &&
-	check fill "verbatim user pass" <<-EOF
-	url=$1
+	check fill 'verbatim user pass' <<-EOF
+	url=$url
 	--
 	protocol=https
-	host=$2
-	path=$3
+	host=$host
+	path=$path
 	username=user
 	password=pass
 	--
 	verbatim: get
 	verbatim: protocol=https
-	verbatim: host=$2
-	verbatim: path=$3
+	verbatim: host=$host
+	verbatim: path=$path
 	EOF
 }
 
-test_expect_success 'url parser handles bare query marker' '
-	check_host_and_path https://example.com?foo.git example.com ?foo.git
-'
+test_expect_success 'url parser handles bare query marker' "
+	check_host_and_path 'https://example.com?foo.git' \
+		example.com '?foo.git'
+"
 
-test_expect_success 'url parser handles bare fragment marker' '
-	check_host_and_path https://example.com#foo.git example.com "#foo.git"
-'
+test_expect_success 'url parser handles bare fragment marker' "
+	check_host_and_path 'https://example.com#foo.git' \
+		example.com '#foo.git'
+"
 
-test_expect_success 'url parser not confused by encoded markers' '
-	check_host_and_path https://example.com%23%3f%2f/foo.git \
-		"example.com#?/" foo.git
-'
+test_expect_success 'url parser not confused by encoded markers' "
+	check_host_and_path 'https://example.com%23%3f%2f/foo.git' \
+		'example.com#?/' foo.git
+"
 
 test_expect_success 'credential config with partial URLs' '
 	echo "echo password=yep" | write_script git-credential-yep &&
-- 
2.26.2.812.g046d49d455

Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh

[Jeff King]

On Thu, May 14, 2020 at 02:05:18PM -0700, Carlo Marcelo Arenas Belón wrote:

> 4c5971e18a (credential: treat "?" and "#" in URLs as end of host,
> 2020-04-14) introduces check_host_and_path to t0300 and some tests that
> use it, but fail in at least FreeBSD 9.3.
> 
> The variables in the here-doc fail to be expanded until they are used as
> part of the eval in check(), resulting in (ex: url=fill) instead of what
> was expected.

Wow, that's very surprising.

Just to be clear, if you run:

foo() {
  for i in "$@"; do
    echo "arg:$i"
  done
  sed s/^/stdin:/
}
set -- outer
foo inner <<EOF
$1
EOF

do you get:

arg:inner
stdin:inner

? (on dash and bash, I get stdin:outer as expected). I don't think the
fact that check() uses eval() should matter, because we'd be
interpreting that here-doc earlier as part of read_chunk().

> While at it, make sure all of the parameters which potentially sensitive
> characters (ex: ?#), are quote protected.

I don't mind more quoting, but...

> -test_expect_success 'url parser handles bare query marker' '
> -	check_host_and_path https://example.com?foo.git example.com ?foo.git
> -'
> +test_expect_success 'url parser handles bare query marker' "
> +	check_host_and_path 'https://example.com?foo.git' \
> +		example.com '?foo.git'
> +"

...please don't invert the double and single quotes. In either case the
metacharacter is subject to double-quote expansion, and by putting
the double-quotes on the outside, you've left a trap for somebody adding
more lines to the test (the shell snippet will now be interpolated
before being eval'd, which is contrary to how most of our tests run).

-Peff

Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh

[Eric Sunshine]

On Thu, May 14, 2020 at 6:03 PM Jeff King <peff@peff.net> wrote:
> Just to be clear, if you run:
>
> foo() {
>   for i in "$@"; do
>     echo "arg:$i"
>   done
>   sed s/^/stdin:/
> }
> set -- outer
> foo inner <<EOF
> $1
> EOF
>
> do you get:
>
> arg:inner
> stdin:inner

That is indeed the result with the FreeBSD 9.3 shell. Seems buggy.
Assigning $1, $2, ... to variables (local or not) as Carlo's patch
does seems to work around the buggy behavior.

Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh

[Carlo Marcelo Arenas Belón]

On Thu, May 14, 2020 at 06:03:46PM -0400, Jeff King wrote:
> On Thu, May 14, 2020 at 02:05:18PM -0700, Carlo Marcelo Arenas Belón wrote:
> 
> > 4c5971e18a (credential: treat "?" and "#" in URLs as end of host,
> > 2020-04-14) introduces check_host_and_path to t0300 and some tests that
> > use it, but fail in at least FreeBSD 9.3.
> > 
> > The variables in the here-doc fail to be expanded until they are used as
> > part of the eval in check(), resulting in (ex: url=fill) instead of what
> > was expected.
> 
> Wow, that's very surprising.

And luckily, also a problem that is no longer present with newer versions of
the shell and neither NetBSD's or OpenBSD's AFAIK.

> Just to be clear, if you run:
> 
> foo() {
>   for i in "$@"; do
>     echo "arg:$i"
>   done
>   sed s/^/stdin:/
> }
> set -- outer
> foo inner <<EOF
> $1
> EOF
> 
> do you get:
> 
> arg:inner
> stdin:inner

correct, so the problem was in the here-doc, not the eval; thanks for your
explanation and reproduction, will update it in the reroll.

$ foo() {
>   for i in "$@"; do
>     echo "arg:$i"
>   done
>   sed s/^/stdin:/
> }
$ set -- outer
$ foo inner <<EOF
> $1
> EOF
arg:inner
stdin:inner

> ? (on dash and bash, I get stdin:outer as expected). I don't think the
> fact that check() uses eval() should matter, because we'd be
> interpreting that here-doc earlier as part of read_chunk().

indeed, apologies for the extra quoting which was part of my original
debugging and wasn't really needed.

will keep it without the inversion, only because I think it looks also
clearer with the added indentantion.

Carlo

Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On 2020-05-14 at 21:05:18, Carlo Marcelo Arenas Belón wrote:
> 4c5971e18a (credential: treat "?" and "#" in URLs as end of host,
> 2020-04-14) introduces check_host_and_path to t0300 and some tests that
> use it, but fail in at least FreeBSD 9.3.

From FreeBSD's website, it looks like the production releases are 11.3
and 12.1.  9.3 is EOL and has been since 2019.  Since FreeBSD is not
supporting this release with security updates, nobody should be using
it.  In light of that, do we need this patch?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2020-05-14 at 21:05:18, Carlo Marcelo Arenas Belón wrote:
>> 4c5971e18a (credential: treat "?" and "#" in URLs as end of host,
>> 2020-04-14) introduces check_host_and_path to t0300 and some tests that
>> use it, but fail in at least FreeBSD 9.3.
>
> From FreeBSD's website, it looks like the production releases are 11.3
> and 12.1.  9.3 is EOL and has been since 2019.  Since FreeBSD is not
> supporting this release with security updates, nobody should be using
> it.  In light of that, do we need this patch?

Perfect comment given on the day I am cutting -rc0---the fewer
patches I need to worry about, the better ;-).

Re: [PATCH] t0300: workaround bug in FreeBSD < 10 sh

[Carlo Marcelo Arenas Belón]

On Thu, May 14, 2020 at 04:04:39PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > On 2020-05-14 at 21:05:18, Carlo Marcelo Arenas Belón wrote:
> >> 4c5971e18a (credential: treat "?" and "#" in URLs as end of host,
> >> 2020-04-14) introduces check_host_and_path to t0300 and some tests that
> >> use it, but fail in at least FreeBSD 9.3.
> >
> > From FreeBSD's website, it looks like the production releases are 11.3
> > and 12.1.  9.3 is EOL and has been since 2019.  Since FreeBSD is not
> > supporting this release with security updates, nobody should be using
> > it.  In light of that, do we need this patch?

Not urgently, and my rationale was that it might get reverted and dropped
in the near future, like we did for those workarounds for defunct versions
of dash.

Which is why it was clearly marked as a bug and the version it affected.

FWIW there are some people still building git in macOS 10.5 (the last with
powerpc support) that was EOL in 2011.

> Perfect comment given on the day I am cutting -rc0---the fewer
> patches I need to worry about, the better ;-).

I was considering it will be worth pu (in case someone might need to pull
it), but definitely not needed in master for this release.

Carlo