[PATCH 0/4] Drop git-exec-path from non-Git child programs

[PATCH 0/4] Drop git-exec-path from non-Git child programs

[D. Ben Knoble]

This has caused trouble in the past [1] [2]; after attempting to help
Git-for-Windows avoid that problem in Vim [3] by recommendation [4], it
was suggested that upstreaming the change instead would be a better
solution. Indeed, this should work for more uses/editors/etc.

[1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/
[2]: https://benknoble.github.io/blog/2020/05/22/libexec-git-core-on-path/
[3]: https://github.com/git-for-windows/build-extra/pull/616
[4]: https://github.com/benknoble/Dotfiles/issues/143#issuecomment-2869525481

I haven't managed to test this on Windows, so any extra eyeballs there
are greatly appreciated. I'd also appreciate suggestions to fix the
memory leak.

Structure: patches 1 & 2 are cleanups. In particular, patch 1 is essential to
the tests in patch 4. I don't think patch 2 is strictly needed. Patch 3
refactors a little to make patch 4 take effect on all platforms. Patch 4 does
the real work.

D. Ben Knoble (4):
  t7005: sanitize test environment for subsequent tests
  editor: use standard strvec API to receive environment for external
    editors
  run-command: prep_childenv on all platforms
  drop git_exec_path() from non-Git commands' PATH

 builtin/commit.c  |  2 +-
 editor.c          | 10 ++++-----
 editor.h          |  7 +++---
 run-command.c     | 55 ++++++++++++++++++++++++++++++++++++++++++-----
 t/t7005-editor.sh | 18 +++++++++++++---
 5 files changed, 75 insertions(+), 17 deletions(-)


base-commit: 7a1d2bd0a596f42a8a7a68d55577967bb454fec0
-- 
2.48.1

[PATCH 1/4] t7005: sanitize test environment for subsequent tests

[D. Ben Knoble]

Some of the editor tests manipulate the environment or config in ways
that affect future tests (because they test a sequence of overrides),
but those modifications are visible to future tests and create a footgun
for them. Use test_config and undo environment modifications once
finished.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 5fcf281dfb..06fa1ecd91 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -111,6 +111,8 @@
 	'
 done
 
+unset EDITOR VISUAL GIT_EDITOR
+git config --unset-all core.editor
 test_expect_success 'editor with a space' '
 	echo "echo space >\"\$1\"" >"e space.sh" &&
 	chmod a+x "e space.sh" &&
@@ -119,13 +121,10 @@
 
 '
 
-unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-
-	git config core.editor \"./e\ space.sh\" &&
+	test_config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
 	test space = "$(git show -s --pretty=format:%s)"
-
 '
 
 test_done
-- 
2.48.1

Re: [PATCH 1/4] t7005: sanitize test environment for subsequent tests

[Patrick Steinhardt]

On Tue, May 20, 2025 at 03:34:55PM -0400, D. Ben Knoble wrote:
> Some of the editor tests manipulate the environment or config in ways
> that affect future tests (because they test a sequence of overrides),
> but those modifications are visible to future tests and create a footgun
> for them. Use test_config and undo environment modifications once
> finished.
> 
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
>  t/t7005-editor.sh | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> index 5fcf281dfb..06fa1ecd91 100755
> --- a/t/t7005-editor.sh
> +++ b/t/t7005-editor.sh
> @@ -111,6 +111,8 @@
>  	'
>  done
>  
> +unset EDITOR VISUAL GIT_EDITOR
> +git config --unset-all core.editor
>  test_expect_success 'editor with a space' '
>  	echo "echo space >\"\$1\"" >"e space.sh" &&
>  	chmod a+x "e space.sh" &&

Could we maybe adapt the tests that set those envvars to use a
`test_when_finished`? Something like this (untested) patch:

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 5fcf281dfbf..a14ff4b38c4 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -93,17 +93,19 @@ unset EDITOR VISUAL GIT_EDITOR
 git config --unset-all core.editor
 for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
-	echo "Edited by $i" >expect
-	case "$i" in
-	core_editor)
-		git config core.editor ./e-core_editor.sh
-		;;
-	[A-Z]*)
-		eval "$i=./e-$i.sh"
-		export $i
-		;;
-	esac
 	test_expect_success "Using $i (override)" '
+		echo "Edited by $i" >expect &&
+		case "$i" in
+		core_editor)
+			test_config core.editor ./e-core_editor.sh
+			;;
+		[A-Z]*)
+			test_when_finished "unset $i" &&
+			eval "$i=./e-$i.sh" &&
+			export $i
+			;;
+		esac &&
+
 		git --exec-path=. commit --amend &&
 		git show -s --pretty=oneline |
 		sed -e "s/^[0-9a-f]* //" >actual &&


> @@ -119,13 +121,10 @@
>  
>  '
>  
> -unset GIT_EDITOR
>  test_expect_success 'core.editor with a space' '
> -
> -	git config core.editor \"./e\ space.sh\" &&
> +	test_config core.editor \"./e\ space.sh\" &&
>  	git commit --amend &&
>  	test space = "$(git show -s --pretty=format:%s)"
> -
>  '

This hunk looks good to me.

Patrick

Re: [PATCH 1/4] t7005: sanitize test environment for subsequent tests

[D. Ben Knoble]

On Wed, May 21, 2025 at 3:56 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, May 20, 2025 at 03:34:55PM -0400, D. Ben Knoble wrote:
> > Some of the editor tests manipulate the environment or config in ways
> > that affect future tests (because they test a sequence of overrides),
> > but those modifications are visible to future tests and create a footgun
> > for them. Use test_config and undo environment modifications once
> > finished.
> >
> > Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> > ---
> >  t/t7005-editor.sh | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> > index 5fcf281dfb..06fa1ecd91 100755
> > --- a/t/t7005-editor.sh
> > +++ b/t/t7005-editor.sh
> > @@ -111,6 +111,8 @@
> >       '
> >  done
> >
> > +unset EDITOR VISUAL GIT_EDITOR
> > +git config --unset-all core.editor
> >  test_expect_success 'editor with a space' '
> >       echo "echo space >\"\$1\"" >"e space.sh" &&
> >       chmod a+x "e space.sh" &&
>
> Could we maybe adapt the tests that set those envvars to use a
> `test_when_finished`? Something like this (untested) patch:
>
> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> index 5fcf281dfbf..a14ff4b38c4 100755
> --- a/t/t7005-editor.sh
> +++ b/t/t7005-editor.sh
> @@ -93,17 +93,19 @@ unset EDITOR VISUAL GIT_EDITOR
>  git config --unset-all core.editor
>  for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
>  do
> -       echo "Edited by $i" >expect
> -       case "$i" in
> -       core_editor)
> -               git config core.editor ./e-core_editor.sh
> -               ;;
> -       [A-Z]*)
> -               eval "$i=./e-$i.sh"
> -               export $i
> -               ;;
> -       esac
>         test_expect_success "Using $i (override)" '
> +               echo "Edited by $i" >expect &&
> +               case "$i" in
> +               core_editor)
> +                       test_config core.editor ./e-core_editor.sh
> +                       ;;
> +               [A-Z]*)
> +                       test_when_finished "unset $i" &&
> +                       eval "$i=./e-$i.sh" &&
> +                       export $i
> +                       ;;
> +               esac &&
> +
>                 git --exec-path=. commit --amend &&
>                 git show -s --pretty=oneline |
>                 sed -e "s/^[0-9a-f]* //" >actual &&

Thanks for the demo; I considered pushing the test inside the loop,
but I assumed (perhaps wrongly?) that the `export` needed to be
_outside_ the test script in order to correctly test the sequence of
overrides.

Thoughts?

>
>
> > @@ -119,13 +121,10 @@
> >
> >  '
> >
> > -unset GIT_EDITOR
> >  test_expect_success 'core.editor with a space' '
> > -
> > -     git config core.editor \"./e\ space.sh\" &&
> > +     test_config core.editor \"./e\ space.sh\" &&
> >       git commit --amend &&
> >       test space = "$(git show -s --pretty=format:%s)"
> > -
> >  '
>
> This hunk looks good to me.
>
> Patrick

[PATCH 2/4] editor: use standard strvec API to receive environment for external editors

[D. Ben Knoble]

Going back to the introduction of the env parameter for the editor in
8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as
well., 2007-11-26), we pass a constant array of strings: as the
surrounding APIs evolved to use strvecs, the editor code did not.

There is only one caller of all 3 editor APIs that does not pass a NULL
environment (the same caller for which this parameter was added), and
it already has a strvec available to use.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/commit.c |  2 +-
 editor.c         | 10 +++++-----
 editor.h         |  7 ++++---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 66bd91fd52..fdda1d1df0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1100,7 +1100,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct strvec env = STRVEC_INIT;
 
 		strvec_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
-		if (launch_editor(git_path_commit_editmsg(), NULL, env.v)) {
+		if (launch_editor(git_path_commit_editmsg(), NULL, &env)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
diff --git a/editor.c b/editor.c
index b79d97b0e7..5bc9d39178 100644
--- a/editor.c
+++ b/editor.c
@@ -58,7 +58,7 @@ const char *git_sequence_editor(void)
 }
 
 static int launch_specified_editor(const char *editor, const char *path,
-				   struct strbuf *buffer, const char *const *env)
+				   struct strbuf *buffer, const struct strvec *env)
 {
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
@@ -89,7 +89,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 
 		strvec_pushl(&p.args, editor, realpath.buf, NULL);
 		if (env)
-			strvec_pushv(&p.env, (const char **)env);
+			strvec_pushv(&p.env, env->v);
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
 		if (start_command(&p) < 0) {
@@ -124,20 +124,20 @@ static int launch_specified_editor(const char *editor, const char *path,
 	return 0;
 }
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+int launch_editor(const char *path, struct strbuf *buffer, const struct strvec *env)
 {
 	return launch_specified_editor(git_editor(), path, buffer, env);
 }
 
 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env)
+			   const struct strvec *env)
 {
 	return launch_specified_editor(git_sequence_editor(), path, buffer, env);
 }
 
 int strbuf_edit_interactively(struct repository *r,
 			      struct strbuf *buffer, const char *path,
-			      const char *const *env)
+			      const struct strvec *env)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int fd, res = 0;
diff --git a/editor.h b/editor.h
index f1c41df378..627e992f4d 100644
--- a/editor.h
+++ b/editor.h
@@ -3,6 +3,7 @@
 
 struct repository;
 struct strbuf;
+struct strvec;
 
 const char *git_editor(void);
 const char *git_sequence_editor(void);
@@ -16,10 +17,10 @@ int is_terminal_dumb(void);
  * file's contents are not read into the buffer upon completion.
  */
 int launch_editor(const char *path, struct strbuf *buffer,
-		  const char *const *env);
+		  const struct strvec *env);
 
 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env);
+			   const struct strvec *env);
 
 /*
  * In contrast to `launch_editor()`, this function writes out the contents
@@ -30,6 +31,6 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,
  * If `path` is relative, it refers to a file in the `.git` directory.
  */
 int strbuf_edit_interactively(struct repository *r, struct strbuf *buffer,
-			      const char *path, const char *const *env);
+			      const char *path, const struct strvec *env);
 
 #endif
-- 
2.48.1

Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors

[Patrick Steinhardt]

On Tue, May 20, 2025 at 03:34:56PM -0400, D. Ben Knoble wrote:
> Going back to the introduction of the env parameter for the editor in
> 8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as
> well., 2007-11-26), we pass a constant array of strings: as the
> surrounding APIs evolved to use strvecs, the editor code did not.
> 
> There is only one caller of all 3 editor APIs that does not pass a NULL
> environment (the same caller for which this parameter was added), and
> it already has a strvec available to use.

Okay. It would've been nice to explain _why_ we want to do this change,
but the change itself looks sensible.

> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>

The order of these trailers should be reversed -- your SOB should always
come last.

Patrick

Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors

[D. Ben Knoble]

On Wed, May 21, 2025 at 3:56 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, May 20, 2025 at 03:34:56PM -0400, D. Ben Knoble wrote:
> > Going back to the introduction of the env parameter for the editor in
> > 8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as
> > well., 2007-11-26), we pass a constant array of strings: as the
> > surrounding APIs evolved to use strvecs, the editor code did not.
> >
> > There is only one caller of all 3 editor APIs that does not pass a NULL
> > environment (the same caller for which this parameter was added), and
> > it already has a strvec available to use.
>
> Okay. It would've been nice to explain _why_ we want to do this change,
> but the change itself looks sensible.

Ah, yes. I think it's really just "cleanup" here—it started out as
"maybe necessary prep for later work," and I kept it in this series.
Will expand slightly in reroll.

>
> > Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> > Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The order of these trailers should be reversed -- your SOB should always
> come last.

Thanks; I didn't know that! (Aside: rebase --signoff seems to add SOB
even when it's already present. Is that a bug in rebase --signoff or a
misuse of the trailer on my end? Setting "trailer.ifExists =
addIfDifferent" didn't seem to affect it.)

>
> Patrick

Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors

[Patrick Steinhardt]

On Wed, May 21, 2025 at 09:26:50AM -0400, D. Ben Knoble wrote:
> On Wed, May 21, 2025 at 3:56 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> > > Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The order of these trailers should be reversed -- your SOB should always
> > come last.
> 
> Thanks; I didn't know that! (Aside: rebase --signoff seems to add SOB
> even when it's already present. Is that a bug in rebase --signoff or a
> misuse of the trailer on my end? Setting "trailer.ifExists =
> addIfDifferent" didn't seem to affect it.)

git-rebase(1) assumes that your SOB comes last, so that's probably why
it re-adds it :)

Patrick

Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

> Thanks; I didn't know that! (Aside: rebase --signoff seems to add SOB
> even when it's already present. Is that a bug in rebase --signoff or a
> misuse of the trailer on my end? Setting "trailer.ifExists =
> addIfDifferent" didn't seem to affect it.)

Why do "rebase --signoff" in the first place?  Unless you forgot to,
that is.  "I do not remember if I did, so I blindly add it" is
something we do not want to see, as we want to keep signing-off a
concious act.

Anyway, as the intent of the trailer is to record what happened
until the patch was finalized and sent out with your sign-off
chronologically, if the order of events were

 - You wrote the patch, gave it to somebody else with your sign-off,
   to show that it is shared under DCO (a).

 - Somebody else may make modification, share it with their
   sign-off under DCO (b).

 - You find that their version is a good one, and with or without
   further change of yours, you sign-off to show that you are
   sharing this final version under DCO, either (b) or (c).

it is perfectly fine for your sign-off to appear twice.

The only case Git's built-in sign-off logic omits adding a sign-off
is when the same sign-off is sitting at the end.  After the above
event, if you took that "final" version and then gave it to somebody
else, with or without further changes, that is still covered by the
last sign-off you made, so there is no point adding a duplicate.

Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
>> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The order of these trailers should be reversed -- your SOB should always
> come last.

Yup.  The intent is to record what happened before the patch was
sent out with your sign-off chronologically.  Dscho helped and then
Ben finialized the patch by signing off, hence the author's sign-off
comes the last.

Thanks.

[PATCH 3/4] run-command: prep_childenv on all platforms

[D. Ben Knoble]

We only prepare the child environment on non-Windows platforms, but
prep_childenv is the natural interposition point for our subprocess
system to adjust the environment as needed. Use it for Windows
platforms, also. In subsequent commits we'll use this interposition
point to modify the environment on all platforms.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 run-command.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index 8833b23367..dee6ae3e62 100644
--- a/run-command.c
+++ b/run-command.c
@@ -680,6 +680,7 @@ int start_command(struct child_process *cmd)
 	int fdin[2], fdout[2], fderr[2];
 	int failed_errno;
 	const char *str;
+	char **childenv;
 
 	/*
 	 * In case of errors we must keep the promise to close FDs
@@ -745,11 +746,12 @@ int start_command(struct child_process *cmd)
 	if (cmd->close_object_store)
 		close_object_store(the_repository->objects);
 
+	childenv = prep_childenv(cmd->env.v);
+
 #ifndef GIT_WINDOWS_NATIVE
 {
 	int notify_pipe[2];
 	int null_fd = -1;
-	char **childenv;
 	struct strvec argv = STRVEC_INIT;
 	struct child_err cerr;
 	struct atfork_state as;
@@ -772,7 +774,6 @@ int start_command(struct child_process *cmd)
 		set_cloexec(null_fd);
 	}
 
-	childenv = prep_childenv(cmd->env.v);
 	atfork_prepare(&as);
 
 	/*
@@ -893,7 +894,6 @@ int start_command(struct child_process *cmd)
 	if (null_fd >= 0)
 		close(null_fd);
 	strvec_clear(&argv);
-	free(childenv);
 }
 end_of_spawn:
 
@@ -933,7 +933,7 @@ int start_command(struct child_process *cmd)
 
 	trace_argv_printf(cmd->args.v, "trace: start_command:");
 	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,
-				  (char**) cmd->env.v,
+				  childenv,
 				  cmd->dir, fhin, fhout, fherr);
 	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
@@ -952,6 +952,8 @@ int start_command(struct child_process *cmd)
 }
 #endif
 
+	free(childenv);
+
 	if (cmd->pid < 0) {
 		trace2_child_exit(cmd, -1);
 
-- 
2.48.1

Re: [PATCH 3/4] run-command: prep_childenv on all platforms

[Patrick Steinhardt]

On Tue, May 20, 2025 at 03:34:57PM -0400, D. Ben Knoble wrote:
> We only prepare the child environment on non-Windows platforms, but
> prep_childenv is the natural interposition point for our subprocess
> system to adjust the environment as needed. Use it for Windows
> platforms, also. In subsequent commits we'll use this interposition
> point to modify the environment on all platforms.

What is the consequence though of calling `prep_childenv()` on Windows
now? Why didn't we call it before this change? Details like this should
definitely go into the commit message to explain why it's safe to add
the call now.

Patrick

Re: [PATCH 3/4] run-command: prep_childenv on all platforms

[Phillip Wood]

On 21/05/2025 08:56, Patrick Steinhardt wrote:
> On Tue, May 20, 2025 at 03:34:57PM -0400, D. Ben Knoble wrote:
>> We only prepare the child environment on non-Windows platforms, but
>> prep_childenv is the natural interposition point for our subprocess
>> system to adjust the environment as needed. Use it for Windows
>> platforms, also. In subsequent commits we'll use this interposition
>> point to modify the environment on all platforms.
> 
> What is the consequence though of calling `prep_childenv()` on Windows
> now? Why didn't we call it before this change? Details like this should
> definitely go into the commit message to explain why it's safe to add
> the call now.

The environment prep for windows is currently implemented in 
compat/mingw.c:make_environment_block(). That function is careful to 
handle unicode characters correctly, it is hard to see how that is 
compatible this change.

Best Wishes

Phillip

Re: [PATCH 3/4] run-command: prep_childenv on all platforms

[D. Ben Knoble]

On Wed, May 21, 2025 at 9:07 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 21/05/2025 08:56, Patrick Steinhardt wrote:
> > On Tue, May 20, 2025 at 03:34:57PM -0400, D. Ben Knoble wrote:
> >> We only prepare the child environment on non-Windows platforms, but
> >> prep_childenv is the natural interposition point for our subprocess
> >> system to adjust the environment as needed. Use it for Windows
> >> platforms, also. In subsequent commits we'll use this interposition
> >> point to modify the environment on all platforms.
> >
> > What is the consequence though of calling `prep_childenv()` on Windows
> > now? Why didn't we call it before this change? Details like this should
> > definitely go into the commit message to explain why it's safe to add
> > the call now.
>
> The environment prep for windows is currently implemented in
> compat/mingw.c:make_environment_block(). That function is careful to
> handle unicode characters correctly, it is hard to see how that is
> compatible this change.
>
> Best Wishes
>
> Phillip

Ah, thanks both—I missed that it got special handling later (since
run-command throws the char **env over to mingw_spawnvpe without
modification). This patch is probably "obviously wrong" with that
context.

That makes me wonder: per Junio's comments in another spot, we could
be more targeted at the launch_editor family (which also requires a
separate patch to contrib/git-jump to scratch my itch); or, we could
try to push these modifications into both environment handling blocks.
I lack the knowledge needed to do the right thing on the Windows side,
though, so I'd probably leave it alone.

Thoughts?

[PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[D. Ben Knoble]

We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify
setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git
starts; as a result, all child processes see Git's exec-path when run,
including editors and other programs that don't need it [1]. This can
cause confusion for such programs or shells, especially when they rely
on finding "git" in PATH to locate other nearby directories, in a
similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of
PATH, 2015-04-22) solved.

Since we only need this for finding git-* subprocesses, drop it from
child processes that aren't Git commands.

[1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

Notes:
    A few interesting points:
    
    - I'm not sure how best to deal with the memory leak here.
    - Dscho suggested the essence of the patch in
      https://github.com/git-for-windows/build-extra/pull/616#pullrequestreview-2839055049,
      for the curious. My only major tweaks were this diff to skip past
      "PATH=" when searching for the matching path (but still modify the
      original buffer; b always points into buf.buf, so the later operations
      with p and buf.buf are valid).
    
    --->8---
    diff --git i/run-command.c w/run-command.c
    index b567e4fdd5..8a8b5c8455 100644
    --- i/run-command.c
    +++ w/run-command.c
    @@ -452,17 +452,24 @@ static void remove_git_exec_path(struct string_list_item *path_item) {
    	struct strbuf buf = STRBUF_INIT;
    	const char *exec_path = git_exec_path();
    	size_t exec_len = strlen(exec_path);
    -	char *p;
    +	char *b, *p;
    
    	/* Value comes from environ; we should not modify it directly. But
    	 * strbuf copies data, so we now have our own playground. */
    	strbuf_addstr(&buf, (const char *)path_item->util);
    -	for (p = strstr(buf.buf, exec_path); p; p = strstr(p, exec_path)) {
    -		if ((p[exec_len] && p[exec_len] != PATH_SEP) || (p != buf.buf && p[-1] != PATH_SEP))
    +
    +	/* skip past "PATH=" to start search */
    +	p = strchr(buf.buf, '=');
    +	if (!p || !*(p + 1))
    +		return;
    +	b = p + 1;
    +
    +	for (p = strstr(b, exec_path); p; p = strstr(p, exec_path)) {
    +		if ((p[exec_len] && p[exec_len] != PATH_SEP) || (p != b && p[-1] != PATH_SEP))
    			p += exec_len; /* false positive, skip */
    		else {
    			size_t offset = p - buf.buf, delete_len = exec_len;
    -			if (p != buf.buf) {
    +			if (p != b) {
    				/* include the preceding path separator */
    				offset--;
    				delete_len++;
    --->8---
    
    - I /think/ this resolves the issues in my earlier mail beyond just Git
      builtins; for example, git-jump also doesn't get exec-path because
      it's not invoked with git_cmd set during execv_dashed_external.

 run-command.c     | 47 +++++++++++++++++++++++++++++++++++++++++++++--
 t/t7005-editor.sh | 13 +++++++++++++
 2 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/run-command.c b/run-command.c
index dee6ae3e62..8a8b5c8455 100644
--- a/run-command.c
+++ b/run-command.c
@@ -448,11 +448,51 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
 	return 0;
 }
 
-static char **prep_childenv(const char *const *deltaenv)
+static void remove_git_exec_path(struct string_list_item *path_item) {
+	struct strbuf buf = STRBUF_INIT;
+	const char *exec_path = git_exec_path();
+	size_t exec_len = strlen(exec_path);
+	char *b, *p;
+
+	/* Value comes from environ; we should not modify it directly. But
+	 * strbuf copies data, so we now have our own playground. */
+	strbuf_addstr(&buf, (const char *)path_item->util);
+
+	/* skip past "PATH=" to start search */
+	p = strchr(buf.buf, '=');
+	if (!p || !*(p + 1))
+		return;
+	b = p + 1;
+
+	for (p = strstr(b, exec_path); p; p = strstr(p, exec_path)) {
+		if ((p[exec_len] && p[exec_len] != PATH_SEP) || (p != b && p[-1] != PATH_SEP))
+			p += exec_len; /* false positive, skip */
+		else {
+			size_t offset = p - buf.buf, delete_len = exec_len;
+			if (p != b) {
+				/* include the preceding path separator */
+				offset--;
+				delete_len++;
+			} else if (p[exec_len] == PATH_SEP) {
+				/* include the path separator following GIT_EXEC_PATH */
+				delete_len++;
+			}
+			strbuf_splice(&buf, offset, delete_len, "", 0);
+		}
+	}
+
+	/* Overwrite PATH value with new (owned) data. This leaks memory because
+	 * the only future owner is a char** childenv, which is freed, but whose
+	 * contents are not (because most of them come from environ). */
+	path_item->util = (void *)strbuf_detach(&buf, NULL);
+}
+
+static char **prep_childenv(const char *const *deltaenv, unsigned git_cmd)
 {
 	extern char **environ;
 	char **childenv;
 	struct string_list env = STRING_LIST_INIT_DUP;
+	struct string_list_item *path_item;
 	struct strbuf key = STRBUF_INIT;
 	const char *const *p;
 	int i;
@@ -486,6 +526,9 @@ static char **prep_childenv(const char *const *deltaenv)
 		}
 	}
 
+	if (!git_cmd && (path_item = string_list_lookup(&env, "PATH")))
+		remove_git_exec_path(path_item);
+
 	/* Create an array of 'char *' to be used as the childenv */
 	ALLOC_ARRAY(childenv, env.nr + 1);
 	for (i = 0; i < env.nr; i++)
@@ -746,7 +789,7 @@ int start_command(struct child_process *cmd)
 	if (cmd->close_object_store)
 		close_object_store(the_repository->objects);
 
-	childenv = prep_childenv(cmd->env.v);
+	childenv = prep_childenv(cmd->env.v, cmd->git_cmd);
 
 #ifndef GIT_WINDOWS_NATIVE
 {
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 06fa1ecd91..560e500b53 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -127,4 +127,17 @@
 	test space = "$(git show -s --pretty=format:%s)"
 '
 
+test_expect_success 'editor does not see GIT_EXEC_PATH on PATH' '
+	cat >e-path <<-EOF &&
+	#!$SHELL_PATH
+	echo "\$PATH" | tr : "\\n" >actual
+	EOF
+	chmod +x e-path &&
+	(
+		test_set_editor ./e-path &&
+		git commit --amend
+	) &&
+	test_grep ! ^"$(git --exec-path)"\$ actual
+'
+
 test_done
-- 
2.48.1

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

> We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify
> setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git
> starts; as a result, all child processes see Git's exec-path when run,
> including editors and other programs that don't need it [1]. This can
> cause confusion for such programs or shells, especially when they rely
> on finding "git" in PATH to locate other nearby directories, in a
> similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of
> PATH, 2015-04-22) solved.

I can understand why we want to do this for things like editors, but
doing this in start_command() would affect running of things like
hook scripts (which may in turn invoke "git" commands) and command
running on the other side of the pipe that does a local transport
(e.g., "git fetch --upload-pack cmd"), and other commands specified
via the configuration (e.g., "trailer.<key>.cmd") that may in turn
invoke "git" commands, wouldn't it?

> @@ -746,7 +789,7 @@ int start_command(struct child_process *cmd)
>  	if (cmd->close_object_store)
>  		close_object_store(the_repository->objects);
>  
> -	childenv = prep_childenv(cmd->env.v);
> +	childenv = prep_childenv(cmd->env.v, cmd->git_cmd);

Regardless of the answer to the above question, I wonder if the API
change for this helper function should go the other direction, by
passing the entire "child_process" structure to the function.  That
would give prep_childenv() more information to work with to decide
when futzing with PATH is appropriate.  

Yes, I am assuming that the answer to the above question is "true,
unconditionally futzing with PATH is a bad idea" and we need this
fix to be more focused, e.g., limit to "launch_specified_editor()".
If that is the case, we can

 - add a member "drop_git_exec_path" bit to "struct child_process";

 - update selected callers, e.g., launch_specified_editor(), that
   prepare child_process to set that bit;

 - update prep_childenv() to accept the entire "struct child_process",
   instead of cmd->env.v, and base the decision in your logic to
   look at the cmd->drop_git_exec_path bit;

perhaps.  It would give us a more surgical and focused update,
hopefully?

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[D. Ben Knoble]

Adding Patrick to CC

On Tue, May 20, 2025 at 4:33 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
>
> > We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify
> > setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git
> > starts; as a result, all child processes see Git's exec-path when run,
> > including editors and other programs that don't need it [1]. This can
> > cause confusion for such programs or shells, especially when they rely
> > on finding "git" in PATH to locate other nearby directories, in a
> > similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of
> > PATH, 2015-04-22) solved.
>
> I can understand why we want to do this for things like editors, but
> doing this in start_command() would affect running of things like
> hook scripts (which may in turn invoke "git" commands) and command
> running on the other side of the pipe that does a local transport
> (e.g., "git fetch --upload-pack cmd"), and other commands specified
> via the configuration (e.g., "trailer.<key>.cmd") that may in turn
> invoke "git" commands, wouldn't it?

It would affect those things, yes, but I'm not sure what /effect/ that
has: doesn't adding GIT_EXEC_PATH to PATH only matter if we use the
dashed forms ("git-*" in my original message), which is long
deprecated?

Patrick pointed out in a parallel thread that it probably matters for
". git-sh-setup", but that manual explicitly describes using

    . "$(git --exec-path)/git-sh-setup"

So I would say the callers that don't do that are wrong ;)

>
> > @@ -746,7 +789,7 @@ int start_command(struct child_process *cmd)
> >       if (cmd->close_object_store)
> >               close_object_store(the_repository->objects);
> >
> > -     childenv = prep_childenv(cmd->env.v);
> > +     childenv = prep_childenv(cmd->env.v, cmd->git_cmd);
>
> Regardless of the answer to the above question, I wonder if the API
> change for this helper function should go the other direction, by
> passing the entire "child_process" structure to the function.  That
> would give prep_childenv() more information to work with to decide
> when futzing with PATH is appropriate.
>
> Yes, I am assuming that the answer to the above question is "true,
> unconditionally futzing with PATH is a bad idea" and we need this
> fix to be more focused, e.g., limit to "launch_specified_editor()".
> If that is the case, we can
>
>  - add a member "drop_git_exec_path" bit to "struct child_process";
>
>  - update selected callers, e.g., launch_specified_editor(), that
>    prepare child_process to set that bit;
>
>  - update prep_childenv() to accept the entire "struct child_process",
>    instead of cmd->env.v, and base the decision in your logic to
>    look at the cmd->drop_git_exec_path bit;
>
> perhaps.  It would give us a more surgical and focused update,
> hopefully?

Indeed, a more surgical approach is what Dscho originally suggested
(IIUC). Failing CI suggests there may be more that relies on
GIT_EXEC_PATH in PATH than I realized, so I can post an alternative
series that should only affect editors (requiring an extra patch for
contrib/git-jump, since it invokes an editor), but it is likely to be
"incomplete" in some sense:
- no Windows support (see replies to 3/4)
- won't affect all editor invocations, since those launched by a
script after retrieving "git var GIT_EDITOR" won't necessarily know to
rollback PATH.

I need to reroll anyway, so I think what I'll do is post a v2 of this
series along with a v3 that refocuses on editors? That way we have
both around for interested folks in the future.

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[Patrick Steinhardt]

On Tue, May 20, 2025 at 03:34:58PM -0400, D. Ben Knoble wrote:
> We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify
> setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git
> starts; as a result, all child processes see Git's exec-path when run,
> including editors and other programs that don't need it [1]. This can
> cause confusion for such programs or shells, especially when they rely
> on finding "git" in PATH to locate other nearby directories, in a
> similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of
> PATH, 2015-04-22) solved.
> 
> Since we only need this for finding git-* subprocesses, drop it from
> child processes that aren't Git commands.

I agree with what Junio mentioned in a parallel thread, especially
around Git hooks. The expectation there is that those may execute other
Git commands, and that should typically be using the same execution
environment as the original Git command that has been invoking the hook.
So refining this patch so that the mechanism is opt-in probably makes
sense.

    A slight tangent: I wonder whether it is even required nowadays to
    adapt PATH at all anymore. As far as I understand this was a
    requirement back when people still executed dashed binaries
    directly. But nowadays scripts don't really do that anymore, but
    instead use the git binary. And that one doesn't need PATH to be
    adapted at all, as it knows to listen to GIT_EXEC_PATH and its
    built-in path anyway.

    So I have to wonder whether this is something that we can deprecate
    to finalize the migration away from dashed builtins. There probably
    are edge cases though. Shell scripts for example still execute ".
    git-sh-setup", which I think relies on PATH to resolve the script's
    location.

    In any case, this is of course outside of the scope of this patch
    series.

> [1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/
> 
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Same comment regarding the order of trailers.

> diff --git a/run-command.c b/run-command.c
> index dee6ae3e62..8a8b5c8455 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -448,11 +448,51 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
>  	return 0;
>  }
>  
> -static char **prep_childenv(const char *const *deltaenv)
> +static void remove_git_exec_path(struct string_list_item *path_item) {

Style: the curly brace should be on its own line.

> +	struct strbuf buf = STRBUF_INIT;
> +	const char *exec_path = git_exec_path();
> +	size_t exec_len = strlen(exec_path);
> +	char *b, *p;
> +
> +	/* Value comes from environ; we should not modify it directly. But
> +	 * strbuf copies data, so we now have our own playground. */

Style:

    /*
     * Multi-line comments should be written like this, with their
     * opening and closing parts on their own lines.
     */

> +	strbuf_addstr(&buf, (const char *)path_item->util);
> +
> +	/* skip past "PATH=" to start search */
> +	p = strchr(buf.buf, '=');
> +	if (!p || !*(p + 1))
> +		return;
> +	b = p + 1;

You could use `if (!skip_prefix(buf.buf, "PATH=", &p))` instead.

> +
> +	for (p = strstr(b, exec_path); p; p = strstr(p, exec_path)) {
> +		if ((p[exec_len] && p[exec_len] != PATH_SEP) || (p != b && p[-1] != PATH_SEP))
> +			p += exec_len; /* false positive, skip */
> +		else {
> +			size_t offset = p - buf.buf, delete_len = exec_len;
> +			if (p != b) {
> +				/* include the preceding path separator */
> +				offset--;
> +				delete_len++;
> +			} else if (p[exec_len] == PATH_SEP) {
> +				/* include the path separator following GIT_EXEC_PATH */
> +				delete_len++;
> +			}
> +			strbuf_splice(&buf, offset, delete_len, "", 0);
> +		}
> +	}
> +
> +	/* Overwrite PATH value with new (owned) data. This leaks memory because
> +	 * the only future owner is a char** childenv, which is freed, but whose
> +	 * contents are not (because most of them come from environ). */
> +	path_item->util = (void *)strbuf_detach(&buf, NULL);

Same comment here regarding style of the comment.

Regarding the memory leak... you could probably track allocations in a
separate array and free them via that array. Another alternative would
be to just copy the whole environment in case we need to modify it. I
doubt that the performance hit would matter given that the cost to spawn
the new process is likely to significantly outweigh the additional
memory allocations.

Patrick

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[Phillip Wood]

On 21/05/2025 09:27, Patrick Steinhardt wrote:
> On Tue, May 20, 2025 at 03:34:58PM -0400, D. Ben Knoble wrote:
>> We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify
>> setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git
>> starts; as a result, all child processes see Git's exec-path when run,
>> including editors and other programs that don't need it [1]. This can
>> cause confusion for such programs or shells, especially when they rely
>> on finding "git" in PATH to locate other nearby directories,

I'm skeptical that using the location of the git executable to find 
"git-jump" and "git-completion.zsh" is a good idea. The location of 
those (and whether they are packaged at all) is entirely at the 
discretion of whoever packages git - they are not installed by the 
Makefile and so there is no fixed relationship between their location in 
the filesystem and the git executable. For instance the example in Ben's 
blog post looks for "git-jump" at 
"$prefix/share/git-core/contrib/git-jump" but on my system it is located 
at "$prefix/share/git/contrib/git-jump"

> I agree with what Junio mentioned in a parallel thread, especially
> around Git hooks. The expectation there is that those may execute other
> Git commands, and that should typically be using the same execution
> environment as the original Git command that has been invoking the hook.
> So refining this patch so that the mechanism is opt-in probably makes
> sense.
> 
>      A slight tangent: I wonder whether it is even required nowadays to
>      adapt PATH at all anymore. As far as I understand this was a
>      requirement back when people still executed dashed binaries
>      directly. But nowadays scripts don't really do that anymore, but
>      instead use the git binary. And that one doesn't need PATH to be
>      adapted at all, as it knows to listen to GIT_EXEC_PATH and its
>      built-in path anyway.

But don't we still need to change PATH so that hooks, shell aliases, git 
rebase --exec, git bisect run, etc. still run the same git executable 
that started them? For example "/usr/bin/git -c alias.g=!git g 
--version" should report the version of /usr/bin/git, not 
~/.local/bin/git which comes first in my PATH if git doesn't change it.

Best Wishes

Phillip

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[Patrick Steinhardt]

On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:
> On 21/05/2025 09:27, Patrick Steinhardt wrote:
> > I agree with what Junio mentioned in a parallel thread, especially
> > around Git hooks. The expectation there is that those may execute other
> > Git commands, and that should typically be using the same execution
> > environment as the original Git command that has been invoking the hook.
> > So refining this patch so that the mechanism is opt-in probably makes
> > sense.
> > 
> >      A slight tangent: I wonder whether it is even required nowadays to
> >      adapt PATH at all anymore. As far as I understand this was a
> >      requirement back when people still executed dashed binaries
> >      directly. But nowadays scripts don't really do that anymore, but
> >      instead use the git binary. And that one doesn't need PATH to be
> >      adapted at all, as it knows to listen to GIT_EXEC_PATH and its
> >      built-in path anyway.
> 
> But don't we still need to change PATH so that hooks, shell aliases, git
> rebase --exec, git bisect run, etc. still run the same git executable that
> started them? For example "/usr/bin/git -c alias.g=!git g --version" should
> report the version of /usr/bin/git, not ~/.local/bin/git which comes first
> in my PATH if git doesn't change it.

There's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust
PATH indeed to contain the location of the 'git' executable. But we also
add GIT_EXEC_PATH to it, which I'm less sure whether it's actually
needed.

Patrick

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[Phillip Wood]

On 21/05/2025 14:17, Patrick Steinhardt wrote:
> On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:
>>
>> But don't we still need to change PATH so that hooks, shell aliases, git
>> rebase --exec, git bisect run, etc. still run the same git executable that
>> started them? For example "/usr/bin/git -c alias.g=!git g --version" should
>> report the version of /usr/bin/git, not ~/.local/bin/git which comes first
>> in my PATH if git doesn't change it.
> 
> There's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust
> PATH indeed to contain the location of the 'git' executable. But we also
> add GIT_EXEC_PATH to it, which I'm less sure whether it's actually
> needed.

If we were to add /usr/bin to beginning of PATH when the user runs 
/usr/bin/git I think that would be more surprising than adding 
GIT_EXEC_PATH which is what we do now. When we add GIT_EXEC_PATH to the 
beginning of PATH we only affect the lookup of git's programs. If 
instead we added /usr/bin to the beginning that would affect the lookup 
of many unrelated programs.

Best Wishes

Phillip

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[Patrick Steinhardt]

On Wed, May 21, 2025 at 04:27:48PM +0100, Phillip Wood wrote:
> On 21/05/2025 14:17, Patrick Steinhardt wrote:
> > On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:
> > > 
> > > But don't we still need to change PATH so that hooks, shell aliases, git
> > > rebase --exec, git bisect run, etc. still run the same git executable that
> > > started them? For example "/usr/bin/git -c alias.g=!git g --version" should
> > > report the version of /usr/bin/git, not ~/.local/bin/git which comes first
> > > in my PATH if git doesn't change it.
> > 
> > There's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust
> > PATH indeed to contain the location of the 'git' executable. But we also
> > add GIT_EXEC_PATH to it, which I'm less sure whether it's actually
> > needed.
> 
> If we were to add /usr/bin to beginning of PATH when the user runs
> /usr/bin/git I think that would be more surprising than adding GIT_EXEC_PATH
> which is what we do now. When we add GIT_EXEC_PATH to the beginning of PATH
> we only affect the lookup of git's programs. If instead we added /usr/bin to
> the beginning that would affect the lookup of many unrelated programs.

That's a valid point indeed.

Patrick

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[Justin Tobler]

On 25/05/21 03:17PM, Patrick Steinhardt wrote:
> On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:
> > On 21/05/2025 09:27, Patrick Steinhardt wrote:
> > > I agree with what Junio mentioned in a parallel thread, especially
> > > around Git hooks. The expectation there is that those may execute other
> > > Git commands, and that should typically be using the same execution
> > > environment as the original Git command that has been invoking the hook.
> > > So refining this patch so that the mechanism is opt-in probably makes
> > > sense.
> > > 
> > >      A slight tangent: I wonder whether it is even required nowadays to
> > >      adapt PATH at all anymore. As far as I understand this was a
> > >      requirement back when people still executed dashed binaries
> > >      directly. But nowadays scripts don't really do that anymore, but
> > >      instead use the git binary. And that one doesn't need PATH to be
> > >      adapted at all, as it knows to listen to GIT_EXEC_PATH and its
> > >      built-in path anyway.
> > 
> > But don't we still need to change PATH so that hooks, shell aliases, git
> > rebase --exec, git bisect run, etc. still run the same git executable that
> > started them? For example "/usr/bin/git -c alias.g=!git g --version" should
> > report the version of /usr/bin/git, not ~/.local/bin/git which comes first
> > in my PATH if git doesn't change it.
> 
> There's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust
> PATH indeed to contain the location of the 'git' executable. But we also
> add GIT_EXEC_PATH to it, which I'm less sure whether it's actually
> needed.

In instances where GIT_EXEC_PATH is set on the parent process to
override the default, wouldn't we also want this configuration to
propagate to child Git processes?

-Justin

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[Patrick Steinhardt]

On Wed, May 21, 2025 at 10:50:03AM -0500, Justin Tobler wrote:
> On 25/05/21 03:17PM, Patrick Steinhardt wrote:
> > On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:
> > > On 21/05/2025 09:27, Patrick Steinhardt wrote:
> > > > I agree with what Junio mentioned in a parallel thread, especially
> > > > around Git hooks. The expectation there is that those may execute other
> > > > Git commands, and that should typically be using the same execution
> > > > environment as the original Git command that has been invoking the hook.
> > > > So refining this patch so that the mechanism is opt-in probably makes
> > > > sense.
> > > > 
> > > >      A slight tangent: I wonder whether it is even required nowadays to
> > > >      adapt PATH at all anymore. As far as I understand this was a
> > > >      requirement back when people still executed dashed binaries
> > > >      directly. But nowadays scripts don't really do that anymore, but
> > > >      instead use the git binary. And that one doesn't need PATH to be
> > > >      adapted at all, as it knows to listen to GIT_EXEC_PATH and its
> > > >      built-in path anyway.
> > > 
> > > But don't we still need to change PATH so that hooks, shell aliases, git
> > > rebase --exec, git bisect run, etc. still run the same git executable that
> > > started them? For example "/usr/bin/git -c alias.g=!git g --version" should
> > > report the version of /usr/bin/git, not ~/.local/bin/git which comes first
> > > in my PATH if git doesn't change it.
> > 
> > There's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust
> > PATH indeed to contain the location of the 'git' executable. But we also
> > add GIT_EXEC_PATH to it, which I'm less sure whether it's actually
> > needed.
> 
> In instances where GIT_EXEC_PATH is set on the parent process to
> override the default, wouldn't we also want this configuration to
> propagate to child Git processes?

Yes, but what we should be propagating is GIT_EXEC_PATH, not
GIT_EXEC_PATH inserted into PATH, shouldn't we?

Patrick

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[D. Ben Knoble]

On Wed, May 21, 2025 at 4:27 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, May 20, 2025 at 03:34:58PM -0400, D. Ben Knoble wrote:
> > We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify
> > setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git
> > starts; as a result, all child processes see Git's exec-path when run,
> > including editors and other programs that don't need it [1]. This can
> > cause confusion for such programs or shells, especially when they rely
> > on finding "git" in PATH to locate other nearby directories, in a
> > similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of
> > PATH, 2015-04-22) solved.
> >
> > Since we only need this for finding git-* subprocesses, drop it from
> > child processes that aren't Git commands.
>
> I agree with what Junio mentioned in a parallel thread, especially
> around Git hooks. The expectation there is that those may execute other
> Git commands, and that should typically be using the same execution
> environment as the original Git command that has been invoking the hook.
> So refining this patch so that the mechanism is opt-in probably makes
> sense.

Ok, will do in a reroll. Though if you have time… see my message in
that thread about only affecting uses of "git-*" dashed commands.

>     A slight tangent: I wonder whether it is even required nowadays to
>     adapt PATH at all anymore. As far as I understand this was a
>     requirement back when people still executed dashed binaries
>     directly. But nowadays scripts don't really do that anymore, but
>     instead use the git binary. And that one doesn't need PATH to be
>     adapted at all, as it knows to listen to GIT_EXEC_PATH and its
>     built-in path anyway.

Good point. I haven't tried running the tests with such a change, and
GitHub CI at least reports a number of failures on this branch
(whether that's because 3/4 is wrong for Windows or not, I'm not sure,
but I suspect it's actually because 4/4 violates some assumptions).

>     So I have to wonder whether this is something that we can deprecate
>     to finalize the migration away from dashed builtins. There probably
>     are edge cases though. Shell scripts for example still execute ".
>     git-sh-setup", which I think relies on PATH to resolve the script's
>     location.

Those scripts are, at least according to the manual for git-sh-setup, wrong ;)

>     In any case, this is of course outside of the scope of this patch
>     series.

Noted; I'll leave it for later to tug that thread.

[snip: style and leak discussion]

Acked

Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I agree with what Junio mentioned in a parallel thread, especially
> around Git hooks. The expectation there is that those may execute other
> Git commands, and that should typically be using the same execution
> environment as the original Git command that has been invoking the hook.

If by "Giving the same execution environment as the original" you
meant "not just git-subcmd from anywhere, but from the same place",
you are right.

The point of GIT_EXEC_PATH is to allow users to tell 'git' that when
they say 'git foo', they mean the specific version of 'foo' found at
$GIT_EXEC_PATH/git-foo, not at $(git --exec-path)/git-foo that may
be different; otherwise they wouldn't be passing GIT_EXEC_PATH in
the first place.

In other words, "These days nobody runs git-foo anymore" may be a
correct statement, but as an argument to draw the conclusion "hence
we should ignore GIT_EXEC_PATH", it is an argument that entirely
misses the point.

> So refining this patch so that the mechanism is opt-in probably makes
> sense.
>
>     A slight tangent: I wonder whether it is even required nowadays to
>     adapt PATH at all anymore.

Meaning we include GIT_EXEC_PATH to PATH?  When a running "git"
invokes a subcommand "foo" that is not built-in, it runs "git-foo"
but doesn't it do so by following the PATH (with the expectation
that GIT_EXEC_PATH has already been taken into account, with
setup_path() in exec-cmd.c, to include it in PATH)?

So, as long as you subscribe to "the same execution environment"
like you said above, I do not think you can avoid the setup_path()
call early in git.c::cmd_main().

Re: [PATCH 0/4] Drop git-exec-path from non-Git child programs

[Phillip Wood]

Hi Ben

On 20/05/2025 20:34, D. Ben Knoble wrote:
> This has caused trouble in the past [1] [2];

Another way of looking at this is that the trouble is caused by a script 
that makes incorrect assumptions about git.

The assumption that the scripts from contrib as installed at a fixed 
location relative to the git binary is false. Where they are installed 
and whether they are installed at all is down to the discretion of the 
distribution that you're using. Looking for "git jump" at a fixed offset 
from the git binary is no more portable than looking for it in a fixed 
location.

I think that the assumption that git should not change the environment 
when it runs the editor is unrealistic. "git commit file" will use a 
temporary index to create the commit and sets GIT_INDEX_FILE when 
running the editor. This means that if the editor wants to display the 
staged changes by running "git diff --cached HEAD" the diff will 
accurately represent the changes being committed. Adding GIT_EXEC_PATH 
to the beginning of PATH ensures that the diff will be created by the 
same version of git the the user ran which avoids subtle bugs where a 
sub-process of git runs a git command using an incompatible version of 
git. There are several other environment variables that may be set when 
running the editor such as GIT_DIR if the command is run from a linked 
wortree.

To create a clean environment when opening a terminal from your editor 
you can add

     PATH="${PATH#$GIT_EXEC_PATH:}"
     unset $(git rev-parse --local-env-vars)

to your shell setup script.

I think the first two patches are very welcome cleanups but I'm not 
convinced by the rationale for patches 3 & 4.

Best Wishes

Phillip

> after attempting to help
> Git-for-Windows avoid that problem in Vim [3] by recommendation [4], it
> was suggested that upstreaming the change instead would be a better
> solution. Indeed, this should work for more uses/editors/etc.
> 
> [1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/
> [2]: https://benknoble.github.io/blog/2020/05/22/libexec-git-core-on-path/
> [3]: https://github.com/git-for-windows/build-extra/pull/616
> [4]: https://github.com/benknoble/Dotfiles/issues/143#issuecomment-2869525481
> 
> I haven't managed to test this on Windows, so any extra eyeballs there
> are greatly appreciated. I'd also appreciate suggestions to fix the
> memory leak.
> 
> Structure: patches 1 & 2 are cleanups. In particular, patch 1 is essential to
> the tests in patch 4. I don't think patch 2 is strictly needed. Patch 3
> refactors a little to make patch 4 take effect on all platforms. Patch 4 does
> the real work.
> 
> D. Ben Knoble (4):
>    t7005: sanitize test environment for subsequent tests
>    editor: use standard strvec API to receive environment for external
>      editors
>    run-command: prep_childenv on all platforms
>    drop git_exec_path() from non-Git commands' PATH
> 
>   builtin/commit.c  |  2 +-
>   editor.c          | 10 ++++-----
>   editor.h          |  7 +++---
>   run-command.c     | 55 ++++++++++++++++++++++++++++++++++++++++++-----
>   t/t7005-editor.sh | 18 +++++++++++++---
>   5 files changed, 75 insertions(+), 17 deletions(-)
> 
> 
> base-commit: 7a1d2bd0a596f42a8a7a68d55577967bb454fec0

Re: [PATCH 0/4] Drop git-exec-path from non-Git child programs

[D. Ben Knoble]

On Thu, May 22, 2025 at 9:21 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Ben
>
> On 20/05/2025 20:34, D. Ben Knoble wrote:
> > This has caused trouble in the past [1] [2];
>
> Another way of looking at this is that the trouble is caused by a script
> that makes incorrect assumptions about git.

I've come around on this idea since fixing my setup to not assume
contrib scripts are installed relative to Git (rather, I've hardcoded
assumptions about where they get installed by my package manager).

>
> The assumption that the scripts from contrib as installed at a fixed
> location relative to the git binary is false. Where they are installed
> and whether they are installed at all is down to the discretion of the
> distribution that you're using. Looking for "git jump" at a fixed offset
> from the git binary is no more portable than looking for it in a fixed
> location.
>
> I think that the assumption that git should not change the environment
> when it runs the editor is unrealistic. "git commit file" will use a
> temporary index to create the commit and sets GIT_INDEX_FILE when
> running the editor. This means that if the editor wants to display the
> staged changes by running "git diff --cached HEAD" the diff will
> accurately represent the changes being committed. Adding GIT_EXEC_PATH
> to the beginning of PATH ensures that the diff will be created by the
> same version of git the the user ran which avoids subtle bugs where a
> sub-process of git runs a git command using an incompatible version of
> git. There are several other environment variables that may be set when
> running the editor such as GIT_DIR if the command is run from a linked
> wortree.
>
> To create a clean environment when opening a terminal from your editor
> you can add
>
>      PATH="${PATH#$GIT_EXEC_PATH:}"
>      unset $(git rev-parse --local-env-vars)
>
> to your shell setup script.
>
> I think the first two patches are very welcome cleanups but I'm not
> convinced by the rationale for patches 3 & 4.

So I'll resend the first 2 cleanups and drop the rest.

[PATCH 0/2] clean up some code around editors

[D. Ben Knoble]

This "v2" of the previous exec-path series is simplified to contain only
the first 2 cleanup patches, which were largely acked by the list. Drop
the controversial and broken PATH munging.

Also, this version is based on a later master 112648dd6b (Merge branch
'master' of https://github.com/j6t/git-gui, 2025-08-04) than the
original from May.

These patches clean up some old code in the editor tests and subsystem
that does not use our modern idioms. Patrick previously argued the test
cleanup doesn't go far enough, and he may be right, but I think the
preserving the semantics of the test for overrides /and/ automatically
resetting the environment is tricky, unless we can use a subshell for
the whole thing?

v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/
Published-as: https://github.com/benknoble/tree/editor-cleanup

D. Ben Knoble (2):
  t7005: sanitize test environment for subsequent tests
  editor: use standard strvec API to receive environment for external
    editors

 builtin/commit.c  |  2 +-
 editor.c          | 10 +++++-----
 editor.h          |  7 ++++---
 t/t7005-editor.sh |  7 +++----
 4 files changed, 13 insertions(+), 13 deletions(-)

Diff-intervalle :
1:  da4fcc237b ! 1:  a37db65107 t7005: sanitize test environment for subsequent tests
    @@ Commit message
         Some of the editor tests manipulate the environment or config in ways
         that affect future tests (because they test a sequence of overrides),
         but those modifications are visible to future tests and create a footgun
    -    for them. Use test_config and undo environment modifications once
    -    finished.
    +    for them.
    +
    +    We can't make the environment-munging override tests undo their
    +    modifications because they rely on editor variables overriding other
    +    previously-set editor variables.
    +
    +    Use test_config and undo environment modifications once finished.
     
         Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
     
2:  7b3b6b08f0 ! 2:  5450c99f59 editor: use standard strvec API to receive environment for external editors
    @@ Commit message
         Going back to the introduction of the env parameter for the editor in
         8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as
         well., 2007-11-26), we pass a constant array of strings: as the
    -    surrounding APIs evolved to use strvecs, the editor code did not.
    +    surrounding APIs evolved to use strvecs (see 8d7aa4ba6a
    +    (builtin/commit.c: remove the PATH_MAX limitation via dynamic
    +    allocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',
    +    2020-08-10)), the editor code did not.
     
         There is only one caller of all 3 editor APIs that does not pass a NULL
         environment (the same caller for which this parameter was added), and
         it already has a strvec available to use.
     
    -    Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
         Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
     
      ## builtin/commit.c ##
     @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,
3:  cb48533115 < -:  ---------- run-command: prep_childenv on all platforms
4:  d2e54fdf75 < -:  ---------- drop git_exec_path() from non-Git commands' PATH

base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be
-- 
2.48.1

Re: [PATCH 0/2] clean up some code around editors

[Phillip Wood]

Hi Ben

On 05/08/2025 03:40, D. Ben Knoble wrote:
> 
> These patches clean up some old code in the editor tests and subsystem
> that does not use our modern idioms. Patrick previously argued the test
> cleanup doesn't go far enough, and he may be right, but I think the
> preserving the semantics of the test for overrides /and/ automatically
> resetting the environment is tricky, unless we can use a subshell for
> the whole thing?

I think we could do it if we put the loop inside a subshell that was 
inside test_expect_success(). That would mean we'd have a single test 
for all the various combinations. That script could certainly use more 
cleanup to stop running git outside of test_expect_success() and use 
things like test_must_fail(), write_script(), test_commit_message() etc. 
but I think that can always come later. The first patch here is a 
welcome improvement even if it does not fix all the issues in that test 
file. The second patch looks good too.

Thanks for re-submitting these

Phillip

> v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/
> Published-as: https://github.com/benknoble/tree/editor-cleanup
> 
> D. Ben Knoble (2):
>    t7005: sanitize test environment for subsequent tests
>    editor: use standard strvec API to receive environment for external
>      editors
> 
>   builtin/commit.c  |  2 +-
>   editor.c          | 10 +++++-----
>   editor.h          |  7 ++++---
>   t/t7005-editor.sh |  7 +++----
>   4 files changed, 13 insertions(+), 13 deletions(-)
> 
> Diff-intervalle :
> 1:  da4fcc237b ! 1:  a37db65107 t7005: sanitize test environment for subsequent tests
>      @@ Commit message
>           Some of the editor tests manipulate the environment or config in ways
>           that affect future tests (because they test a sequence of overrides),
>           but those modifications are visible to future tests and create a footgun
>      -    for them. Use test_config and undo environment modifications once
>      -    finished.
>      +    for them.
>      +
>      +    We can't make the environment-munging override tests undo their
>      +    modifications because they rely on editor variables overriding other
>      +    previously-set editor variables.
>      +
>      +    Use test_config and undo environment modifications once finished.
>       
>           Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
>       
> 2:  7b3b6b08f0 ! 2:  5450c99f59 editor: use standard strvec API to receive environment for external editors
>      @@ Commit message
>           Going back to the introduction of the env parameter for the editor in
>           8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as
>           well., 2007-11-26), we pass a constant array of strings: as the
>      -    surrounding APIs evolved to use strvecs, the editor code did not.
>      +    surrounding APIs evolved to use strvecs (see 8d7aa4ba6a
>      +    (builtin/commit.c: remove the PATH_MAX limitation via dynamic
>      +    allocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',
>      +    2020-08-10)), the editor code did not.
>       
>           There is only one caller of all 3 editor APIs that does not pass a NULL
>           environment (the same caller for which this parameter was added), and
>           it already has a strvec available to use.
>       
>      -    Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
>           Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>      +    Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
>       
>        ## builtin/commit.c ##
>       @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,
> 3:  cb48533115 < -:  ---------- run-command: prep_childenv on all platforms
> 4:  d2e54fdf75 < -:  ---------- drop git_exec_path() from non-Git commands' PATH
> 
> base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be

[PATCH 1/2] t7005: sanitize test environment for subsequent tests

[D. Ben Knoble]

Some of the editor tests manipulate the environment or config in ways
that affect future tests (because they test a sequence of overrides),
but those modifications are visible to future tests and create a footgun
for them.

We can't make the environment-munging override tests undo their
modifications because they rely on editor variables overriding other
previously-set editor variables.

Use test_config and undo environment modifications once finished.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 5fcf281dfb..06fa1ecd91 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -111,6 +111,8 @@
 	'
 done
 
+unset EDITOR VISUAL GIT_EDITOR
+git config --unset-all core.editor
 test_expect_success 'editor with a space' '
 	echo "echo space >\"\$1\"" >"e space.sh" &&
 	chmod a+x "e space.sh" &&
@@ -119,13 +121,10 @@
 
 '
 
-unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-
-	git config core.editor \"./e\ space.sh\" &&
+	test_config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
 	test space = "$(git show -s --pretty=format:%s)"
-
 '
 
 test_done
-- 
2.48.1

[PATCH 2/2] editor: use standard strvec API to receive environment for external editors

[D. Ben Knoble]

Going back to the introduction of the env parameter for the editor in
8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as
well., 2007-11-26), we pass a constant array of strings: as the
surrounding APIs evolved to use strvecs (see 8d7aa4ba6a
(builtin/commit.c: remove the PATH_MAX limitation via dynamic
allocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',
2020-08-10)), the editor code did not.

There is only one caller of all 3 editor APIs that does not pass a NULL
environment (the same caller for which this parameter was added), and
it already has a strvec available to use.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 builtin/commit.c |  2 +-
 editor.c         | 10 +++++-----
 editor.h         |  7 ++++---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index b5b9608813..16cad7fb03 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1116,7 +1116,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct strvec env = STRVEC_INIT;
 
 		strvec_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
-		if (launch_editor(git_path_commit_editmsg(), NULL, env.v)) {
+		if (launch_editor(git_path_commit_editmsg(), NULL, &env)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
diff --git a/editor.c b/editor.c
index fd174e6a03..0bc781d50c 100644
--- a/editor.c
+++ b/editor.c
@@ -58,7 +58,7 @@ const char *git_sequence_editor(void)
 }
 
 static int launch_specified_editor(const char *editor, const char *path,
-				   struct strbuf *buffer, const char *const *env)
+				   struct strbuf *buffer, const struct strvec *env)
 {
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
@@ -89,7 +89,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 
 		strvec_pushl(&p.args, editor, realpath.buf, NULL);
 		if (env)
-			strvec_pushv(&p.env, (const char **)env);
+			strvec_pushv(&p.env, env->v);
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
 		if (start_command(&p) < 0) {
@@ -124,20 +124,20 @@ static int launch_specified_editor(const char *editor, const char *path,
 	return 0;
 }
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+int launch_editor(const char *path, struct strbuf *buffer, const struct strvec *env)
 {
 	return launch_specified_editor(git_editor(), path, buffer, env);
 }
 
 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env)
+			   const struct strvec *env)
 {
 	return launch_specified_editor(git_sequence_editor(), path, buffer, env);
 }
 
 int strbuf_edit_interactively(struct repository *r,
 			      struct strbuf *buffer, const char *path,
-			      const char *const *env)
+			      const struct strvec *env)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int fd, res = 0;
diff --git a/editor.h b/editor.h
index f1c41df378..627e992f4d 100644
--- a/editor.h
+++ b/editor.h
@@ -3,6 +3,7 @@
 
 struct repository;
 struct strbuf;
+struct strvec;
 
 const char *git_editor(void);
 const char *git_sequence_editor(void);
@@ -16,10 +17,10 @@ int is_terminal_dumb(void);
  * file's contents are not read into the buffer upon completion.
  */
 int launch_editor(const char *path, struct strbuf *buffer,
-		  const char *const *env);
+		  const struct strvec *env);
 
 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env);
+			   const struct strvec *env);
 
 /*
  * In contrast to `launch_editor()`, this function writes out the contents
@@ -30,6 +31,6 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,
  * If `path` is relative, it refers to a file in the `.git` directory.
  */
 int strbuf_edit_interactively(struct repository *r, struct strbuf *buffer,
-			      const char *path, const char *const *env);
+			      const char *path, const struct strvec *env);
 
 #endif
-- 
2.48.1

[PATCH 0/3] clean up some code around editors

[D. Ben Knoble]

Changes from v1:
- add a prep patch with style fixes to t7005
- rework the environment munging to use subshells, per Phillip Wood's
  suggestion

This reroll of the previous exec-path series is simplified to contain
only the first 2 cleanup patches, which were largely acked by the list.
Drop the controversial and broken PATH munging.

Also, this version is (still) based on a later master 112648dd6b (Merge
branch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the
original from May.

These patches clean up some old code in the editor tests and subsystem
that does not use our modern idioms.

v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/
Published-as: https://github.com/benknoble/tree/editor-cleanup

D. Ben Knoble (3):
  t7005: use modern test style
  t7005: sanitize test environment for subsequent tests
  editor: use standard strvec API to receive environment for external
    editors

 builtin/commit.c  |   2 +-
 editor.c          |  10 +--
 editor.h          |   7 ++-
 t/t7005-editor.sh | 152 ++++++++++++++++++++++++----------------------
 4 files changed, 89 insertions(+), 82 deletions(-)

Diff-intervalle :
-:  ---------- > 1:  e7629202a1 t7005: use modern test style
1:  a37db65107 ! 2:  0467e33ed0 t7005: sanitize test environment for subsequent tests
    @@ Commit message
         t7005: sanitize test environment for subsequent tests
     
         Some of the editor tests manipulate the environment or config in ways
    -    that affect future tests (because they test a sequence of overrides),
    -    but those modifications are visible to future tests and create a footgun
    -    for them.
    +    that affect future tests, but those modifications are visible to future
    +    tests and create a footgun for them.
     
    -    We can't make the environment-munging override tests undo their
    -    modifications because they rely on editor variables overriding other
    -    previously-set editor variables.
    -
    -    Use test_config and undo environment modifications once finished.
    +    Use test_config, subshells, and test helpers to automatically undo
    +    environment and config modifications once finished.
     
    +    Best-viewed-with: --ignore-all-space
         Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
     
      ## t/t7005-editor.sh ##
     @@
    - 	'
    - done
    + 	test_cmp expect actual
    + '
    + 
    +-TERM=dumb
    +-export TERM
    + test_expect_success 'dumb should error out when falling back on vi' '
    +-	if git commit --amend
    +-	then
    +-		echo "Oops?"
    +-		false
    +-	else
    +-		: happy
    +-	fi
    ++	(
    ++		TERM=dumb &&
    ++		export TERM &&
    ++		if git commit --amend
    ++		then
    ++			echo "Oops?"
    ++			false
    ++		else
    ++			: happy
    ++		fi
    ++	)
    + '
    + 
    + test_expect_success 'dumb should prefer EDITOR to VISUAL' '
    +-	EDITOR=./e-EDITOR.sh &&
    +-	VISUAL=./e-VISUAL.sh &&
    +-	export EDITOR VISUAL &&
    +-	git commit --amend &&
    +-	echo "Edited by EDITOR" >expect &&
    +-	git show -s --format=%s >actual &&
    ++	(
    ++		TERM=dumb &&
    ++		export TERM &&
    ++		EDITOR=./e-EDITOR.sh &&
    ++		VISUAL=./e-VISUAL.sh &&
    ++		export EDITOR VISUAL &&
    ++		git commit --amend &&
    ++		echo "Edited by EDITOR" >expect &&
    ++		git show -s --format=%s >actual
    ++	) &&
    + 	test_cmp expect actual
    + '
    + 
    +-TERM=vt100
    +-export TERM
    +-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
    +-do
    +-	echo "Edited by $i" >expect
    +-	unset EDITOR VISUAL GIT_EDITOR
    +-	git config --unset-all core.editor
    +-	case "$i" in
    +-	core_editor)
    +-		git config core.editor ./e-core_editor.sh
    +-		;;
    +-	[A-Z]*)
    +-		eval "$i=./e-$i.sh"
    +-		export $i
    +-		;;
    +-	esac
    +-	test_expect_success "Using $i" '
    +-		git --exec-path=. commit --amend &&
    +-		git show -s --pretty=oneline >show &&
    +-		<show sed -e "s/^[0-9a-f]* //" >actual &&
    +-		test_cmp expect actual
    +-	'
    +-done
    ++test_expect_success 'Using individual editors' '
    ++	test_when_finished "test_unconfig --unset-all core.editor" &&
    ++	(
    ++		TERM=vt100 &&
    ++		export TERM &&
    ++		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
    ++		do
    ++			sane_unset EDITOR VISUAL GIT_EDITOR &&
    ++			test_might_fail git config --unset-all core.editor &&
    ++			echo "Edited by $i" >expect &&
    ++			case "$i" in
    ++			core_editor)
    ++				git config core.editor ./e-core_editor.sh
    ++				;;
    ++			[A-Z]*)
    ++				eval "$i=./e-$i.sh" &&
    ++				export $i
    ++				;;
    ++			esac &&
    ++			git --exec-path=. commit --amend &&
    ++			git show -s --pretty=oneline >show &&
    ++			<show sed -e "s/^[0-9a-f]* //" >actual &&
    ++			test_cmp expect actual
    ++		done
    ++	)
    ++'
    + 
    +-unset EDITOR VISUAL GIT_EDITOR
    +-git config --unset-all core.editor
    +-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
    +-do
    +-	echo "Edited by $i" >expect
    +-	case "$i" in
    +-	core_editor)
    +-		git config core.editor ./e-core_editor.sh
    +-		;;
    +-	[A-Z]*)
    +-		eval "$i=./e-$i.sh"
    +-		export $i
    +-		;;
    +-	esac
    +-	test_expect_success "Using $i (override)" '
    +-		git --exec-path=. commit --amend &&
    +-		git show -s --pretty=oneline >show &&
    +-		<show sed -e "s/^[0-9a-f]* //" >actual &&
    +-		test_cmp expect actual
    +-	'
    +-done
    ++test_expect_success 'Using editors with overrides' '
    ++	(
    ++		TERM=vt100 &&
    ++		export TERM &&
    ++		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
    ++		do
    ++			echo "Edited by $i" >expect &&
    ++			case "$i" in
    ++			core_editor)
    ++				git config core.editor ./e-core_editor.sh
    ++				;;
    ++			[A-Z]*)
    ++				eval "$i=./e-$i.sh" &&
    ++				export $i
    ++				;;
    ++			esac &&
    ++			git --exec-path=. commit --amend &&
    ++			git show -s --pretty=oneline >show &&
    ++			<show sed -e "s/^[0-9a-f]* //" >actual &&
    ++			test_cmp expect actual
    ++		done
    ++	)
    ++'
      
    -+unset EDITOR VISUAL GIT_EDITOR
    -+git config --unset-all core.editor
      test_expect_success 'editor with a space' '
      	echo "echo space >\"\$1\"" >"e space.sh" &&
    - 	chmod a+x "e space.sh" &&
     @@
    - 
    + 	test_cmp expect actual
      '
      
     -unset GIT_EDITOR
      test_expect_success 'core.editor with a space' '
    --
     -	git config core.editor \"./e\ space.sh\" &&
     +	test_config core.editor \"./e\ space.sh\" &&
      	git commit --amend &&
    - 	test space = "$(git show -s --pretty=format:%s)"
    --
    - '
    - 
    - test_done
    + 	echo space >expect &&
    + 	git show -s --pretty=tformat:%s >actual &&
2:  5450c99f59 = 3:  4a9bb52470 editor: use standard strvec API to receive environment for external editors

base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be
-- 
2.48.1

Re: [PATCH 0/3] clean up some code around editors

[D. Ben Knoble]

On Sun, Aug 10, 2025 at 12:03 PM D. Ben Knoble
<ben.knoble+github@gmail.com> wrote:
>
> Changes from v1:
> - add a prep patch with style fixes to t7005
> - rework the environment munging to use subshells, per Phillip Wood's
>   suggestion

Apologies; this should have had the "v2" subject.

Re: [PATCH 0/3] clean up some code around editors

[Ben Knoble]

> Le 10 août 2025 à 12:06, D. Ben Knoble <ben.knoble+github@gmail.com> a écrit :
> 
> On Sun, Aug 10, 2025 at 12:03 PM D. Ben Knoble
> <ben.knoble+github@gmail.com> wrote:
>> 
>> Changes from v1:
>> - add a prep patch with style fixes to t7005
>> - rework the environment munging to use subshells, per Phillip Wood's
>>  suggestion
> 
> Apologies; this should have had the "v2" subject.

GitHub CI caught an issue from my rebase, so I’ll send a proper v2 later. 

[PATCH v3 0/4] clean up some code around editors

[D. Ben Knoble]

Changes from v2:
- shuffle setup code and use more helpers in 1/4
- insert 2/4 to stop abusing --exec-path
- improve environment-cleansing idioms in {2 => 3}/4

Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)

Changes from v1:
- add a prep patch with style fixes to t7005
- rework the environment munging to use subshells, per Phillip Wood's
  suggestion

This reroll of the previous exec-path series is simplified to contain
only the first 2 cleanup patches, which were largely acked by the list.
Drop the controversial and broken PATH munging.

Also, this version is (still) based on a later master 112648dd6b (Merge
branch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the
original from May.

These patches clean up some old code in the editor tests and subsystem
that does not use our modern idioms.

v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/
v2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/
Published-as: https://github.com/benknoble/tree/editor-cleanup

D. Ben Knoble (4):
  t7005: use modern test style
  t7005: stop abusing --exec-path
  t7005: sanitize test environment for subsequent tests
  editor: use standard strvec API to receive environment for external
    editors

 builtin/commit.c  |   2 +-
 editor.c          |  10 ++--
 editor.h          |   7 ++-
 t/t7005-editor.sh | 147 +++++++++++++++++++---------------------------
 4 files changed, 69 insertions(+), 97 deletions(-)

Diff-intervalle contre v2 :
1:  e7629202a1 ! 1:  8ad2904a18 t7005: use modern test style
    @@ Metadata
      ## Commit message ##
         t7005: use modern test style
     
    -    Tests in t7005 mask Git error codes and do not use our nice helpers for
    -    comparing results. Improve that, and drop a few old-style blank lines
    -    while at it.
    +    Tests in t7005 mask Git error codes and do not use our nice test
    +    helpers. Improve that, move some code into the setup test, and drop a
    +    few old-style blank lines while at it.
     
    +    Best-viewed-with: --ignore-all-space
         Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
     
      ## t/t7005-editor.sh ##
    @@ t/t7005-editor.sh
     -
      '
      
    - if ! expr "$vi" : '[a-z]*$' >/dev/null
    -@@
    - fi
    - 
    - test_expect_success setup '
    +-if ! expr "$vi" : '[a-z]*$' >/dev/null
    +-then
    +-	vi=
    +-fi
     -
    +-for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
    +-do
    +-	cat >e-$i.sh <<-EOF
    +-	#!$SHELL_PATH
    +-	echo "Edited by $i" >"\$1"
    +-	EOF
    +-	chmod +x e-$i.sh
    +-done
    +-
    +-if ! test -z "$vi"
    +-then
    +-	mv e-$vi.sh $vi
    +-fi
    +-
    + test_expect_success setup '
    ++	if ! expr "$vi" : "[a-z]*$" >/dev/null
    ++	then
    ++		vi=
    ++	fi &&
    ++
    ++	for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
    ++	do
    ++		write_script e-$i.sh <<-EOF || return 1
    ++			echo "Edited by $i" >"\$1"
    ++		EOF
    ++	done &&
    ++
    ++	if ! test -z "$vi"
    ++	then
    ++		mv e-$vi.sh $vi
    ++	fi &&
    + 
      	msg="Hand-edited" &&
      	test_commit "$msg" &&
    - 	echo "$msg" >expect &&
    +-	echo "$msg" >expect &&
     -	git show -s --format=%s > actual &&
    -+	git show -s --format=%s >actual &&
    - 	test_cmp expect actual
    +-	test_cmp expect actual
     -
    ++	test_commit_message HEAD -m "$msg"
      '
      
      TERM=dumb
      export TERM
      test_expect_success 'dumb should error out when falling back on vi' '
     -
    - 	if git commit --amend
    - 	then
    - 		echo "Oops?"
    -@@
    +-	if git commit --amend
    +-	then
    +-		echo "Oops?"
    +-		false
    +-	else
    +-		: happy
    +-	fi
    ++	test_must_fail git commit --amend
      '
      
      test_expect_success 'dumb should prefer EDITOR to VISUAL' '
    @@ t/t7005-editor.sh
      	git commit --amend &&
     -	test "$(git show -s --format=%s)" = "Edited by EDITOR"
     -
    -+	echo "Edited by EDITOR" >expect &&
    -+	git show -s --format=%s >actual &&
    -+	test_cmp expect actual
    ++	test_commit_message HEAD -m "Edited by EDITOR"
      '
      
      TERM=vt100
    @@ t/t7005-editor.sh
      		git --exec-path=. commit --amend &&
     -		git show -s --pretty=oneline |
     -		sed -e "s/^[0-9a-f]* //" >actual &&
    -+		git show -s --pretty=oneline >show &&
    -+		<show sed -e "s/^[0-9a-f]* //" >actual &&
    - 		test_cmp expect actual
    +-		test_cmp expect actual
    ++		test_commit_message HEAD expect
      	'
      done
    + 
     @@
      	esac
      	test_expect_success "Using $i (override)" '
      		git --exec-path=. commit --amend &&
     -		git show -s --pretty=oneline |
     -		sed -e "s/^[0-9a-f]* //" >actual &&
    -+		git show -s --pretty=oneline >show &&
    -+		<show sed -e "s/^[0-9a-f]* //" >actual &&
    - 		test_cmp expect actual
    +-		test_cmp expect actual
    ++		test_commit_message HEAD expect
      	'
      done
    + 
     @@
      	echo "echo space >\"\$1\"" >"e space.sh" &&
      	chmod a+x "e space.sh" &&
      	GIT_EDITOR="./e\ space.sh" git commit --amend &&
     -	test space = "$(git show -s --pretty=format:%s)"
     -
    -+	echo space >expect &&
    -+	git show -s --pretty=tformat:%s >actual &&
    -+	test_cmp expect actual
    ++	test_commit_message HEAD -m space
      '
      
      unset GIT_EDITOR
    @@ t/t7005-editor.sh
      	git commit --amend &&
     -	test space = "$(git show -s --pretty=format:%s)"
     -
    -+	echo space >expect &&
    -+	git show -s --pretty=tformat:%s >actual &&
    -+	test_cmp expect actual
    ++	test_commit_message HEAD -m space
      '
      
      test_done
-:  ---------- > 2:  9451e4f0f6 t7005: stop abusing --exec-path
2:  0467e33ed0 ! 3:  61cb116780 t7005: sanitize test environment for subsequent tests
    @@ Commit message
         that affect future tests, but those modifications are visible to future
         tests and create a footgun for them.
     
    -    Use test_config, subshells, and test helpers to automatically undo
    -    environment and config modifications once finished.
    +    Use test_config, subshells, single-command environment overrides, and
    +    test helpers to automatically undo environment and config modifications
    +    once finished.
     
         Best-viewed-with: --ignore-all-space
         Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
     
      ## t/t7005-editor.sh ##
     @@
    - 	test_cmp expect actual
    + 	test_commit_message HEAD -m "$msg"
      '
      
     -TERM=dumb
     -export TERM
      test_expect_success 'dumb should error out when falling back on vi' '
    --	if git commit --amend
    --	then
    --		echo "Oops?"
    --		false
    --	else
    --		: happy
    --	fi
    -+	(
    -+		TERM=dumb &&
    -+		export TERM &&
    -+		if git commit --amend
    -+		then
    -+			echo "Oops?"
    -+			false
    -+		else
    -+			: happy
    -+		fi
    -+	)
    +-	test_must_fail git commit --amend
    ++	TERM=dumb test_must_fail git commit --amend
      '
      
      test_expect_success 'dumb should prefer EDITOR to VISUAL' '
    @@ t/t7005-editor.sh
     -	VISUAL=./e-VISUAL.sh &&
     -	export EDITOR VISUAL &&
     -	git commit --amend &&
    --	echo "Edited by EDITOR" >expect &&
    --	git show -s --format=%s >actual &&
    -+	(
    -+		TERM=dumb &&
    -+		export TERM &&
    -+		EDITOR=./e-EDITOR.sh &&
    -+		VISUAL=./e-VISUAL.sh &&
    -+		export EDITOR VISUAL &&
    ++	TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
     +		git commit --amend &&
    -+		echo "Edited by EDITOR" >expect &&
    -+		git show -s --format=%s >actual
    -+	) &&
    - 	test_cmp expect actual
    + 	test_commit_message HEAD -m "Edited by EDITOR"
      '
      
     -TERM=vt100
     -export TERM
    --for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
    --do
    + for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
    + do
     -	echo "Edited by $i" >expect
     -	unset EDITOR VISUAL GIT_EDITOR
     -	git config --unset-all core.editor
    @@ t/t7005-editor.sh
     -		export $i
     -		;;
     -	esac
    --	test_expect_success "Using $i" '
    --		git --exec-path=. commit --amend &&
    --		git show -s --pretty=oneline >show &&
    --		<show sed -e "s/^[0-9a-f]* //" >actual &&
    --		test_cmp expect actual
    --	'
    --done
    -+test_expect_success 'Using individual editors' '
    -+	test_when_finished "test_unconfig --unset-all core.editor" &&
    -+	(
    -+		TERM=vt100 &&
    -+		export TERM &&
    -+		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
    -+		do
    -+			sane_unset EDITOR VISUAL GIT_EDITOR &&
    -+			test_might_fail git config --unset-all core.editor &&
    -+			echo "Edited by $i" >expect &&
    + 	test_expect_success "Using $i" '
    +-		PATH="$PWD:$PATH" git commit --amend &&
    +-		test_commit_message HEAD expect
    ++		if test "$i" = core_editor
    ++		then
    ++			test_config core.editor ./e-core_editor.sh
    ++		fi &&
    ++		(
     +			case "$i" in
    -+			core_editor)
    -+				git config core.editor ./e-core_editor.sh
    -+				;;
     +			[A-Z]*)
     +				eval "$i=./e-$i.sh" &&
     +				export $i
     +				;;
     +			esac &&
    -+			git --exec-path=. commit --amend &&
    -+			git show -s --pretty=oneline >show &&
    -+			<show sed -e "s/^[0-9a-f]* //" >actual &&
    -+			test_cmp expect actual
    -+		done
    -+	)
    -+'
    ++			PATH="$PWD:$PATH" TERM=vt100 git commit --amend
    ++		) &&
    ++		test_commit_message HEAD -m "Edited by $i"
    + 	'
    + done
      
     -unset EDITOR VISUAL GIT_EDITOR
     -git config --unset-all core.editor
    @@ t/t7005-editor.sh
     -		;;
     -	esac
     -	test_expect_success "Using $i (override)" '
    --		git --exec-path=. commit --amend &&
    --		git show -s --pretty=oneline >show &&
    --		<show sed -e "s/^[0-9a-f]* //" >actual &&
    --		test_cmp expect actual
    +-		PATH="$PWD:$PATH" git commit --amend &&
    +-		test_commit_message HEAD expect
     -	'
     -done
     +test_expect_success 'Using editors with overrides' '
    @@ t/t7005-editor.sh
     +				export $i
     +				;;
     +			esac &&
    -+			git --exec-path=. commit --amend &&
    -+			git show -s --pretty=oneline >show &&
    -+			<show sed -e "s/^[0-9a-f]* //" >actual &&
    -+			test_cmp expect actual
    ++			PATH="$PWD:$PATH" git commit --amend &&
    ++			test_commit_message HEAD expect || exit 1
     +		done
     +	)
     +'
    @@ t/t7005-editor.sh
      test_expect_success 'editor with a space' '
      	echo "echo space >\"\$1\"" >"e space.sh" &&
     @@
    - 	test_cmp expect actual
    + 	test_commit_message HEAD -m space
      '
      
     -unset GIT_EDITOR
    @@ t/t7005-editor.sh
     -	git config core.editor \"./e\ space.sh\" &&
     +	test_config core.editor \"./e\ space.sh\" &&
      	git commit --amend &&
    - 	echo space >expect &&
    - 	git show -s --pretty=tformat:%s >actual &&
    + 	test_commit_message HEAD -m space
    + '
3:  4a9bb52470 = 4:  ea269f2442 editor: use standard strvec API to receive environment for external editors

base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be
-- 
2.48.1

Re: [PATCH v3 0/4] clean up some code around editors

[Ben Knoble]

> Le 11 août 2025 à 18:17, D. Ben Knoble <ben.knoble+github@gmail.com> a écrit :
> 
> Changes from v2:
> - shuffle setup code and use more helpers in 1/4
> - insert 2/4 to stop abusing --exec-path
> - improve environment-cleansing idioms in {2 => 3}/4
> 
> Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)

Some of this shuffling turned out to be unportable, which CI caught (but running the test locally didn’t??). Fortunately it pointed me at test_env and I’ll either use it or go back to the subshells.

Re: [PATCH v3 0/4] clean up some code around editors

[Eric Sunshine]

On Mon, Aug 11, 2025 at 6:59 PM Ben Knoble <ben.knoble@gmail.com> wrote:
> > Le 11 août 2025 à 18:17, D. Ben Knoble <ben.knoble+github@gmail.com> a écrit :
> > Changes from v2:
> > - shuffle setup code and use more helpers in 1/4
> > - insert 2/4 to stop abusing --exec-path
> > - improve environment-cleansing idioms in {2 => 3}/4
>
> Some of this shuffling turned out to be unportable, which CI caught (but running the test locally didn’t??). Fortunately it pointed me at test_env and I’ll either use it or go back to the subshells.

To catch it locally, you can run:

    make test-lint-shell-syntax

in the "t" directory. Alternatively, `make test-lint`, `make test` or
`make prove` would also have caught the problem.

Re: [PATCH v3 0/4] clean up some code around editors

[D. Ben Knoble]

On Mon, Aug 11, 2025 at 8:16 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Aug 11, 2025 at 6:59 PM Ben Knoble <ben.knoble@gmail.com> wrote:
> > > Le 11 août 2025 à 18:17, D. Ben Knoble <ben.knoble+github@gmail.com> a écrit :
> > > Changes from v2:
> > > - shuffle setup code and use more helpers in 1/4
> > > - insert 2/4 to stop abusing --exec-path
> > > - improve environment-cleansing idioms in {2 => 3}/4
> >
> > Some of this shuffling turned out to be unportable, which CI caught (but running the test locally didn’t??). Fortunately it pointed me at test_env and I’ll either use it or go back to the subshells.
>
> To catch it locally, you can run:
>
>     make test-lint-shell-syntax
>
> in the "t" directory. Alternatively, `make test-lint`, `make test` or
> `make prove` would also have caught the problem.

Ah, thanks. I was particularly confused because running the test file
directly does run the chainlint check, I think, but not this lint.

-- 
D. Ben Knoble

Re: [PATCH v3 0/4] clean up some code around editors

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Aug 11, 2025 at 6:59 PM Ben Knoble <ben.knoble@gmail.com> wrote:
>> > Le 11 août 2025 à 18:17, D. Ben Knoble <ben.knoble+github@gmail.com> a écrit :
>> > Changes from v2:
>> > - shuffle setup code and use more helpers in 1/4
>> > - insert 2/4 to stop abusing --exec-path
>> > - improve environment-cleansing idioms in {2 => 3}/4
>>
>> Some of this shuffling turned out to be unportable, which CI caught (but running the test locally didn’t??). Fortunately it pointed me at test_env and I’ll either use it or go back to the subshells.
>
> To catch it locally, you can run:
>
>     make test-lint-shell-syntax
>
> in the "t" directory. Alternatively, `make test-lint`, `make test` or
> `make prove` would also have caught the problem.

Among these three `make test` can be run from the top-level.  The
other two cannot.

"make test" is a bit too heavy-weight to use as an initial sanity
check for tests that are being newly developed, and I am wondering
if something like this can be added as a first-line sanity checking
tool.  The idea is a simple "make" to build, while DEVELOPER=Yes is
set, would trigger the common linting any developer who is working
on new things needs to pay attention to.



 Makefile | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git c/Makefile w/Makefile
index e11340c1ae..7f21afeaf9 100644
--- c/Makefile
+++ w/Makefile
@@ -1471,6 +1471,7 @@ include config.mak.uname
 
 ifdef DEVELOPER
 include config.mak.dev
+all:: check-developer
 endif
 
 GIT-VERSION-FILE: FORCE
@@ -3350,6 +3351,10 @@ check:
 		exit 1; \
 	fi
 
+# We may want to take over 'make check' for this, but for now...
+.PHONY: check-developer
+check-developer: check-docs check-tests check-builtins check-headers
+
 COCCI_GEN_ALL = .build/contrib/coccinelle/ALL.cocci
 COCCI_GLOB = $(wildcard contrib/coccinelle/*.cocci)
 COCCI_RULES_TRACKED = $(COCCI_GLOB:%=.build/%)
@@ -3942,6 +3947,10 @@ build-unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG)
 unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) t/helper/test-tool$X
 	$(MAKE) -C t/ unit-tests
 
+.PHONY: check-tests
+check-tests:
+	$(MAKE) -C t/ test-lint
+
 .PHONY: libgit-sys libgit-rs
 libgit-sys libgit-rs:
 	$(QUIET)(\

Re: [PATCH v3 0/4] clean up some code around editors

[Eric Sunshine]

On Tue, Aug 12, 2025 at 1:36 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > To catch it locally, you can run:
> >     make test-lint-shell-syntax
> > in the "t" directory. Alternatively, `make test-lint`, `make test` or
> > `make prove` would also have caught the problem.
>
> "make test" is a bit too heavy-weight to use as an initial sanity
> check for tests that are being newly developed, and I am wondering
> if something like this can be added as a first-line sanity checking
> tool.  The idea is a simple "make" to build, while DEVELOPER=Yes is
> set, would trigger the common linting any developer who is working
> on new things needs to pay attention to.
>
>  ifdef DEVELOPER
>  include config.mak.dev
> +all:: check-developer
>  endif
>
> +check-developer: check-docs check-tests check-builtins check-headers
>
> +.PHONY: check-tests
> +check-tests:
> +       $(MAKE) -C t/ test-lint

Not a bad idea, though I don't think we need to hide the target behind
DEVELOPER.

The "first-line sanity checking" phrasing you used above suggested the
name "make check-sanity" to me as being more meaningful and obvious
than "make check-developer". However, upon reflection, "sanity" is
perhaps too generic, thus might not convey that these checks are for
code newly-developed (or changed) by developers, so perhaps "make
check-developer" is indeed the better name choice.

Re: [PATCH v3 0/4] clean up some code around editors

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

Eric Sunshine <sunshine@sunshineco.com> writes:

>>  ifdef DEVELOPER
>>  include config.mak.dev
>> +all:: check-developer
>>  endif
>> ...
> Not a bad idea, though I don't think we need to hide the target behind
> DEVELOPER.

This target is designed to be a collection of light-weight tests for
your uncooked code, so running it when somebody makes a production
build should *not* be too costly, but at the same time, it is more
or less pointless---if it catches somethig for a build engineer, it
is way too late.

On the other hand, if you have only started to add a new command and
trying to see if your skeletal implementation even compiles, it may
be annoying to be told that you still have to write documentation.
You may already know you need to, but you are not ready to do so
yet.  Even though I on purpose made the checks run as part of "all"
to give the target more exposure, I am not sure if limiting to
developer is still too aggressive.

Re: [PATCH v3 0/4] clean up some code around editors

[Eric Sunshine]

On Tue, Aug 12, 2025 at 2:22 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> >>  ifdef DEVELOPER
> >>  include config.mak.dev
> >> +all:: check-developer
> >>  endif
> >> ...
> > Not a bad idea, though I don't think we need to hide the target behind
> > DEVELOPER.
>
> This target is designed to be a collection of light-weight tests for
> your uncooked code, so running it when somebody makes a production
> build should *not* be too costly, but at the same time, it is more
> or less pointless---if it catches somethig for a build engineer, it
> is way too late.

Ugh, I missed the bit where you had wired it into the "all" target; I
misread it as defining the "check-developer" target only if DEVELOPER
was set, so my response was bogus. Sorry for the noise.

> On the other hand, if you have only started to add a new command and
> trying to see if your skeletal implementation even compiles, it may
> be annoying to be told that you still have to write documentation.
> You may already know you need to, but you are not ready to do so
> yet.  Even though I on purpose made the checks run as part of "all"
> to give the target more exposure, I am not sure if limiting to
> developer is still too aggressive.

Indeed, wiring it into "all" may be too aggressive. Because I missed
that bit, I had thought that you just meant for developers to run
"make check-developer" manually. Documenting it in SubmittingPatches
may be the lesser evil.

[PATCH v4 0/3] clean up some code around editors

[D. Ben Knoble]

Changes from v3:
- drop 4/4
- use test_env (including a case our lint does not catch when the value
  has spaces)

Changes from v2:
- shuffle setup code and use more helpers in 1/4
- insert 2/4 to stop abusing --exec-path
- improve environment-cleansing idioms in {2 => 3}/4

Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)

Changes from v1:
- add a prep patch with style fixes to t7005
- rework the environment munging to use subshells, per Phillip Wood's
  suggestion

This reroll of the previous exec-path series is simplified to contain
only the first 2 cleanup patches, which were largely acked by the list.
Drop the controversial and broken PATH munging.

Also, this version is (still) based on a later master 112648dd6b (Merge
branch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the
original from May.

These patches clean up some old code in the editor tests and subsystem
that does not use our modern idioms.

v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/
v2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/
v3: https://lore.kernel.org/git/20250811221706.67168-1-ben.knoble+github@gmail.com/
Published-as: https://github.com/benknoble/tree/editor-cleanup

D. Ben Knoble (3):
  t7005: use modern test style
  t7005: stop abusing --exec-path
  t7005: sanitize test environment for subsequent tests

 t/t7005-editor.sh | 149 +++++++++++++++++++---------------------------
 1 file changed, 60 insertions(+), 89 deletions(-)

Diff-intervalle contre v3 :
1:  8ad2904a18 = 1:  8ad2904a18 t7005: use modern test style
2:  9451e4f0f6 ! 2:  44a6fd8eb3 t7005: stop abusing --exec-path
    @@ t/t7005-editor.sh
      	esac
      	test_expect_success "Using $i" '
     -		git --exec-path=. commit --amend &&
    -+		PATH="$PWD:$PATH" git commit --amend &&
    ++		test_env PATH="$PWD:$PATH" git commit --amend &&
      		test_commit_message HEAD expect
      	'
      done
    @@ t/t7005-editor.sh
      	esac
      	test_expect_success "Using $i (override)" '
     -		git --exec-path=. commit --amend &&
    -+		PATH="$PWD:$PATH" git commit --amend &&
    ++		test_env PATH="$PWD:$PATH" git commit --amend &&
      		test_commit_message HEAD expect
      	'
      done
3:  61cb116780 ! 3:  135d4368d6 t7005: sanitize test environment for subsequent tests
    @@ t/t7005-editor.sh
     -export TERM
      test_expect_success 'dumb should error out when falling back on vi' '
     -	test_must_fail git commit --amend
    -+	TERM=dumb test_must_fail git commit --amend
    ++	test_env TERM=dumb test_must_fail git commit --amend
      '
      
      test_expect_success 'dumb should prefer EDITOR to VISUAL' '
    @@ t/t7005-editor.sh
     -	VISUAL=./e-VISUAL.sh &&
     -	export EDITOR VISUAL &&
     -	git commit --amend &&
    -+	TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
    ++	test_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
     +		git commit --amend &&
      	test_commit_message HEAD -m "Edited by EDITOR"
      '
    @@ t/t7005-editor.sh
     -		;;
     -	esac
      	test_expect_success "Using $i" '
    --		PATH="$PWD:$PATH" git commit --amend &&
    +-		test_env PATH="$PWD:$PATH" git commit --amend &&
     -		test_commit_message HEAD expect
     +		if test "$i" = core_editor
     +		then
    @@ t/t7005-editor.sh
     +				export $i
     +				;;
     +			esac &&
    -+			PATH="$PWD:$PATH" TERM=vt100 git commit --amend
    ++			test_env PATH="$PWD:$PATH" TERM=vt100 git commit --amend
     +		) &&
     +		test_commit_message HEAD -m "Edited by $i"
      	'
    @@ t/t7005-editor.sh
     -		;;
     -	esac
     -	test_expect_success "Using $i (override)" '
    --		PATH="$PWD:$PATH" git commit --amend &&
    +-		test_env PATH="$PWD:$PATH" git commit --amend &&
     -		test_commit_message HEAD expect
     -	'
     -done
    @@ t/t7005-editor.sh
     +				export $i
     +				;;
     +			esac &&
    -+			PATH="$PWD:$PATH" git commit --amend &&
    ++			test_env PATH="$PWD:$PATH" git commit --amend &&
     +			test_commit_message HEAD expect || exit 1
     +		done
     +	)
    @@ t/t7005-editor.sh
      
      test_expect_success 'editor with a space' '
      	echo "echo space >\"\$1\"" >"e space.sh" &&
    -@@
    + 	chmod a+x "e space.sh" &&
    +-	GIT_EDITOR="./e\ space.sh" git commit --amend &&
    ++	test_env GIT_EDITOR="./e\ space.sh" git commit --amend &&
      	test_commit_message HEAD -m space
      '
      
4:  ea269f2442 < -:  ---------- editor: use standard strvec API to receive environment for external editors

base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be
-- 
2.48.1

Re: [PATCH v4 0/3] clean up some code around editors

[Phillip Wood]

Hi Ben

On 12/08/2025 18:02, D. Ben Knoble wrote:
> Changes from v3:
> - drop 4/4
> - use test_env (including a case our lint does not catch when the value
>    has spaces)

It's not worth a re-roll but for future reference

	test_env FOO=bar git commit --amend

uses an extra process compared to

	FOO=bar git commit --amend

which slows the test suite down for no real gain. We should only need to 
use test_env to set environment variables when calling a shell function. 
In the special case of test_must_fail it supports

	test_must_fail env FOO=bar git commit --amend

which is widely used in our test suite

$ git grep  'test_must_fail env ' origin/master t/t\*.sh | wc -l
152
$ git grep  'test_env .* test_must_fail ' origin/master t/t\*.sh | wc -l
1

Thanks for cleaning up this test file, it's looking much nicer now.

Phillip

> Changes from v2:
> - shuffle setup code and use more helpers in 1/4
> - insert 2/4 to stop abusing --exec-path
> - improve environment-cleansing idioms in {2 => 3}/4
> 
> Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)
> 
> Changes from v1:
> - add a prep patch with style fixes to t7005
> - rework the environment munging to use subshells, per Phillip Wood's
>    suggestion
> 
> This reroll of the previous exec-path series is simplified to contain
> only the first 2 cleanup patches, which were largely acked by the list.
> Drop the controversial and broken PATH munging.
> 
> Also, this version is (still) based on a later master 112648dd6b (Merge
> branch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the
> original from May.
> 
> These patches clean up some old code in the editor tests and subsystem
> that does not use our modern idioms.
> 
> v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/
> v2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/
> v3: https://lore.kernel.org/git/20250811221706.67168-1-ben.knoble+github@gmail.com/
> Published-as: https://github.com/benknoble/tree/editor-cleanup
> 
> D. Ben Knoble (3):
>    t7005: use modern test style
>    t7005: stop abusing --exec-path
>    t7005: sanitize test environment for subsequent tests
> 
>   t/t7005-editor.sh | 149 +++++++++++++++++++---------------------------
>   1 file changed, 60 insertions(+), 89 deletions(-)
> 
> Diff-intervalle contre v3 :
> 1:  8ad2904a18 = 1:  8ad2904a18 t7005: use modern test style
> 2:  9451e4f0f6 ! 2:  44a6fd8eb3 t7005: stop abusing --exec-path
>      @@ t/t7005-editor.sh
>        	esac
>        	test_expect_success "Using $i" '
>       -		git --exec-path=. commit --amend &&
>      -+		PATH="$PWD:$PATH" git commit --amend &&
>      ++		test_env PATH="$PWD:$PATH" git commit --amend &&
>        		test_commit_message HEAD expect
>        	'
>        done
>      @@ t/t7005-editor.sh
>        	esac
>        	test_expect_success "Using $i (override)" '
>       -		git --exec-path=. commit --amend &&
>      -+		PATH="$PWD:$PATH" git commit --amend &&
>      ++		test_env PATH="$PWD:$PATH" git commit --amend &&
>        		test_commit_message HEAD expect
>        	'
>        done
> 3:  61cb116780 ! 3:  135d4368d6 t7005: sanitize test environment for subsequent tests
>      @@ t/t7005-editor.sh
>       -export TERM
>        test_expect_success 'dumb should error out when falling back on vi' '
>       -	test_must_fail git commit --amend
>      -+	TERM=dumb test_must_fail git commit --amend
>      ++	test_env TERM=dumb test_must_fail git commit --amend
>        '
>        
>        test_expect_success 'dumb should prefer EDITOR to VISUAL' '
>      @@ t/t7005-editor.sh
>       -	VISUAL=./e-VISUAL.sh &&
>       -	export EDITOR VISUAL &&
>       -	git commit --amend &&
>      -+	TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
>      ++	test_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
>       +		git commit --amend &&
>        	test_commit_message HEAD -m "Edited by EDITOR"
>        '
>      @@ t/t7005-editor.sh
>       -		;;
>       -	esac
>        	test_expect_success "Using $i" '
>      --		PATH="$PWD:$PATH" git commit --amend &&
>      +-		test_env PATH="$PWD:$PATH" git commit --amend &&
>       -		test_commit_message HEAD expect
>       +		if test "$i" = core_editor
>       +		then
>      @@ t/t7005-editor.sh
>       +				export $i
>       +				;;
>       +			esac &&
>      -+			PATH="$PWD:$PATH" TERM=vt100 git commit --amend
>      ++			test_env PATH="$PWD:$PATH" TERM=vt100 git commit --amend
>       +		) &&
>       +		test_commit_message HEAD -m "Edited by $i"
>        	'
>      @@ t/t7005-editor.sh
>       -		;;
>       -	esac
>       -	test_expect_success "Using $i (override)" '
>      --		PATH="$PWD:$PATH" git commit --amend &&
>      +-		test_env PATH="$PWD:$PATH" git commit --amend &&
>       -		test_commit_message HEAD expect
>       -	'
>       -done
>      @@ t/t7005-editor.sh
>       +				export $i
>       +				;;
>       +			esac &&
>      -+			PATH="$PWD:$PATH" git commit --amend &&
>      ++			test_env PATH="$PWD:$PATH" git commit --amend &&
>       +			test_commit_message HEAD expect || exit 1
>       +		done
>       +	)
>      @@ t/t7005-editor.sh
>        
>        test_expect_success 'editor with a space' '
>        	echo "echo space >\"\$1\"" >"e space.sh" &&
>      -@@
>      + 	chmod a+x "e space.sh" &&
>      +-	GIT_EDITOR="./e\ space.sh" git commit --amend &&
>      ++	test_env GIT_EDITOR="./e\ space.sh" git commit --amend &&
>        	test_commit_message HEAD -m space
>        '
>        
> 4:  ea269f2442 < -:  ---------- editor: use standard strvec API to receive environment for external editors
> 
> base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be

Re: [PATCH v4 0/3] clean up some code around editors

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Ben
>
> On 12/08/2025 18:02, D. Ben Knoble wrote:
>> Changes from v3:
>> - drop 4/4
>> - use test_env (including a case our lint does not catch when the value
>>    has spaces)
>
> It's not worth a re-roll but for future reference
>
> 	test_env FOO=bar git commit --amend
>
> uses an extra process compared to
>
> 	FOO=bar git commit --amend
>
> which slows the test suite down for no real gain. We should only need
> to use test_env to set environment variables when calling a shell
> function. In the special case of test_must_fail it supports
>
> 	test_must_fail env FOO=bar git commit --amend
>
> which is widely used in our test suite

If this were some feature series, the story may be different, but
since the theme of the topic is "clean up", the above clean-up is
something that ought to be part of an update.  A clean-up topic
should not be adding things that need to be further cleaned up ;-)

Thank you, both of you, for writing and reviewing the series.

Re: [PATCH v4 0/3] clean up some code around editors

[D. Ben Knoble]

On Wed, Aug 13, 2025 at 11:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Hi Ben
> >
> > On 12/08/2025 18:02, D. Ben Knoble wrote:
> >> Changes from v3:
> >> - drop 4/4
> >> - use test_env (including a case our lint does not catch when the value
> >>    has spaces)
> >
> > It's not worth a re-roll but for future reference
> >
> >       test_env FOO=bar git commit --amend
> >
> > uses an extra process compared to
> >
> >       FOO=bar git commit --amend
> >
> > which slows the test suite down for no real gain. We should only need
> > to use test_env to set environment variables when calling a shell
> > function. In the special case of test_must_fail it supports
> >
> >       test_must_fail env FOO=bar git commit --amend
> >
> > which is widely used in our test suite

Aha, I learned something new (again)—although that still means my
diagnosis for this about the space in the value is wrong :) It's not
flagged because it's not a shell function. Great!

> If this were some feature series, the story may be different, but
> since the theme of the topic is "clean up", the above clean-up is
> something that ought to be part of an update.  A clean-up topic
> should not be adding things that need to be further cleaned up ;-)

Agreed; I don't want to add more problems here ;)

[PATCH v5 0/3] clean up some code around editors

[D. Ben Knoble]

Changes from v4:
- revert use of test_env: fix the one instance that needs adjusted for
  VAR=val cmd syntax by using test_must_fail env VAR=val cmd.

Changes from v3:
- drop 4/4
- use test_env (including a case our lint does not catch when the value
  has spaces)

Changes from v2:
- shuffle setup code and use more helpers in 1/4
- insert 2/4 to stop abusing --exec-path
- improve environment-cleansing idioms in {2 => 3}/4

Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)

Changes from v1:
- add a prep patch with style fixes to t7005
- rework the environment munging to use subshells, per Phillip Wood's
  suggestion

These patches clean up some old code in the editor tests that does not use our
modern idioms.

Also, this version is (still) based on a later master 112648dd6b (Merge
branch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the
original from May.

v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/
v2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/
v3: https://lore.kernel.org/git/20250811221706.67168-1-ben.knoble+github@gmail.com/
v4: https://lore.kernel.org/git/20250812170256.71751-1-ben.knoble+github@gmail.com/
Published-as: https://github.com/benknoble/tree/editor-cleanup

D. Ben Knoble (3):
  t7005: use modern test style
  t7005: stop abusing --exec-path
  t7005: sanitize test environment for subsequent tests

 t/t7005-editor.sh | 147 +++++++++++++++++++---------------------------
 1 file changed, 59 insertions(+), 88 deletions(-)

Diff-intervalle contre v4 :
1:  8ad2904a18 = 1:  8ad2904a18 t7005: use modern test style
2:  44a6fd8eb3 ! 2:  5a35889571 t7005: stop abusing --exec-path
    @@ t/t7005-editor.sh
      	esac
      	test_expect_success "Using $i" '
     -		git --exec-path=. commit --amend &&
    -+		test_env PATH="$PWD:$PATH" git commit --amend &&
    ++		PATH="$PWD:$PATH" git commit --amend &&
      		test_commit_message HEAD expect
      	'
      done
    @@ t/t7005-editor.sh
      	esac
      	test_expect_success "Using $i (override)" '
     -		git --exec-path=. commit --amend &&
    -+		test_env PATH="$PWD:$PATH" git commit --amend &&
    ++		PATH="$PWD:$PATH" git commit --amend &&
      		test_commit_message HEAD expect
      	'
      done
3:  135d4368d6 ! 3:  e6e31ab98c t7005: sanitize test environment for subsequent tests
    @@ t/t7005-editor.sh
     -export TERM
      test_expect_success 'dumb should error out when falling back on vi' '
     -	test_must_fail git commit --amend
    -+	test_env TERM=dumb test_must_fail git commit --amend
    ++	test_must_fail env TERM=dumb git commit --amend
      '
      
      test_expect_success 'dumb should prefer EDITOR to VISUAL' '
    @@ t/t7005-editor.sh
     -	VISUAL=./e-VISUAL.sh &&
     -	export EDITOR VISUAL &&
     -	git commit --amend &&
    -+	test_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
    ++	TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
     +		git commit --amend &&
      	test_commit_message HEAD -m "Edited by EDITOR"
      '
    @@ t/t7005-editor.sh
     -		;;
     -	esac
      	test_expect_success "Using $i" '
    --		test_env PATH="$PWD:$PATH" git commit --amend &&
    +-		PATH="$PWD:$PATH" git commit --amend &&
     -		test_commit_message HEAD expect
     +		if test "$i" = core_editor
     +		then
    @@ t/t7005-editor.sh
     +				export $i
     +				;;
     +			esac &&
    -+			test_env PATH="$PWD:$PATH" TERM=vt100 git commit --amend
    ++			PATH="$PWD:$PATH" TERM=vt100 git commit --amend
     +		) &&
     +		test_commit_message HEAD -m "Edited by $i"
      	'
    @@ t/t7005-editor.sh
     -		;;
     -	esac
     -	test_expect_success "Using $i (override)" '
    --		test_env PATH="$PWD:$PATH" git commit --amend &&
    +-		PATH="$PWD:$PATH" git commit --amend &&
     -		test_commit_message HEAD expect
     -	'
     -done
    @@ t/t7005-editor.sh
     +				export $i
     +				;;
     +			esac &&
    -+			test_env PATH="$PWD:$PATH" git commit --amend &&
    ++			PATH="$PWD:$PATH" git commit --amend &&
     +			test_commit_message HEAD expect || exit 1
     +		done
     +	)
    @@ t/t7005-editor.sh
      
      test_expect_success 'editor with a space' '
      	echo "echo space >\"\$1\"" >"e space.sh" &&
    - 	chmod a+x "e space.sh" &&
    --	GIT_EDITOR="./e\ space.sh" git commit --amend &&
    -+	test_env GIT_EDITOR="./e\ space.sh" git commit --amend &&
    +@@
      	test_commit_message HEAD -m space
      '
      

base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be
-- 
2.48.1

Re: [PATCH v5 0/3] clean up some code around editors

[Phillip Wood]

Hi Ben

On 13/08/2025 18:50, D. Ben Knoble wrote:
> Changes from v4:
> - revert use of test_env: fix the one instance that needs adjusted for
>    VAR=val cmd syntax by using test_must_fail env VAR=val cmd.

Thanks for re-rolling, the range-diff below looks good

Phillip

> 
> Changes from v3:
> - drop 4/4
> - use test_env (including a case our lint does not catch when the value
>    has spaces)
> 
> Changes from v2:
> - shuffle setup code and use more helpers in 1/4
> - insert 2/4 to stop abusing --exec-path
> - improve environment-cleansing idioms in {2 => 3}/4
> 
> Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)
> 
> Changes from v1:
> - add a prep patch with style fixes to t7005
> - rework the environment munging to use subshells, per Phillip Wood's
>    suggestion
> 
> These patches clean up some old code in the editor tests that does not use our
> modern idioms.
> 
> Also, this version is (still) based on a later master 112648dd6b (Merge
> branch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the
> original from May.
> 
> v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/
> v2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/
> v3: https://lore.kernel.org/git/20250811221706.67168-1-ben.knoble+github@gmail.com/
> v4: https://lore.kernel.org/git/20250812170256.71751-1-ben.knoble+github@gmail.com/
> Published-as: https://github.com/benknoble/tree/editor-cleanup
> 
> D. Ben Knoble (3):
>    t7005: use modern test style
>    t7005: stop abusing --exec-path
>    t7005: sanitize test environment for subsequent tests
> 
>   t/t7005-editor.sh | 147 +++++++++++++++++++---------------------------
>   1 file changed, 59 insertions(+), 88 deletions(-)
> 
> Diff-intervalle contre v4 :
> 1:  8ad2904a18 = 1:  8ad2904a18 t7005: use modern test style
> 2:  44a6fd8eb3 ! 2:  5a35889571 t7005: stop abusing --exec-path
>      @@ t/t7005-editor.sh
>        	esac
>        	test_expect_success "Using $i" '
>       -		git --exec-path=. commit --amend &&
>      -+		test_env PATH="$PWD:$PATH" git commit --amend &&
>      ++		PATH="$PWD:$PATH" git commit --amend &&
>        		test_commit_message HEAD expect
>        	'
>        done
>      @@ t/t7005-editor.sh
>        	esac
>        	test_expect_success "Using $i (override)" '
>       -		git --exec-path=. commit --amend &&
>      -+		test_env PATH="$PWD:$PATH" git commit --amend &&
>      ++		PATH="$PWD:$PATH" git commit --amend &&
>        		test_commit_message HEAD expect
>        	'
>        done
> 3:  135d4368d6 ! 3:  e6e31ab98c t7005: sanitize test environment for subsequent tests
>      @@ t/t7005-editor.sh
>       -export TERM
>        test_expect_success 'dumb should error out when falling back on vi' '
>       -	test_must_fail git commit --amend
>      -+	test_env TERM=dumb test_must_fail git commit --amend
>      ++	test_must_fail env TERM=dumb git commit --amend
>        '
>        
>        test_expect_success 'dumb should prefer EDITOR to VISUAL' '
>      @@ t/t7005-editor.sh
>       -	VISUAL=./e-VISUAL.sh &&
>       -	export EDITOR VISUAL &&
>       -	git commit --amend &&
>      -+	test_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
>      ++	TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
>       +		git commit --amend &&
>        	test_commit_message HEAD -m "Edited by EDITOR"
>        '
>      @@ t/t7005-editor.sh
>       -		;;
>       -	esac
>        	test_expect_success "Using $i" '
>      --		test_env PATH="$PWD:$PATH" git commit --amend &&
>      +-		PATH="$PWD:$PATH" git commit --amend &&
>       -		test_commit_message HEAD expect
>       +		if test "$i" = core_editor
>       +		then
>      @@ t/t7005-editor.sh
>       +				export $i
>       +				;;
>       +			esac &&
>      -+			test_env PATH="$PWD:$PATH" TERM=vt100 git commit --amend
>      ++			PATH="$PWD:$PATH" TERM=vt100 git commit --amend
>       +		) &&
>       +		test_commit_message HEAD -m "Edited by $i"
>        	'
>      @@ t/t7005-editor.sh
>       -		;;
>       -	esac
>       -	test_expect_success "Using $i (override)" '
>      --		test_env PATH="$PWD:$PATH" git commit --amend &&
>      +-		PATH="$PWD:$PATH" git commit --amend &&
>       -		test_commit_message HEAD expect
>       -	'
>       -done
>      @@ t/t7005-editor.sh
>       +				export $i
>       +				;;
>       +			esac &&
>      -+			test_env PATH="$PWD:$PATH" git commit --amend &&
>      ++			PATH="$PWD:$PATH" git commit --amend &&
>       +			test_commit_message HEAD expect || exit 1
>       +		done
>       +	)
>      @@ t/t7005-editor.sh
>        
>        test_expect_success 'editor with a space' '
>        	echo "echo space >\"\$1\"" >"e space.sh" &&
>      - 	chmod a+x "e space.sh" &&
>      --	GIT_EDITOR="./e\ space.sh" git commit --amend &&
>      -+	test_env GIT_EDITOR="./e\ space.sh" git commit --amend &&
>      +@@
>        	test_commit_message HEAD -m space
>        '
>        
> 
> base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be

[PATCH v5 1/3] t7005: use modern test style

[D. Ben Knoble]

Tests in t7005 mask Git error codes and do not use our nice test
helpers. Improve that, move some code into the setup test, and drop a
few old-style blank lines while at it.

Best-viewed-with: --ignore-all-space
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 70 ++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 47 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 5fcf281dfb..791e2a0e74 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -7,62 +7,45 @@
 unset EDITOR VISUAL GIT_EDITOR
 
 test_expect_success 'determine default editor' '
-
 	vi=$(TERM=vt100 git var GIT_EDITOR) &&
 	test -n "$vi"
-
 '
 
-if ! expr "$vi" : '[a-z]*$' >/dev/null
-then
-	vi=
-fi
-
-for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
-do
-	cat >e-$i.sh <<-EOF
-	#!$SHELL_PATH
-	echo "Edited by $i" >"\$1"
-	EOF
-	chmod +x e-$i.sh
-done
-
-if ! test -z "$vi"
-then
-	mv e-$vi.sh $vi
-fi
-
 test_expect_success setup '
+	if ! expr "$vi" : "[a-z]*$" >/dev/null
+	then
+		vi=
+	fi &&
+
+	for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
+	do
+		write_script e-$i.sh <<-EOF || return 1
+			echo "Edited by $i" >"\$1"
+		EOF
+	done &&
+
+	if ! test -z "$vi"
+	then
+		mv e-$vi.sh $vi
+	fi &&
 
 	msg="Hand-edited" &&
 	test_commit "$msg" &&
-	echo "$msg" >expect &&
-	git show -s --format=%s > actual &&
-	test_cmp expect actual
-
+	test_commit_message HEAD -m "$msg"
 '
 
 TERM=dumb
 export TERM
 test_expect_success 'dumb should error out when falling back on vi' '
-
-	if git commit --amend
-	then
-		echo "Oops?"
-		false
-	else
-		: happy
-	fi
+	test_must_fail git commit --amend
 '
 
 test_expect_success 'dumb should prefer EDITOR to VISUAL' '
-
 	EDITOR=./e-EDITOR.sh &&
 	VISUAL=./e-VISUAL.sh &&
 	export EDITOR VISUAL &&
 	git commit --amend &&
-	test "$(git show -s --format=%s)" = "Edited by EDITOR"
-
+	test_commit_message HEAD -m "Edited by EDITOR"
 '
 
 TERM=vt100
@@ -83,9 +66,7 @@
 	esac
 	test_expect_success "Using $i" '
 		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline |
-		sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp expect actual
+		test_commit_message HEAD expect
 	'
 done
 
@@ -105,9 +86,7 @@
 	esac
 	test_expect_success "Using $i (override)" '
 		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline |
-		sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp expect actual
+		test_commit_message HEAD expect
 	'
 done
 
@@ -115,17 +94,14 @@
 	echo "echo space >\"\$1\"" >"e space.sh" &&
 	chmod a+x "e space.sh" &&
 	GIT_EDITOR="./e\ space.sh" git commit --amend &&
-	test space = "$(git show -s --pretty=format:%s)"
-
+	test_commit_message HEAD -m space
 '
 
 unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-
 	git config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
-	test space = "$(git show -s --pretty=format:%s)"
-
+	test_commit_message HEAD -m space
 '
 
 test_done
-- 
2.48.1

[PATCH v5 2/3] t7005: stop abusing --exec-path

[D. Ben Knoble]

We want the editors in this test on PATH, so put them there.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 791e2a0e74..0a5861b7f0 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -65,7 +65,7 @@
 		;;
 	esac
 	test_expect_success "Using $i" '
-		git --exec-path=. commit --amend &&
+		PATH="$PWD:$PATH" git commit --amend &&
 		test_commit_message HEAD expect
 	'
 done
@@ -85,7 +85,7 @@
 		;;
 	esac
 	test_expect_success "Using $i (override)" '
-		git --exec-path=. commit --amend &&
+		PATH="$PWD:$PATH" git commit --amend &&
 		test_commit_message HEAD expect
 	'
 done
-- 
2.48.1

[PATCH v5 3/3] t7005: sanitize test environment for subsequent tests

[D. Ben Knoble]

Some of the editor tests manipulate the environment or config in ways
that affect future tests, but those modifications are visible to future
tests and create a footgun for them.

Use test_config, subshells, single-command environment overrides, and
test helpers to automatically undo environment and config modifications
once finished.

Best-viewed-with: --ignore-all-space
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 83 ++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 44 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 0a5861b7f0..c490e5707a 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -34,61 +34,57 @@
 	test_commit_message HEAD -m "$msg"
 '
 
-TERM=dumb
-export TERM
 test_expect_success 'dumb should error out when falling back on vi' '
-	test_must_fail git commit --amend
+	test_must_fail env TERM=dumb git commit --amend
 '
 
 test_expect_success 'dumb should prefer EDITOR to VISUAL' '
-	EDITOR=./e-EDITOR.sh &&
-	VISUAL=./e-VISUAL.sh &&
-	export EDITOR VISUAL &&
-	git commit --amend &&
+	TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
+		git commit --amend &&
 	test_commit_message HEAD -m "Edited by EDITOR"
 '
 
-TERM=vt100
-export TERM
 for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
-	echo "Edited by $i" >expect
-	unset EDITOR VISUAL GIT_EDITOR
-	git config --unset-all core.editor
-	case "$i" in
-	core_editor)
-		git config core.editor ./e-core_editor.sh
-		;;
-	[A-Z]*)
-		eval "$i=./e-$i.sh"
-		export $i
-		;;
-	esac
 	test_expect_success "Using $i" '
-		PATH="$PWD:$PATH" git commit --amend &&
-		test_commit_message HEAD expect
+		if test "$i" = core_editor
+		then
+			test_config core.editor ./e-core_editor.sh
+		fi &&
+		(
+			case "$i" in
+			[A-Z]*)
+				eval "$i=./e-$i.sh" &&
+				export $i
+				;;
+			esac &&
+			PATH="$PWD:$PATH" TERM=vt100 git commit --amend
+		) &&
+		test_commit_message HEAD -m "Edited by $i"
 	'
 done
 
-unset EDITOR VISUAL GIT_EDITOR
-git config --unset-all core.editor
-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
-do
-	echo "Edited by $i" >expect
-	case "$i" in
-	core_editor)
-		git config core.editor ./e-core_editor.sh
-		;;
-	[A-Z]*)
-		eval "$i=./e-$i.sh"
-		export $i
-		;;
-	esac
-	test_expect_success "Using $i (override)" '
-		PATH="$PWD:$PATH" git commit --amend &&
-		test_commit_message HEAD expect
-	'
-done
+test_expect_success 'Using editors with overrides' '
+	(
+		TERM=vt100 &&
+		export TERM &&
+		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
+		do
+			echo "Edited by $i" >expect &&
+			case "$i" in
+			core_editor)
+				git config core.editor ./e-core_editor.sh
+				;;
+			[A-Z]*)
+				eval "$i=./e-$i.sh" &&
+				export $i
+				;;
+			esac &&
+			PATH="$PWD:$PATH" git commit --amend &&
+			test_commit_message HEAD expect || exit 1
+		done
+	)
+'
 
 test_expect_success 'editor with a space' '
 	echo "echo space >\"\$1\"" >"e space.sh" &&
@@ -97,9 +93,8 @@
 	test_commit_message HEAD -m space
 '
 
-unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-	git config core.editor \"./e\ space.sh\" &&
+	test_config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
 	test_commit_message HEAD -m space
 '
-- 
2.48.1

[PATCH v4 1/3] t7005: use modern test style

[D. Ben Knoble]

Tests in t7005 mask Git error codes and do not use our nice test
helpers. Improve that, move some code into the setup test, and drop a
few old-style blank lines while at it.

Best-viewed-with: --ignore-all-space
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 70 ++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 47 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 5fcf281dfb..791e2a0e74 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -7,62 +7,45 @@
 unset EDITOR VISUAL GIT_EDITOR
 
 test_expect_success 'determine default editor' '
-
 	vi=$(TERM=vt100 git var GIT_EDITOR) &&
 	test -n "$vi"
-
 '
 
-if ! expr "$vi" : '[a-z]*$' >/dev/null
-then
-	vi=
-fi
-
-for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
-do
-	cat >e-$i.sh <<-EOF
-	#!$SHELL_PATH
-	echo "Edited by $i" >"\$1"
-	EOF
-	chmod +x e-$i.sh
-done
-
-if ! test -z "$vi"
-then
-	mv e-$vi.sh $vi
-fi
-
 test_expect_success setup '
+	if ! expr "$vi" : "[a-z]*$" >/dev/null
+	then
+		vi=
+	fi &&
+
+	for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
+	do
+		write_script e-$i.sh <<-EOF || return 1
+			echo "Edited by $i" >"\$1"
+		EOF
+	done &&
+
+	if ! test -z "$vi"
+	then
+		mv e-$vi.sh $vi
+	fi &&
 
 	msg="Hand-edited" &&
 	test_commit "$msg" &&
-	echo "$msg" >expect &&
-	git show -s --format=%s > actual &&
-	test_cmp expect actual
-
+	test_commit_message HEAD -m "$msg"
 '
 
 TERM=dumb
 export TERM
 test_expect_success 'dumb should error out when falling back on vi' '
-
-	if git commit --amend
-	then
-		echo "Oops?"
-		false
-	else
-		: happy
-	fi
+	test_must_fail git commit --amend
 '
 
 test_expect_success 'dumb should prefer EDITOR to VISUAL' '
-
 	EDITOR=./e-EDITOR.sh &&
 	VISUAL=./e-VISUAL.sh &&
 	export EDITOR VISUAL &&
 	git commit --amend &&
-	test "$(git show -s --format=%s)" = "Edited by EDITOR"
-
+	test_commit_message HEAD -m "Edited by EDITOR"
 '
 
 TERM=vt100
@@ -83,9 +66,7 @@
 	esac
 	test_expect_success "Using $i" '
 		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline |
-		sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp expect actual
+		test_commit_message HEAD expect
 	'
 done
 
@@ -105,9 +86,7 @@
 	esac
 	test_expect_success "Using $i (override)" '
 		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline |
-		sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp expect actual
+		test_commit_message HEAD expect
 	'
 done
 
@@ -115,17 +94,14 @@
 	echo "echo space >\"\$1\"" >"e space.sh" &&
 	chmod a+x "e space.sh" &&
 	GIT_EDITOR="./e\ space.sh" git commit --amend &&
-	test space = "$(git show -s --pretty=format:%s)"
-
+	test_commit_message HEAD -m space
 '
 
 unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-
 	git config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
-	test space = "$(git show -s --pretty=format:%s)"
-
+	test_commit_message HEAD -m space
 '
 
 test_done
-- 
2.48.1

[PATCH v4 2/3] t7005: stop abusing --exec-path

[D. Ben Knoble]

We want the editors in this test on PATH, so put them there.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 791e2a0e74..b384eff5d8 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -65,7 +65,7 @@
 		;;
 	esac
 	test_expect_success "Using $i" '
-		git --exec-path=. commit --amend &&
+		test_env PATH="$PWD:$PATH" git commit --amend &&
 		test_commit_message HEAD expect
 	'
 done
@@ -85,7 +85,7 @@
 		;;
 	esac
 	test_expect_success "Using $i (override)" '
-		git --exec-path=. commit --amend &&
+		test_env PATH="$PWD:$PATH" git commit --amend &&
 		test_commit_message HEAD expect
 	'
 done
-- 
2.48.1

[PATCH v4 3/3] t7005: sanitize test environment for subsequent tests

[D. Ben Knoble]

Some of the editor tests manipulate the environment or config in ways
that affect future tests, but those modifications are visible to future
tests and create a footgun for them.

Use test_config, subshells, single-command environment overrides, and
test helpers to automatically undo environment and config modifications
once finished.

Best-viewed-with: --ignore-all-space
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 85 ++++++++++++++++++++++-------------------------
 1 file changed, 40 insertions(+), 45 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b384eff5d8..6cfefa916b 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -34,72 +34,67 @@
 	test_commit_message HEAD -m "$msg"
 '
 
-TERM=dumb
-export TERM
 test_expect_success 'dumb should error out when falling back on vi' '
-	test_must_fail git commit --amend
+	test_env TERM=dumb test_must_fail git commit --amend
 '
 
 test_expect_success 'dumb should prefer EDITOR to VISUAL' '
-	EDITOR=./e-EDITOR.sh &&
-	VISUAL=./e-VISUAL.sh &&
-	export EDITOR VISUAL &&
-	git commit --amend &&
+	test_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
+		git commit --amend &&
 	test_commit_message HEAD -m "Edited by EDITOR"
 '
 
-TERM=vt100
-export TERM
 for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
-	echo "Edited by $i" >expect
-	unset EDITOR VISUAL GIT_EDITOR
-	git config --unset-all core.editor
-	case "$i" in
-	core_editor)
-		git config core.editor ./e-core_editor.sh
-		;;
-	[A-Z]*)
-		eval "$i=./e-$i.sh"
-		export $i
-		;;
-	esac
 	test_expect_success "Using $i" '
-		test_env PATH="$PWD:$PATH" git commit --amend &&
-		test_commit_message HEAD expect
+		if test "$i" = core_editor
+		then
+			test_config core.editor ./e-core_editor.sh
+		fi &&
+		(
+			case "$i" in
+			[A-Z]*)
+				eval "$i=./e-$i.sh" &&
+				export $i
+				;;
+			esac &&
+			test_env PATH="$PWD:$PATH" TERM=vt100 git commit --amend
+		) &&
+		test_commit_message HEAD -m "Edited by $i"
 	'
 done
 
-unset EDITOR VISUAL GIT_EDITOR
-git config --unset-all core.editor
-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
-do
-	echo "Edited by $i" >expect
-	case "$i" in
-	core_editor)
-		git config core.editor ./e-core_editor.sh
-		;;
-	[A-Z]*)
-		eval "$i=./e-$i.sh"
-		export $i
-		;;
-	esac
-	test_expect_success "Using $i (override)" '
-		test_env PATH="$PWD:$PATH" git commit --amend &&
-		test_commit_message HEAD expect
-	'
-done
+test_expect_success 'Using editors with overrides' '
+	(
+		TERM=vt100 &&
+		export TERM &&
+		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
+		do
+			echo "Edited by $i" >expect &&
+			case "$i" in
+			core_editor)
+				git config core.editor ./e-core_editor.sh
+				;;
+			[A-Z]*)
+				eval "$i=./e-$i.sh" &&
+				export $i
+				;;
+			esac &&
+			test_env PATH="$PWD:$PATH" git commit --amend &&
+			test_commit_message HEAD expect || exit 1
+		done
+	)
+'
 
 test_expect_success 'editor with a space' '
 	echo "echo space >\"\$1\"" >"e space.sh" &&
 	chmod a+x "e space.sh" &&
-	GIT_EDITOR="./e\ space.sh" git commit --amend &&
+	test_env GIT_EDITOR="./e\ space.sh" git commit --amend &&
 	test_commit_message HEAD -m space
 '
 
-unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-	git config core.editor \"./e\ space.sh\" &&
+	test_config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
 	test_commit_message HEAD -m space
 '
-- 
2.48.1

[PATCH v3 1/4] t7005: use modern test style

[D. Ben Knoble]

Tests in t7005 mask Git error codes and do not use our nice test
helpers. Improve that, move some code into the setup test, and drop a
few old-style blank lines while at it.

Best-viewed-with: --ignore-all-space
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 70 ++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 47 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 5fcf281dfb..791e2a0e74 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -7,62 +7,45 @@
 unset EDITOR VISUAL GIT_EDITOR
 
 test_expect_success 'determine default editor' '
-
 	vi=$(TERM=vt100 git var GIT_EDITOR) &&
 	test -n "$vi"
-
 '
 
-if ! expr "$vi" : '[a-z]*$' >/dev/null
-then
-	vi=
-fi
-
-for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
-do
-	cat >e-$i.sh <<-EOF
-	#!$SHELL_PATH
-	echo "Edited by $i" >"\$1"
-	EOF
-	chmod +x e-$i.sh
-done
-
-if ! test -z "$vi"
-then
-	mv e-$vi.sh $vi
-fi
-
 test_expect_success setup '
+	if ! expr "$vi" : "[a-z]*$" >/dev/null
+	then
+		vi=
+	fi &&
+
+	for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
+	do
+		write_script e-$i.sh <<-EOF || return 1
+			echo "Edited by $i" >"\$1"
+		EOF
+	done &&
+
+	if ! test -z "$vi"
+	then
+		mv e-$vi.sh $vi
+	fi &&
 
 	msg="Hand-edited" &&
 	test_commit "$msg" &&
-	echo "$msg" >expect &&
-	git show -s --format=%s > actual &&
-	test_cmp expect actual
-
+	test_commit_message HEAD -m "$msg"
 '
 
 TERM=dumb
 export TERM
 test_expect_success 'dumb should error out when falling back on vi' '
-
-	if git commit --amend
-	then
-		echo "Oops?"
-		false
-	else
-		: happy
-	fi
+	test_must_fail git commit --amend
 '
 
 test_expect_success 'dumb should prefer EDITOR to VISUAL' '
-
 	EDITOR=./e-EDITOR.sh &&
 	VISUAL=./e-VISUAL.sh &&
 	export EDITOR VISUAL &&
 	git commit --amend &&
-	test "$(git show -s --format=%s)" = "Edited by EDITOR"
-
+	test_commit_message HEAD -m "Edited by EDITOR"
 '
 
 TERM=vt100
@@ -83,9 +66,7 @@
 	esac
 	test_expect_success "Using $i" '
 		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline |
-		sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp expect actual
+		test_commit_message HEAD expect
 	'
 done
 
@@ -105,9 +86,7 @@
 	esac
 	test_expect_success "Using $i (override)" '
 		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline |
-		sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp expect actual
+		test_commit_message HEAD expect
 	'
 done
 
@@ -115,17 +94,14 @@
 	echo "echo space >\"\$1\"" >"e space.sh" &&
 	chmod a+x "e space.sh" &&
 	GIT_EDITOR="./e\ space.sh" git commit --amend &&
-	test space = "$(git show -s --pretty=format:%s)"
-
+	test_commit_message HEAD -m space
 '
 
 unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-
 	git config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
-	test space = "$(git show -s --pretty=format:%s)"
-
+	test_commit_message HEAD -m space
 '
 
 test_done
-- 
2.48.1

[PATCH v3 2/4] t7005: stop abusing --exec-path

[D. Ben Knoble]

We want the editors in this test on PATH, so put them there.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 791e2a0e74..0a5861b7f0 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -65,7 +65,7 @@
 		;;
 	esac
 	test_expect_success "Using $i" '
-		git --exec-path=. commit --amend &&
+		PATH="$PWD:$PATH" git commit --amend &&
 		test_commit_message HEAD expect
 	'
 done
@@ -85,7 +85,7 @@
 		;;
 	esac
 	test_expect_success "Using $i (override)" '
-		git --exec-path=. commit --amend &&
+		PATH="$PWD:$PATH" git commit --amend &&
 		test_commit_message HEAD expect
 	'
 done
-- 
2.48.1

[PATCH v3 3/4] t7005: sanitize test environment for subsequent tests

[D. Ben Knoble]

Some of the editor tests manipulate the environment or config in ways
that affect future tests, but those modifications are visible to future
tests and create a footgun for them.

Use test_config, subshells, single-command environment overrides, and
test helpers to automatically undo environment and config modifications
once finished.

Best-viewed-with: --ignore-all-space
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 83 ++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 44 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 0a5861b7f0..3a5f4b897e 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -34,61 +34,57 @@
 	test_commit_message HEAD -m "$msg"
 '
 
-TERM=dumb
-export TERM
 test_expect_success 'dumb should error out when falling back on vi' '
-	test_must_fail git commit --amend
+	TERM=dumb test_must_fail git commit --amend
 '
 
 test_expect_success 'dumb should prefer EDITOR to VISUAL' '
-	EDITOR=./e-EDITOR.sh &&
-	VISUAL=./e-VISUAL.sh &&
-	export EDITOR VISUAL &&
-	git commit --amend &&
+	TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
+		git commit --amend &&
 	test_commit_message HEAD -m "Edited by EDITOR"
 '
 
-TERM=vt100
-export TERM
 for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
-	echo "Edited by $i" >expect
-	unset EDITOR VISUAL GIT_EDITOR
-	git config --unset-all core.editor
-	case "$i" in
-	core_editor)
-		git config core.editor ./e-core_editor.sh
-		;;
-	[A-Z]*)
-		eval "$i=./e-$i.sh"
-		export $i
-		;;
-	esac
 	test_expect_success "Using $i" '
-		PATH="$PWD:$PATH" git commit --amend &&
-		test_commit_message HEAD expect
+		if test "$i" = core_editor
+		then
+			test_config core.editor ./e-core_editor.sh
+		fi &&
+		(
+			case "$i" in
+			[A-Z]*)
+				eval "$i=./e-$i.sh" &&
+				export $i
+				;;
+			esac &&
+			PATH="$PWD:$PATH" TERM=vt100 git commit --amend
+		) &&
+		test_commit_message HEAD -m "Edited by $i"
 	'
 done
 
-unset EDITOR VISUAL GIT_EDITOR
-git config --unset-all core.editor
-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
-do
-	echo "Edited by $i" >expect
-	case "$i" in
-	core_editor)
-		git config core.editor ./e-core_editor.sh
-		;;
-	[A-Z]*)
-		eval "$i=./e-$i.sh"
-		export $i
-		;;
-	esac
-	test_expect_success "Using $i (override)" '
-		PATH="$PWD:$PATH" git commit --amend &&
-		test_commit_message HEAD expect
-	'
-done
+test_expect_success 'Using editors with overrides' '
+	(
+		TERM=vt100 &&
+		export TERM &&
+		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
+		do
+			echo "Edited by $i" >expect &&
+			case "$i" in
+			core_editor)
+				git config core.editor ./e-core_editor.sh
+				;;
+			[A-Z]*)
+				eval "$i=./e-$i.sh" &&
+				export $i
+				;;
+			esac &&
+			PATH="$PWD:$PATH" git commit --amend &&
+			test_commit_message HEAD expect || exit 1
+		done
+	)
+'
 
 test_expect_success 'editor with a space' '
 	echo "echo space >\"\$1\"" >"e space.sh" &&
@@ -97,9 +93,8 @@
 	test_commit_message HEAD -m space
 '
 
-unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-	git config core.editor \"./e\ space.sh\" &&
+	test_config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
 	test_commit_message HEAD -m space
 '
-- 
2.48.1

Re: [PATCH v3 3/4] t7005: sanitize test environment for subsequent tests

[Eric Sunshine]

On Mon, Aug 11, 2025 at 6:17 PM D. Ben Knoble
<ben.knoble+github@gmail.com> wrote:
> Some of the editor tests manipulate the environment or config in ways
> that affect future tests, but those modifications are visible to future
> tests and create a footgun for them.
>
> Use test_config, subshells, single-command environment overrides, and
> test helpers to automatically undo environment and config modifications
> once finished.
>
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> @@ -34,61 +34,57 @@
> -TERM=dumb
> -export TERM
>  test_expect_success 'dumb should error out when falling back on vi' '
> -       test_must_fail git commit --amend
> +       TERM=dumb test_must_fail git commit --amend
>  '

Don't use one-shot environment variable assignments when calling shell
functions. Instead, you can do this:

    test_env TERM=dumb test_must_fail git commit --amend

or employ the standard assignment/export boilerplate:

    TERM=dumb &&
    export TERM &&
    test_must_fail git commit --amend

References:
https://lore.kernel.org/git/20240727053509.34339-1-ericsunshine@charter.net/T/#u
https://lore.kernel.org/git/20180713055205.32351-1-sunshine@sunshineco.com/T/#u

Re: [PATCH v3 3/4] t7005: sanitize test environment for subsequent tests

[D. Ben Knoble]

On Mon, Aug 11, 2025 at 6:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Aug 11, 2025 at 6:17 PM D. Ben Knoble
> <ben.knoble+github@gmail.com> wrote:
> > Some of the editor tests manipulate the environment or config in ways
> > that affect future tests, but those modifications are visible to future
> > tests and create a footgun for them.
> >
> > Use test_config, subshells, single-command environment overrides, and
> > test helpers to automatically undo environment and config modifications
> > once finished.
> >
> > Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> > ---
> > diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> > @@ -34,61 +34,57 @@
> > -TERM=dumb
> > -export TERM
> >  test_expect_success 'dumb should error out when falling back on vi' '
> > -       test_must_fail git commit --amend
> > +       TERM=dumb test_must_fail git commit --amend
> >  '
>
> Don't use one-shot environment variable assignments when calling shell
> functions. Instead, you can do this:
>
>     test_env TERM=dumb test_must_fail git commit --amend
>
> or employ the standard assignment/export boilerplate:
>
>     TERM=dumb &&
>     export TERM &&
>     test_must_fail git commit --amend
>
> References:
> https://lore.kernel.org/git/20240727053509.34339-1-ericsunshine@charter.net/T/#u
> https://lore.kernel.org/git/20180713055205.32351-1-sunshine@sunshineco.com/T/#u

Yep ;) I had the latter, switched (see range-diff, I think), and then
CI caught me. Why doesn't the local test run catch it, though?

Re: [PATCH v3 3/4] t7005: sanitize test environment for subsequent tests

[Eric Sunshine]

On Tue, Aug 12, 2025 at 12:41 PM D. Ben Knoble
<ben.knoble+github@gmail.com> wrote:
> On Mon, Aug 11, 2025 at 6:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Mon, Aug 11, 2025 at 6:17 PM D. Ben Knoble
> > <ben.knoble+github@gmail.com> wrote:
> > > +       TERM=dumb test_must_fail git commit --amend
> >
> > Don't use one-shot environment variable assignments when calling shell
> > functions. Instead, you can do this:
> >
> >     test_env TERM=dumb test_must_fail git commit --amend
>
> Yep ;) I had the latter, switched (see range-diff, I think), and then
> CI caught me. Why doesn't the local test run catch it, though?

Although the `chainlint` linter gets wired into each test script, thus
is invoked automatically when manually running any individual test
script, the `check-non-portable-shell` linter does not get wired into
each test script, thus does not get run automatically when manually
running a test script.

Almost all of the "lints" performed by `check-non-portable`
could/should eventually be folded into `chainlint`, thus eliminating
this difference in behavior, but the
"shell-function-one-shot-variable-assignment" lint is an outlier
because it can't be easily localized. Specifically, whereas all the
other lints can operate just by consulting local context, that one
lint can't because it can't know what command names are in fact shell
functions without looking at other test scripts, as well. Hence, the
operation of check-non-portable-shell.sh is two-phase: (1) it first
scans all scripts to find all shell function definitions, and then (2)
it "lints" each test script individually, in the process consulting
the table of function names compiled in step 1.

When you run a test script manually, the script triggers a run of
`chainlint`, but `chainlint` only knows about that one script it's
checking, thus it can't compile a table of shell functions which might
be defined in other scripts. Obviously, there is no technical reason
that this couldn't be implemented directly in `chainlint`; for
instance, `chainlint` could detect other scripts which are pulled in
by the script it is testing and compile a table of names of shell
functions defined in those scripts. Eventually, I think it would be
nice for `chainlint` to subsume all the checks performed by
`check-non-portable-shell`, thus eliminating the latter script and
(hopefully) speeding up linting altogether (at least a bit) but the
work to do so simply hasn't been done yet.

[PATCH v3 4/4] editor: use standard strvec API to receive environment for external editors

[D. Ben Knoble]

Going back to the introduction of the env parameter for the editor in
8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as
well., 2007-11-26), we pass a constant array of strings: as the
surrounding APIs evolved to use strvecs (see 8d7aa4ba6a
(builtin/commit.c: remove the PATH_MAX limitation via dynamic
allocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',
2020-08-10)), the editor code did not.

There is only one caller of all 3 editor APIs that does not pass a NULL
environment (the same caller for which this parameter was added), and
it already has a strvec available to use.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 builtin/commit.c |  2 +-
 editor.c         | 10 +++++-----
 editor.h         |  7 ++++---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index b5b9608813..16cad7fb03 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1116,7 +1116,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct strvec env = STRVEC_INIT;
 
 		strvec_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
-		if (launch_editor(git_path_commit_editmsg(), NULL, env.v)) {
+		if (launch_editor(git_path_commit_editmsg(), NULL, &env)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
diff --git a/editor.c b/editor.c
index fd174e6a03..0bc781d50c 100644
--- a/editor.c
+++ b/editor.c
@@ -58,7 +58,7 @@ const char *git_sequence_editor(void)
 }
 
 static int launch_specified_editor(const char *editor, const char *path,
-				   struct strbuf *buffer, const char *const *env)
+				   struct strbuf *buffer, const struct strvec *env)
 {
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
@@ -89,7 +89,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 
 		strvec_pushl(&p.args, editor, realpath.buf, NULL);
 		if (env)
-			strvec_pushv(&p.env, (const char **)env);
+			strvec_pushv(&p.env, env->v);
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
 		if (start_command(&p) < 0) {
@@ -124,20 +124,20 @@ static int launch_specified_editor(const char *editor, const char *path,
 	return 0;
 }
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+int launch_editor(const char *path, struct strbuf *buffer, const struct strvec *env)
 {
 	return launch_specified_editor(git_editor(), path, buffer, env);
 }
 
 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env)
+			   const struct strvec *env)
 {
 	return launch_specified_editor(git_sequence_editor(), path, buffer, env);
 }
 
 int strbuf_edit_interactively(struct repository *r,
 			      struct strbuf *buffer, const char *path,
-			      const char *const *env)
+			      const struct strvec *env)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int fd, res = 0;
diff --git a/editor.h b/editor.h
index f1c41df378..627e992f4d 100644
--- a/editor.h
+++ b/editor.h
@@ -3,6 +3,7 @@
 
 struct repository;
 struct strbuf;
+struct strvec;
 
 const char *git_editor(void);
 const char *git_sequence_editor(void);
@@ -16,10 +17,10 @@ int is_terminal_dumb(void);
  * file's contents are not read into the buffer upon completion.
  */
 int launch_editor(const char *path, struct strbuf *buffer,
-		  const char *const *env);
+		  const struct strvec *env);
 
 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env);
+			   const struct strvec *env);
 
 /*
  * In contrast to `launch_editor()`, this function writes out the contents
@@ -30,6 +31,6 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,
  * If `path` is relative, it refers to a file in the `.git` directory.
  */
 int strbuf_edit_interactively(struct repository *r, struct strbuf *buffer,
-			      const char *path, const char *const *env);
+			      const char *path, const struct strvec *env);
 
 #endif
-- 
2.48.1

Re: [PATCH v3 4/4] editor: use standard strvec API to receive environment for external editors

[Eric Sunshine]

On Mon, Aug 11, 2025 at 6:17 PM D. Ben Knoble
<ben.knoble+github@gmail.com> wrote:
> Going back to the introduction of the env parameter for the editor in
> 8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as
> well., 2007-11-26), we pass a constant array of strings: as the
> surrounding APIs evolved to use strvecs (see 8d7aa4ba6a
> (builtin/commit.c: remove the PATH_MAX limitation via dynamic
> allocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',
> 2020-08-10)), the editor code did not.
>
> There is only one caller of all 3 editor APIs that does not pass a NULL
> environment (the same caller for which this parameter was added), and
> it already has a strvec available to use.

I'm confused as to *why* we want to make this change, and the commit
message doesn't seem to explain the motivation or why doing so is
beneficial.

> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
> diff --git a/editor.h b/editor.h
> @@ -3,6 +3,7 @@
>  int launch_editor(const char *path, struct strbuf *buffer,
> -                 const char *const *env);
> +                 const struct strvec *env);

Typically, we would want to have the API accept the narrowest type in
order to remain as general as possible, but this change makes it a
requirement that a caller must have the much wider `strvec` type at
hand, which seems counterproductive.

Re: [PATCH v3 4/4] editor: use standard strvec API to receive environment for external editors

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>  int launch_editor(const char *path, struct strbuf *buffer,
>> -                 const char *const *env);
>> +                 const struct strvec *env);
>
> Typically, we would want to have the API accept the narrowest type in
> order to remain as general as possible, but this change makes it a
> requirement that a caller must have the much wider `strvec` type at
> hand, which seems counterproductive.

Yup, very good point.  I do not see the point of this change even
after looking at the whole of this patch.  It is not like (being
const) this would make it easier for launch_editor() to further
tweak the environment.  Let's not do this.

[PATCH 1/3] t7005: use modern test style

[D. Ben Knoble]

Tests in t7005 mask Git error codes and do not use our nice helpers for
comparing results. Improve that, and drop a few old-style blank lines
while at it.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 5fcf281dfb..2f59fc0549 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -7,10 +7,8 @@
 unset EDITOR VISUAL GIT_EDITOR
 
 test_expect_success 'determine default editor' '
-
 	vi=$(TERM=vt100 git var GIT_EDITOR) &&
 	test -n "$vi"
-
 '
 
 if ! expr "$vi" : '[a-z]*$' >/dev/null
@@ -33,19 +31,16 @@
 fi
 
 test_expect_success setup '
-
 	msg="Hand-edited" &&
 	test_commit "$msg" &&
 	echo "$msg" >expect &&
-	git show -s --format=%s > actual &&
+	git show -s --format=%s >actual &&
 	test_cmp expect actual
-
 '
 
 TERM=dumb
 export TERM
 test_expect_success 'dumb should error out when falling back on vi' '
-
 	if git commit --amend
 	then
 		echo "Oops?"
@@ -56,13 +51,13 @@
 '
 
 test_expect_success 'dumb should prefer EDITOR to VISUAL' '
-
 	EDITOR=./e-EDITOR.sh &&
 	VISUAL=./e-VISUAL.sh &&
 	export EDITOR VISUAL &&
 	git commit --amend &&
-	test "$(git show -s --format=%s)" = "Edited by EDITOR"
-
+	echo "Edited by EDITOR" >expect &&
+	git show -s --format=%s >actual &&
+	test_cmp expect actual
 '
 
 TERM=vt100
@@ -83,8 +78,8 @@
 	esac
 	test_expect_success "Using $i" '
 		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline |
-		sed -e "s/^[0-9a-f]* //" >actual &&
+		git show -s --pretty=oneline >show &&
+		<show sed -e "s/^[0-9a-f]* //" >actual &&
 		test_cmp expect actual
 	'
 done
@@ -105,8 +100,8 @@
 	esac
 	test_expect_success "Using $i (override)" '
 		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline |
-		sed -e "s/^[0-9a-f]* //" >actual &&
+		git show -s --pretty=oneline >show &&
+		<show sed -e "s/^[0-9a-f]* //" >actual &&
 		test_cmp expect actual
 	'
 done
@@ -115,17 +110,18 @@
 	echo "echo space >\"\$1\"" >"e space.sh" &&
 	chmod a+x "e space.sh" &&
 	GIT_EDITOR="./e\ space.sh" git commit --amend &&
-	test space = "$(git show -s --pretty=format:%s)"
-
+	echo space >expect &&
+	git show -s --pretty=tformat:%s >actual &&
+	test_cmp expect actual
 '
 
 unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-
 	git config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
-	test space = "$(git show -s --pretty=format:%s)"
-
+	echo space >expect &&
+	git show -s --pretty=tformat:%s >actual &&
+	test_cmp expect actual
 '
 
 test_done
-- 
2.48.1

Re: [PATCH 1/3] t7005: use modern test style

[Phillip Wood]

Hi Ben

Thanks for cleaning up these tests

On 10/08/2025 17:03, D. Ben Knoble wrote:
> 
>   TERM=dumb
>   export TERM
>   test_expect_success 'dumb should error out when falling back on vi' '
> -
>   	if git commit --amend

Instead of this "if" we should be using test_must_fail here

>   	then
>   		echo "Oops?"
> @@ -56,13 +51,13 @@
>   '
>   
>   test_expect_success 'dumb should prefer EDITOR to VISUAL' '
> -
>   	EDITOR=./e-EDITOR.sh &&
>   	VISUAL=./e-VISUAL.sh &&
>   	export EDITOR VISUAL &&
>   	git commit --amend &&
> -	test "$(git show -s --format=%s)" = "Edited by EDITOR"
> -
> +	echo "Edited by EDITOR" >expect &&
> +	git show -s --format=%s >actual &&
> +	test_cmp expect actual

This is a faithful conversion of the original but I'd be tempted to use 
test_commit_message instead as I think we really should be checking the 
whole message rather than just the subject line in all these tests.

	test_commit_message HEAD -m "Edited by EDITOR"

The whitespace cleanups all look good to me

Thanks

Phillip

>   '
>   
>   TERM=vt100
> @@ -83,8 +78,8 @@
>   	esac
>   	test_expect_success "Using $i" '
>   		git --exec-path=. commit --amend &&
> -		git show -s --pretty=oneline |
> -		sed -e "s/^[0-9a-f]* //" >actual &&
> +		git show -s --pretty=oneline >show &&
> +		<show sed -e "s/^[0-9a-f]* //" >actual &&
>   		test_cmp expect actual
>   	'
>   done
> @@ -105,8 +100,8 @@
>   	esac
>   	test_expect_success "Using $i (override)" '
>   		git --exec-path=. commit --amend &&
> -		git show -s --pretty=oneline |
> -		sed -e "s/^[0-9a-f]* //" >actual &&
> +		git show -s --pretty=oneline >show &&
> +		<show sed -e "s/^[0-9a-f]* //" >actual &&
>   		test_cmp expect actual
>   	'
>   done
> @@ -115,17 +110,18 @@
>   	echo "echo space >\"\$1\"" >"e space.sh" &&
>   	chmod a+x "e space.sh" &&
>   	GIT_EDITOR="./e\ space.sh" git commit --amend &&
> -	test space = "$(git show -s --pretty=format:%s)"
> -
> +	echo space >expect &&
> +	git show -s --pretty=tformat:%s >actual &&
> +	test_cmp expect actual
>   '
>   
>   unset GIT_EDITOR
>   test_expect_success 'core.editor with a space' '
> -
>   	git config core.editor \"./e\ space.sh\" &&
>   	git commit --amend &&
> -	test space = "$(git show -s --pretty=format:%s)"
> -
> +	echo space >expect &&
> +	git show -s --pretty=tformat:%s >actual &&
> +	test_cmp expect actual
>   '
>   
>   test_done

Re: [PATCH 1/3] t7005: use modern test style

[Phillip Wood]

Hi Ben

On 10/08/2025 17:03, D. Ben Knoble wrote:
> 
> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> index 5fcf281dfb..2f59fc0549 100755
> --- a/t/t7005-editor.sh
> +++ b/t/t7005-editor.sh
> @@ -7,10 +7,8 @@
>   unset EDITOR VISUAL GIT_EDITOR
>   
>   test_expect_success 'determine default editor' '
> -
>   	vi=$(TERM=vt100 git var GIT_EDITOR) &&
>   	test -n "$vi"
> -
>   '
>   
>   if ! expr "$vi" : '[a-z]*$' >/dev/null

All of the code starting here that is outside of a test could usefully 
be moved inside the "setup" test below it. It could also be cleaned up 
to use write_script() to create the editor scripts as well.

Thanks

Phillip

> @@ -33,19 +31,16 @@
>   fi
>   
>   test_expect_success setup '
> -
>   	msg="Hand-edited" &&
>   	test_commit "$msg" &&
>   	echo "$msg" >expect &&
> -	git show -s --format=%s > actual &&
> +	git show -s --format=%s >actual &&
>   	test_cmp expect actual
> -
>   '
>   
>   TERM=dumb
>   export TERM
>   test_expect_success 'dumb should error out when falling back on vi' '
> -
>   	if git commit --amend
>   	then
>   		echo "Oops?"
> @@ -56,13 +51,13 @@
>   '
>   
>   test_expect_success 'dumb should prefer EDITOR to VISUAL' '
> -
>   	EDITOR=./e-EDITOR.sh &&
>   	VISUAL=./e-VISUAL.sh &&
>   	export EDITOR VISUAL &&
>   	git commit --amend &&
> -	test "$(git show -s --format=%s)" = "Edited by EDITOR"
> -
> +	echo "Edited by EDITOR" >expect &&
> +	git show -s --format=%s >actual &&
> +	test_cmp expect actual
>   '
>   
>   TERM=vt100
> @@ -83,8 +78,8 @@
>   	esac
>   	test_expect_success "Using $i" '
>   		git --exec-path=. commit --amend &&
> -		git show -s --pretty=oneline |
> -		sed -e "s/^[0-9a-f]* //" >actual &&
> +		git show -s --pretty=oneline >show &&
> +		<show sed -e "s/^[0-9a-f]* //" >actual &&
>   		test_cmp expect actual
>   	'
>   done
> @@ -105,8 +100,8 @@
>   	esac
>   	test_expect_success "Using $i (override)" '
>   		git --exec-path=. commit --amend &&
> -		git show -s --pretty=oneline |
> -		sed -e "s/^[0-9a-f]* //" >actual &&
> +		git show -s --pretty=oneline >show &&
> +		<show sed -e "s/^[0-9a-f]* //" >actual &&
>   		test_cmp expect actual
>   	'
>   done
> @@ -115,17 +110,18 @@
>   	echo "echo space >\"\$1\"" >"e space.sh" &&
>   	chmod a+x "e space.sh" &&
>   	GIT_EDITOR="./e\ space.sh" git commit --amend &&
> -	test space = "$(git show -s --pretty=format:%s)"
> -
> +	echo space >expect &&
> +	git show -s --pretty=tformat:%s >actual &&
> +	test_cmp expect actual
>   '
>   
>   unset GIT_EDITOR
>   test_expect_success 'core.editor with a space' '
> -
>   	git config core.editor \"./e\ space.sh\" &&
>   	git commit --amend &&
> -	test space = "$(git show -s --pretty=format:%s)"
> -
> +	echo space >expect &&
> +	git show -s --pretty=tformat:%s >actual &&
> +	test_cmp expect actual
>   '
>   
>   test_done

[PATCH 2/3] t7005: sanitize test environment for subsequent tests

[D. Ben Knoble]

Some of the editor tests manipulate the environment or config in ways
that affect future tests, but those modifications are visible to future
tests and create a footgun for them.

Use test_config, subshells, and test helpers to automatically undo
environment and config modifications once finished.

Best-viewed-with: --ignore-all-space
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t7005-editor.sh | 132 +++++++++++++++++++++++++---------------------
 1 file changed, 71 insertions(+), 61 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 2f59fc0549..eae76cc2f2 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -38,73 +38,84 @@
 	test_cmp expect actual
 '
 
-TERM=dumb
-export TERM
 test_expect_success 'dumb should error out when falling back on vi' '
-	if git commit --amend
-	then
-		echo "Oops?"
-		false
-	else
-		: happy
-	fi
+	(
+		TERM=dumb &&
+		export TERM &&
+		if git commit --amend
+		then
+			echo "Oops?"
+			false
+		else
+			: happy
+		fi
+	)
 '
 
 test_expect_success 'dumb should prefer EDITOR to VISUAL' '
-	EDITOR=./e-EDITOR.sh &&
-	VISUAL=./e-VISUAL.sh &&
-	export EDITOR VISUAL &&
-	git commit --amend &&
-	echo "Edited by EDITOR" >expect &&
-	git show -s --format=%s >actual &&
+	(
+		TERM=dumb &&
+		export TERM &&
+		EDITOR=./e-EDITOR.sh &&
+		VISUAL=./e-VISUAL.sh &&
+		export EDITOR VISUAL &&
+		git commit --amend &&
+		echo "Edited by EDITOR" >expect &&
+		git show -s --format=%s >actual
+	) &&
 	test_cmp expect actual
 '
 
-TERM=vt100
-export TERM
-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
-do
-	echo "Edited by $i" >expect
-	unset EDITOR VISUAL GIT_EDITOR
-	git config --unset-all core.editor
-	case "$i" in
-	core_editor)
-		git config core.editor ./e-core_editor.sh
-		;;
-	[A-Z]*)
-		eval "$i=./e-$i.sh"
-		export $i
-		;;
-	esac
-	test_expect_success "Using $i" '
-		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline >show &&
-		<show sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp expect actual
-	'
-done
+test_expect_success 'Using individual editors' '
+	test_when_finished "test_unconfig --unset-all core.editor" &&
+	(
+		TERM=vt100 &&
+		export TERM &&
+		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
+		do
+			sane_unset EDITOR VISUAL GIT_EDITOR &&
+			test_might_fail git config --unset-all core.editor &&
+			echo "Edited by $i" >expect &&
+			case "$i" in
+			core_editor)
+				git config core.editor ./e-core_editor.sh
+				;;
+			[A-Z]*)
+				eval "$i=./e-$i.sh" &&
+				export $i
+				;;
+			esac &&
+			git --exec-path=. commit --amend &&
+			git show -s --pretty=oneline >show &&
+			<show sed -e "s/^[0-9a-f]* //" >actual &&
+			test_cmp expect actual
+		done
+	)
+'
 
-unset EDITOR VISUAL GIT_EDITOR
-git config --unset-all core.editor
-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
-do
-	echo "Edited by $i" >expect
-	case "$i" in
-	core_editor)
-		git config core.editor ./e-core_editor.sh
-		;;
-	[A-Z]*)
-		eval "$i=./e-$i.sh"
-		export $i
-		;;
-	esac
-	test_expect_success "Using $i (override)" '
-		git --exec-path=. commit --amend &&
-		git show -s --pretty=oneline >show &&
-		<show sed -e "s/^[0-9a-f]* //" >actual &&
-		test_cmp expect actual
-	'
-done
+test_expect_success 'Using editors with overrides' '
+	(
+		TERM=vt100 &&
+		export TERM &&
+		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
+		do
+			echo "Edited by $i" >expect &&
+			case "$i" in
+			core_editor)
+				git config core.editor ./e-core_editor.sh
+				;;
+			[A-Z]*)
+				eval "$i=./e-$i.sh" &&
+				export $i
+				;;
+			esac &&
+			git --exec-path=. commit --amend &&
+			git show -s --pretty=oneline >show &&
+			<show sed -e "s/^[0-9a-f]* //" >actual &&
+			test_cmp expect actual
+		done
+	)
+'
 
 test_expect_success 'editor with a space' '
 	echo "echo space >\"\$1\"" >"e space.sh" &&
@@ -115,9 +126,8 @@
 	test_cmp expect actual
 '
 
-unset GIT_EDITOR
 test_expect_success 'core.editor with a space' '
-	git config core.editor \"./e\ space.sh\" &&
+	test_config core.editor \"./e\ space.sh\" &&
 	git commit --amend &&
 	echo space >expect &&
 	git show -s --pretty=tformat:%s >actual &&
-- 
2.48.1

Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests

[Phillip Wood]

Hi Ben

On 10/08/2025 17:03, D. Ben Knoble wrote:
>   
> -TERM=dumb
> -export TERM
>   test_expect_success 'dumb should error out when falling back on vi' '
> -	if git commit --amend
> -	then
> -		echo "Oops?"
> -		false
> -	else
> -		: happy
> -	fi
> +	(
> +		TERM=dumb &&
> +		export TERM &&
> +		if git commit --amend
> +		then
> +			echo "Oops?"
> +			false
> +		else
> +			: happy
> +		fi
> +	)
>   '
>   
>   test_expect_success 'dumb should prefer EDITOR to VISUAL' '
> -	EDITOR=./e-EDITOR.sh &&
> -	VISUAL=./e-VISUAL.sh &&
> -	export EDITOR VISUAL &&
> -	git commit --amend &&
> -	echo "Edited by EDITOR" >expect &&
> -	git show -s --format=%s >actual &&
> +	(
> +		TERM=dumb &&
> +		export TERM &&

We may as well export this with EDITOR and VISUAL

> +		EDITOR=./e-EDITOR.sh &&
> +		VISUAL=./e-VISUAL.sh &&
> +		export EDITOR VISUAL &&
> +		git commit --amend &&
> +		echo "Edited by EDITOR" >expect &&
> +		git show -s --format=%s >actual
> +	) &&
>   	test_cmp expect actual
>   '
>   
> -TERM=vt100
> -export TERM
> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
> -do
> -	echo "Edited by $i" >expect
> -	unset EDITOR VISUAL GIT_EDITOR
> -	git config --unset-all core.editor
> -	case "$i" in
> -	core_editor)
> -		git config core.editor ./e-core_editor.sh
> -		;;
> -	[A-Z]*)
> -		eval "$i=./e-$i.sh"
> -		export $i
> -		;;
> -	esac
> -	test_expect_success "Using $i" '
> -		git --exec-path=. commit --amend &&
> -		git show -s --pretty=oneline >show &&
> -		<show sed -e "s/^[0-9a-f]* //" >actual &&
> -		test_cmp expect actual
> -	'
> -done
> +test_expect_success 'Using individual editors' '
> +	test_when_finished "test_unconfig --unset-all core.editor" &&
> +	(
> +		TERM=vt100 &&
> +		export TERM &&
> +		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
> +		do
> +			sane_unset EDITOR VISUAL GIT_EDITOR &&
> +			test_might_fail git config --unset-all core.editor &&
> +			echo "Edited by $i" >expect &&
> +			case "$i" in
> +			core_editor)
> +				git config core.editor ./e-core_editor.sh
> +				;;
> +			[A-Z]*)
> +				eval "$i=./e-$i.sh" &&
> +				export $i
> +				;;
> +			esac &&
> +			git --exec-path=. commit --amend &&

It would be nice to stop abusing --exec-path here and in the next test 
by adding the current directory to $PATH with

	PATH="$(pwd):$PATH" git commit --amend

> +			git show -s --pretty=oneline >show &&
> +			<show sed -e "s/^[0-9a-f]* //" >actual &&
> +			test_cmp expect actual

We need to add "|| return 1" to the last line here and in the test below 
to reliably error out when test_cmp fails. I'd have thought that our 
test linting should hove picked this up but maybe it is confused by the 
subshell.

Thanks

Phillip
> +		done
> +	)
> +'
>   
> -unset EDITOR VISUAL GIT_EDITOR
> -git config --unset-all core.editor
> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
> -do
> -	echo "Edited by $i" >expect
> -	case "$i" in
> -	core_editor)
> -		git config core.editor ./e-core_editor.sh
> -		;;
> -	[A-Z]*)
> -		eval "$i=./e-$i.sh"
> -		export $i
> -		;;
> -	esac
> -	test_expect_success "Using $i (override)" '
> -		git --exec-path=. commit --amend &&
> -		git show -s --pretty=oneline >show &&
> -		<show sed -e "s/^[0-9a-f]* //" >actual &&
> -		test_cmp expect actual
> -	'
> -done
> +test_expect_success 'Using editors with overrides' '
> +	(
> +		TERM=vt100 &&
> +		export TERM &&
> +		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
> +		do
> +			echo "Edited by $i" >expect &&
> +			case "$i" in
> +			core_editor)
> +				git config core.editor ./e-core_editor.sh
> +				;;
> +			[A-Z]*)
> +				eval "$i=./e-$i.sh" &&
> +				export $i
> +				;;
> +			esac &&
> +			git --exec-path=. commit --amend &&
> +			git show -s --pretty=oneline >show &&
> +			<show sed -e "s/^[0-9a-f]* //" >actual &&
> +			test_cmp expect actual
> +		done
> +	)
> +'
>   
>   test_expect_success 'editor with a space' '
>   	echo "echo space >\"\$1\"" >"e space.sh" &&
> @@ -115,9 +126,8 @@
>   	test_cmp expect actual
>   '
>   
> -unset GIT_EDITOR
>   test_expect_success 'core.editor with a space' '
> -	git config core.editor \"./e\ space.sh\" &&
> +	test_config core.editor \"./e\ space.sh\" &&
>   	git commit --amend &&
>   	echo space >expect &&
>   	git show -s --pretty=tformat:%s >actual &&

Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests

[Ben Knoble]

> Le 10 août 2025 à 15:44, Phillip Wood <phillip.wood123@gmail.com> a écrit :
> 
> Hi Ben
> 
>> On 10/08/2025 17:03, D. Ben Knoble wrote:
>> +test_expect_success 'Using individual editors' '
>> +    test_when_finished "test_unconfig --unset-all core.editor" &&
>> +    (
>> +        TERM=vt100 &&
>> +        export TERM &&
>> +        for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
>> +        do
>> +            sane_unset EDITOR VISUAL GIT_EDITOR &&
>> +            test_might_fail git config --unset-all core.editor &&
>> +            echo "Edited by $i" >expect &&
>> +            case "$i" in
>> +            core_editor)
>> +                git config core.editor ./e-core_editor.sh
>> +                ;;
>> +            [A-Z]*)
>> +                eval "$i=./e-$i.sh" &&
>> +                export $i
>> +                ;;
>> +            esac &&
>> +            git --exec-path=. commit --amend &&
> [snip]
>> +            git show -s --pretty=oneline >show &&
>> +            <show sed -e "s/^[0-9a-f]* //" >actual &&
>> +            test_cmp expect actual
> 
> We need to add "|| return 1" to the last line here and in the test below to reliably error out when test_cmp fails. I'd have thought that our test linting should hove picked this up but maybe it is confused by the subshell

AFK for a bit, but didn’t want anyone to think the linter was broken:

That’s what I must have lost when rebasing 1/3 from the end of the series to the beginning, good eyes!

In fact I believe it’s what CI flagged, since chainlint is what suggested it. I should have run the tests again before sending this set (I’d been running frequently them while working on this file).

PS I think it’a actually “exit 1” in this case?

Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests

[Eric Sunshine]

On Sun, Aug 10, 2025 at 3:54 PM Ben Knoble <ben.knoble@gmail.com> wrote:
> > Le 10 août 2025 à 15:44, Phillip Wood <phillip.wood123@gmail.com> a écrit :
> >> +            git show -s --pretty=oneline >show &&
> >> +            <show sed -e "s/^[0-9a-f]* //" >actual &&
> >> +            test_cmp expect actual
> >
> > We need to add "|| return 1" to the last line here and in the test below to reliably error out when test_cmp fails. I'd have thought that our test linting should hove picked this up but maybe it is confused by the subshell
>
> PS I think it’a actually “exit 1” in this case?

Indeed, it should be `|| exit 1` since this is inside a subshell.

(I was just about to respond to Phillip's email but continued reading
the thread and saw that you correctly mentioned `|| exit 1` already.
Thanks.)

Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests

[Phillip Wood]

On 10/08/2025 20:44, Phillip Wood wrote:
> On 10/08/2025 17:03, D. Ben Knoble wrote:
>>   '
>> -TERM=vt100
>> -export TERM
>> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
>> -do
>> -    echo "Edited by $i" >expect
>> -    unset EDITOR VISUAL GIT_EDITOR
>> -    git config --unset-all core.editor
>> -    case "$i" in
>> -    core_editor)
>> -        git config core.editor ./e-core_editor.sh
>> -        ;;
>> -    [A-Z]*)
>> -        eval "$i=./e-$i.sh"
>> -        export $i
>> -        ;;
>> -    esac
>> -    test_expect_success "Using $i" '
>> -        git --exec-path=. commit --amend &&
>> -        git show -s --pretty=oneline >show &&
>> -        <show sed -e "s/^[0-9a-f]* //" >actual &&
>> -        test_cmp expect actual
>> -    '
>> -done

Thinking about it some more, for this test we can put the loop outside 
of the subshell and test_expect_success so that we don't have to worry 
about explicitly clearing the variables set from previous iterations.

for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
do
	test_expect_success "Using $i" '
		if test "$1" = core.editor
		then
			test_config core.editor ./e-core_editor.sh
		fi &&
		(
			case "$i" in
			[A-Z]*)
				eval "$i=./e-$i.sh" &&
				export $i
				;;
			esac &&
			PATH="$(pwd):$PATH" \
			    TERM=vt100 git commit --amend &&
		) &&
		test_commit_message HEAD -m "Edited by $i"
	'
done

Thanks

Phillip

>> +test_expect_success 'Using individual editors' '
>> +    test_when_finished "test_unconfig --unset-all core.editor" &&
>> +    (
>> +        TERM=vt100 &&
>> +        export TERM &&
>> +        for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
>> +        do
>> +            sane_unset EDITOR VISUAL GIT_EDITOR &&
>> +            test_might_fail git config --unset-all core.editor &&
>> +            echo "Edited by $i" >expect &&
>> +            case "$i" in
>> +            core_editor)
>> +                git config core.editor ./e-core_editor.sh
>> +                ;;
>> +            [A-Z]*)
>> +                eval "$i=./e-$i.sh" &&
>> +                export $i
>> +                ;;
>> +            esac &&
>> +            git --exec-path=. commit --amend &&
> 
> It would be nice to stop abusing --exec-path here and in the next test 
> by adding the current directory to $PATH with
> 
>      PATH="$(pwd):$PATH" git commit --amend
> 
>> +            git show -s --pretty=oneline >show &&
>> +            <show sed -e "s/^[0-9a-f]* //" >actual &&
>> +            test_cmp expect actual
> 
> We need to add "|| return 1" to the last line here and in the test below 
> to reliably error out when test_cmp fails. I'd have thought that our 
> test linting should hove picked this up but maybe it is confused by the 
> subshell.
> 
> Thanks
> 
> Phillip
>> +        done
>> +    )
>> +'
>> -unset EDITOR VISUAL GIT_EDITOR
>> -git config --unset-all core.editor
>> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
>> -do
>> -    echo "Edited by $i" >expect
>> -    case "$i" in
>> -    core_editor)
>> -        git config core.editor ./e-core_editor.sh
>> -        ;;
>> -    [A-Z]*)
>> -        eval "$i=./e-$i.sh"
>> -        export $i
>> -        ;;
>> -    esac
>> -    test_expect_success "Using $i (override)" '
>> -        git --exec-path=. commit --amend &&
>> -        git show -s --pretty=oneline >show &&
>> -        <show sed -e "s/^[0-9a-f]* //" >actual &&
>> -        test_cmp expect actual
>> -    '
>> -done
>> +test_expect_success 'Using editors with overrides' '
>> +    (
>> +        TERM=vt100 &&
>> +        export TERM &&
>> +        for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
>> +        do
>> +            echo "Edited by $i" >expect &&
>> +            case "$i" in
>> +            core_editor)
>> +                git config core.editor ./e-core_editor.sh
>> +                ;;
>> +            [A-Z]*)
>> +                eval "$i=./e-$i.sh" &&
>> +                export $i
>> +                ;;
>> +            esac &&
>> +            git --exec-path=. commit --amend &&
>> +            git show -s --pretty=oneline >show &&
>> +            <show sed -e "s/^[0-9a-f]* //" >actual &&
>> +            test_cmp expect actual
>> +        done
>> +    )
>> +'
>>   test_expect_success 'editor with a space' '
>>       echo "echo space >\"\$1\"" >"e space.sh" &&
>> @@ -115,9 +126,8 @@
>>       test_cmp expect actual
>>   '
>> -unset GIT_EDITOR
>>   test_expect_success 'core.editor with a space' '
>> -    git config core.editor \"./e\ space.sh\" &&
>> +    test_config core.editor \"./e\ space.sh\" &&
>>       git commit --amend &&
>>       echo space >expect &&
>>       git show -s --pretty=tformat:%s >actual &&
> 

[PATCH 3/3] editor: use standard strvec API to receive environment for external editors

[D. Ben Knoble]

Going back to the introduction of the env parameter for the editor in
8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as
well., 2007-11-26), we pass a constant array of strings: as the
surrounding APIs evolved to use strvecs (see 8d7aa4ba6a
(builtin/commit.c: remove the PATH_MAX limitation via dynamic
allocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',
2020-08-10)), the editor code did not.

There is only one caller of all 3 editor APIs that does not pass a NULL
environment (the same caller for which this parameter was added), and
it already has a strvec available to use.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 builtin/commit.c |  2 +-
 editor.c         | 10 +++++-----
 editor.h         |  7 ++++---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index b5b9608813..16cad7fb03 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1116,7 +1116,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		struct strvec env = STRVEC_INIT;
 
 		strvec_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
-		if (launch_editor(git_path_commit_editmsg(), NULL, env.v)) {
+		if (launch_editor(git_path_commit_editmsg(), NULL, &env)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
diff --git a/editor.c b/editor.c
index fd174e6a03..0bc781d50c 100644
--- a/editor.c
+++ b/editor.c
@@ -58,7 +58,7 @@ const char *git_sequence_editor(void)
 }
 
 static int launch_specified_editor(const char *editor, const char *path,
-				   struct strbuf *buffer, const char *const *env)
+				   struct strbuf *buffer, const struct strvec *env)
 {
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
@@ -89,7 +89,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 
 		strvec_pushl(&p.args, editor, realpath.buf, NULL);
 		if (env)
-			strvec_pushv(&p.env, (const char **)env);
+			strvec_pushv(&p.env, env->v);
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
 		if (start_command(&p) < 0) {
@@ -124,20 +124,20 @@ static int launch_specified_editor(const char *editor, const char *path,
 	return 0;
 }
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+int launch_editor(const char *path, struct strbuf *buffer, const struct strvec *env)
 {
 	return launch_specified_editor(git_editor(), path, buffer, env);
 }
 
 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env)
+			   const struct strvec *env)
 {
 	return launch_specified_editor(git_sequence_editor(), path, buffer, env);
 }
 
 int strbuf_edit_interactively(struct repository *r,
 			      struct strbuf *buffer, const char *path,
-			      const char *const *env)
+			      const struct strvec *env)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int fd, res = 0;
diff --git a/editor.h b/editor.h
index f1c41df378..627e992f4d 100644
--- a/editor.h
+++ b/editor.h
@@ -3,6 +3,7 @@
 
 struct repository;
 struct strbuf;
+struct strvec;
 
 const char *git_editor(void);
 const char *git_sequence_editor(void);
@@ -16,10 +17,10 @@ int is_terminal_dumb(void);
  * file's contents are not read into the buffer upon completion.
  */
 int launch_editor(const char *path, struct strbuf *buffer,
-		  const char *const *env);
+		  const struct strvec *env);
 
 int launch_sequence_editor(const char *path, struct strbuf *buffer,
-			   const char *const *env);
+			   const struct strvec *env);
 
 /*
  * In contrast to `launch_editor()`, this function writes out the contents
@@ -30,6 +31,6 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,
  * If `path` is relative, it refers to a file in the `.git` directory.
  */
 int strbuf_edit_interactively(struct repository *r, struct strbuf *buffer,
-			      const char *path, const char *const *env);
+			      const char *path, const struct strvec *env);
 
 #endif
-- 
2.48.1