| Message ID | patch-11.11-217c5aed491-20210510T141055Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f0d4d398e281009bc5e34d830b37c0c1df2fb8a8 |
| Headers | show |
| Series | test-lib.sh: new test_commit args, simplification & fixes | expand |
Ævar Arnfjörð Bjarmason wrote: > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -1243,21 +1243,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 () { If this is deprecated why not add a warning? echo "warning: test_create_repo is deprecated in favor of git init" >&2
On Thu, May 13 2021, Felipe Contreras wrote: > Ævar Arnfjörð Bjarmason wrote: >> --- a/t/test-lib-functions.sh >> +++ b/t/test-lib-functions.sh >> @@ -1243,21 +1243,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 () { > > If this is deprecated why not add a warning? > > echo "warning: test_create_repo is deprecated in favor of git init" >&2 Because like test_i18ncmp, test_i18ngrep or whatever this is in the state of "don't use this for new code", but annoying everyone who runs the test suite with loads of this output under -v would be too distracting. The attention of the developer community is much better spent on one person doing a s/test_create_repo/git init/ patch than having everyone see this warning until somebody does that.
Ævar Arnfjörð Bjarmason wrote: > > On Thu, May 13 2021, Felipe Contreras wrote: > > > Ævar Arnfjörð Bjarmason wrote: > >> --- a/t/test-lib-functions.sh > >> +++ b/t/test-lib-functions.sh > >> @@ -1243,21 +1243,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 () { > > > > If this is deprecated why not add a warning? > > > > echo "warning: test_create_repo is deprecated in favor of git init" >&2 > > Because like test_i18ncmp, test_i18ngrep or whatever this is in the > state of "don't use this for new code", but annoying everyone who runs > the test suite with loads of this output under -v would be too > distracting. > > The attention of the developer community is much better spent on one > person doing a s/test_create_repo/git init/ patch than having everyone > see this warning until somebody does that. Then it's not really deprecated; it's merely disfavored. To deprecate means to express disapproval of. If we are not going to express disapproval (i.e. annoy the users of the test suite), then it's not really deprecated. You can't eat your cake and have it too. I agree we shouldn't throw a warning right now, but at some point in the future we should, *then* it will be deprecated. Cheers.
On Thu, May 13 2021, Felipe Contreras wrote: > Ævar Arnfjörð Bjarmason wrote: >> >> On Thu, May 13 2021, Felipe Contreras wrote: >> >> > Ævar Arnfjörð Bjarmason wrote: >> >> --- a/t/test-lib-functions.sh >> >> +++ b/t/test-lib-functions.sh >> >> @@ -1243,21 +1243,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 () { >> > >> > If this is deprecated why not add a warning? >> > >> > echo "warning: test_create_repo is deprecated in favor of git init" >&2 >> >> Because like test_i18ncmp, test_i18ngrep or whatever this is in the >> state of "don't use this for new code", but annoying everyone who runs >> the test suite with loads of this output under -v would be too >> distracting. >> >> The attention of the developer community is much better spent on one >> person doing a s/test_create_repo/git init/ patch than having everyone >> see this warning until somebody does that. > > Then it's not really deprecated; it's merely disfavored. > > To deprecate means to express disapproval of. If we are not going to > express disapproval (i.e. annoy the users of the test suite), then it's > not really deprecated. You can't eat your cake and have it too. > > I agree we shouldn't throw a warning right now, but at some point in the > future we should, *then* it will be deprecated. I don't feel strongly about either term, as long as it's consistent. I do think that "deprecated" is consistent in the way I'm using it within the git project. If you grep various things we've "deprecated" we usually just do so by noting so in the docs. In this case the docs are the comments in test-lib.sh. We do also have things like git rebase --preserve-merges which emit a warning, but we use "deprecated" for both. By contrast we don't have a single git for "git grep -i disfavor". Anyway, for now I'd prefer to just have this land as-is and sort out such minor things later, given the v6 and having two other topics waiting on this...
Ævar Arnfjörð Bjarmason wrote: > On Thu, May 13 2021, Felipe Contreras wrote: > > Then it's not really deprecated; it's merely disfavored. > > > > To deprecate means to express disapproval of. If we are not going to > > express disapproval (i.e. annoy the users of the test suite), then it's > > not really deprecated. You can't eat your cake and have it too. > > > > I agree we shouldn't throw a warning right now, but at some point in the > > future we should, *then* it will be deprecated. > > I don't feel strongly about either term, as long as it's consistent. > > I do think that "deprecated" is consistent in the way I'm using it > within the git project. If you grep various things we've "deprecated" we > usually just do so by noting so in the docs. In this case the docs are > the comments in test-lib.sh. > > We do also have things like git rebase --preserve-merges which emit a > warning, but we use "deprecated" for both. By contrast we don't have a > single git for "git grep -i disfavor". > > Anyway, for now I'd prefer to just have this land as-is and sort out > such minor things later, given the v6 and having two other topics > waiting on this... Fair enough.
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 669645a812f..f0448daa74b 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1243,21 +1243,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 f3671b1766d..e6ca7a22826 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1365,7 +1365,8 @@ rm -fr "$TRASH_DIRECTORY" || { remove_trash=t if test -z "$TEST_NO_CREATE_REPO" then - test_create_repo "$TRASH_DIRECTORY" + git 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. This leave us with a pass-through wrapper for "git init" in test-lib-functions.sh, in test-lib.sh we have the same, except for needing to redirect stdout/stderr, and emitting an error ourselves if it fails. We don't need to error() ourselves when test_create_repo() is invoked, as the invocation will be a part of a test's "&&"-chain. Everything below this paragraph is a detailed summary of the history of test_create_repo() explaining why it's safe to remove the various things it was doing: 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. This makes us implicitly depend on the default hooks being disabled, which is a good thing. If and when we'd have any on-by-default hooks (I see no reason we ever would) we'd want to see the subtle and not so subtle ways that would break the test suite. 5. We don't need to "cd" to the "$repo" directory at all anymore. In the code being removed here we both "cd"'d to the repository before calling "init", and did so in a subshell. It's not important to do either, so both of those can be removed. We cd'd because this code grew from test-lib.sh code where we'd have done so already, see eedf8f97e58 (Abstract test_create_repo out for use in tests., 2006-02-17), and later "cd"'d inside a subshell since 0d314ce834d to avoid having to keep track of an "old pwd" variable to cd back after the setup. Being in the repository directory made moving the hooks around easier (we wouldn't have to fully qualify the path). Since we're not moving the hooks per #4 above we don't need to "cd" for that reason either. 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. We could still invoke test_create_repo() there, but as the invocation is now trivial and we don't have a good reason to use test_create_repo() elsewhere let's call "git init" there ourselves. 8. We didn't need to resolve "git" as "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" in test_create_repo(), even for the use of test-lib.sh PATH is already set up in test-lib.sh to start with GIT_TEST_INSTALLED and/or GIT_EXEC_PATH before test_create_repo() (now "git init") is called.. So we can simply run "git" and rely on the PATH lookup choosing the right executable. 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 | 3 ++- 5 files changed, 4 insertions(+), 18 deletions(-)