{"thread":{"id":"63965","subject":"[PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED","startedAt":"2025-08-16T10:45:34Z","lastAt":"2025-08-19T18:22:13Z","messageCount":5,"participants":["Adam Dinwoodie","Usman Akinyemi","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"524291","messageId":"20250816103656.1693607-1-adam@dinwoodie.org","threadId":"63965","inReplyTo":null,"subject":"[PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2025-08-16T10:36:53Z","receivedAt":"2025-08-16T10:45:34Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"The changes added by 39fc408562 (t/t1517: automate `git subcmd -h` tests\noutside a repository, 2025-08-08) to automatically loop over all \"main\"\nGit commands will, when run against an installed build using\nGIT_TEST_INSTALLED rather than the build in the build directory, include\nsome extra git-gui commands that are installed by `make install`.  These\nfail the test, so record them as such.\n\nSigned-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n---\n t/t1517-outside-repo.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex 1c69d52c76..61fdd0170c 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -111,8 +111,9 @@ for cmd in $(git --list-cmds=main)\n do\n \tcmd=${cmd%.*} # strip .sh, .perl, etc.\n \tcase \"$cmd\" in\n-\tarchimport | cvsexportcommit | cvsimport | cvsserver | daemon | \\\n+\tarchimport | citool | cvsexportcommit | cvsimport | cvsserver | daemon | \\\n \tdifftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \\\n+\tgui | gui--askpass | \\\n \thttp-backend | http-fetch | http-push | init-db | \\\n \tmerge-octopus | merge-one-file | merge-resolve | mergetool | \\\n \tmktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \\\n-- \n2.49.0\n\n"},{"id":"524315","messageId":"CAPSxiM_tOW7iGxMrekazXHSRhQj76vyGQUvbA+yHYWg-bVp14Q@mail.gmail.com","threadId":"63965","inReplyTo":"20250816103656.1693607-1-adam@dinwoodie.org","subject":"Re: [PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-08-17T06:42:31Z","receivedAt":"2025-08-17T06:42:42Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"> +       gui | gui--askpass | \\\n>         http-backend | http-fetch | http-push | init-db | \\\n>         merge-octopus | merge-one-file | merge-resolve | mergetool | \\\n>         mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \\\nThanks for this change, I also tried applying it on my local machine\nand it runs successfully.\n> --\n> 2.49.0\n>\n"},{"id":"524388","messageId":"20250819074631.3303-1-adam@dinwoodie.org","threadId":"63965","inReplyTo":"20250816103656.1693607-1-adam@dinwoodie.org","subject":"[PATCH v2] t/t1517: mark tests that fail with GIT_TEST_INSTALLED","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2025-08-19T07:43:29Z","receivedAt":"2025-08-19T07:46:43Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"The changes added by 39fc408562 (t/t1517: automate `git subcmd -h` tests\noutside a repository, 2025-08-08) to automatically loop over all \"main\"\nGit commands will, when run against an installed build using\nGIT_TEST_INSTALLED rather than the build in the build directory, include\nsome extra git-gui commands that are installed by `make install`, or\ncredential helpers that might be installed manually from the contrib\ndirectories.  These fail the test, so record them as such.\n\nSigned-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n---\n\nThis re-roll adds a few more commands to those marked as known failures,\nnotably credential helpers I see installed in various builds for the\nNixpkgs packaging of Git.\n\n t/t1517-outside-repo.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex 1c69d52c76..c824c1a25c 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -111,8 +111,11 @@ for cmd in $(git --list-cmds=main)\n do\n \tcmd=${cmd%.*} # strip .sh, .perl, etc.\n \tcase \"$cmd\" in\n-\tarchimport | cvsexportcommit | cvsimport | cvsserver | daemon | \\\n+\tarchimport | citool | credential-netrc | credential-libsecret | \\\n+\tcredential-osxkeychain | cvsexportcommit | cvsimport | cvsserver | \\\n+\tdaemon | \\\n \tdifftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \\\n+\tgui | gui--askpass | \\\n \thttp-backend | http-fetch | http-push | init-db | \\\n \tmerge-octopus | merge-one-file | merge-resolve | mergetool | \\\n \tmktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \\\n-- \n2.49.0\n\n"},{"id":"524449","messageId":"xmqqect7fhnp.fsf@gitster.g","threadId":"63965","inReplyTo":"20250819074631.3303-1-adam@dinwoodie.org","subject":"Re: [PATCH v2] t/t1517: mark tests that fail with GIT_TEST_INSTALLED","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-19T15:35:54Z","receivedAt":"2025-08-19T15:35:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie <adam@dinwoodie.org> writes:\n\n> The changes added by 39fc408562 (t/t1517: automate `git subcmd -h` tests\n> outside a repository, 2025-08-08) to automatically loop over all \"main\"\n> Git commands will, when run against an installed build using\n> GIT_TEST_INSTALLED rather than the build in the build directory, include\n> some extra git-gui commands that are installed by `make install`, or\n> credential helpers that might be installed manually from the contrib\n> directories.  These fail the test, so record them as such.\n>\n> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n> ---\n>\n> This re-roll adds a few more commands to those marked as known failures,\n> notably credential helpers I see installed in various builds for the\n> Nixpkgs packaging of Git.\n>\n>  t/t1517-outside-repo.sh | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n\nI'd appreciate these efforts, but I am not sure if this is a losing\nbattle.  Your ~/libexec/git-core/ directory, when GIT_TEST_INSTALLED\nis in effect, likely has old commands that are retired, commands\nthat are added by third-parties (so that their users can say \"git\nfrotz\" and run their \"frotz\" software), and/or commands from the\nfuture that the running t1517 has not seen yet (while bisecting and\nrunning t1517 from an older commit, say).  For example, I have these\ndifferences...\n\n\tarchimport.perl\n\t        citool\n\tcvsexportcommit.perl\n\tcvsimport.perl\n\tcvsserver.perl\n\tdifftool--helper.sh\n\tfilter-branch.sh\n\t        gui\n\t        gui--askpass\n\tinstaweb.sh\n\tlast-modified\n\tmerge-octopus.sh\n\tmerge-one-file.sh\n\tmerge-resolve.sh\n\tmergetool.sh\n\tp4.py\n\tquiltimport.sh\n\trequest-pull.sh\n\tsend-email.perl\n\tsubmodule.sh\n\tsvn.perl\n\tweb--browse.sh\n\n... in what t1517 $(git --list-cmds=main) sees between 'master' in\nnormal test mode and with GIT_TEST_INSTALLED set to ~/git/jch/bin\n(i.e. the version I run for my everyday use).  \"last-modified\" is an\nexample of a new-ish command that the t1517 test being run is not\nyet aware of but included in GIT_TEST_INSTALLED.\n\nI am wondering if we are better off skipping this test, or at least\nlimiting to some known subset (e.g. \"git --list-cmds=builtins\") to\nskip the files on disk when GIT_TEST_INSTALLED is in effect, instead\nof \"git --list-cmds=main\" that is quite broad)?\n\nIn any case, this is a strict improvement over the previous one, so\nI'll replace and queue this for now, but we may want to rethink the\napproach this test uses.  Even without GIT_TEST_INSTALLED, the fake\nGIT_EXEC_PATH we use during test has somewhat different from the\nreal thing, I suspect.  \n\nThanks.\n\n\n> diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\n> index 1c69d52c76..c824c1a25c 100755\n> --- a/t/t1517-outside-repo.sh\n> +++ b/t/t1517-outside-repo.sh\n> @@ -111,8 +111,11 @@ for cmd in $(git --list-cmds=main)\n>  do\n>  \tcmd=${cmd%.*} # strip .sh, .perl, etc.\n>  \tcase \"$cmd\" in\n> -\tarchimport | cvsexportcommit | cvsimport | cvsserver | daemon | \\\n> +\tarchimport | citool | credential-netrc | credential-libsecret | \\\n> +\tcredential-osxkeychain | cvsexportcommit | cvsimport | cvsserver | \\\n> +\tdaemon | \\\n>  \tdifftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \\\n> +\tgui | gui--askpass | \\\n>  \thttp-backend | http-fetch | http-push | init-db | \\\n>  \tmerge-octopus | merge-one-file | merge-resolve | mergetool | \\\n>  \tmktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \\\n"},{"id":"524461","messageId":"CA+kUOak76QJXnWhNOmS0W4q9emOtJp7RWO42y7FCLLL4WmsDdw@mail.gmail.com","threadId":"63965","inReplyTo":"xmqqect7fhnp.fsf@gitster.g","subject":"Re: [PATCH v2] t/t1517: mark tests that fail with GIT_TEST_INSTALLED","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2025-08-19T18:21:34Z","receivedAt":"2025-08-19T18:22:13Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Tue, 19 Aug 2025 at 16:35, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Adam Dinwoodie <adam@dinwoodie.org> writes:\n>\n> > The changes added by 39fc408562 (t/t1517: automate `git subcmd -h` tests\n> > outside a repository, 2025-08-08) to automatically loop over all \"main\"\n> > Git commands will, when run against an installed build using\n> > GIT_TEST_INSTALLED rather than the build in the build directory, include\n> > some extra git-gui commands that are installed by `make install`, or\n> > credential helpers that might be installed manually from the contrib\n> > directories.  These fail the test, so record them as such.\n> >\n> > Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>\n> > ---\n> >\n> > This re-roll adds a few more commands to those marked as known failures,\n> > notably credential helpers I see installed in various builds for the\n> > Nixpkgs packaging of Git.\n> >\n> >  t/t1517-outside-repo.sh | 5 ++++-\n> >  1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> I'd appreciate these efforts, but I am not sure if this is a losing\n> battle.  Your ~/libexec/git-core/ directory, when GIT_TEST_INSTALLED\n> is in effect, likely has old commands that are retired, commands\n> that are added by third-parties (so that their users can say \"git\n> frotz\" and run their \"frotz\" software), and/or commands from the\n> future that the running t1517 has not seen yet (while bisecting and\n> running t1517 from an older commit, say).  For example, I have these\n> differences...\n>\n>         archimport.perl\n>                 citool\n>         cvsexportcommit.perl\n>         cvsimport.perl\n>         cvsserver.perl\n>         difftool--helper.sh\n>         filter-branch.sh\n>                 gui\n>                 gui--askpass\n>         instaweb.sh\n>         last-modified\n>         merge-octopus.sh\n>         merge-one-file.sh\n>         merge-resolve.sh\n>         mergetool.sh\n>         p4.py\n>         quiltimport.sh\n>         request-pull.sh\n>         send-email.perl\n>         submodule.sh\n>         svn.perl\n>         web--browse.sh\n>\n> ... in what t1517 $(git --list-cmds=main) sees between 'master' in\n> normal test mode and with GIT_TEST_INSTALLED set to ~/git/jch/bin\n> (i.e. the version I run for my everyday use).  \"last-modified\" is an\n> example of a new-ish command that the t1517 test being run is not\n> yet aware of but included in GIT_TEST_INSTALLED.\n>\n> I am wondering if we are better off skipping this test, or at least\n> limiting to some known subset (e.g. \"git --list-cmds=builtins\") to\n> skip the files on disk when GIT_TEST_INSTALLED is in effect, instead\n> of \"git --list-cmds=main\" that is quite broad)?\n>\n> In any case, this is a strict improvement over the previous one, so\n> I'll replace and queue this for now, but we may want to rethink the\n> approach this test uses.  Even without GIT_TEST_INSTALLED, the fake\n> GIT_EXEC_PATH we use during test has somewhat different from the\n> real thing, I suspect.\n>\n> Thanks.\n\nIf someone's using GIT_TEST_INSTALLED, I think it's reasonable to\nexpect them to keep their install directory fairly clean, so this test\nwould only need to worry about things that might be there because\nthey're included with Git. The cases I've patched for are all ones\nthat are installed as part of the Nixpkgs Git build, which does copy\nin some things from contrib directories, but by design always installs\ninto a new, empty root directory.\n\nNone of which is to disagree with everything you've said. I imagine\nmost people building Git aren't doing it using the Nixpkgs build\nprocesses, so if they're using GIT_TEST_INSTALLED, they're much more\nlikely to be testing in an environment that includes other old scripts\nor tools or whatever.\n\nHaving t1517 know what executables to check without requiring someone\nto remember to update a list is clearly valuable, but it seems that\nlist should _somehow_ be built based on what's in the Git repository\nat build time, not what's in the user's environment.\n--list-cmds=builtins seems like it's more limited than ideal, but it\nmight be a better approach than the current one. This is a balancing\nact I'll leave to people who are much more involved in the project\ndevelopment!\n"}]}