Re: [PATCH] t5614: don't use subshells

[Jeff King <peff@peff.net>]

On Tue, Jun 21, 2016 at 12:18:29PM -0700, Junio C Hamano wrote:

> Stefan Beller <sbeller@google.com> writes:
> 
> >  Unlike the prior patch we would not want this patch to fall through
> >  to origin/maint fast, but allow cooking?
> 
> I do not see anything that makes this treated differently from the
> other fix.  The only difference in behaviour is that "lines" file is
> now created at the root level of the trash repository's working tree
> instead of tested clones', and I do not think any test depends on
> the number of untracked paths in the trash working tree or tries to
> make sure a file called "lines" is not in there.

I think it is only that the other patch is actually fixing something,
whereas this is cleanup. So the cost/benefit equation is different. I
agree neither is high-risk and a test cleanup is generally OK for maint
(the other is a serious-ish regression IMHO).

> Having said that, I wonder if we want further reduction of the
> repetition.  Each test except "setup" in this script does an
> identical thing with very small set of parameters:
> 
>     - make sure super_clone will be removed when done.
>     - clone file://$pwd/. to super_clone but with different set of options.
>     - check the commits in super_clone and super_clone/sub.
> 
> So, the above would ideally become something like
> 
> do_test 3 3 --recurse-submodules
> 
> where the helper would look like
> 
> do_test () {
> 	cnt_super=$1 cnt_sub=$2 &&
>         shift 2 &&
>         test_when_finished "rm -fr super_clone" &&
>         git clone "$@" "file://$pwd/." super_clone &&
>         git -C super_clone log --oneline >lines &&
>         test_line_count = $cnt_super lines &&
>         git -C super_clone/sub log --oneline >lines &&
>         test_line_count = $cnt_sub lines
> }
> 
> Would it rob too much flexibility from future tests to be added to
> this script if we did it that way?

I think that's an improvement, too. Even if we add further tests, they
don't have to follow the same format. I would give the function a better
name than "do_test" though. :P

-Peff