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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 12, 2016, 16:58 UTC
Message-ID: <xmqqvb3mrcgj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160411173224.GE4011@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 31 quoted lines

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