| Message ID | patch-12.16-424caad189f-20210412T110456Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: new test_commit args, simplification & fixes | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 4. We don't need to move .git/hooks out of the way.
It might be unsafe to assume that we'd never have an
'enabled-by-default' hook in blt/ template directory forever,
though.
On Mon, Apr 12 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> 4. We don't need to move .git/hooks out of the way. > > It might be unsafe to assume that we'd never have an > 'enabled-by-default' hook in blt/ template directory forever, > though. I would think that if we injected run-by-default hooks into the top-level repo creation any such breakage in the test suite would be a feature, i.e. we'd want to see and adjust for the breakage, because surely the same sort of breakage would occur in the wild. FWIW I've got some WIP local patches for the "just ship a README in hooks/": https://lore.kernel.org/git/20180601070609.GC15578@sigill.intra.peff.net/ From memory the cumultive footprint of foot a -d run on the whole test suite is on the order of ~1GB, and ~100MB is just the hooks over and over again...
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. > In the end it turns out that all we needed was a plain "git init" > invocation with a custom --template directory. We don't need '--template', either; see below. > 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 >
On Thu, Apr 15, 2021 at 11:10:13PM +0200, SZEDER Gábor wrote: > > 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. > > - 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.
On Thu, Apr 15 2021, SZEDER Gábor wrote: > 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'. Which is fine, no? I'll make it clearer in the commit message, but I'm aiming to address plausible bugs due to these functions not erroring out in one way or another, if you call it with "--bare repo" that's clearly what you wanted, as opposed to "foo bar" and we don't create a repo under "bar". >> 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'? I mean to say we won't hit it for the reasons we'd have that error there in the first place. Of course "git init" can still error, but we'll catch that in other ways. >> 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. *nod* >> 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. Well, yes and no, a minor point but we'd need to do some juggling with saving the $PWD in a variable etc. if it wasn't for the subshell, I think that's why it got added in the first place. >> In the end it turns out that all we needed was a plain "git init" >> invocation with a custom --template directory. > > We don't need '--template', either; see below. Ah, interesting. >> 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. *nod* > - 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. I'll fix that code. FWIW I the "|| exit 1" is effectively not removed in almost all cases where "git init" would error, because in test-lib.sh we cd to the just-init-ed directory, and if we can't we'll fail. But yes, it won't catch e.g. git-init dying midway through, us running out of space while populating that directory etc. >> + "${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. I'd missed how GIT_TEMPLATE_DIR was being set, that allows us to make this much simpler indeed! >> } >> >> # This function helps on symlink challenged file systems when it is not >> -- >> 2.31.1.634.gb41287a30b0 >>
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 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 + "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" \ + init \ + "--template=$GIT_BUILD_DIR/templates/blt/" "$@" >&3 2>&4 } # This function helps on symlink challenged file systems when it is not
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. 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. In the end it turns out that all we needed was a plain "git init" invocation with a custom --template directory. 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 +++------------ 4 files changed, 3 insertions(+), 16 deletions(-)