Re: [RTC/PATCH] Add 'update-branch' hook

From: Ilya Bobyr <ilya.bobyr@gmail.com>
Date: Apr 22, 2014, 06:05 UTC
Message-ID: <535606A3.8040704@gmail.com>
In-Reply-To: <53559a8333aaa_6c39e772f07f@nysa.notmuch>

On 4/21/2014 3:24 PM, Felipe Contreras wrote:

Show 20 quoted lines

> Ilya Bobyr wrote:
>> On Mon, Apr 21, 2014 at 2:35 PM, Felipe Contreras <
>> felipe.contreras@gmail.com> wrote:
>>> Ilya Bobyr wrote:
>>>> test_expect_success 'setup' "
>>>>       mkdir -p .git/hooks &&
>>>>       cat > .git/hooks/update-branch <<-\\EOF &&
>>>>       #!/bin/sh
>>>>       echo \$@ > .git/update-branch.args
>>>>       EOF
>>>>       chmod +x .git/hooks/update-branch &&
>>>>       echo one > content &&
>>>>       git add content &&
>>>>       git commit -a -m one
>>>> "
>>> That is not maintainable at all.
>> Maybe you could explain how is this less maintainable, compared to a separate
>> function?
> Do I really have to explain that manually escaping a shell script is not
> maintainable?

This is rude.

Here is how you can do it without escaping:

test_expect_success 'setup' '
	mkdir -p .git/hooks &&
	cat > .git/hooks/update-branch <<-\EOF &&
	#!/bin/sh
	echo $@ > .git/update-branch.args
	EOF
	chmod +x .git/hooks/update-branch &&
	echo one > content &&
	git add content &&
	git commit -a -m one
'

It is not different from most of the tests, I think.

Show 6 quoted lines

>> This is how it is suggested by t/README and how it is done in the other
>> test suites.
>> I can not see how your case is different, but I might be missing something.
> Let's take a cursoy look at `git grep -l "'EOF'" t`.
>
> [...]

So the point is that some existing tests violate best practices? I do not think this is a good justification to do the same for new tests.

> In fact my version is actually cleaner than these, because the code that is run
> outside the cage is clearly delimited by a function.

It depends on the perspective. If it fails, the failure would be missed regardless of if it is in a function or not. Most examples that you quoted only create files outside test_expect_success. Even that is not necessary.

I am not telling you how you should write it. I am just saying that you are breaking one of the recommendations on how to write tests. There are different options that adhere to the suggestions in t/README.