From: Frans Klaver <fransklaver@gmail.com>
Date: Wed, 25 Jan 2012 06:47:37 GMT
Subject: Re: [PATCH 2/5] t0061: Add tests
Message-ID: <op.v8mntno90aolir@keputer>
In-Reply-To: <20120124225636.GF8222@burratino>

On Tue, 24 Jan 2012 23:56:36 +0100, Jonathan Nieder <jrnieder@gmail.com>  
wrote:

>> +>empty
>> +
>> +cat >incorrect-interpreter-script <<-EOF
>> +#!someinterpreter
>> +cat hello-script
>> +EOF
>> +>empty
>
> What is the point of repreatedly writing an empty file named "empty"?

There isn't. I copied it along with the hello-script lines and didn't pay  
it much heed. I'll remove the excessive '>empty's.


> I think this would be easier to read and maintain if scripts not
> shared between multiple tests were written in the body of the relevant
> tests.  For example, that way it is easier to remember to remove a
> helper script if the relevant test assertion changes to no longer need
> it.

Makes sense. I'll reorder.


>
> [...]
>> @@ -26,7 +44,7 @@ test_expect_success 'run_command can run a command' '
>>  	test_cmp empty err
>>  '
>>
>>  test_expect_success POSIXPERM 'run_command reports EACCES' '
>>  	cat hello-script >hello.sh &&
>>  	chmod -x hello.sh &&
>>  	test_must_fail test-run-command run-command ./hello.sh 2>err &&
> [...]
>> +test_expect_success POSIXPERM 'run_command reports EACCES, search path  
>> perms' '
>> +	mkdir -p inaccessible &&
>> +	PATH=$(pwd)/inaccessible:$PATH &&
>> +	export PATH &&
>> +
>> +	cat hello-script >inaccessible/hello.sh &&
>> +	chmod 400 inaccessible &&
>> +	test_must_fail test-run-command run-command hello.sh 2>err &&
>> +	chmod 755 inaccessible &&
>> +
>> +	grep "fatal: cannot exec.*hello.sh" err
>> +'
>
> (*) These tests would be easier to understand if squashed with the
> relevant later patch in the series that changes the error message.

You mean "Elaborate execvp error checking"?


> Maybe they could be less repetitive that way, too.
>
> 	test_expect_success POSIXPERM 'diagnose command in inaccessible part of  
> $PATH' '
> 		mkdir -p subdir &&
> 		cat hello-script >subdir/hello.sh &&
> 		chmod +x subdir/hello.sh &&
> 		chmod -x subdir &&
> 		(
> 			PATH=$(pwd)/inaccessible:$PATH &&
> 			test_must_fail test-run-command run-command hello.sh 2>err
> 		) &&
> 		test_i18ngrep ...
> 	'
>
> [...]
>> +test_expect_success POSIXPERM 'run_command reports EACCES, interpreter  
>> fails' '
>> +	cat incorrect-interpreter-script >hello.sh &&
>> +	chmod +x hello.sh &&
>> +	chmod -x someinterpreter &&
>> +	test_must_fail test-run-command run-command ./hello.sh 2>err &&
>> +
>> +	grep "fatal: cannot exec.*hello.sh" err
>> +'
>
> Is this the common case?  Why would my interpreter be in the designated
> spot but not marked executable?  Is there some other motivating
> example?  (I'm genuinely curious; it's ok if the answer is "no".)

I wouldn't think so. This particular one is addressing a concern raised by  
Johannes Sixt in reaction to a patch from Junio.

http://article.gmane.org/gmane.comp.version-control.git/171848


>
> [...]
>> +
>> +test_expect_failure POSIXPERM 'run_command reports ENOENT,  
>> interpreter' '
>> +	cat non-existing-interpreter >hello.sh &&
>> +	chmod +x hello.sh &&
>> +	test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err  
>> &&
>> +
>> +	grep "error: cannot exec.*hello.sh" err
>> +'
>
> Maybe:
>
> 	test_expect_success POSIXPERM 'diagnose missing interpreter' '
> 		echo "#!/nonexistent/interpreter" >hello.sh &&
> 		chmod +x hello.sh &&
> 		test_must_fail test-run-command run-command hello.sh 2>err &&
> 		test_i18ngrep ...
> 	'

Will check.


> Hope that helps,
> Jonathan