Subject: Re: [PATCH v3 12/35] userdiff tests: change setup loop to individual test setup

From: Junio C Hamano <[email-masked]>
Date: Thu, 25 Feb 2021 11:06:52 -0800

Johannes Sixt <[email-masked]> writes:

> Am 25.02.21 um 03:52 schrieb Junio C Hamano:
>> Ævar Arnfjörð Bjarmason  <[email-masked]> writes:
>> 
>>> +# check each individual file
>>> +for i in $(git -C t4018 ls-files)
>>> +do
>>> +	test_expect_success "setup hunk header: $i" "
>>> +		grep -v '^t4018' \"t4018/$i\" >\"t4018/$i.content\" &&
>>> +		sed -n -e 's/^t4018 header: //p' <\"t4018/$i\" >\"t4018/$i.header\" &&
>>> +		cp \"t4018/$i.content\" \"$i\" &&
>>>  
>>>  		# add test file to the index
>>> -		git add "$i" &&
>>> +		git add \"$i\" &&
>>>  		# place modified file in the worktree
>>> -		sed -e "s/ChangeMe/IWasChanged/" <"t4018/$i.content" >"$i" || return 1
>>> -	done
>>> -'
>>> +		sed -e 's/ChangeMe/IWasChanged/' <\"t4018/$i.content\" >\"$i\"
>>> +	"
>> 
>> Please use '' around the second argument (i.e. test body) of the
>> test_expect_success, and use "" inside it.  "$i" that is used in the
>> loop is visible perfectly fine inside the test body when it is
>> eval'ed, and we won't have to count ugly backslashes that way.
>
> If we do that, then we better be sure that the implementation of
> test_expect_success does not clobber $i. Looks like we are OK at this time.

That's a good point.  Often we use more specific variable name than
$i to take advantage of the 'eval'-ed nature of the test body, and
it may probably be wise to do so here, too.