[PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Ben Walton]

Solaris' /bin/sh was making the IFS setting permanent instead of
temporary when using it to slurp in credentials in the generated
'dump' script of the 'setup helper scripts' test in t0300-credentials.

The stderr file that was being compared to expected-stderr contained the
following stray line from the credential helper run:

warning: invalid credential line: username foo

To avoid this bug, capture the original IFS and force it to be reset
after its use is no longer required.  For now, this is lighter weight
than altering which shell these scripts use as their shebang.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 t/t0300-credentials.sh |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 885af8f..1be3fe2 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -8,10 +8,12 @@ test_expect_success 'setup helper scripts' '
 	cat >dump <<-\EOF &&
 	whoami=`echo $0 | sed s/.*git-credential-//`
 	echo >&2 "$whoami: $*"
+	OIFS=$IFS
 	while IFS== read key value; do
 		echo >&2 "$whoami: $key=$value"
 		eval "$key=$value"
 	done
+	IFS=$OIFS
 	EOF
 
 	cat >git-credential-useless <<-\EOF &&
-- 
1.7.8.3

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Frans Klaver]

Wor_k_ around ...


On Thu, 02 Feb 2012 20:32:15 +0100, Ben Walton  
<bwalton@artsci.utoronto.ca> wrote:

> Solaris' /bin/sh was making the IFS setting permanent instead of
> temporary when using it to slurp in credentials in the generated
> 'dump' script of the 'setup helper scripts' test in t0300-credentials.
>
> The stderr file that was being compared to expected-stderr contained the
> following stray line from the credential helper run:
>
> warning: invalid credential line: username foo
>
> To avoid this bug, capture the original IFS and force it to be reset
> after its use is no longer required.  For now, this is lighter weight
> than altering which shell these scripts use as their shebang.
>
> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
> ---
>  t/t0300-credentials.sh |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> index 885af8f..1be3fe2 100755
> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -8,10 +8,12 @@ test_expect_success 'setup helper scripts' '
>  	cat >dump <<-\EOF &&
>  	whoami=`echo $0 | sed s/.*git-credential-//`
>  	echo >&2 "$whoami: $*"
> +	OIFS=$IFS
>  	while IFS== read key value; do
>  		echo >&2 "$whoami: $key=$value"
>  		eval "$key=$value"
>  	done
> +	IFS=$OIFS
>  	EOF
> 	cat >git-credential-useless <<-\EOF &&


-- 
Using Opera's revolutionary e-mail client: http://www.opera.com/mail/

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Ben Walton]

Excerpts from Frans Klaver's message of Thu Feb 02 14:44:08 -0500 2012:

> Wor_k_ around ...

*face*palm*

Thanks for catching that.

Junio, can you make that tweak if the patch is ok or would you prefer
a new mail?

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Jeff King]

On Thu, Feb 02, 2012 at 02:32:15PM -0500, Ben Walton wrote:

> Solaris' /bin/sh was making the IFS setting permanent instead of
> temporary when using it to slurp in credentials in the generated
> 'dump' script of the 'setup helper scripts' test in t0300-credentials.

Hmm. Presumably you are setting SHELL_PATH, as Solaris /bin/sh would be
useless for running the rest of the tests. Usually scripts inside the
tests use #!$SHELL_PATH, but I often don't bother if it's a simple "even
Solaris /bin/sh could run this" script. But in this case I either
underestimated the complexity of my script or overestimated the quality
of the Solaris /bin/sh.

I wonder if a better solution is to use a known-good shell instead of
trying to work around problems in a bogus shell. Does the patch below
fix it for you?

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 885af8f..edf6547 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -14,15 +14,15 @@ test_expect_success 'setup helper scripts' '
 	done
 	EOF
 
-	cat >git-credential-useless <<-\EOF &&
-	#!/bin/sh
+	cat >git-credential-useless <<-EOF &&
+	#!$SHELL_PATH
 	. ./dump
 	exit 0
 	EOF
 	chmod +x git-credential-useless &&
 
-	cat >git-credential-verbatim <<-\EOF &&
-	#!/bin/sh
+	echo "#!$SHELL_PATH" >git-credential-verbatim &&
+	cat >>git-credential-verbatim <<-\EOF &&
 	user=$1; shift
 	pass=$1; shift
 	. ./dump

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I wonder if a better solution is to use a known-good shell instead of
> trying to work around problems in a bogus shell.

Yeah, I think that is a better approach.

What prevents us from doing 's|^#! */bin/sh|$#$SHELL_PATH|' on everything
in t/ directory (I am not suggesting to do this. I just want to know if
there is a reason we want hardcoded "#!/bin/sh" for some instances).

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Jeff King]

On Thu, Feb 02, 2012 at 05:02:17PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I wonder if a better solution is to use a known-good shell instead of
> > trying to work around problems in a bogus shell.
> 
> Yeah, I think that is a better approach.
> 
> What prevents us from doing 's|^#! */bin/sh|$#$SHELL_PATH|' on everything
> in t/ directory (I am not suggesting to do this. I just want to know if
> there is a reason we want hardcoded "#!/bin/sh" for some instances).

The quoting is more annoying, because you usually don't want
interpolation on the rest of the lines of your embedded script. So:

  cat >foo.sh <<\EOF
  #!/bin/sh
  echo my arguments are "$@"
  EOF

cannot have the mechanical replace you mentioned above. It would need:

  cat >foo.sh <<EOF
  #!$SHELL_PATH
  echo my arguments are "\$@"
  EOF

or:

  {
    echo "#!$SHELL_PATH" &&
    cat <<EOF
    echo my arguments are "$@"
    EOF
  } >foo.sh

When I have hard-coded "#!/bin/sh", my thinking is usually "this is less
cumbersome to type and to read, and this script-let is so small that
even Solaris will get it right".

-Peff

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Ben Walton]

Excerpts from Jeff King's message of Fri Feb 03 07:06:57 -0500 2012:

> When I have hard-coded "#!/bin/sh", my thinking is usually "this is
> less cumbersome to type and to read, and this script-let is so small
> that even Solaris will get it right".

This is why I opted to stick with /bin/sh and just avoid the damage.
Overall, using a sane shell is a better option...It is harder to read
though.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   cat >foo.sh <<\EOF
>   #!/bin/sh
>   echo my arguments are "$@"
>   EOF
>
> cannot have the mechanical replace you mentioned above. It would need:
>
>   cat >foo.sh <<EOF
>   #!$SHELL_PATH
>   echo my arguments are "\$@"
>   EOF
>
> or:
>
>   {
>     echo "#!$SHELL_PATH" &&
>     cat <<EOF
>     echo my arguments are "$@"
>     EOF
>   } >foo.sh
>
> When I have hard-coded "#!/bin/sh", my thinking is usually "this is less
> cumbersome to type and to read, and this script-let is so small that
> even Solaris will get it right".

I am toying with the pros-and-cons of

	write_script () {
		echo "#!$1"
		shift
                cat
	}

so that the above can become

	write_script "$SHELL_PATH" >foo.sh <<-EOF
        echo my arguments are "\$@"
	EOF

without requiring the brain-cycle to waste on the "Is this simple enough
for even Solaris to grok?" guess game.  This should also be reusable for
other stuff like $PERL_PATH, I would think.

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Jeff King]

On Fri, Feb 03, 2012 at 12:32:15PM -0800, Junio C Hamano wrote:

> I am toying with the pros-and-cons of
> 
> 	write_script () {
> 		echo "#!$1"
> 		shift
>                 cat
> 	}
> 
> so that the above can become
> 
> 	write_script "$SHELL_PATH" >foo.sh <<-EOF
>         echo my arguments are "\$@"
> 	EOF
> 
> without requiring the brain-cycle to waste on the "Is this simple enough
> for even Solaris to grok?" guess game.  This should also be reusable for
> other stuff like $PERL_PATH, I would think.

I like it. Even better would be:

  write_script() {
        echo "#!$2" >"$1" &&
        cat >>"$1" &&
        chmod +x "$1"
  }

  write_script foo.sh "$SHELL_PATH" <<-\EOF
    echo my arguments are "$@"
  EOF

-Peff

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> without requiring the brain-cycle to waste on the "Is this simple enough
>> for even Solaris to grok?" guess game.  This should also be reusable for
>> other stuff like $PERL_PATH, I would think.
>
> I like it. Even better would be:
>
>   write_script() {
>         echo "#!$2" >"$1" &&
>         cat >>"$1" &&
>         chmod +x "$1"
>   }
>
>   write_script foo.sh "$SHELL_PATH" <<-\EOF
>     echo my arguments are "$@"
>   EOF

I first thought that the order of parameters were unusual, but with that
order, you could even go something fancier like:

	write_script () {
		case "$#" in
		1)	case "$1" in
			*.perl | *.pl) echo "#!$PERL_PATH" ;;
			*) echo "#!$SHELL_PATH" ;;
			esac
                2)	echo "#!$2" ;;
		*)	BUG ;;
                esac >"$1" &&
                cat >>"$1" &&
                chmod +x "$1"
	}

	write_script foo.sh
        write_script bar.perl
        write_script pre-receive /no/frobnication/today

The tongue-in-cheek comment aside, I think ${2-"$SHELL_PATH"} or some form
of fallback would be a good idea in any case, as 99% of the time what we
write in the test scripts is a shell script.

Also "chmod +x" is a very good idea.

        

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Jeff King]

On Fri, Feb 03, 2012 at 01:50:33PM -0800, Junio C Hamano wrote:

> >   write_script foo.sh "$SHELL_PATH" <<-\EOF
> >     echo my arguments are "$@"
> >   EOF
> 
> I first thought that the order of parameters were unusual, but with that
> order, you could even go something fancier like:
> 
> 	write_script () {
> 		case "$#" in
> 		1)	case "$1" in
> 			*.perl | *.pl) echo "#!$PERL_PATH" ;;
> 			*) echo "#!$SHELL_PATH" ;;
> 			esac
>                 2)	echo "#!$2" ;;
> 		*)	BUG ;;
>                 esac >"$1" &&
>                 cat >>"$1" &&
>                 chmod +x "$1"
> 	}
> 

Nice. I was going to suggest a wrapper like "write_sh_script" so you
didn't have to spell out $SHELL_PATH, but I think the auto-detection
makes sense (and falling back to shell makes even more sense, as that
covers 99% of the cases anyway).

-Peff

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Ben Walton]

Excerpts from Jeff King's message of Fri Feb 03 16:55:07 -0500 2012:

> >     write_script () {
> >         case "$#" in
> >         1)    case "$1" in
> >             *.perl | *.pl) echo "#!$PERL_PATH" ;;
> >             *) echo "#!$SHELL_PATH" ;;
> >             esac
> >                 2)    echo "#!$2" ;;
> >         *)    BUG ;;
> >                 esac >"$1" &&
> >                 cat >>"$1" &&
> >                 chmod +x "$1"
> >     }
> > 
> 
> Nice. I was going to suggest a wrapper like "write_sh_script" so you
> didn't have to spell out $SHELL_PATH, but I think the auto-detection
> makes sense (and falling back to shell makes even more sense, as that
> covers 99% of the cases anyway).

This looks like a very nice, general purpose, solution to the problem.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>                 2)	echo "#!$2" ;;
>> 		*)	BUG ;;
>>                 esac >"$1" &&
>>                 cat >>"$1" &&
>>                 chmod +x "$1"
>> 	}
>> 
>
> Nice. I was going to suggest a wrapper like "write_sh_script" so you
> didn't have to spell out $SHELL_PATH, but I think the auto-detection
> makes sense (and falling back to shell makes even more sense, as that
> covers 99% of the cases anyway).

Let's not over-engineer this and stick to the simple-stupid-sufficient.

Something like this?

 t/test-lib.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..1b9c461 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -379,6 +379,15 @@ test_config () {
 	git config "$@"
 }
 
+# Prepare a script to be used in the test
+write_script () {
+	{
+		echo "#!${2-"$SHELL_PATH"}"
+		cat
+	} >"$1" &&
+	chmod +x "$1"
+}
+
 # Use test_set_prereq to tell that a particular prerequisite is available.
 # The prerequisite can later be checked for in two ways:
 #

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Jeff King]

On Fri, Feb 03, 2012 at 02:45:25PM -0800, Junio C Hamano wrote:

> Let's not over-engineer this and stick to the simple-stupid-sufficient.

Fair enough.

> Something like this?
> [...]
> +# Prepare a script to be used in the test
> +write_script () {
> +	{
> +		echo "#!${2-"$SHELL_PATH"}"
> +		cat
> +	} >"$1" &&
> +	chmod +x "$1"
> +}

Looks good to me (it probably doesn't matter, but you may want to
connect the echo and cat via &&).

-Peff

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Jeff King]

On Fri, Feb 03, 2012 at 02:45:25PM -0800, Junio C Hamano wrote:

> > Nice. I was going to suggest a wrapper like "write_sh_script" so you
> > didn't have to spell out $SHELL_PATH, but I think the auto-detection
> > makes sense (and falling back to shell makes even more sense, as that
> > covers 99% of the cases anyway).
> 
> Let's not over-engineer this and stick to the simple-stupid-sufficient.
> 
> Something like this?

Here it is as patches with commit messages.  I don't think it's worth
doing a mechanical conversion of the whole test suite to write_script.

  [1/2]: tests: add write_script helper function
  [2/2]: t0300: use write_script helper

-Peff

[PATCH 1/2] tests: add write_script helper function

[Jeff King]

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

Many of the scripts in the test suite write small helper
shell scripts to disk. It's best if these shell scripts
start with "#!$SHELL_PATH" rather than "#!/bin/sh", because
/bin/sh on some platforms is too buggy to be used.

However, it can be cumbersome to expand $SHELL_PATH, because
the usual recipe for writing a script is:

	cat >foo.sh <<-\EOF
	#!/bin/sh
	echo my arguments are "$@"
	EOF

To expand $SHELL_PATH, you have to either interpolate the
here-doc (which would require quoting "\$@"), or split the
creation into two commands (interpolating the $SHELL_PATH
line, but not the rest of the script). Let's provide a
helper function that makes that less syntactically painful.

While we're at it, this helper can also take care of the
"chmod +x" that typically comes after the creation of such a
script, saving the caller a line.

Signed-off-by: Jeff King <peff@peff.net>
---
I suspect you already have this in your repo, but maybe the commit
message is useful.

 t/test-lib.sh |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index b22bee7..254849e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -400,6 +400,14 @@ test_config_global () {
 	git config --global "$@"
 }
 
+write_script () {
+	{
+		echo "#!${2-"$SHELL_PATH"}" &&
+		cat
+	} >"$1" &&
+	chmod +x "$1"
+}
+
 # Use test_set_prereq to tell that a particular prerequisite is available.
 # The prerequisite can later be checked for in two ways:
 #
-- 
1.7.9.rc1.28.gf4be5

[PATCH 2/2] t0300: use write_script helper

[Jeff King]

t0300 creates some helper shell scripts, and marks them with
"!/bin/sh". Even though the scripts are fairly simple, they
can fail on broken shells (specifically, Solaris /bin/sh
will persist a temporary assignment to IFS in a "read"
command).

Rather than work around the problem for Solaris /bin/sh,
using write_script will make sure we point to a known-good
shell that the user has given us.

Signed-off-by: Jeff King <peff@peff.net>
---
This works fine on my Linux box, but just to sanity check that I didn't
screw anything up in the whopping 5 lines of changes, can you confirm
this fixes the issue for you, Ben?

 t/t0300-credentials.sh |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 885af8f..0b46248 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -14,14 +14,13 @@ test_expect_success 'setup helper scripts' '
 	done
 	EOF
 
-	cat >git-credential-useless <<-\EOF &&
+	write_script git-credential-useless <<-\EOF &&
 	#!/bin/sh
 	. ./dump
 	exit 0
 	EOF
-	chmod +x git-credential-useless &&
 
-	cat >git-credential-verbatim <<-\EOF &&
+	write_script git-credential-verbatim <<-\EOF &&
 	#!/bin/sh
 	user=$1; shift
 	pass=$1; shift
@@ -29,7 +28,6 @@ test_expect_success 'setup helper scripts' '
 	test -z "$user" || echo username=$user
 	test -z "$pass" || echo password=$pass
 	EOF
-	chmod +x git-credential-verbatim &&
 
 	PATH="$PWD:$PATH"
 '
-- 
1.7.9.rc1.28.gf4be5

Re: [PATCH 2/2] t0300: use write_script helper

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> t0300 creates some helper shell scripts, and marks them with
> "!/bin/sh". Even though the scripts are fairly simple, they
> can fail on broken shells (specifically, Solaris /bin/sh
> will persist a temporary assignment to IFS in a "read"
> command).
>
> Rather than work around the problem for Solaris /bin/sh,
> using write_script will make sure we point to a known-good
> shell that the user has given us.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This works fine on my Linux box, but just to sanity check that I didn't
> screw anything up in the whopping 5 lines of changes, can you confirm
> this fixes the issue for you, Ben?
>
>  t/t0300-credentials.sh |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> index 885af8f..0b46248 100755
> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -14,14 +14,13 @@ test_expect_success 'setup helper scripts' '
>  	done
>  	EOF
>  
> -	cat >git-credential-useless <<-\EOF &&
> +	write_script git-credential-useless <<-\EOF &&
>  	#!/bin/sh

An innocuous facepalm I'd be glad to remove myself ;-)

>  	. ./dump
>  	exit 0
>  	EOF
> -	chmod +x git-credential-useless &&
>  
> -	cat >git-credential-verbatim <<-\EOF &&
> +	write_script git-credential-verbatim <<-\EOF &&
>  	#!/bin/sh

But other than that, looks good.

>  	user=$1; shift
>  	pass=$1; shift
> @@ -29,7 +28,6 @@ test_expect_success 'setup helper scripts' '
>  	test -z "$user" || echo username=$user
>  	test -z "$pass" || echo password=$pass
>  	EOF
> -	chmod +x git-credential-verbatim &&
>  
>  	PATH="$PWD:$PATH"
>  '

Re: [PATCH 2/2] t0300: use write_script helper

[Jeff King]

On Fri, Feb 03, 2012 at 10:58:06PM -0800, Junio C Hamano wrote:

> > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> > index 885af8f..0b46248 100755
> > --- a/t/t0300-credentials.sh
> > +++ b/t/t0300-credentials.sh
> > @@ -14,14 +14,13 @@ test_expect_success 'setup helper scripts' '
> >  	done
> >  	EOF
> >  
> > -	cat >git-credential-useless <<-\EOF &&
> > +	write_script git-credential-useless <<-\EOF &&
> >  	#!/bin/sh
> 
> An innocuous facepalm I'd be glad to remove myself ;-)

Heh, it took me a second to notice it, even after you mentioned it. And
it's even right there in the context. At least the line written by
write_script takes precedence. :)

Thanks.

-Peff

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Jonathan Nieder]

Ben Walton wrote:

> --- a/t/t0300-credentials.sh
> +++ b/t/t0300-credentials.sh
> @@ -8,10 +8,12 @@ test_expect_success 'setup helper scripts' '
>  	cat >dump <<-\EOF &&
>  	whoami=`echo $0 | sed s/.*git-credential-//`
>  	echo >&2 "$whoami: $*"
> +	OIFS=$IFS
>  	while IFS== read key value; do
>  		echo >&2 "$whoami: $key=$value"
>  		eval "$key=$value"
>  	done
> +	IFS=$OIFS

Oh, good catch.  Technically "read" is not a special builtin so POSIX shells
are not supposed to do this (and Jeff's patch definitely looks right), but in
any case temporary variable settings while running a builtin are close
enough to the assignment-during-special-builtin-or-function case to
make me shiver a little. ;-)

Would something like

	(
		IFS==
		while read key value
		do
			...
		done
	)

make sense?

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Would something like
>
> 	(
> 		IFS==
> 		while read key value
> 		do
> 			...
> 		done
> 	)
>
> make sense?

I don't think so since the "..." contains

    eval "$key=$value"

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] t0300-credentials: Word around a solaris /bin/sh bug

[Jonathan Nieder]

Matthieu Moy wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> 	(
>> 		IFS==
>> 		while read key value
>> 		do
>> 			...
>> 		done
>> 	)
[...]
> I don't think so since the "..." contains
>
>     eval "$key=$value"

Oh, whoops.  Thanks for noticing.

Here's an updated patch, for amusement value.  No functional change
intended.  I don't think it's actually worth applying unless people
actively working on this file find the result easier to work with.

-- >8 --
Subject: t0300 (credentials): shell scripting style cleanups

As Ben noticed, the helper used by this test script assigns a
temporary value to IFS while calling the "read" builtin, which in
ancient shells causes the value to leak into the environment and
affect later code in the same script.  Explicitly save and restore IFS
to avoid rekindling old memories.

While at it, put the "do" associated to a "while" statement on its own
line to match the house style and define helper scripts in the test
data section above all test assertions so the "setup" test itself is
less cluttered and we can worry a little less about quoting issues.

Inspired-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t0300-credentials.sh |   36 ++++++++++++++++++++----------------
 1 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index edf65478..780d5dcb 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -4,33 +4,37 @@ test_description='basic credential helper tests'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-test_expect_success 'setup helper scripts' '
-	cat >dump <<-\EOF &&
+cat >dump <<-\EOF
 	whoami=`echo $0 | sed s/.*git-credential-//`
 	echo >&2 "$whoami: $*"
-	while IFS== read key value; do
+	save_IFS=$IFS
+	IFS==
+	while read key value
+	do
 		echo >&2 "$whoami: $key=$value"
 		eval "$key=$value"
 	done
-	EOF
+	IFS=$save_IFS
+EOF
 
-	cat >git-credential-useless <<-EOF &&
+cat >git-credential-useless <<-EOF
 	#!$SHELL_PATH
 	. ./dump
 	exit 0
-	EOF
+EOF
+
+cat >git-credential-verbatim <<-EOF
+	#!$SHELL_PATH
+	user=\$1; shift
+	pass=\$1; shift
+	. ./dump
+	test -z "\$user" || echo username=\$user
+	test -z "\$pass" || echo password=\$pass
+EOF
+
+test_expect_success setup '
 	chmod +x git-credential-useless &&
-
-	echo "#!$SHELL_PATH" >git-credential-verbatim &&
-	cat >>git-credential-verbatim <<-\EOF &&
-	user=$1; shift
-	pass=$1; shift
-	. ./dump
-	test -z "$user" || echo username=$user
-	test -z "$pass" || echo password=$pass
-	EOF
 	chmod +x git-credential-verbatim &&
-
 	PATH="$PWD:$PATH"
 '
 
-- 
1.7.9