* [PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED @ 2025-08-16 10:36 Adam Dinwoodie 2025-08-17 6:42 ` Usman Akinyemi 2025-08-19 7:43 ` [PATCH v2] " Adam Dinwoodie 0 siblings, 2 replies; 5+ messages in thread From: Adam Dinwoodie @ 2025-08-16 10:36 UTC (permalink / raw) To: git; +Cc: Usman Akinyemi, Patrick Steinhardt, Junio C Hamano, D . Ben Knoble The changes added by 39fc408562 (t/t1517: automate `git subcmd -h` tests outside a repository, 2025-08-08) to automatically loop over all "main" Git commands will, when run against an installed build using GIT_TEST_INSTALLED rather than the build in the build directory, include some extra git-gui commands that are installed by `make install`. These fail the test, so record them as such. Signed-off-by: Adam Dinwoodie <adam@dinwoodie•org> --- t/t1517-outside-repo.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh index 1c69d52c76..61fdd0170c 100755 --- a/t/t1517-outside-repo.sh +++ b/t/t1517-outside-repo.sh @@ -111,8 +111,9 @@ for cmd in $(git --list-cmds=main) do cmd=${cmd%.*} # strip .sh, .perl, etc. case "$cmd" in - archimport | cvsexportcommit | cvsimport | cvsserver | daemon | \ + archimport | citool | cvsexportcommit | cvsimport | cvsserver | daemon | \ difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \ + gui | gui--askpass | \ http-backend | http-fetch | http-push | init-db | \ merge-octopus | merge-one-file | merge-resolve | mergetool | \ mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \ -- 2.49.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED 2025-08-16 10:36 [PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED Adam Dinwoodie @ 2025-08-17 6:42 ` Usman Akinyemi 2025-08-19 7:43 ` [PATCH v2] " Adam Dinwoodie 1 sibling, 0 replies; 5+ messages in thread From: Usman Akinyemi @ 2025-08-17 6:42 UTC (permalink / raw) To: Adam Dinwoodie; +Cc: git, Patrick Steinhardt, Junio C Hamano, D . Ben Knoble > + gui | gui--askpass | \ > http-backend | http-fetch | http-push | init-db | \ > merge-octopus | merge-one-file | merge-resolve | mergetool | \ > mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \ Thanks for this change, I also tried applying it on my local machine and it runs successfully. > -- > 2.49.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] t/t1517: mark tests that fail with GIT_TEST_INSTALLED 2025-08-16 10:36 [PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED Adam Dinwoodie 2025-08-17 6:42 ` Usman Akinyemi @ 2025-08-19 7:43 ` Adam Dinwoodie 2025-08-19 15:35 ` Junio C Hamano 1 sibling, 1 reply; 5+ messages in thread From: Adam Dinwoodie @ 2025-08-19 7:43 UTC (permalink / raw) To: git; +Cc: Usman Akinyemi, Patrick Steinhardt, Junio C Hamano, D . Ben Knoble The changes added by 39fc408562 (t/t1517: automate `git subcmd -h` tests outside a repository, 2025-08-08) to automatically loop over all "main" Git commands will, when run against an installed build using GIT_TEST_INSTALLED rather than the build in the build directory, include some extra git-gui commands that are installed by `make install`, or credential helpers that might be installed manually from the contrib directories. These fail the test, so record them as such. Signed-off-by: Adam Dinwoodie <adam@dinwoodie•org> --- This re-roll adds a few more commands to those marked as known failures, notably credential helpers I see installed in various builds for the Nixpkgs packaging of Git. t/t1517-outside-repo.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh index 1c69d52c76..c824c1a25c 100755 --- a/t/t1517-outside-repo.sh +++ b/t/t1517-outside-repo.sh @@ -111,8 +111,11 @@ for cmd in $(git --list-cmds=main) do cmd=${cmd%.*} # strip .sh, .perl, etc. case "$cmd" in - archimport | cvsexportcommit | cvsimport | cvsserver | daemon | \ + archimport | citool | credential-netrc | credential-libsecret | \ + credential-osxkeychain | cvsexportcommit | cvsimport | cvsserver | \ + daemon | \ difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \ + gui | gui--askpass | \ http-backend | http-fetch | http-push | init-db | \ merge-octopus | merge-one-file | merge-resolve | mergetool | \ mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \ -- 2.49.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] t/t1517: mark tests that fail with GIT_TEST_INSTALLED 2025-08-19 7:43 ` [PATCH v2] " Adam Dinwoodie @ 2025-08-19 15:35 ` Junio C Hamano 2025-08-19 18:21 ` Adam Dinwoodie 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2025-08-19 15:35 UTC (permalink / raw) To: Adam Dinwoodie; +Cc: git, Usman Akinyemi, Patrick Steinhardt, D . Ben Knoble Adam Dinwoodie <adam@dinwoodie•org> writes: > The changes added by 39fc408562 (t/t1517: automate `git subcmd -h` tests > outside a repository, 2025-08-08) to automatically loop over all "main" > Git commands will, when run against an installed build using > GIT_TEST_INSTALLED rather than the build in the build directory, include > some extra git-gui commands that are installed by `make install`, or > credential helpers that might be installed manually from the contrib > directories. These fail the test, so record them as such. > > Signed-off-by: Adam Dinwoodie <adam@dinwoodie•org> > --- > > This re-roll adds a few more commands to those marked as known failures, > notably credential helpers I see installed in various builds for the > Nixpkgs packaging of Git. > > t/t1517-outside-repo.sh | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) I'd appreciate these efforts, but I am not sure if this is a losing battle. Your ~/libexec/git-core/ directory, when GIT_TEST_INSTALLED is in effect, likely has old commands that are retired, commands that are added by third-parties (so that their users can say "git frotz" and run their "frotz" software), and/or commands from the future that the running t1517 has not seen yet (while bisecting and running t1517 from an older commit, say). For example, I have these differences... archimport.perl citool cvsexportcommit.perl cvsimport.perl cvsserver.perl difftool--helper.sh filter-branch.sh gui gui--askpass instaweb.sh last-modified merge-octopus.sh merge-one-file.sh merge-resolve.sh mergetool.sh p4.py quiltimport.sh request-pull.sh send-email.perl submodule.sh svn.perl web--browse.sh ... in what t1517 $(git --list-cmds=main) sees between 'master' in normal test mode and with GIT_TEST_INSTALLED set to ~/git/jch/bin (i.e. the version I run for my everyday use). "last-modified" is an example of a new-ish command that the t1517 test being run is not yet aware of but included in GIT_TEST_INSTALLED. I am wondering if we are better off skipping this test, or at least limiting to some known subset (e.g. "git --list-cmds=builtins") to skip the files on disk when GIT_TEST_INSTALLED is in effect, instead of "git --list-cmds=main" that is quite broad)? In any case, this is a strict improvement over the previous one, so I'll replace and queue this for now, but we may want to rethink the approach this test uses. Even without GIT_TEST_INSTALLED, the fake GIT_EXEC_PATH we use during test has somewhat different from the real thing, I suspect. Thanks. > diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh > index 1c69d52c76..c824c1a25c 100755 > --- a/t/t1517-outside-repo.sh > +++ b/t/t1517-outside-repo.sh > @@ -111,8 +111,11 @@ for cmd in $(git --list-cmds=main) > do > cmd=${cmd%.*} # strip .sh, .perl, etc. > case "$cmd" in > - archimport | cvsexportcommit | cvsimport | cvsserver | daemon | \ > + archimport | citool | credential-netrc | credential-libsecret | \ > + credential-osxkeychain | cvsexportcommit | cvsimport | cvsserver | \ > + daemon | \ > difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \ > + gui | gui--askpass | \ > http-backend | http-fetch | http-push | init-db | \ > merge-octopus | merge-one-file | merge-resolve | mergetool | \ > mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] t/t1517: mark tests that fail with GIT_TEST_INSTALLED 2025-08-19 15:35 ` Junio C Hamano @ 2025-08-19 18:21 ` Adam Dinwoodie 0 siblings, 0 replies; 5+ messages in thread From: Adam Dinwoodie @ 2025-08-19 18:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Usman Akinyemi, Patrick Steinhardt, D . Ben Knoble On Tue, 19 Aug 2025 at 16:35, Junio C Hamano <gitster@pobox•com> wrote: > > Adam Dinwoodie <adam@dinwoodie•org> writes: > > > The changes added by 39fc408562 (t/t1517: automate `git subcmd -h` tests > > outside a repository, 2025-08-08) to automatically loop over all "main" > > Git commands will, when run against an installed build using > > GIT_TEST_INSTALLED rather than the build in the build directory, include > > some extra git-gui commands that are installed by `make install`, or > > credential helpers that might be installed manually from the contrib > > directories. These fail the test, so record them as such. > > > > Signed-off-by: Adam Dinwoodie <adam@dinwoodie•org> > > --- > > > > This re-roll adds a few more commands to those marked as known failures, > > notably credential helpers I see installed in various builds for the > > Nixpkgs packaging of Git. > > > > t/t1517-outside-repo.sh | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > I'd appreciate these efforts, but I am not sure if this is a losing > battle. Your ~/libexec/git-core/ directory, when GIT_TEST_INSTALLED > is in effect, likely has old commands that are retired, commands > that are added by third-parties (so that their users can say "git > frotz" and run their "frotz" software), and/or commands from the > future that the running t1517 has not seen yet (while bisecting and > running t1517 from an older commit, say). For example, I have these > differences... > > archimport.perl > citool > cvsexportcommit.perl > cvsimport.perl > cvsserver.perl > difftool--helper.sh > filter-branch.sh > gui > gui--askpass > instaweb.sh > last-modified > merge-octopus.sh > merge-one-file.sh > merge-resolve.sh > mergetool.sh > p4.py > quiltimport.sh > request-pull.sh > send-email.perl > submodule.sh > svn.perl > web--browse.sh > > ... in what t1517 $(git --list-cmds=main) sees between 'master' in > normal test mode and with GIT_TEST_INSTALLED set to ~/git/jch/bin > (i.e. the version I run for my everyday use). "last-modified" is an > example of a new-ish command that the t1517 test being run is not > yet aware of but included in GIT_TEST_INSTALLED. > > I am wondering if we are better off skipping this test, or at least > limiting to some known subset (e.g. "git --list-cmds=builtins") to > skip the files on disk when GIT_TEST_INSTALLED is in effect, instead > of "git --list-cmds=main" that is quite broad)? > > In any case, this is a strict improvement over the previous one, so > I'll replace and queue this for now, but we may want to rethink the > approach this test uses. Even without GIT_TEST_INSTALLED, the fake > GIT_EXEC_PATH we use during test has somewhat different from the > real thing, I suspect. > > Thanks. If someone's using GIT_TEST_INSTALLED, I think it's reasonable to expect them to keep their install directory fairly clean, so this test would only need to worry about things that might be there because they're included with Git. The cases I've patched for are all ones that are installed as part of the Nixpkgs Git build, which does copy in some things from contrib directories, but by design always installs into a new, empty root directory. None of which is to disagree with everything you've said. I imagine most people building Git aren't doing it using the Nixpkgs build processes, so if they're using GIT_TEST_INSTALLED, they're much more likely to be testing in an environment that includes other old scripts or tools or whatever. Having t1517 know what executables to check without requiring someone to remember to update a list is clearly valuable, but it seems that list should _somehow_ be built based on what's in the Git repository at build time, not what's in the user's environment. --list-cmds=builtins seems like it's more limited than ideal, but it might be a better approach than the current one. This is a balancing act I'll leave to people who are much more involved in the project development! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-08-19 18:22 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-16 10:36 [PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED Adam Dinwoodie 2025-08-17 6:42 ` Usman Akinyemi 2025-08-19 7:43 ` [PATCH v2] " Adam Dinwoodie 2025-08-19 15:35 ` Junio C Hamano 2025-08-19 18:21 ` Adam Dinwoodie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox