Re: [PATCH 12/16] test-lib: modernize test_create_repo() function

On Mon, Apr 12, 2021 at 01:09:01PM +0200, Ævar Arnfjörð Bjarmason wrote:

Remove redundant "mkdir -p", argument number checking', test
environment sanity checking, and disabling of hooks from
test_create_repo(). As we'll see below these were all either redundant
to other test other framework code, or to changes in git itself.

Respectively:

 1. "mkdir -p" isn't needed because "git init" itself will create
    leading directories if needed.

 2. We don't need to check the number of arguments anymore, instead
    we'll feed "git init" with "$@". It will die if given too many
    arguments.

Or it will succeed if invoked as e.g. 'test_create_repo --bare repo'.

 3. We won't ever hit that "Cannot setup test environment"
    error.

ENOSPC?  Some rogue background process on Windows still desperately
clinging to an open file descriptor to some file in the same
directory, preventing 'rm -rf "$TRASH_DIRECTORY"' near the beginning
of 'test-lib.sh' and interfering with 'git init'?

    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.

I agree that if we already have a 'git' binary that can run 'git
version', then we can safely assume that it will be able to run 'git
init' as well.  It might be that 'git init' is buggy and segfaults,
but that is not a "have you built things yet?" kind of error.

 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 we don't need
    the subshell here either.

We needed the subshell because we changed directory in
'test_create_repo' (even you referenced 0d314ce834d above), not
because we moved the hooks directory.

In the end it turns out that all we needed was a plain "git init"
invocation with a custom --template directory.

We don't need '--template', either; see below.

This patch removes this '|| exit', which is...

  - good: if 'test_create_repo' is invoked in a test case (and not in
    a subshell) and if it were to fail for some reason, then it won't
    abort the whole test script, but will fail only that test case.

  - bad: 'test_create_repo' is responsible for creating the repository
    in the trash directory as well; if that were to fail for any
    reason, then the test script will not be aborted early.

I think the 'exit' on error should be removed from 'test_create_repo',
but the callsite in 'test-lib.sh' should become 'test_create_repo ||
exit 1'.

In any case, removing this '|| exit' is not mentioned in the commit
message.

+	"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" \

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.

+		init \
+		"--template=$GIT_BUILD_DIR/templates/blt/" "$@" >&3 2>&4

Likewise, GIT_TEMPLATE_DIR is already set up to the same value as in
this '--template=...' option, so we could omit this option as well.

And after that all that would remain in this function is:

  git init "$1" >&3 2>&4

And those redirections are only needed when this function is called to
initialize the repo in the trash directory, but not when it creates a
repo in a test case.  This makes me question the value of having this
'test_create_repo' helper function at all; there are already over 800
cases where we run plain 'git init' outside of 't0001-init.sh' instead
of 'test_create_repo', though removing it would be definitely cause
some churn with over 300 callsites.

 }
 
 # This function helps on symlink challenged file systems when it is not
-- 
2.31.1.634.gb41287a30b0