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.

>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index c81726acb9e..1258329fdd8 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -1252,18 +1252,9 @@ test_atexit () {
>>  # Most tests can use the created repository, but some may need to create more.
>>  # Usage: test_create_repo <directory>
>>  test_create_repo () {
>> -	test "$#" = 1 ||
>> -	BUG "not 1 parameter to test-create-repo"
>> -	repo="$1"
>> -	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'.
>
> 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
>>