patcht0300-credentials: Word around a solaris /bin/sh bug

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(-)

Wor_k_ around ...

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

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

*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

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

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?

Jeff King <peff@peff.net> writes:

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).

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

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

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

Jeff King <peff@peff.net> writes:

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.

On Fri, Feb 03, 2012 at 01:50:33PM -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).

-Peff

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

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

Jeff King <peff@peff.net> writes:

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(-)

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

Fair enough.

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

-Peff

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

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

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(-)

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(-)

Jeff King <peff@peff.net> writes:

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

But other than that, looks good.

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

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

Ben Walton wrote:

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?

Jonathan Nieder <jrnieder@gmail.com> writes:

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

    eval "$key=$value"

Matthieu Moy wrote:

[...]

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(-)