| Message ID | patch-12.12-a3e20ef18f7-20210417T124424Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: new test_commit args, simplification & fixes | expand |
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. > 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 || > + error "cannot run git init" > else > mkdir -p "$TRASH_DIRECTORY" > fi > -- > 2.31.1.722.g788886f50a2 >
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.
On Sat, Apr 17, 2021 at 11:45:41PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> 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). That commit doesn't mention hooks at all, so I'm not sure what I should see there. OTOH, you did specifically write "use a subshell instead of keeping track of the old working directory and cd-ing back when it's done"... which is in line with our best practices on how to change directories in our tests in order to reliably change back even in case of an error. And that cd-ing back and forth was not added because of the hooks either, but because back in the day when 'test_create_repo' was created 'git init-db' could only create a new repository in the current directory. > 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, Agreed. > and assure the reader that e.g. the > subshelling isn't needed for some other subtle reason. The role of moving the hooks with respect to the subshell is not a subtlety, it is simply irrelevant. And while you are so focused on this irrevelancy you keep forgetting to mention the factor that actually does matter: that we won't change directories anymore.
diff --git a/t/t5406-remote-rejects.sh b/t/t5406-remote-rejects.sh index ff06f99649e..5c509db6fc3 100755 --- a/t/t5406-remote-rejects.sh +++ b/t/t5406-remote-rejects.sh @@ -5,7 +5,6 @@ test_description='remote push rejects are reported by client' . ./test-lib.sh test_expect_success 'setup' ' - mkdir .git/hooks && write_script .git/hooks/update <<-\EOF && exit 1 EOF diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh index 5bb23cc3a4e..6da8d760e28 100755 --- a/t/t5407-post-rewrite-hook.sh +++ b/t/t5407-post-rewrite-hook.sh @@ -20,8 +20,6 @@ test_expect_success 'setup' ' git checkout main ' -mkdir .git/hooks - cat >.git/hooks/post-rewrite <<EOF #!/bin/sh echo \$@ > "$TRASH_DIRECTORY"/post-rewrite.args diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index 5d8f401d8ec..9f1a483f426 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -5,7 +5,6 @@ test_description='remote messages are colorized on the client' . ./test-lib.sh test_expect_success 'setup' ' - mkdir .git/hooks && write_script .git/hooks/update <<-\EOF && echo error: error echo ERROR: also highlighted diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 8e75a013a43..bd64a15c731 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1248,21 +1248,10 @@ test_atexit () { } && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup" } -# Most tests can use the created repository, but some may need to create more. +# Deprecated wrapper for "git init", use "git init" directly instead # 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 + git init "$@" } # This function helps on symlink challenged file systems when it is not 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 || + error "cannot run git init" else mkdir -p "$TRASH_DIRECTORY" fi
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 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. 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. In the end it turns out that all we needed was a plain "git init" invocation. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5406-remote-rejects.sh | 1 - t/t5407-post-rewrite-hook.sh | 2 -- t/t5409-colorize-remote-messages.sh | 1 - t/test-lib-functions.sh | 15 ++------------- t/test-lib.sh | 5 ++++- 5 files changed, 6 insertions(+), 18 deletions(-)