[PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED

[PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED

[Adam Dinwoodie]

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

Re: [PATCH] t/t1517: mark tests that fail with GIT_TEST_INSTALLED

[Usman Akinyemi]

> +       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
>

[PATCH v2] t/t1517: mark tests that fail with GIT_TEST_INSTALLED

[Adam Dinwoodie]

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

Re: [PATCH v2] t/t1517: mark tests that fail with GIT_TEST_INSTALLED

[Junio C Hamano]

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 | \

Re: [PATCH v2] t/t1517: mark tests that fail with GIT_TEST_INSTALLED

[Adam Dinwoodie]

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!