| Message ID | 20190621223107.8022-12-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion: a bunch of updates | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > +offgit () > +{ Style: opening brace comes on the same line, like offgit () { > + GIT_CEILING_DIRECTORIES="$ROOT" && > + export GIT_CEILING_DIRECTORIES && > + test_when_finished "ROOT='$ROOT'; cd '$TRASH_DIRECTORY'; unset GIT_CEILING_DIRECTORIES" && > + ROOT="$ROOT"/non-repo && > + cd "$ROOT" > +} All of these means that anytime some test uses offgit outside a subshell, all the subsequent test will start from outside a repository, with nonstandard GIT_CEILING_DIRECTORIES settings. The test should avoid using this outside a subshell when able (and if it apparently cannot easily, we should try to find a way). > @@ -358,10 +367,8 @@ test_expect_success SYMLINKS '__git_find_repo_path - resulting path avoids symli > ' > > test_expect_success '__git_find_repo_path - not a git repository' ' > + offgit && > ( > - cd non-repo && > - GIT_CEILING_DIRECTORIES="$ROOT" && > - export GIT_CEILING_DIRECTORIES && > test_must_fail __git_find_repo_path && > printf "$__git_repo_path" >"$actual" > ) && > @@ -1388,6 +1395,7 @@ test_expect_success '__git_pretty_aliases' ' > ' > > test_expect_success 'basic' ' > + offgit && > run_completion "git " && Adding "offgit" everywhere like this patch does means that this "basic" test, for example, no longer is performed in the condition we have been testing the completion script for, doesn't it? If so, the patch is trading test coverage outside repo with coverage inside repo, which is not a very good tradeoff.
On Fri, Jun 21, 2019 at 05:31:04PM -0500, Felipe Contreras wrote: > There's a regression in the completion since the introduction of > __gitcomp. > > Go to any directory that doesn't contain a git repository, like /tmp. > Then type the following: > > git checkout --<tab> > > You will see nothing. That's because > `git checkout --git-completion-helper` fails when you run it outside a > git repository. > > You might change to a directory that has a git repository, but it's too > late, because the empty options have been cached. This will get outdated rather soonish, as soon as 69702523af (completion: do not cache if --git-completion-helper fails, 2019-06-12) graduates to master. > It's unclear how many commands are affected, but this patch attempts to > at least detect some already in the testing framework. It seems that several changes in this patch modify tests in a way that defeats the purpose of the given test, e.g. the tests 'completion.commands removes multiple commands' or 'sourcing the completion script clears cached merge strategies' I would rather see tests specifically focusing on the __gitcomp_builtin() helper function, including test cases when it's excersized outside of a repository and when it gets additional parameters to include and exclude some options. > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > t/t9902-completion.sh | 37 ++++++++++++++++++++++++++++--------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index 43cf313a1c..7bef41eaf5 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -122,6 +122,15 @@ test_gitcomp_nl () > test_cmp expected out > } > > +offgit () > +{ > + GIT_CEILING_DIRECTORIES="$ROOT" && > + export GIT_CEILING_DIRECTORIES && > + test_when_finished "ROOT='$ROOT'; cd '$TRASH_DIRECTORY'; unset GIT_CEILING_DIRECTORIES" && > + ROOT="$ROOT"/non-repo && > + cd "$ROOT" I think fiddling with $ROOT is unnecessary here. > +} > + > invalid_variable_name='${foo.bar}' > > actual="$TRASH_DIRECTORY/actual" > @@ -358,10 +367,8 @@ test_expect_success SYMLINKS '__git_find_repo_path - resulting path avoids symli > ' > > test_expect_success '__git_find_repo_path - not a git repository' ' > + offgit && > ( > - cd non-repo && > - GIT_CEILING_DIRECTORIES="$ROOT" && > - export GIT_CEILING_DIRECTORIES && > test_must_fail __git_find_repo_path && > printf "$__git_repo_path" >"$actual" > ) && > @@ -1388,6 +1395,7 @@ test_expect_success '__git_pretty_aliases' ' > ' > > test_expect_success 'basic' ' > + offgit && > run_completion "git " && > # built-in > grep -q "^add \$" out && > @@ -1401,6 +1409,7 @@ test_expect_success 'basic' ' > ' > > test_expect_success 'double dash "git" itself' ' > + offgit && > test_completion "git --" <<-\EOF > --paginate Z > --no-pager Z > @@ -1419,7 +1428,8 @@ test_expect_success 'double dash "git" itself' ' > EOF > ' > > -test_expect_success 'double dash "git checkout"' ' > +test_expect_failure 'double dash "git checkout"' ' > + offgit && > test_completion "git checkout --" <<-\EOF > --quiet Z > --detach Z > @@ -1442,6 +1452,7 @@ test_expect_success 'double dash "git checkout"' ' > ' > > test_expect_success 'general options' ' > + offgit && > test_completion "git --ver" "--version " && > test_completion "git --hel" "--help " && > test_completion "git --exe" <<-\EOF && > @@ -1460,6 +1471,7 @@ test_expect_success 'general options' ' > ' > > test_expect_success 'general options plus command' ' > + offgit && > test_completion "git --version check" "checkout " && > test_completion "git --paginate check" "checkout " && > test_completion "git --git-dir=foo check" "checkout " && > @@ -1480,11 +1492,13 @@ test_expect_success 'general options plus command' ' > ' > > test_expect_success 'git --help completion' ' > + offgit && > test_completion "git --help ad" "add " && > test_completion "git --help core" "core-tutorial " > ' > > -test_expect_success 'completion.commands removes multiple commands' ' > +test_expect_failure 'completion.commands removes multiple commands' ' > + offgit && > test_config completion.commands "-cherry -mergetool" && > git --list-cmds=list-mainporcelain,list-complete,config >out && > ! grep -E "^(cherry|mergetool)$" out > @@ -1547,9 +1561,10 @@ test_expect_success 'complete tree filename with metacharacters' ' > EOF > ' > > -test_expect_success PERL 'send-email' ' > - test_completion "git send-email --cov" "--cover-letter " && > - test_completion "git send-email ma" "master " > +test_expect_failure PERL 'send-email' ' > + test_completion "git send-email ma" "master " && > + offgit && > + test_completion "git send-email --cov" "--cover-letter " > ' > > test_expect_success 'complete files' ' > @@ -1649,6 +1664,7 @@ test_expect_success 'completion used <cmd> completion for alias: !f() { : git <c > ' > > test_expect_success 'completion without explicit _git_xxx function' ' > + offgit && > test_completion "git version --" <<-\EOF > --build-options Z > --no-build-options Z > @@ -1699,13 +1715,15 @@ do > done > > test_expect_success 'sourcing the completion script clears cached commands' ' > + offgit && > __git_compute_all_commands && > verbose test -n "$__git_all_commands" && > . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" && > verbose test -z "$__git_all_commands" > ' > > -test_expect_success 'sourcing the completion script clears cached merge strategies' ' > +test_expect_failure 'sourcing the completion script clears cached merge strategies' ' > + offgit && > GIT_TEST_GETTEXT_POISON= && > __git_compute_merge_strategies && > verbose test -n "$__git_merge_strategies" && > @@ -1714,6 +1732,7 @@ test_expect_success 'sourcing the completion script clears cached merge strategi > ' > > test_expect_success 'sourcing the completion script clears cached --options' ' > + offgit && > __gitcomp_builtin checkout && > verbose test -n "$__gitcomp_builtin_checkout" && > __gitcomp_builtin notes_edit && > -- > 2.22.0 >
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 43cf313a1c..7bef41eaf5 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -122,6 +122,15 @@ test_gitcomp_nl () test_cmp expected out } +offgit () +{ + GIT_CEILING_DIRECTORIES="$ROOT" && + export GIT_CEILING_DIRECTORIES && + test_when_finished "ROOT='$ROOT'; cd '$TRASH_DIRECTORY'; unset GIT_CEILING_DIRECTORIES" && + ROOT="$ROOT"/non-repo && + cd "$ROOT" +} + invalid_variable_name='${foo.bar}' actual="$TRASH_DIRECTORY/actual" @@ -358,10 +367,8 @@ test_expect_success SYMLINKS '__git_find_repo_path - resulting path avoids symli ' test_expect_success '__git_find_repo_path - not a git repository' ' + offgit && ( - cd non-repo && - GIT_CEILING_DIRECTORIES="$ROOT" && - export GIT_CEILING_DIRECTORIES && test_must_fail __git_find_repo_path && printf "$__git_repo_path" >"$actual" ) && @@ -1388,6 +1395,7 @@ test_expect_success '__git_pretty_aliases' ' ' test_expect_success 'basic' ' + offgit && run_completion "git " && # built-in grep -q "^add \$" out && @@ -1401,6 +1409,7 @@ test_expect_success 'basic' ' ' test_expect_success 'double dash "git" itself' ' + offgit && test_completion "git --" <<-\EOF --paginate Z --no-pager Z @@ -1419,7 +1428,8 @@ test_expect_success 'double dash "git" itself' ' EOF ' -test_expect_success 'double dash "git checkout"' ' +test_expect_failure 'double dash "git checkout"' ' + offgit && test_completion "git checkout --" <<-\EOF --quiet Z --detach Z @@ -1442,6 +1452,7 @@ test_expect_success 'double dash "git checkout"' ' ' test_expect_success 'general options' ' + offgit && test_completion "git --ver" "--version " && test_completion "git --hel" "--help " && test_completion "git --exe" <<-\EOF && @@ -1460,6 +1471,7 @@ test_expect_success 'general options' ' ' test_expect_success 'general options plus command' ' + offgit && test_completion "git --version check" "checkout " && test_completion "git --paginate check" "checkout " && test_completion "git --git-dir=foo check" "checkout " && @@ -1480,11 +1492,13 @@ test_expect_success 'general options plus command' ' ' test_expect_success 'git --help completion' ' + offgit && test_completion "git --help ad" "add " && test_completion "git --help core" "core-tutorial " ' -test_expect_success 'completion.commands removes multiple commands' ' +test_expect_failure 'completion.commands removes multiple commands' ' + offgit && test_config completion.commands "-cherry -mergetool" && git --list-cmds=list-mainporcelain,list-complete,config >out && ! grep -E "^(cherry|mergetool)$" out @@ -1547,9 +1561,10 @@ test_expect_success 'complete tree filename with metacharacters' ' EOF ' -test_expect_success PERL 'send-email' ' - test_completion "git send-email --cov" "--cover-letter " && - test_completion "git send-email ma" "master " +test_expect_failure PERL 'send-email' ' + test_completion "git send-email ma" "master " && + offgit && + test_completion "git send-email --cov" "--cover-letter " ' test_expect_success 'complete files' ' @@ -1649,6 +1664,7 @@ test_expect_success 'completion used <cmd> completion for alias: !f() { : git <c ' test_expect_success 'completion without explicit _git_xxx function' ' + offgit && test_completion "git version --" <<-\EOF --build-options Z --no-build-options Z @@ -1699,13 +1715,15 @@ do done test_expect_success 'sourcing the completion script clears cached commands' ' + offgit && __git_compute_all_commands && verbose test -n "$__git_all_commands" && . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" && verbose test -z "$__git_all_commands" ' -test_expect_success 'sourcing the completion script clears cached merge strategies' ' +test_expect_failure 'sourcing the completion script clears cached merge strategies' ' + offgit && GIT_TEST_GETTEXT_POISON= && __git_compute_merge_strategies && verbose test -n "$__git_merge_strategies" && @@ -1714,6 +1732,7 @@ test_expect_success 'sourcing the completion script clears cached merge strategi ' test_expect_success 'sourcing the completion script clears cached --options' ' + offgit && __gitcomp_builtin checkout && verbose test -n "$__gitcomp_builtin_checkout" && __gitcomp_builtin notes_edit &&
There's a regression in the completion since the introduction of __gitcomp. Go to any directory that doesn't contain a git repository, like /tmp. Then type the following: git checkout --<tab> You will see nothing. That's because `git checkout --git-completion-helper` fails when you run it outside a git repository. You might change to a directory that has a git repository, but it's too late, because the empty options have been cached. It's unclear how many commands are affected, but this patch attempts to at least detect some already in the testing framework. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- t/t9902-completion.sh | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-)