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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

> On Sat, Apr 17, 2021 at 02:52:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 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...
>
>> 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.

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