Re: [PATCH v2] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

On Fri, Mar 08, 2024 at 02:36:22PM -0800, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> >> -test_expect_success 'difftool honors exit status if command not found' '
> >> -	test_config difftool.nonexistent.cmd i-dont-exist &&
> >> -	test_config difftool.trustExitCode false &&
> >> -	test_must_fail git difftool -y -t nonexistent branch
> >> -'
> >> +	test_expect_success "difftool ${opt} honors exit status if command not found" "
> >> +		test_config difftool.nonexistent.cmd i-dont-exist &&
> >> +		test_config difftool.trustExitCode false &&
> >> +		if test "${opt}" = '--dir-diff'
> >
> > The quoting doesn't quite work here.
> 
> Thanks for looking at them carefully.
> 
> In general, when you want to interpolate an variable that exists
> outside test_expect_success, you should write it this way:
> 
> 	for var in a "b c"
> 	do
> 		test_expect_success "message with $var interpolated" '
> 			command and "$var" as its argument
> 		'
> 	done
> 
> The last parameter to test_expect_{success,failure} is eval'ed, so 
> enclose it within a pair of single quotes, and let the eval to
> interpolate references to $variables at runtime (as opposed to when
> the parameters to test_expect_success are formulated) avoids a lot
> of surprises and headaches.
> 
> Perhaps we should have something like the above as a hint in
> t/README?

Makes sense. I've sent a separate patch series addressing this issue in
[1].

Thanks!

Patrick

[1]: https://lore.kernel.org/git/cover.1711028473.git.ps@pks.im/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]