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

On Thu, Apr 15, 2021 at 11:10:13PM +0200, SZEDER Gábor wrote:

quoted

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

quoted

    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.

quoted

-	mkdir -p "$repo"
-	(
-		cd "$repo" || error "Cannot setup test environment"
-		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" \
-			init \
-			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
-		error "cannot run git init -- have you built things yet?"
-		mv .git/hooks .git/hooks-disabled
-	) || exit

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

Case in point: the bug I just reported in

  https://public-inbox.org/git/20210416211451.GP2947267@szeder.dev/

does break Git in a way that in one of our CI jobs 'git init' is
unable to create the repository in the trash directory.