Re: [PATCH] t/perf: export variable used in other blocks

From: Jeff King <peff@peff.net>
Date: Mar 3, 2017, 06:45 UTC
Message-ID: <20170303064512.khs2seru5onl54mh@sigill.intra.peff.net>
In-Reply-To: <20170302195041.1699-1-jonathantanmy@google.com>

On Thu, Mar 02, 2017 at 11:50:41AM -0800, Jonathan Tan wrote:

Show 5 quoted lines

> In p0001, a variable was created in a test_expect_success block to be
> used in later test_perf blocks, but was not exported. This caused the
> variable to not appear in those blocks (this can be verified by writing
> 'test -n "$commit"' in those blocks), resulting in a slightly different
> invocation than what was intended. Export that variable.

Thanks, this is obviously the right thing to do, and the mistake is mine from ea97002fc (t/perf: time rev-list with UNINTERESTING commits, 2014-01-20). This is not the first time I've been confused by missing variables in t/perf scripts, since it behaves differently than the normal test suite. I wonder if we should turn on "set -a" during t/perf setup snippets. That's a bit of a blunt tool, but I suspect it would just be easier to work with.

I was curious that the tests added by ea97002fc showed something useful even with the bug you're fixing here. But it's because the actual slice of history we meant to traverse isn't important. It's intentionally tiny to show off the time spent dealing with the UNINTERESTING commits. So in effect we were traversing no commits instead of a tiny set, but the timing results were the same.

I repeated the tests over fbd4a703 given in the commit message of ee9a7002fc and confirmed that it behaves the same with your fixed version of the test. I did have to tweak a few other things to get the test to run against such an old version of git, though. I'll follow-up with a patch.

-Peff