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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 22, 2014, 06:45 UTC
Message-ID: <5356100296994_268bd0b30839@nysa.notmuch>
In-Reply-To: <535606A3.8040704@gmail.com>

Ilya Bobyr wrote:

Show 23 quoted lines

> On 4/21/2014 3:24 PM, Felipe Contreras wrote:
> > 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.

So? I really don't see the need to explain that such a monstrosity would be unmaintainable, that's a given.

Show 15 quoted lines

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

This is what I originally asked for.

Show 8 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 don't know what you mean by "best practices", but these are Git's best practices.

> I do not think this is a good justification to do the same for new tests.

It is not a justification to reject a patch either, specially if no better alternative has been put forward.

Fortunately a better alternative has been put forward, so this is moot.

-- 
Felipe Contreras