Re: [PATCH v4] branch: introduce --show-current display option

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Daniels Umanovskis <daniels@umanovskis.se>,
	Git List <git@vger.kernel.org>
Subject: Re: [PATCH v4] branch: introduce --show-current display option
Date: Wed, 17 Oct 2018 12:18:47 +0200 (DST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1810171211440.4546@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <CAPig+cRwy2Xhq7uJJ0OfY2nRZgPK9yHr=G+KMKuWx-PXyWv8Gg@mail.gmail.com>

Hi Eric,

On Tue, 16 Oct 2018, Eric Sunshine wrote:

> On Tue, Oct 16, 2018 at 7:09 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > This cleanup "checkout" needs to be encapsulated within a
> > > test_when_finished(), doesn't it? Preferably just after the "git
> > > checkout -b" invocation.
> >
> > In the meantime, here is what I'll have in 'pu' on top.
> >
> > diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> > @@ -119,12 +119,14 @@ test_expect_success 'git branch `--show-current` works properly when tag exists'
> >         cat >expect <<-\EOF &&
> >         branch-and-tag-name
> >         EOF
> > -       test_when_finished "git branch -D branch-and-tag-name" &&
> > +       test_when_finished "
> > +               git checkout branch-one
> > +               git branch -D branch-and-tag-name
> > +       " &&
> >         git checkout -b branch-and-tag-name &&
> >         test_when_finished "git tag -d branch-and-tag-name" &&
> >         git tag branch-and-tag-name &&
> >         git branch --show-current >actual &&
> > -       git checkout branch-one &&
> >         test_cmp expect actual
> >  '
> 
> This make sense to me.
> 
> > @@ -137,8 +139,7 @@ test_expect_success 'git branch `--show-current` works properly with worktrees'
> >         git worktree add worktree branch-two &&
> >         (
> >                 git branch --show-current &&
> > -               cd worktree &&
> > -               git branch --show-current
> > +               git -C worktree branch --show-current
> >         ) >actual &&
> >         test_cmp expect actual
> >  '
> 
> The subshell '(...)' could become '{...}' now that the 'cd' is gone,
> but that's a minor point.

Maybe not so minor.

I realized yesterday that the &&-chain linting we use for every single
test case takes a noticeable chunk of time:

	$ time ./t0006-date.sh --quiet
	# passed all 67 test(s)
	1..67

	real    0m20.973s
	user    0m2.662s
	sys     0m14.789s

	$ time ./t0006-date.sh --quiet --no-chain-lint
	# passed all 67 test(s)
	1..67

	real    0m13.607s
	user    0m1.330s
	sys     0m8.070s

My suspicion: it is essentially the `(exit 117)` that adds about 100ms to
every of those 67 test cases.

(Remember: a subshell requires a fork, and the `fork()` emulation on
Windows requires all kinds of things to be copied to a new process,
including memory and open file descriptors, before the `exec()` will undo
at least part of that.)

With that in mind, I would like to suggest that we should start to be very
careful about using subshells in our test suite.

Ciao,
Dscho