Re: [PATCH 7/8] t/perf: add 'GIT_PERF_USE_SCALAR' run option
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 2, 2022, 09:17 UTC
- Message-ID
- <p78rqo9o-o1p2-qq6q-6oo4-94828so928nq@tzk.qr>
- In-Reply-To
- <72f6661d-be02-dbb8-7110-b8de6bac77f6@github.com>
Hi Victoria,
On Thu, 1 Sep 2022, Victoria Dye wrote:
Show 41 quoted lines
> Johannes Schindelin wrote:
> >
> > On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> >
> >> From: Victoria Dye <vdye@github.com>
> >>
> >> Add a 'GIT_PERF_USE_SCALAR' environment variable (and corresponding perf
> >> config 'useScalar') to register a repository created with any of:
> >>
> >> * test_perf_fresh_repo
> >> * test_perf_default_repo
> >> * test_perf_large_repo
> >>
> >> as a Scalar enlistment. This is intended to allow a developer to test the
> >> impact of Scalar on already-defined performance scenarios.
> >
> > Great idea!
> >
> >> [...]
> >> @@ -130,7 +137,11 @@ test_perf_fresh_repo () {
> >> "$MODERN_GIT" init -q "$repo" &&
> >> (
> >> cd "$repo" &&
> >> - test_perf_do_repo_symlink_config_
> >> + test_perf_do_repo_symlink_config_ &&
> >> + if test_bool_env "$GIT_PERF_USE_SCALAR" false
> >> + then
> >> + "$MODERN_SCALAR" register
> >
> > Do we need to unregister anything here? My guess is that no, the "global"
> > config we're using in tests is "$TRASH_DIRECTORY/.gitconfig", and the side
> > effect of scheduling the maintenance task won't matter in practice. But I
> > might have missed something and we may want to have an explicit
> > `unregister` step.
> >
> > What's your take on this?
>
> As you guessed, a '.gitconfig' is created in the trash directory of each
> test containing the Scalar registration and I haven't seen any issues
> arising from the scheduled maintenance, so I don't think an 'unregister' is
> necessary.Thank you for checking!
> However, while verifying that, I noticed that the registration wasn't > happening *at all* because 'test_bool_env' is currently being used > incorrectly. The fix is straightforward - I'll make sure to correct it > in the next version.
Oh, great, then my feedback was at least _somewhat_ helpful... ;-)
Ciao, Dscho