threads / patch / 63965

patcht/t1517: mark tests that fail with GIT_TEST_INSTALLED

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

## tl;dr

5 messages between Aug 16, 2025 and Aug 19, 2025. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Adam Dinwoodie· Aug 16, 2025, 10:36 UTC · lore

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(-)
Show changes to t/t1517-outside-repo.sh +2 −1
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
Usman Akinyemi· Aug 17, 2025, 06:42 UTC · re: Adam Dinwoodie · lore

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

> +       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
>
Adam Dinwoodie· Aug 19, 2025, 07:43 UTC · re: Adam Dinwoodie · lore

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

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(-)
Show changes to t/t1517-outside-repo.sh +4 −1
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
Junio C Hamano· Aug 19, 2025, 15:35 UTC · re: Adam Dinwoodie · lore

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

Adam Dinwoodie <adam@dinwoodie.org> writes:
Show 17 quoted lines
> 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.
Show 17 quoted lines
> 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 | \
Adam Dinwoodie· Aug 19, 2025, 18:21 UTC · re: Junio C Hamano · lore

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

On Tue, 19 Aug 2025 at 16:35, Junio C Hamano <gitster@pobox.com> wrote:
Show 71 quoted lines
>
> 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!

← back to recent threads