Re: [PATCH v2 12/12] test-lib: split up and deprecate test_create_repo()

On Sat, Apr 17 2021, SZEDER Gábor wrote:

On Sat, Apr 17, 2021 at 02:52:45PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

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...

quoted

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.

Why dubious? That's why we had the subshell-ing. See 0d314ce834
(test-lib: use subshell instead of cd $new && .. && cd $old,
2010-08-30).

I don't mind rewording or not including some of this verbosity per-se,
but I wonder why you're honing in on the subshell part in
particular. Maybe there's something I'm missing...

The goal of the commit messsage is to point-by-point tear apart facets
of the previous behavior, and assure the reader that e.g. the
subshelling isn't needed for some other subtle reason.

quoted

 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.

quoted

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 ||

Ah yes, I see that now. FWIW I managed to misread that in the last round
as it applying only once we were calling the test-lib-functions.sh
helper, but I see it's finishe setting up a few lines before the
test_create_repo in test-lib.sh too, nice.