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

Jeff King [off-list ref] writes:

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

quoted

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.

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

quoted

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