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.
