Hardcoded #!/bin/sh in t5532 causes problems on Solaris

Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Tom G. Christensen]

Hello,

Looking at the testsuite results on Solaris I see a failure in t5532.3.

Running the testsuite with -v -i revealed a shell syntax error:

proxying for example.com 9418
./proxy: syntax error at line 3: `cmd=$' unexpected
not ok 3 - fetch through proxy works
#
#               git fetch fake &&
#               echo one >expect &&
#               git log -1 --format=%s FETCH_HEAD >actual &&
#               test_cmp expect actual
#


Looking a t5532-fetch-proxy.sh the problem is obvious, it writes out a 
helper script which explicitly uses #!/bin/sh but fails to take into 
account that systems like Solaris has an ancient /bin/sh that knows 
nothing about POSIX things like $().
Replacing $() with `` was enough to make the test pass.

-tgc

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Jeff King]

On Sat, Apr 09, 2016 at 10:27:37PM +0200, Tom G. Christensen wrote:

> Looking at the testsuite results on Solaris I see a failure in t5532.3.
> 
> Running the testsuite with -v -i revealed a shell syntax error:
> 
> proxying for example.com 9418
> ./proxy: syntax error at line 3: `cmd=$' unexpected
> not ok 3 - fetch through proxy works
> #
> #               git fetch fake &&
> #               echo one >expect &&
> #               git log -1 --format=%s FETCH_HEAD >actual &&
> #               test_cmp expect actual
> #
> 
> 
> Looking a t5532-fetch-proxy.sh the problem is obvious, it writes out a
> helper script which explicitly uses #!/bin/sh but fails to take into account
> that systems like Solaris has an ancient /bin/sh that knows nothing about
> POSIX things like $().
> Replacing $() with `` was enough to make the test pass.

Right, this is a recent regression from Elia's $() series. Rather than
just revert, I think the cleanup below is the best fix.

I did some quick grepping around, and I suspect you may run
into the same thing in other places (e.g., t3404.40 looks
like a similar case). We left a lot of "#!/bin/sh" cases
unconverted to write_script because we knew what they were
doing was trivial enough not to matter. But the $()
conversion made them non-trivial.

-- >8 --
Subject: [PATCH] t5532: use write_script

The recent cleanup in b7cbbff switched t5532's use of
backticks to $(). This matches our normal shell style, which
is good. But it also breaks the test on Solaris, where
/bin/sh does not understand $().

Our normal shell style assumes a modern-ish shell which
knows about $(). However, some tests create small helper
scripts and just write "#!/bin/sh" into them. These scripts
either need to go back to using backticks, or they need to
respect $SHELL_PATH. The easiest way to do the latter is to
use write_script.

While we're at it, let's also stick the script creation
inside a test_expect block (our usual style), and split the
perl snippet into its own script (to prevent quoting
madness).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5532-fetch-proxy.sh | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/t/t5532-fetch-proxy.sh b/t/t5532-fetch-proxy.sh
index d75ef0e..51c9669 100755
--- a/t/t5532-fetch-proxy.sh
+++ b/t/t5532-fetch-proxy.sh
@@ -12,10 +12,8 @@ test_expect_success 'setup remote repo' '
 	)
 '
 
-cat >proxy <<'EOF'
-#!/bin/sh
-echo >&2 "proxying for $*"
-cmd=$("$PERL_PATH" -e '
+test_expect_success 'setup proxy script' '
+	write_script proxy-get-cmd "$PERL_PATH" <<-\EOF &&
 	read(STDIN, $buf, 4);
 	my $n = hex($buf) - 4;
 	read(STDIN, $buf, $n);
@@ -23,11 +21,16 @@ cmd=$("$PERL_PATH" -e '
 	# drop absolute-path on repo name
 	$cmd =~ s{ /}{ };
 	print $cmd;
-')
-echo >&2 "Running '$cmd'"
-exec $cmd
-EOF
-chmod +x proxy
+	EOF
+
+	write_script proxy <<-\EOF
+	echo >&2 "proxying for $*"
+	cmd=$(./proxy-get-cmd)
+	echo >&2 "Running $cmd"
+	exec $cmd
+	EOF
+'
+
 test_expect_success 'setup local repo' '
 	git remote add fake git://example.com/remote &&
 	git config core.gitproxy ./proxy
-- 
2.8.1.245.g18e0f5c

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Tom G. Christensen]

On 09/04/16 23:04, Jeff King wrote:
> I did some quick grepping around, and I suspect you may run
> into the same thing in other places (e.g., t3404.40 looks
> like a similar case).

There are only a few tests that fail and just t5532.3 seems affected by 
this issue.

> Subject: [PATCH] t5532: use write_script
>
> The recent cleanup in b7cbbff switched t5532's use of
> backticks to $(). This matches our normal shell style, which
> is good. But it also breaks the test on Solaris, where
> /bin/sh does not understand $().
>
> Our normal shell style assumes a modern-ish shell which
> knows about $(). However, some tests create small helper
> scripts and just write "#!/bin/sh" into them. These scripts
> either need to go back to using backticks, or they need to
> respect $SHELL_PATH. The easiest way to do the latter is to
> use write_script.
>
> While we're at it, let's also stick the script creation
> inside a test_expect block (our usual style), and split the
> perl snippet into its own script (to prevent quoting
> madness).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   t/t5532-fetch-proxy.sh | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
>

I applied this to 2.8.1 and as expected the test now passes on Solaris.

-tgc

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Jeff King]

On Sun, Apr 10, 2016 at 12:29:45AM +0200, Tom G. Christensen wrote:

> On 09/04/16 23:04, Jeff King wrote:
> >I did some quick grepping around, and I suspect you may run
> >into the same thing in other places (e.g., t3404.40 looks
> >like a similar case).
> 
> There are only a few tests that fail and just t5532.3 seems affected by this
> issue.

Hmm. t3404.40 does this:

        echo "#!/bin/sh" > $PRE_COMMIT &&
	echo "test -z \"\$(git diff --cached --check)\"" >>$PRE_COMMIT &&
	chmod a+x $PRE_COMMIT &&

So I'm pretty sure that $PRE_COMMIT script should be barfing each time
it is called on Solaris. I think the test itself doesn't notice because
"/bin/sh barfed" and "the pre-commit check said no" look the same from
git's perspective (both non-zero exits), and we test only cases where we
expect the hook to fail.

I think that particular test could simplify its pre-commit hook to just
"exit 1".

I didn't dig into any other cases, so that might be the only one. If
you're not seeing problems, I'm not inclined to explore each one
manually.

> I applied this to 2.8.1 and as expected the test now passes on Solaris.

Thanks.

-Peff

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmm. t3404.40 does this:
>
>         echo "#!/bin/sh" > $PRE_COMMIT &&
> 	echo "test -z \"\$(git diff --cached --check)\"" >>$PRE_COMMIT &&
> 	chmod a+x $PRE_COMMIT &&
>
> So I'm pretty sure that $PRE_COMMIT script should be barfing each time
> it is called on Solaris. I think the test itself doesn't notice because
> "/bin/sh barfed" and "the pre-commit check said no" look the same from
> git's perspective (both non-zero exits), and we test only cases where we
> expect the hook to fail.

I looked at

    $ git grep -c '#! */bin/sh' t | grep -v ':1$'

and did a few just for fun.  Doing it fully may be a good
microproject for next year ;-)

 t/t1020-subdirectory.sh       |  6 +++---
 t/t2050-git-dir-relative.sh   | 11 ++++++-----
 t/t3404-rebase-interactive.sh |  7 +++----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index 8e22b03..6dedb1c 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -142,9 +142,9 @@ test_expect_success 'GIT_PREFIX for built-ins' '
 	# Use GIT_EXTERNAL_DIFF to test that the "diff" built-in
 	# receives the GIT_PREFIX variable.
 	printf "dir/" >expect &&
-	printf "#!/bin/sh\n" >diff &&
-	printf "printf \"\$GIT_PREFIX\"" >>diff &&
-	chmod +x diff &&
+	write_script diff <<-\EOF &&
+	printf "%s" "$GIT_PREFIX"
+	EOF
 	(
 		cd dir &&
 		printf "change" >two &&
diff --git a/t/t2050-git-dir-relative.sh b/t/t2050-git-dir-relative.sh
index 21f4659..7a05b20 100755
--- a/t/t2050-git-dir-relative.sh
+++ b/t/t2050-git-dir-relative.sh
@@ -18,11 +18,12 @@ COMMIT_FILE="$(pwd)/output"
 export COMMIT_FILE
 
 test_expect_success 'Setting up post-commit hook' '
-mkdir -p .git/hooks &&
-echo >.git/hooks/post-commit "#!/bin/sh
-touch \"\${COMMIT_FILE}\"
-echo Post commit hook was called." &&
-chmod +x .git/hooks/post-commit'
+	mkdir -p .git/hooks &&
+	write_script .git/hooks/post-commit <<-\EOF
+	>"${COMMIT_FILE}"
+	echo Post commit hook was called.
+	EOF
+'
 
 test_expect_success 'post-commit hook used ordinarily' '
 echo initial >top &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index b79f442..d96d0e4 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -555,10 +555,9 @@ test_expect_success 'rebase a detached HEAD' '
 test_expect_success 'rebase a commit violating pre-commit' '
 
 	mkdir -p .git/hooks &&
-	PRE_COMMIT=.git/hooks/pre-commit &&
-	echo "#!/bin/sh" > $PRE_COMMIT &&
-	echo "test -z \"\$(git diff --cached --check)\"" >> $PRE_COMMIT &&
-	chmod a+x $PRE_COMMIT &&
+	write_script .git/hooks/pre-commit <<-\EOF &&
+	test -z "$(git diff --cached --check)"
+	EOF
 	echo "monde! " >> file1 &&
 	test_tick &&
 	test_must_fail git commit -m doesnt-verify file1 &&

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Junio C Hamano]

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

> I looked at
>
>     $ git grep -c '#! */bin/sh' t | grep -v ':1$'
>
> and did a few just for fun.  Doing it fully may be a good
> microproject for next year ;-)
>
>  t/t1020-subdirectory.sh       |  6 +++---
>  t/t2050-git-dir-relative.sh   | 11 ++++++-----
>  t/t3404-rebase-interactive.sh |  7 +++----
>  3 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
> index 8e22b03..6dedb1c 100755
> --- a/t/t1020-subdirectory.sh
> +++ b/t/t1020-subdirectory.sh
> @@ -142,9 +142,9 @@ test_expect_success 'GIT_PREFIX for built-ins' '
>  	# Use GIT_EXTERNAL_DIFF to test that the "diff" built-in
>  	# receives the GIT_PREFIX variable.
>  	printf "dir/" >expect &&
> -	printf "#!/bin/sh\n" >diff &&
> -	printf "printf \"\$GIT_PREFIX\"" >>diff &&
> -	chmod +x diff &&
> +	write_script diff <<-\EOF &&
> +	printf "%s" "$GIT_PREFIX"
> +	EOF
>  	(
>  		cd dir &&
>  		printf "change" >two &&

Regarding this one, I notice that "expect" and "actual" (produced
later in this script by executing "diff" script) are eventually
compared by test_cmp, which runs "diff" to show the actual
differences.  If we are doing this modernization to use write_script
more, we probably should make "expect" and "actual" text files that
end with a complete line.

I.e.

-- >8 --
Subject: t1020: do not overuse printf and use write_script

The test prepares a sample file "dir/two" with a single incomplete
line in it with "printf", and also prepares a small helper script
"diff" to create a file with a single incomplete line in it, again
with "printf".  The output from the latter is compared with an
expected output, again prepared with "printf" hance lacking the
final LF.  There is no reason for this test to be using files with
an incomplete line at the end, and these look more like a mistake
of not using

	printf "%s\n" "string to be written"

and using

	printf "string to be written"

Depending on what would be in $GIT_PREFIX, using the latter form
could be a bug waiting to happen.  Correct them.

Also, the test uses hardcoded #!/bin/sh to create a small helper
script.  For a small task like what the generated script does, it
does not matter too much in that what appears as /bin/sh would not
be _so_ broken, but while we are at it, use write_script instead,
which happens to make the result easier to read by reducing need
of one level of quoting.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1020-subdirectory.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index 8e22b03..df3183e 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -141,13 +141,13 @@ test_expect_success 'GIT_PREFIX for !alias' '
 test_expect_success 'GIT_PREFIX for built-ins' '
 	# Use GIT_EXTERNAL_DIFF to test that the "diff" built-in
 	# receives the GIT_PREFIX variable.
-	printf "dir/" >expect &&
-	printf "#!/bin/sh\n" >diff &&
-	printf "printf \"\$GIT_PREFIX\"" >>diff &&
-	chmod +x diff &&
+	echo "dir/" >expect &&
+	write_script diff <<-\EOF &&
+	printf "%s\n" "$GIT_PREFIX"
+	EOF
 	(
 		cd dir &&
-		printf "change" >two &&
+		echo "change" >two &&
 		GIT_EXTERNAL_DIFF=./diff git diff >../actual
 		git checkout -- two
 	) &&

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Eric Sunshine]

On Sun, Apr 10, 2016 at 3:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Subject: t1020: do not overuse printf and use write_script
>
> The test prepares a sample file "dir/two" with a single incomplete
> line in it with "printf", and also prepares a small helper script
> "diff" to create a file with a single incomplete line in it, again
> with "printf".  The output from the latter is compared with an
> expected output, again prepared with "printf" hance lacking the

s/hance/hence/

> final LF.  There is no reason for this test to be using files with
> an incomplete line at the end, and these look more like a mistake
> of not using
>
>         printf "%s\n" "string to be written"
>
> and using
>
>         printf "string to be written"
>
> Depending on what would be in $GIT_PREFIX, using the latter form
> could be a bug waiting to happen.  Correct them.
>
> Also, the test uses hardcoded #!/bin/sh to create a small helper
> script.  For a small task like what the generated script does, it
> does not matter too much in that what appears as /bin/sh would not
> be _so_ broken, but while we are at it, use write_script instead,
> which happens to make the result easier to read by reducing need
> of one level of quoting.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> with "printf".  The output from the latter is compared with an
>> expected output, again prepared with "printf" hance lacking the
>
> s/hance/hence/

Thanks

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Jeff King]

On Sun, Apr 10, 2016 at 12:01:30PM -0700, Junio C Hamano wrote:

> > diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
> > index 8e22b03..6dedb1c 100755
> > --- a/t/t1020-subdirectory.sh
> > +++ b/t/t1020-subdirectory.sh
> > @@ -142,9 +142,9 @@ test_expect_success 'GIT_PREFIX for built-ins' '
> >  	# Use GIT_EXTERNAL_DIFF to test that the "diff" built-in
> >  	# receives the GIT_PREFIX variable.
> >  	printf "dir/" >expect &&
> > -	printf "#!/bin/sh\n" >diff &&
> > -	printf "printf \"\$GIT_PREFIX\"" >>diff &&
> > -	chmod +x diff &&
> > +	write_script diff <<-\EOF &&
> > +	printf "%s" "$GIT_PREFIX"
> > +	EOF
> >  	(
> >  		cd dir &&
> >  		printf "change" >two &&
> 
> Regarding this one, I notice that "expect" and "actual" (produced
> later in this script by executing "diff" script) are eventually
> compared by test_cmp, which runs "diff" to show the actual
> differences.  If we are doing this modernization to use write_script
> more, we probably should make "expect" and "actual" text files that
> end with a complete line.

Yeah I wondered about that. And also the fact that the shell script
itself doesn't end in newline. But I think that is just an accident, and
no shell happened to complain (not that I would expect them to, but we
come across enough weirdness around final newlines with tools like sed
and tr, I wouldn't have been surprised).

> -- >8 --
> Subject: t1020: do not overuse printf and use write_script
> 
> The test prepares a sample file "dir/two" with a single incomplete
> line in it with "printf", and also prepares a small helper script
> "diff" to create a file with a single incomplete line in it, again
> with "printf".  The output from the latter is compared with an
> expected output, again prepared with "printf" hance lacking the
> final LF.  There is no reason for this test to be using files with
> an incomplete line at the end, and these look more like a mistake
> of not using
> 
> 	printf "%s\n" "string to be written"
> 
> and using
> 
> 	printf "string to be written"
> 
> Depending on what would be in $GIT_PREFIX, using the latter form
> could be a bug waiting to happen.  Correct them.
> 
> Also, the test uses hardcoded #!/bin/sh to create a small helper
> script.  For a small task like what the generated script does, it
> does not matter too much in that what appears as /bin/sh would not
> be _so_ broken, but while we are at it, use write_script instead,
> which happens to make the result easier to read by reducing need
> of one level of quoting.

Looks good to me. I suspect you could actually just use:

  echo "$GIT_PREFIX"

in the helper script. That is also not completely safe against arbitrary
bytes in $GIT_PREFIX (due to unportable backslash escapes), though I
suspect it would be fine for the purposes of the test script. Using a
proper printf isn't that many more bytes, though.

-Peff

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Jeff King]

On Sat, Apr 09, 2016 at 05:37:43PM -0700, Junio C Hamano wrote:

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index b79f442..d96d0e4 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -555,10 +555,9 @@ test_expect_success 'rebase a detached HEAD' '
>  test_expect_success 'rebase a commit violating pre-commit' '
>  
>  	mkdir -p .git/hooks &&
> -	PRE_COMMIT=.git/hooks/pre-commit &&
> -	echo "#!/bin/sh" > $PRE_COMMIT &&
> -	echo "test -z \"\$(git diff --cached --check)\"" >> $PRE_COMMIT &&
> -	chmod a+x $PRE_COMMIT &&
> +	write_script .git/hooks/pre-commit <<-\EOF &&
> +	test -z "$(git diff --cached --check)"
> +	EOF

Looks good and is the minimal change. I kind of wonder if the example
would be more clear, though, as just:

  write_script .git/hooks/pre-commit <<-\EOF &&
  exit 1
  EOF
  echo whatever >file1 &&
  ...

I don't think we ever actually need the pre-commit check to pass, as we
simply override it with --no-verify. But I dunno. Maybe people find it
easier to read with a pseudo-realistic example (it took me a minute to
realize the trailing whitespace in the content was important).

It could also stand to clean up its hook with test_when_finished. The
next test resorts to "rm -rf" on the hooks directory at the beginning.
Yuck.

-Peff

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Apr 09, 2016 at 05:37:43PM -0700, Junio C Hamano wrote:
>
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index b79f442..d96d0e4 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -555,10 +555,9 @@ test_expect_success 'rebase a detached HEAD' '
>>  test_expect_success 'rebase a commit violating pre-commit' '
>>  
>>  	mkdir -p .git/hooks &&
>> -	PRE_COMMIT=.git/hooks/pre-commit &&
>> -	echo "#!/bin/sh" > $PRE_COMMIT &&
>> -	echo "test -z \"\$(git diff --cached --check)\"" >> $PRE_COMMIT &&
>> -	chmod a+x $PRE_COMMIT &&
>> +	write_script .git/hooks/pre-commit <<-\EOF &&
>> +	test -z "$(git diff --cached --check)"
>> +	EOF
>
> Looks good and is the minimal change. I kind of wonder if the example
> would be more clear, though, as just:
>
>   write_script .git/hooks/pre-commit <<-\EOF &&
>   exit 1
>   EOF
>   echo whatever >file1 &&
>   ...
>
> I don't think we ever actually need the pre-commit check to pass, as we
> simply override it with --no-verify. But I dunno. Maybe people find it
> easier to read with a pseudo-realistic example (it took me a minute to
> realize the trailing whitespace in the content was important).

I was mostly worried about closing the door for future enhancement
where there are multiple commits to be replayed, some of which fail
and others pass the test.  Unconditional "exit 1" would have to be
reverted when it happens.

> It could also stand to clean up its hook with test_when_finished. The
> next test resorts to "rm -rf" on the hooks directory at the beginning.
> Yuck.

Yeah, that may be an accident waiting to happen.

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
> ...
>> Looks good and is the minimal change. I kind of wonder if the example
>> would be more clear, though, as just:
>>
>>   write_script .git/hooks/pre-commit <<-\EOF &&
>>   exit 1
>>   EOF
>>   echo whatever >file1 &&
>>   ...
>>
>> I don't think we ever actually need the pre-commit check to pass, as we
>> simply override it with --no-verify. But I dunno. Maybe people find it
>> easier to read with a pseudo-realistic example (it took me a minute to
>> realize the trailing whitespace in the content was important).
>
> I was mostly worried about closing the door for future enhancement
> where there are multiple commits to be replayed, some of which fail
> and others pass the test.  Unconditional "exit 1" would have to be
> reverted when it happens.
>
>> It could also stand to clean up its hook with test_when_finished. The
>> next test resorts to "rm -rf" on the hooks directory at the beginning.
>> Yuck.
>
> Yeah, that may be an accident waiting to happen.

In any case, lest we forget...

-- >8 --
Subject: [PATCH] t3404: use write_script

The test uses hardcoded #!/bin/sh to create a pre-commit hook
script.  Because the generated script uses $(command substitution),
which is not supported by /bin/sh on some platforms (e.g. Solaris),
the resulting pre-commit always fails.

Which is not noticeable as the test that uses the hook is about
checking the behaviour of the command when the hook fails ;-), but
nevertheless it is not testing what we wanted to test.

Use write_script so that the resulting script is run under the same
shell our scripted Porcelain commands are run, which must support
the necessary $(construct).

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3404-rebase-interactive.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 544f9ad..d6d65a3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -555,10 +555,9 @@ test_expect_success 'rebase a detached HEAD' '
 test_expect_success 'rebase a commit violating pre-commit' '
 
 	mkdir -p .git/hooks &&
-	PRE_COMMIT=.git/hooks/pre-commit &&
-	echo "#!/bin/sh" > $PRE_COMMIT &&
-	echo "test -z \"\$(git diff --cached --check)\"" >> $PRE_COMMIT &&
-	chmod a+x $PRE_COMMIT &&
+	write_script .git/hooks/pre-commit <<-\EOF &&
+	test -z "$(git diff --cached --check)"
+	EOF
 	echo "monde! " >> file1 &&
 	test_tick &&
 	test_must_fail git commit -m doesnt-verify file1 &&
-- 
2.8.1-339-gc925d85

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Jeff King]

On Tue, Apr 12, 2016 at 10:12:39AM -0700, Junio C Hamano wrote:

> In any case, lest we forget...
> 
> -- >8 --
> Subject: [PATCH] t3404: use write_script

Yep, looks good. Thanks.

-Peff

Re: Hardcoded #!/bin/sh in t5532 causes problems on Solaris

[Jeff King]

On Tue, Apr 12, 2016 at 09:58:20AM -0700, Junio C Hamano wrote:

> > Looks good and is the minimal change. I kind of wonder if the example
> > would be more clear, though, as just:
> >
> >   write_script .git/hooks/pre-commit <<-\EOF &&
> >   exit 1
> >   EOF
> >   echo whatever >file1 &&
> >   ...
> >
> > I don't think we ever actually need the pre-commit check to pass, as we
> > simply override it with --no-verify. But I dunno. Maybe people find it
> > easier to read with a pseudo-realistic example (it took me a minute to
> > realize the trailing whitespace in the content was important).
> 
> I was mostly worried about closing the door for future enhancement
> where there are multiple commits to be replayed, some of which fail
> and others pass the test.  Unconditional "exit 1" would have to be
> reverted when it happens.

Yeah, that's fair. It is at least trying to re-create a real-world
situation.

-Peff