Re: [PATCH] t5614: don't use subshells
- From
Jeff King <peff@peff.net>
- Date
- Jun 21, 2016, 19:38 UTC
- Message-ID
- <20160621193824.GA22183@sigill.intra.peff.net>
- In-Reply-To
- <xmqqh9cmpcze.fsf@gitster.mtv.corp.google.com>
On Tue, Jun 21, 2016 at 12:18:29PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> 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).
Show 27 quoted lines
> 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