Re: [PATCH v2 12/12] test-lib: split up and deprecate test_create_repo()
On Sat, Apr 17, 2021 at 02:52:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 49 quoted lines
> Remove various redundant or obsolete code from the test_create_repo()
> function, and split up its use in test-lib.sh from what tests need
> from it, leaving us with a pass-through wrapper for "git init" in
> test-lib-functions.sh
>
> Reasons for why we can remove various code from test_create_repo():
>
> 1. "mkdir -p" isn't needed because "git init" itself will create
> leading directories if needed.
>
> 2. Since we're now a simple wrapper for "git init" we don't need to
> check that we have only one argument. If someone wants to run
> "test_create_repo --bare x" that's OK.
>
> 3. We won't ever hit that "Cannot setup test environment"
> error.
>
> Checking the test environment sanity when doing "git init" dates
> back to eea420693be (t0000: catch trivial pilot errors.,
> 2005-12-10) and 2ccd2027b01 (trivial: check, if t/trash directory
> was successfully created, 2006-01-05).
>
> We can also see it in another form a bit later in my own
> 0d314ce834d (test-lib: use subshell instead of cd $new && .. && cd
> $old, 2010-08-30).
>
> But since 2006f0adaee (t/test-lib: make sure Git has already been
> built, 2012-09-17) we already check if we have a built git
> earlier.
>
> The one thing this was testing after that 2012 change was that
> we'd just built "git", but not "git-init", but since
> 3af4c7156c4 (tests: respect GIT_TEST_INSTALLED when initializing
> repositories, 2018-11-12) we invoke "git", not "git-init".
>
> So all of that's been checked already, and we don't need to
> re-check it here.
>
> 4. We don't need to move .git/hooks out of the way.
>
> That dates back to c09a69a83e3 (Disable hooks during tests.,
> 2005-10-16), since then hooks became disabled by default in
> f98f8cbac01 (Ship sample hooks with .sample suffix, 2008-06-24).
>
> So the hooks were already disabled by default, but as can be seen
> from "mkdir .git/hooks" changes various tests needed to re-setup
> that directory. Now they no longer do.
>
> 5. Since we don't need to move the .git/hooks directory
Since we don't change directory anymore...
Show 6 quoted lines
> we don't need
> the subshell here either.
>
> That wasn't really needed for the .git/hooks either, but was being
> done for the convenience of not having to quote the path to the
> repository as we moved the hooks.
And then this dubious explanation will not be necessary.
Show 8 quoted lines
> 6. We can drop the --template argument and instead rely on the
> GIT_TEMPLATE_DIR set to the same path earlier in test-lib.sh. See
> 8683a45d669 (Introduce GIT_TEMPLATE_DIR, 2006-12-19)
>
> 7. We only needed that ">&3 2>&4" redirection when invoked from
> test-lib.sh, and the same goes for needing the full path to "git".
>
> Let's move that special behavior into test-lib.sh itself.
Quoting myself from my review of the previous version of this patch:
PATH is already set up to start with GIT_TEST_INSTALLED and/or
GIT_EXEC_PATH before 'test_create_repo' is called to init the repo in
the trash directory, so we could simply run 'git' and rely on PATH
lookup choosing the right executable.
Show 12 quoted lines
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 9ebb595c335..f73c3c6fc72 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1364,7 +1364,10 @@ rm -fr "$TRASH_DIRECTORY" || {
> remove_trash=t
> if test -z "$TEST_NO_CREATE_REPO"
> then
> - test_create_repo "$TRASH_DIRECTORY"
> + "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" \
> + init \
> + "$TRASH_DIRECTORY" >&3 2>&4 ||So this could be just:
git init "$TRASH_DIRECTORY" >&3 2>&4 ||
Show 7 quoted lines
> + error "cannot run git init"
> else
> mkdir -p "$TRASH_DIRECTORY"
> fi
> --
> 2.31.1.722.g788886f50a2
>
SZEDER Gábor <szeder.dev@gmail.com>