{"thread":{"id":"63489","subject":"[PATCH 0/4] Drop git-exec-path from non-Git child programs","startedAt":"2025-05-20T19:35:20Z","lastAt":"2025-08-15T16:00:53Z","messageCount":73,"participants":["D. Ben Knoble","Junio C Hamano","Patrick Steinhardt","Phillip Wood","Justin Tobler","Ben Knoble","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"518526","messageId":"20250520193506.95199-1-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":null,"subject":"[PATCH 0/4] Drop git-exec-path from non-Git child programs","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-20T19:34:54Z","receivedAt":"2025-05-20T19:35:20Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"This has caused trouble in the past [1] [2]; after attempting to help\nGit-for-Windows avoid that problem in Vim [3] by recommendation [4], it\nwas suggested that upstreaming the change instead would be a better\nsolution. Indeed, this should work for more uses/editors/etc.\n\n[1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/\n[2]: https://benknoble.github.io/blog/2020/05/22/libexec-git-core-on-path/\n[3]: https://github.com/git-for-windows/build-extra/pull/616\n[4]: https://github.com/benknoble/Dotfiles/issues/143#issuecomment-2869525481\n\nI haven't managed to test this on Windows, so any extra eyeballs there\nare greatly appreciated. I'd also appreciate suggestions to fix the\nmemory leak.\n\nStructure: patches 1 & 2 are cleanups. In particular, patch 1 is essential to\nthe tests in patch 4. I don't think patch 2 is strictly needed. Patch 3\nrefactors a little to make patch 4 take effect on all platforms. Patch 4 does\nthe real work.\n\nD. Ben Knoble (4):\n t7005: sanitize test environment for subsequent tests\n editor: use standard strvec API to receive environment for external\n editors\n run-command: prep_childenv on all platforms\n drop git_exec_path() from non-Git commands' PATH\n\n builtin/commit.c | 2 +-\n editor.c | 10 ++++-----\n editor.h | 7 +++---\n run-command.c | 55 ++++++++++++++++++++++++++++++++++++++++++-----\n t/t7005-editor.sh | 18 +++++++++++++---\n 5 files changed, 75 insertions(+), 17 deletions(-)\n\n\nbase-commit: 7a1d2bd0a596f42a8a7a68d55577967bb454fec0\n-- \n2.48.1\n\n"},{"id":"518527","messageId":"20250520193506.95199-2-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 1/4] t7005: sanitize test environment for subsequent tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-20T19:34:55Z","receivedAt":"2025-05-20T19:35:28Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Some of the editor tests manipulate the environment or config in ways\nthat affect future tests (because they test a sequence of overrides),\nbut those modifications are visible to future tests and create a footgun\nfor them. Use test_config and undo environment modifications once\nfinished.\n\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 5fcf281dfb..06fa1ecd91 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -111,6 +111,8 @@\n \t'\n done\n \n+unset EDITOR VISUAL GIT_EDITOR\n+git config --unset-all core.editor\n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n@@ -119,13 +121,10 @@\n \n '\n \n-unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\n-\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n+\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n \ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n '\n \n test_done\n-- \n2.48.1\n\n"},{"id":"518528","messageId":"20250520193506.95199-3-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 2/4] editor: use standard strvec API to receive environment for external editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-20T19:34:56Z","receivedAt":"2025-05-20T19:35:43Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Going back to the introduction of the env parameter for the editor in\n8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as\nwell., 2007-11-26), we pass a constant array of strings: as the\nsurrounding APIs evolved to use strvecs, the editor code did not.\n\nThere is only one caller of all 3 editor APIs that does not pass a NULL\nenvironment (the same caller for which this parameter was added), and\nit already has a strvec available to use.\n\nSigned-off-by: D. Ben Knoble \nHelped-by: Johannes Schindelin \n---\n builtin/commit.c | 2 +-\n editor.c | 10 +++++-----\n editor.h | 7 ++++---\n 3 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 66bd91fd52..fdda1d1df0 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1100,7 +1100,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstruct strvec env = STRVEC_INIT;\n \n \t\tstrvec_pushf(&env, \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env.v)) {\n+\t\tif (launch_editor(git_path_commit_editmsg(), NULL, &env)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n \t\t\texit(1);\ndiff --git a/editor.c b/editor.c\nindex b79d97b0e7..5bc9d39178 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -58,7 +58,7 @@ const char *git_sequence_editor(void)\n }\n \n static int launch_specified_editor(const char *editor, const char *path,\n-\t\t\t\t struct strbuf *buffer, const char *const *env)\n+\t\t\t\t struct strbuf *buffer, const struct strvec *env)\n {\n \tif (!editor)\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n@@ -89,7 +89,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \n \t\tstrvec_pushl(&p.args, editor, realpath.buf, NULL);\n \t\tif (env)\n-\t\t\tstrvec_pushv(&p.env, (const char **)env);\n+\t\t\tstrvec_pushv(&p.env, env->v);\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n \t\tif (start_command(&p) < 0) {\n@@ -124,20 +124,20 @@ static int launch_specified_editor(const char *editor, const char *path,\n \treturn 0;\n }\n \n-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)\n+int launch_editor(const char *path, struct strbuf *buffer, const struct strvec *env)\n {\n \treturn launch_specified_editor(git_editor(), path, buffer, env);\n }\n \n int launch_sequence_editor(const char *path, struct strbuf *buffer,\n-\t\t\t const char *const *env)\n+\t\t\t const struct strvec *env)\n {\n \treturn launch_specified_editor(git_sequence_editor(), path, buffer, env);\n }\n \n int strbuf_edit_interactively(struct repository *r,\n \t\t\t struct strbuf *buffer, const char *path,\n-\t\t\t const char *const *env)\n+\t\t\t const struct strvec *env)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd, res = 0;\ndiff --git a/editor.h b/editor.h\nindex f1c41df378..627e992f4d 100644\n--- a/editor.h\n+++ b/editor.h\n@@ -3,6 +3,7 @@\n \n struct repository;\n struct strbuf;\n+struct strvec;\n \n const char *git_editor(void);\n const char *git_sequence_editor(void);\n@@ -16,10 +17,10 @@ int is_terminal_dumb(void);\n * file's contents are not read into the buffer upon completion.\n */\n int launch_editor(const char *path, struct strbuf *buffer,\n-\t\t const char *const *env);\n+\t\t const struct strvec *env);\n \n int launch_sequence_editor(const char *path, struct strbuf *buffer,\n-\t\t\t const char *const *env);\n+\t\t\t const struct strvec *env);\n \n /*\n * In contrast to `launch_editor()`, this function writes out the contents\n@@ -30,6 +31,6 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,\n * If `path` is relative, it refers to a file in the `.git` directory.\n */\n int strbuf_edit_interactively(struct repository *r, struct strbuf *buffer,\n-\t\t\t const char *path, const char *const *env);\n+\t\t\t const char *path, const struct strvec *env);\n \n #endif\n-- \n2.48.1\n\n"},{"id":"518529","messageId":"20250520193506.95199-4-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 3/4] run-command: prep_childenv on all platforms","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-20T19:34:57Z","receivedAt":"2025-05-20T19:35:51Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"We only prepare the child environment on non-Windows platforms, but\nprep_childenv is the natural interposition point for our subprocess\nsystem to adjust the environment as needed. Use it for Windows\nplatforms, also. In subsequent commits we'll use this interposition\npoint to modify the environment on all platforms.\n\nSigned-off-by: D. Ben Knoble \n---\n run-command.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 8833b23367..dee6ae3e62 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -680,6 +680,7 @@ int start_command(struct child_process *cmd)\n \tint fdin[2], fdout[2], fderr[2];\n \tint failed_errno;\n \tconst char *str;\n+\tchar **childenv;\n \n \t/*\n \t * In case of errors we must keep the promise to close FDs\n@@ -745,11 +746,12 @@ int start_command(struct child_process *cmd)\n \tif (cmd->close_object_store)\n \t\tclose_object_store(the_repository->objects);\n \n+\tchildenv = prep_childenv(cmd->env.v);\n+\n #ifndef GIT_WINDOWS_NATIVE\n {\n \tint notify_pipe[2];\n \tint null_fd = -1;\n-\tchar **childenv;\n \tstruct strvec argv = STRVEC_INIT;\n \tstruct child_err cerr;\n \tstruct atfork_state as;\n@@ -772,7 +774,6 @@ int start_command(struct child_process *cmd)\n \t\tset_cloexec(null_fd);\n \t}\n \n-\tchildenv = prep_childenv(cmd->env.v);\n \tatfork_prepare(&as);\n \n \t/*\n@@ -893,7 +894,6 @@ int start_command(struct child_process *cmd)\n \tif (null_fd >= 0)\n \t\tclose(null_fd);\n \tstrvec_clear(&argv);\n-\tfree(childenv);\n }\n end_of_spawn:\n \n@@ -933,7 +933,7 @@ int start_command(struct child_process *cmd)\n \n \ttrace_argv_printf(cmd->args.v, \"trace: start_command:\");\n \tcmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v,\n-\t\t\t\t (char**) cmd->env.v,\n+\t\t\t\t childenv,\n \t\t\t\t cmd->dir, fhin, fhout, fherr);\n \tfailed_errno = errno;\n \tif (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))\n@@ -952,6 +952,8 @@ int start_command(struct child_process *cmd)\n }\n #endif\n \n+\tfree(childenv);\n+\n \tif (cmd->pid < 0) {\n \t\ttrace2_child_exit(cmd, -1);\n \n-- \n2.48.1\n\n"},{"id":"518530","messageId":"20250520193506.95199-5-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-20T19:34:58Z","receivedAt":"2025-05-20T19:36:00Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify\nsetup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git\nstarts; as a result, all child processes see Git's exec-path when run,\nincluding editors and other programs that don't need it [1]. This can\ncause confusion for such programs or shells, especially when they rely\non finding \"git\" in PATH to locate other nearby directories, in a\nsimilar vein as a0b4507ef7 (stop putting argv[0] dirname at front of\nPATH, 2015-04-22) solved.\n\nSince we only need this for finding git-* subprocesses, drop it from\nchild processes that aren't Git commands.\n\n[1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/\n\nSigned-off-by: D. Ben Knoble \nSuggested-by: Johannes Schindelin \n---\n\nNotes:\n A few interesting points:\n \n - I'm not sure how best to deal with the memory leak here.\n - Dscho suggested the essence of the patch in\n https://github.com/git-for-windows/build-extra/pull/616#pullrequestreview-2839055049,\n for the curious. My only major tweaks were this diff to skip past\n \"PATH=\" when searching for the matching path (but still modify the\n original buffer; b always points into buf.buf, so the later operations\n with p and buf.buf are valid).\n \n --->8---\n diff --git i/run-command.c w/run-command.c\n index b567e4fdd5..8a8b5c8455 100644\n --- i/run-command.c\n +++ w/run-command.c\n @@ -452,17 +452,24 @@ static void remove_git_exec_path(struct string_list_item *path_item) {\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *exec_path = git_exec_path();\n \tsize_t exec_len = strlen(exec_path);\n -\tchar *p;\n +\tchar *b, *p;\n \n \t/* Value comes from environ; we should not modify it directly. But\n \t * strbuf copies data, so we now have our own playground. */\n \tstrbuf_addstr(&buf, (const char *)path_item->util);\n -\tfor (p = strstr(buf.buf, exec_path); p; p = strstr(p, exec_path)) {\n -\t\tif ((p[exec_len] && p[exec_len] != PATH_SEP) || (p != buf.buf && p[-1] != PATH_SEP))\n +\n +\t/* skip past \"PATH=\" to start search */\n +\tp = strchr(buf.buf, '=');\n +\tif (!p || !*(p + 1))\n +\t\treturn;\n +\tb = p + 1;\n +\n +\tfor (p = strstr(b, exec_path); p; p = strstr(p, exec_path)) {\n +\t\tif ((p[exec_len] && p[exec_len] != PATH_SEP) || (p != b && p[-1] != PATH_SEP))\n \t\t\tp += exec_len; /* false positive, skip */\n \t\telse {\n \t\t\tsize_t offset = p - buf.buf, delete_len = exec_len;\n -\t\t\tif (p != buf.buf) {\n +\t\t\tif (p != b) {\n \t\t\t\t/* include the preceding path separator */\n \t\t\t\toffset--;\n \t\t\t\tdelete_len++;\n --->8---\n \n - I /think/ this resolves the issues in my earlier mail beyond just Git\n builtins; for example, git-jump also doesn't get exec-path because\n it's not invoked with git_cmd set during execv_dashed_external.\n\n run-command.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--\n t/t7005-editor.sh | 13 +++++++++++++\n 2 files changed, 58 insertions(+), 2 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex dee6ae3e62..8a8b5c8455 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -448,11 +448,51 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)\n \treturn 0;\n }\n \n-static char **prep_childenv(const char *const *deltaenv)\n+static void remove_git_exec_path(struct string_list_item *path_item) {\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *exec_path = git_exec_path();\n+\tsize_t exec_len = strlen(exec_path);\n+\tchar *b, *p;\n+\n+\t/* Value comes from environ; we should not modify it directly. But\n+\t * strbuf copies data, so we now have our own playground. */\n+\tstrbuf_addstr(&buf, (const char *)path_item->util);\n+\n+\t/* skip past \"PATH=\" to start search */\n+\tp = strchr(buf.buf, '=');\n+\tif (!p || !*(p + 1))\n+\t\treturn;\n+\tb = p + 1;\n+\n+\tfor (p = strstr(b, exec_path); p; p = strstr(p, exec_path)) {\n+\t\tif ((p[exec_len] && p[exec_len] != PATH_SEP) || (p != b && p[-1] != PATH_SEP))\n+\t\t\tp += exec_len; /* false positive, skip */\n+\t\telse {\n+\t\t\tsize_t offset = p - buf.buf, delete_len = exec_len;\n+\t\t\tif (p != b) {\n+\t\t\t\t/* include the preceding path separator */\n+\t\t\t\toffset--;\n+\t\t\t\tdelete_len++;\n+\t\t\t} else if (p[exec_len] == PATH_SEP) {\n+\t\t\t\t/* include the path separator following GIT_EXEC_PATH */\n+\t\t\t\tdelete_len++;\n+\t\t\t}\n+\t\t\tstrbuf_splice(&buf, offset, delete_len, \"\", 0);\n+\t\t}\n+\t}\n+\n+\t/* Overwrite PATH value with new (owned) data. This leaks memory because\n+\t * the only future owner is a char** childenv, which is freed, but whose\n+\t * contents are not (because most of them come from environ). */\n+\tpath_item->util = (void *)strbuf_detach(&buf, NULL);\n+}\n+\n+static char **prep_childenv(const char *const *deltaenv, unsigned git_cmd)\n {\n \textern char **environ;\n \tchar **childenv;\n \tstruct string_list env = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *path_item;\n \tstruct strbuf key = STRBUF_INIT;\n \tconst char *const *p;\n \tint i;\n@@ -486,6 +526,9 @@ static char **prep_childenv(const char *const *deltaenv)\n \t\t}\n \t}\n \n+\tif (!git_cmd && (path_item = string_list_lookup(&env, \"PATH\")))\n+\t\tremove_git_exec_path(path_item);\n+\n \t/* Create an array of 'char *' to be used as the childenv */\n \tALLOC_ARRAY(childenv, env.nr + 1);\n \tfor (i = 0; i < env.nr; i++)\n@@ -746,7 +789,7 @@ int start_command(struct child_process *cmd)\n \tif (cmd->close_object_store)\n \t\tclose_object_store(the_repository->objects);\n \n-\tchildenv = prep_childenv(cmd->env.v);\n+\tchildenv = prep_childenv(cmd->env.v, cmd->git_cmd);\n \n #ifndef GIT_WINDOWS_NATIVE\n {\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 06fa1ecd91..560e500b53 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -127,4 +127,17 @@\n \ttest space = \"$(git show -s --pretty=format:%s)\"\n '\n \n+test_expect_success 'editor does not see GIT_EXEC_PATH on PATH' '\n+\tcat >e-path <<-EOF &&\n+\t#!$SHELL_PATH\n+\techo \"\\$PATH\" | tr : \"\\\\n\" >actual\n+\tEOF\n+\tchmod +x e-path &&\n+\t(\n+\t\ttest_set_editor ./e-path &&\n+\t\tgit commit --amend\n+\t) &&\n+\ttest_grep ! ^\"$(git --exec-path)\"\\$ actual\n+'\n+\n test_done\n-- \n2.48.1\n\n"},{"id":"518531","messageId":"xmqqy0urggzf.fsf@gitster.g","threadId":"63489","inReplyTo":"20250520193506.95199-5-ben.knoble+github@gmail.com","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-20T20:33:08Z","receivedAt":"2025-05-20T20:33:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\n> We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify\n> setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git\n> starts; as a result, all child processes see Git's exec-path when run,\n> including editors and other programs that don't need it [1]. This can\n> cause confusion for such programs or shells, especially when they rely\n> on finding \"git\" in PATH to locate other nearby directories, in a\n> similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of\n> PATH, 2015-04-22) solved.\n\nI can understand why we want to do this for things like editors, but\ndoing this in start_command() would affect running of things like\nhook scripts (which may in turn invoke \"git\" commands) and command\nrunning on the other side of the pipe that does a local transport\n(e.g., \"git fetch --upload-pack cmd\"), and other commands specified\nvia the configuration (e.g., \"trailer..cmd\") that may in turn\ninvoke \"git\" commands, wouldn't it?\n\n> @@ -746,7 +789,7 @@ int start_command(struct child_process *cmd)\n> \tif (cmd->close_object_store)\n> \t\tclose_object_store(the_repository->objects);\n> \n> -\tchildenv = prep_childenv(cmd->env.v);\n> +\tchildenv = prep_childenv(cmd->env.v, cmd->git_cmd);\n\nRegardless of the answer to the above question, I wonder if the API\nchange for this helper function should go the other direction, by\npassing the entire \"child_process\" structure to the function. That\nwould give prep_childenv() more information to work with to decide\nwhen futzing with PATH is appropriate. \n\nYes, I am assuming that the answer to the above question is \"true,\nunconditionally futzing with PATH is a bad idea\" and we need this\nfix to be more focused, e.g., limit to \"launch_specified_editor()\".\nIf that is the case, we can\n\n - add a member \"drop_git_exec_path\" bit to \"struct child_process\";\n\n - update selected callers, e.g., launch_specified_editor(), that\n prepare child_process to set that bit;\n\n - update prep_childenv() to accept the entire \"struct child_process\",\n instead of cmd->env.v, and base the decision in your logic to\n look at the cmd->drop_git_exec_path bit;\n\nperhaps. It would give us a more surgical and focused update,\nhopefully?\n\n\n\n"},{"id":"518546","messageId":"aC2HF1VEosDMY_A2@pks.im","threadId":"63489","inReplyTo":"20250520193506.95199-3-ben.knoble+github@gmail.com","subject":"Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-21T07:56:07Z","receivedAt":"2025-05-21T07:56:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 20, 2025 at 03:34:56PM -0400, D. Ben Knoble wrote:\n> Going back to the introduction of the env parameter for the editor in\n> 8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as\n> well., 2007-11-26), we pass a constant array of strings: as the\n> surrounding APIs evolved to use strvecs, the editor code did not.\n> \n> There is only one caller of all 3 editor APIs that does not pass a NULL\n> environment (the same caller for which this parameter was added), and\n> it already has a strvec available to use.\n\nOkay. It would've been nice to explain _why_ we want to do this change,\nbut the change itself looks sensible.\n\n> Signed-off-by: D. Ben Knoble \n> Helped-by: Johannes Schindelin \n\nThe order of these trailers should be reversed -- your SOB should always\ncome last.\n\nPatrick\n"},{"id":"518547","messageId":"aC2HI4sMF3t8K4Jv@pks.im","threadId":"63489","inReplyTo":"20250520193506.95199-2-ben.knoble+github@gmail.com","subject":"Re: [PATCH 1/4] t7005: sanitize test environment for subsequent tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-21T07:56:19Z","receivedAt":"2025-05-21T07:56:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 20, 2025 at 03:34:55PM -0400, D. Ben Knoble wrote:\n> Some of the editor tests manipulate the environment or config in ways\n> that affect future tests (because they test a sequence of overrides),\n> but those modifications are visible to future tests and create a footgun\n> for them. Use test_config and undo environment modifications once\n> finished.\n> \n> Signed-off-by: D. Ben Knoble \n> ---\n> t/t7005-editor.sh | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n> \n> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n> index 5fcf281dfb..06fa1ecd91 100755\n> --- a/t/t7005-editor.sh\n> +++ b/t/t7005-editor.sh\n> @@ -111,6 +111,8 @@\n> \t'\n> done\n> \n> +unset EDITOR VISUAL GIT_EDITOR\n> +git config --unset-all core.editor\n> test_expect_success 'editor with a space' '\n> \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n> \tchmod a+x \"e space.sh\" &&\n\nCould we maybe adapt the tests that set those envvars to use a\n`test_when_finished`? Something like this (untested) patch:\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 5fcf281dfbf..a14ff4b38c4 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -93,17 +93,19 @@ unset EDITOR VISUAL GIT_EDITOR\n git config --unset-all core.editor\n for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n do\n-\techo \"Edited by $i\" >expect\n-\tcase \"$i\" in\n-\tcore_editor)\n-\t\tgit config core.editor ./e-core_editor.sh\n-\t\t;;\n-\t[A-Z]*)\n-\t\teval \"$i=./e-$i.sh\"\n-\t\texport $i\n-\t\t;;\n-\tesac\n \ttest_expect_success \"Using $i (override)\" '\n+\t\techo \"Edited by $i\" >expect &&\n+\t\tcase \"$i\" in\n+\t\tcore_editor)\n+\t\t\ttest_config core.editor ./e-core_editor.sh\n+\t\t\t;;\n+\t\t[A-Z]*)\n+\t\t\ttest_when_finished \"unset $i\" &&\n+\t\t\teval \"$i=./e-$i.sh\" &&\n+\t\t\texport $i\n+\t\t\t;;\n+\t\tesac &&\n+\n \t\tgit --exec-path=. commit --amend &&\n \t\tgit show -s --pretty=oneline |\n \t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n\n\n> @@ -119,13 +121,10 @@\n> \n> '\n> \n> -unset GIT_EDITOR\n> test_expect_success 'core.editor with a space' '\n> -\n> -\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n> +\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n> \tgit commit --amend &&\n> \ttest space = \"$(git show -s --pretty=format:%s)\"\n> -\n> '\n\nThis hunk looks good to me.\n\nPatrick\n"},{"id":"518548","messageId":"aC2HJxvrorJdMkL9@pks.im","threadId":"63489","inReplyTo":"20250520193506.95199-4-ben.knoble+github@gmail.com","subject":"Re: [PATCH 3/4] run-command: prep_childenv on all platforms","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-21T07:56:23Z","receivedAt":"2025-05-21T07:56:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 20, 2025 at 03:34:57PM -0400, D. Ben Knoble wrote:\n> We only prepare the child environment on non-Windows platforms, but\n> prep_childenv is the natural interposition point for our subprocess\n> system to adjust the environment as needed. Use it for Windows\n> platforms, also. In subsequent commits we'll use this interposition\n> point to modify the environment on all platforms.\n\nWhat is the consequence though of calling `prep_childenv()` on Windows\nnow? Why didn't we call it before this change? Details like this should\ndefinitely go into the commit message to explain why it's safe to add\nthe call now.\n\nPatrick\n"},{"id":"518549","messageId":"aC2OaeLYJQAOE_S1@pks.im","threadId":"63489","inReplyTo":"20250520193506.95199-5-ben.knoble+github@gmail.com","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-21T08:27:21Z","receivedAt":"2025-05-21T08:27:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 20, 2025 at 03:34:58PM -0400, D. Ben Knoble wrote:\n> We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify\n> setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git\n> starts; as a result, all child processes see Git's exec-path when run,\n> including editors and other programs that don't need it [1]. This can\n> cause confusion for such programs or shells, especially when they rely\n> on finding \"git\" in PATH to locate other nearby directories, in a\n> similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of\n> PATH, 2015-04-22) solved.\n> \n> Since we only need this for finding git-* subprocesses, drop it from\n> child processes that aren't Git commands.\n\nI agree with what Junio mentioned in a parallel thread, especially\naround Git hooks. The expectation there is that those may execute other\nGit commands, and that should typically be using the same execution\nenvironment as the original Git command that has been invoking the hook.\nSo refining this patch so that the mechanism is opt-in probably makes\nsense.\n\n A slight tangent: I wonder whether it is even required nowadays to\n adapt PATH at all anymore. As far as I understand this was a\n requirement back when people still executed dashed binaries\n directly. But nowadays scripts don't really do that anymore, but\n instead use the git binary. And that one doesn't need PATH to be\n adapted at all, as it knows to listen to GIT_EXEC_PATH and its\n built-in path anyway.\n\n So I have to wonder whether this is something that we can deprecate\n to finalize the migration away from dashed builtins. There probably\n are edge cases though. Shell scripts for example still execute \".\n git-sh-setup\", which I think relies on PATH to resolve the script's\n location.\n\n In any case, this is of course outside of the scope of this patch\n series.\n\n> [1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/\n> \n> Signed-off-by: D. Ben Knoble \n> Suggested-by: Johannes Schindelin \n\nSame comment regarding the order of trailers.\n\n> diff --git a/run-command.c b/run-command.c\n> index dee6ae3e62..8a8b5c8455 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -448,11 +448,51 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)\n> \treturn 0;\n> }\n> \n> -static char **prep_childenv(const char *const *deltaenv)\n> +static void remove_git_exec_path(struct string_list_item *path_item) {\n\nStyle: the curly brace should be on its own line.\n\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tconst char *exec_path = git_exec_path();\n> +\tsize_t exec_len = strlen(exec_path);\n> +\tchar *b, *p;\n> +\n> +\t/* Value comes from environ; we should not modify it directly. But\n> +\t * strbuf copies data, so we now have our own playground. */\n\nStyle:\n\n /*\n * Multi-line comments should be written like this, with their\n * opening and closing parts on their own lines.\n */\n\n> +\tstrbuf_addstr(&buf, (const char *)path_item->util);\n> +\n> +\t/* skip past \"PATH=\" to start search */\n> +\tp = strchr(buf.buf, '=');\n> +\tif (!p || !*(p + 1))\n> +\t\treturn;\n> +\tb = p + 1;\n\nYou could use `if (!skip_prefix(buf.buf, \"PATH=\", &p))` instead.\n\n> +\n> +\tfor (p = strstr(b, exec_path); p; p = strstr(p, exec_path)) {\n> +\t\tif ((p[exec_len] && p[exec_len] != PATH_SEP) || (p != b && p[-1] != PATH_SEP))\n> +\t\t\tp += exec_len; /* false positive, skip */\n> +\t\telse {\n> +\t\t\tsize_t offset = p - buf.buf, delete_len = exec_len;\n> +\t\t\tif (p != b) {\n> +\t\t\t\t/* include the preceding path separator */\n> +\t\t\t\toffset--;\n> +\t\t\t\tdelete_len++;\n> +\t\t\t} else if (p[exec_len] == PATH_SEP) {\n> +\t\t\t\t/* include the path separator following GIT_EXEC_PATH */\n> +\t\t\t\tdelete_len++;\n> +\t\t\t}\n> +\t\t\tstrbuf_splice(&buf, offset, delete_len, \"\", 0);\n> +\t\t}\n> +\t}\n> +\n> +\t/* Overwrite PATH value with new (owned) data. This leaks memory because\n> +\t * the only future owner is a char** childenv, which is freed, but whose\n> +\t * contents are not (because most of them come from environ). */\n> +\tpath_item->util = (void *)strbuf_detach(&buf, NULL);\n\nSame comment here regarding style of the comment.\n\nRegarding the memory leak... you could probably track allocations in a\nseparate array and free them via that array. Another alternative would\nbe to just copy the whole environment in case we need to modify it. I\ndoubt that the performance hit would matter given that the cost to spawn\nthe new process is likely to significantly outweigh the additional\nmemory allocations.\n\nPatrick\n"},{"id":"518561","messageId":"e5efc450-dfe1-48fa-9dcd-7e0634957e1f@crinan.ddns.net","threadId":"63489","inReplyTo":"aC2HJxvrorJdMkL9@pks.im","subject":"Re: [PATCH 3/4] run-command: prep_childenv on all platforms","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-21T13:07:04Z","receivedAt":"2025-05-21T13:07:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/05/2025 08:56, Patrick Steinhardt wrote:\n> On Tue, May 20, 2025 at 03:34:57PM -0400, D. Ben Knoble wrote:\n>> We only prepare the child environment on non-Windows platforms, but\n>> prep_childenv is the natural interposition point for our subprocess\n>> system to adjust the environment as needed. Use it for Windows\n>> platforms, also. In subsequent commits we'll use this interposition\n>> point to modify the environment on all platforms.\n> \n> What is the consequence though of calling `prep_childenv()` on Windows\n> now? Why didn't we call it before this change? Details like this should\n> definitely go into the commit message to explain why it's safe to add\n> the call now.\n\nThe environment prep for windows is currently implemented in \ncompat/mingw.c:make_environment_block(). That function is careful to \nhandle unicode characters correctly, it is hard to see how that is \ncompatible this change.\n\nBest Wishes\n\nPhillip\n"},{"id":"518562","messageId":"01301fa6-eea7-427d-a61f-5cb8dac99afd@gmail.com","threadId":"63489","inReplyTo":"aC2OaeLYJQAOE_S1@pks.im","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-21T13:07:25Z","receivedAt":"2025-05-21T13:07:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/05/2025 09:27, Patrick Steinhardt wrote:\n> On Tue, May 20, 2025 at 03:34:58PM -0400, D. Ben Knoble wrote:\n>> We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify\n>> setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git\n>> starts; as a result, all child processes see Git's exec-path when run,\n>> including editors and other programs that don't need it [1]. This can\n>> cause confusion for such programs or shells, especially when they rely\n>> on finding \"git\" in PATH to locate other nearby directories,\n\nI'm skeptical that using the location of the git executable to find \n\"git-jump\" and \"git-completion.zsh\" is a good idea. The location of \nthose (and whether they are packaged at all) is entirely at the \ndiscretion of whoever packages git - they are not installed by the \nMakefile and so there is no fixed relationship between their location in \nthe filesystem and the git executable. For instance the example in Ben's \nblog post looks for \"git-jump\" at \n\"$prefix/share/git-core/contrib/git-jump\" but on my system it is located \nat \"$prefix/share/git/contrib/git-jump\"\n\n> I agree with what Junio mentioned in a parallel thread, especially\n> around Git hooks. The expectation there is that those may execute other\n> Git commands, and that should typically be using the same execution\n> environment as the original Git command that has been invoking the hook.\n> So refining this patch so that the mechanism is opt-in probably makes\n> sense.\n> \n> A slight tangent: I wonder whether it is even required nowadays to\n> adapt PATH at all anymore. As far as I understand this was a\n> requirement back when people still executed dashed binaries\n> directly. But nowadays scripts don't really do that anymore, but\n> instead use the git binary. And that one doesn't need PATH to be\n> adapted at all, as it knows to listen to GIT_EXEC_PATH and its\n> built-in path anyway.\n\nBut don't we still need to change PATH so that hooks, shell aliases, git \nrebase --exec, git bisect run, etc. still run the same git executable \nthat started them? For example \"/usr/bin/git -c alias.g=!git g \n--version\" should report the version of /usr/bin/git, not \n~/.local/bin/git which comes first in my PATH if git doesn't change it.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"518567","messageId":"aC3SZtwQSt0mSVuo@pks.im","threadId":"63489","inReplyTo":"01301fa6-eea7-427d-a61f-5cb8dac99afd@gmail.com","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-21T13:17:26Z","receivedAt":"2025-05-21T13:17:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:\n> On 21/05/2025 09:27, Patrick Steinhardt wrote:\n> > I agree with what Junio mentioned in a parallel thread, especially\n> > around Git hooks. The expectation there is that those may execute other\n> > Git commands, and that should typically be using the same execution\n> > environment as the original Git command that has been invoking the hook.\n> > So refining this patch so that the mechanism is opt-in probably makes\n> > sense.\n> > \n> > A slight tangent: I wonder whether it is even required nowadays to\n> > adapt PATH at all anymore. As far as I understand this was a\n> > requirement back when people still executed dashed binaries\n> > directly. But nowadays scripts don't really do that anymore, but\n> > instead use the git binary. And that one doesn't need PATH to be\n> > adapted at all, as it knows to listen to GIT_EXEC_PATH and its\n> > built-in path anyway.\n> \n> But don't we still need to change PATH so that hooks, shell aliases, git\n> rebase --exec, git bisect run, etc. still run the same git executable that\n> started them? For example \"/usr/bin/git -c alias.g=!git g --version\" should\n> report the version of /usr/bin/git, not ~/.local/bin/git which comes first\n> in my PATH if git doesn't change it.\n\nThere's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust\nPATH indeed to contain the location of the 'git' executable. But we also\nadd GIT_EXEC_PATH to it, which I'm less sure whether it's actually\nneeded.\n\nPatrick\n"},{"id":"518569","messageId":"CALnO6CAer5sZM9qr4G6KsdCEmw6SVEZgp0pdipq72SHvfDsk4A@mail.gmail.com","threadId":"63489","inReplyTo":"aC2HI4sMF3t8K4Jv@pks.im","subject":"Re: [PATCH 1/4] t7005: sanitize test environment for subsequent tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-21T13:23:48Z","receivedAt":"2025-05-21T13:24:05Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Wed, May 21, 2025 at 3:56 AM Patrick Steinhardt wrote:\n>\n> On Tue, May 20, 2025 at 03:34:55PM -0400, D. Ben Knoble wrote:\n> > Some of the editor tests manipulate the environment or config in ways\n> > that affect future tests (because they test a sequence of overrides),\n> > but those modifications are visible to future tests and create a footgun\n> > for them. Use test_config and undo environment modifications once\n> > finished.\n> >\n> > Signed-off-by: D. Ben Knoble \n> > ---\n> > t/t7005-editor.sh | 7 +++----\n> > 1 file changed, 3 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n> > index 5fcf281dfb..06fa1ecd91 100755\n> > --- a/t/t7005-editor.sh\n> > +++ b/t/t7005-editor.sh\n> > @@ -111,6 +111,8 @@\n> > '\n> > done\n> >\n> > +unset EDITOR VISUAL GIT_EDITOR\n> > +git config --unset-all core.editor\n> > test_expect_success 'editor with a space' '\n> > echo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n> > chmod a+x \"e space.sh\" &&\n>\n> Could we maybe adapt the tests that set those envvars to use a\n> `test_when_finished`? Something like this (untested) patch:\n>\n> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n> index 5fcf281dfbf..a14ff4b38c4 100755\n> --- a/t/t7005-editor.sh\n> +++ b/t/t7005-editor.sh\n> @@ -93,17 +93,19 @@ unset EDITOR VISUAL GIT_EDITOR\n> git config --unset-all core.editor\n> for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n> do\n> - echo \"Edited by $i\" >expect\n> - case \"$i\" in\n> - core_editor)\n> - git config core.editor ./e-core_editor.sh\n> - ;;\n> - [A-Z]*)\n> - eval \"$i=./e-$i.sh\"\n> - export $i\n> - ;;\n> - esac\n> test_expect_success \"Using $i (override)\" '\n> + echo \"Edited by $i\" >expect &&\n> + case \"$i\" in\n> + core_editor)\n> + test_config core.editor ./e-core_editor.sh\n> + ;;\n> + [A-Z]*)\n> + test_when_finished \"unset $i\" &&\n> + eval \"$i=./e-$i.sh\" &&\n> + export $i\n> + ;;\n> + esac &&\n> +\n> git --exec-path=. commit --amend &&\n> git show -s --pretty=oneline |\n> sed -e \"s/^[0-9a-f]* //\" >actual &&\n\nThanks for the demo; I considered pushing the test inside the loop,\nbut I assumed (perhaps wrongly?) that the `export` needed to be\n_outside_ the test script in order to correctly test the sequence of\noverrides.\n\nThoughts?\n\n>\n>\n> > @@ -119,13 +121,10 @@\n> >\n> > '\n> >\n> > -unset GIT_EDITOR\n> > test_expect_success 'core.editor with a space' '\n> > -\n> > - git config core.editor \\\"./e\\ space.sh\\\" &&\n> > + test_config core.editor \\\"./e\\ space.sh\\\" &&\n> > git commit --amend &&\n> > test space = \"$(git show -s --pretty=format:%s)\"\n> > -\n> > '\n>\n> This hunk looks good to me.\n>\n> Patrick\n"},{"id":"518571","messageId":"CALnO6CDGGiw2HJTjfhvnmLQpQYDJqQux7UdO1tv0c9-LqNXMxg@mail.gmail.com","threadId":"63489","inReplyTo":"aC2HF1VEosDMY_A2@pks.im","subject":"Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-21T13:26:50Z","receivedAt":"2025-05-21T13:27:08Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Wed, May 21, 2025 at 3:56 AM Patrick Steinhardt wrote:\n>\n> On Tue, May 20, 2025 at 03:34:56PM -0400, D. Ben Knoble wrote:\n> > Going back to the introduction of the env parameter for the editor in\n> > 8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as\n> > well., 2007-11-26), we pass a constant array of strings: as the\n> > surrounding APIs evolved to use strvecs, the editor code did not.\n> >\n> > There is only one caller of all 3 editor APIs that does not pass a NULL\n> > environment (the same caller for which this parameter was added), and\n> > it already has a strvec available to use.\n>\n> Okay. It would've been nice to explain _why_ we want to do this change,\n> but the change itself looks sensible.\n\nAh, yes. I think it's really just \"cleanup\" here—it started out as\n\"maybe necessary prep for later work,\" and I kept it in this series.\nWill expand slightly in reroll.\n\n>\n> > Signed-off-by: D. Ben Knoble \n> > Helped-by: Johannes Schindelin \n>\n> The order of these trailers should be reversed -- your SOB should always\n> come last.\n\nThanks; I didn't know that! (Aside: rebase --signoff seems to add SOB\neven when it's already present. Is that a bug in rebase --signoff or a\nmisuse of the trailer on my end? Setting \"trailer.ifExists =\naddIfDifferent\" didn't seem to affect it.)\n\n>\n> Patrick\n"},{"id":"518572","messageId":"CALnO6CDWoOKZ6NHNT_Xh2Jd48cWU=qHkkwYbbZHfYGvEz5LWGw@mail.gmail.com","threadId":"63489","inReplyTo":"e5efc450-dfe1-48fa-9dcd-7e0634957e1f@crinan.ddns.net","subject":"Re: [PATCH 3/4] run-command: prep_childenv on all platforms","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-21T13:33:38Z","receivedAt":"2025-05-21T13:33:55Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Wed, May 21, 2025 at 9:07 AM Phillip Wood wrote:\n>\n> On 21/05/2025 08:56, Patrick Steinhardt wrote:\n> > On Tue, May 20, 2025 at 03:34:57PM -0400, D. Ben Knoble wrote:\n> >> We only prepare the child environment on non-Windows platforms, but\n> >> prep_childenv is the natural interposition point for our subprocess\n> >> system to adjust the environment as needed. Use it for Windows\n> >> platforms, also. In subsequent commits we'll use this interposition\n> >> point to modify the environment on all platforms.\n> >\n> > What is the consequence though of calling `prep_childenv()` on Windows\n> > now? Why didn't we call it before this change? Details like this should\n> > definitely go into the commit message to explain why it's safe to add\n> > the call now.\n>\n> The environment prep for windows is currently implemented in\n> compat/mingw.c:make_environment_block(). That function is careful to\n> handle unicode characters correctly, it is hard to see how that is\n> compatible this change.\n>\n> Best Wishes\n>\n> Phillip\n\nAh, thanks both—I missed that it got special handling later (since\nrun-command throws the char **env over to mingw_spawnvpe without\nmodification). This patch is probably \"obviously wrong\" with that\ncontext.\n\nThat makes me wonder: per Junio's comments in another spot, we could\nbe more targeted at the launch_editor family (which also requires a\nseparate patch to contrib/git-jump to scratch my itch); or, we could\ntry to push these modifications into both environment handling blocks.\nI lack the knowledge needed to do the right thing on the Windows side,\nthough, so I'd probably leave it alone.\n\nThoughts?\n"},{"id":"518573","messageId":"aC3WSePH3-7jJk-5@pks.im","threadId":"63489","inReplyTo":"CALnO6CDGGiw2HJTjfhvnmLQpQYDJqQux7UdO1tv0c9-LqNXMxg@mail.gmail.com","subject":"Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-21T13:34:01Z","receivedAt":"2025-05-21T13:34:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 21, 2025 at 09:26:50AM -0400, D. Ben Knoble wrote:\n> On Wed, May 21, 2025 at 3:56 AM Patrick Steinhardt wrote:\n> > > Signed-off-by: D. Ben Knoble \n> > > Helped-by: Johannes Schindelin \n> >\n> > The order of these trailers should be reversed -- your SOB should always\n> > come last.\n> \n> Thanks; I didn't know that! (Aside: rebase --signoff seems to add SOB\n> even when it's already present. Is that a bug in rebase --signoff or a\n> misuse of the trailer on my end? Setting \"trailer.ifExists =\n> addIfDifferent\" didn't seem to affect it.)\n\ngit-rebase(1) assumes that your SOB comes last, so that's probably why\nit re-adds it :)\n\nPatrick\n"},{"id":"518575","messageId":"CALnO6CD=qX17v8Hyt06haR+ZjG3xFVC0-622QyXR5eryaRrb8w@mail.gmail.com","threadId":"63489","inReplyTo":"xmqqy0urggzf.fsf@gitster.g","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-21T13:44:34Z","receivedAt":"2025-05-21T13:44:48Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Adding Patrick to CC\n\nOn Tue, May 20, 2025 at 4:33 PM Junio C Hamano wrote:\n>\n> \"D. Ben Knoble\" writes:\n>\n> > We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify\n> > setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git\n> > starts; as a result, all child processes see Git's exec-path when run,\n> > including editors and other programs that don't need it [1]. This can\n> > cause confusion for such programs or shells, especially when they rely\n> > on finding \"git\" in PATH to locate other nearby directories, in a\n> > similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of\n> > PATH, 2015-04-22) solved.\n>\n> I can understand why we want to do this for things like editors, but\n> doing this in start_command() would affect running of things like\n> hook scripts (which may in turn invoke \"git\" commands) and command\n> running on the other side of the pipe that does a local transport\n> (e.g., \"git fetch --upload-pack cmd\"), and other commands specified\n> via the configuration (e.g., \"trailer..cmd\") that may in turn\n> invoke \"git\" commands, wouldn't it?\n\nIt would affect those things, yes, but I'm not sure what /effect/ that\nhas: doesn't adding GIT_EXEC_PATH to PATH only matter if we use the\ndashed forms (\"git-*\" in my original message), which is long\ndeprecated?\n\nPatrick pointed out in a parallel thread that it probably matters for\n\". git-sh-setup\", but that manual explicitly describes using\n\n . \"$(git --exec-path)/git-sh-setup\"\n\nSo I would say the callers that don't do that are wrong ;)\n\n>\n> > @@ -746,7 +789,7 @@ int start_command(struct child_process *cmd)\n> > if (cmd->close_object_store)\n> > close_object_store(the_repository->objects);\n> >\n> > - childenv = prep_childenv(cmd->env.v);\n> > + childenv = prep_childenv(cmd->env.v, cmd->git_cmd);\n>\n> Regardless of the answer to the above question, I wonder if the API\n> change for this helper function should go the other direction, by\n> passing the entire \"child_process\" structure to the function. That\n> would give prep_childenv() more information to work with to decide\n> when futzing with PATH is appropriate.\n>\n> Yes, I am assuming that the answer to the above question is \"true,\n> unconditionally futzing with PATH is a bad idea\" and we need this\n> fix to be more focused, e.g., limit to \"launch_specified_editor()\".\n> If that is the case, we can\n>\n> - add a member \"drop_git_exec_path\" bit to \"struct child_process\";\n>\n> - update selected callers, e.g., launch_specified_editor(), that\n> prepare child_process to set that bit;\n>\n> - update prep_childenv() to accept the entire \"struct child_process\",\n> instead of cmd->env.v, and base the decision in your logic to\n> look at the cmd->drop_git_exec_path bit;\n>\n> perhaps. It would give us a more surgical and focused update,\n> hopefully?\n\nIndeed, a more surgical approach is what Dscho originally suggested\n(IIUC). Failing CI suggests there may be more that relies on\nGIT_EXEC_PATH in PATH than I realized, so I can post an alternative\nseries that should only affect editors (requiring an extra patch for\ncontrib/git-jump, since it invokes an editor), but it is likely to be\n\"incomplete\" in some sense:\n- no Windows support (see replies to 3/4)\n- won't affect all editor invocations, since those launched by a\nscript after retrieving \"git var GIT_EDITOR\" won't necessarily know to\nrollback PATH.\n\nI need to reroll anyway, so I think what I'll do is post a v2 of this\nseries along with a v3 that refocuses on editors? That way we have\nboth around for interested folks in the future.\n"},{"id":"518576","messageId":"CALnO6CCjrYB+ejLfeRaopXW2CytrxtTtOaFDznEfXDuN73EYjQ@mail.gmail.com","threadId":"63489","inReplyTo":"aC2OaeLYJQAOE_S1@pks.im","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-05-21T13:48:01Z","receivedAt":"2025-05-21T13:48:14Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Wed, May 21, 2025 at 4:27 AM Patrick Steinhardt wrote:\n>\n> On Tue, May 20, 2025 at 03:34:58PM -0400, D. Ben Knoble wrote:\n> > We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify\n> > setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git\n> > starts; as a result, all child processes see Git's exec-path when run,\n> > including editors and other programs that don't need it [1]. This can\n> > cause confusion for such programs or shells, especially when they rely\n> > on finding \"git\" in PATH to locate other nearby directories, in a\n> > similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of\n> > PATH, 2015-04-22) solved.\n> >\n> > Since we only need this for finding git-* subprocesses, drop it from\n> > child processes that aren't Git commands.\n>\n> I agree with what Junio mentioned in a parallel thread, especially\n> around Git hooks. The expectation there is that those may execute other\n> Git commands, and that should typically be using the same execution\n> environment as the original Git command that has been invoking the hook.\n> So refining this patch so that the mechanism is opt-in probably makes\n> sense.\n\nOk, will do in a reroll. Though if you have time… see my message in\nthat thread about only affecting uses of \"git-*\" dashed commands.\n\n> A slight tangent: I wonder whether it is even required nowadays to\n> adapt PATH at all anymore. As far as I understand this was a\n> requirement back when people still executed dashed binaries\n> directly. But nowadays scripts don't really do that anymore, but\n> instead use the git binary. And that one doesn't need PATH to be\n> adapted at all, as it knows to listen to GIT_EXEC_PATH and its\n> built-in path anyway.\n\nGood point. I haven't tried running the tests with such a change, and\nGitHub CI at least reports a number of failures on this branch\n(whether that's because 3/4 is wrong for Windows or not, I'm not sure,\nbut I suspect it's actually because 4/4 violates some assumptions).\n\n> So I have to wonder whether this is something that we can deprecate\n> to finalize the migration away from dashed builtins. There probably\n> are edge cases though. Shell scripts for example still execute \".\n> git-sh-setup\", which I think relies on PATH to resolve the script's\n> location.\n\nThose scripts are, at least according to the manual for git-sh-setup, wrong ;)\n\n> In any case, this is of course outside of the scope of this patch\n> series.\n\nNoted; I'll leave it for later to tug that thread.\n\n[snip: style and leak discussion]\n\nAcked\n"},{"id":"518580","messageId":"xmqq1psidnqc.fsf@gitster.g","threadId":"63489","inReplyTo":"aC2OaeLYJQAOE_S1@pks.im","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-21T14:47:55Z","receivedAt":"2025-05-21T14:47:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I agree with what Junio mentioned in a parallel thread, especially\n> around Git hooks. The expectation there is that those may execute other\n> Git commands, and that should typically be using the same execution\n> environment as the original Git command that has been invoking the hook.\n\nIf by \"Giving the same execution environment as the original\" you\nmeant \"not just git-subcmd from anywhere, but from the same place\",\nyou are right.\n\nThe point of GIT_EXEC_PATH is to allow users to tell 'git' that when\nthey say 'git foo', they mean the specific version of 'foo' found at\n$GIT_EXEC_PATH/git-foo, not at $(git --exec-path)/git-foo that may\nbe different; otherwise they wouldn't be passing GIT_EXEC_PATH in\nthe first place.\n\nIn other words, \"These days nobody runs git-foo anymore\" may be a\ncorrect statement, but as an argument to draw the conclusion \"hence\nwe should ignore GIT_EXEC_PATH\", it is an argument that entirely\nmisses the point.\n\n> So refining this patch so that the mechanism is opt-in probably makes\n> sense.\n>\n> A slight tangent: I wonder whether it is even required nowadays to\n> adapt PATH at all anymore.\n\nMeaning we include GIT_EXEC_PATH to PATH? When a running \"git\"\ninvokes a subcommand \"foo\" that is not built-in, it runs \"git-foo\"\nbut doesn't it do so by following the PATH (with the expectation\nthat GIT_EXEC_PATH has already been taken into account, with\nsetup_path() in exec-cmd.c, to include it in PATH)?\n\nSo, as long as you subscribe to \"the same execution environment\"\nlike you said above, I do not think you can avoid the setup_path()\ncall early in git.c::cmd_main().\n\n"},{"id":"518587","messageId":"xmqqa576c83j.fsf@gitster.g","threadId":"63489","inReplyTo":"aC2HF1VEosDMY_A2@pks.im","subject":"Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-21T15:10:56Z","receivedAt":"2025-05-21T15:10:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> Signed-off-by: D. Ben Knoble \n>> Helped-by: Johannes Schindelin \n>\n> The order of these trailers should be reversed -- your SOB should always\n> come last.\n\nYup. The intent is to record what happened before the patch was\nsent out with your sign-off chronologically. Dscho helped and then\nBen finialized the patch by signing off, hence the author's sign-off\ncomes the last.\n\nThanks.\n"},{"id":"518590","messageId":"xmqq1psic7nf.fsf@gitster.g","threadId":"63489","inReplyTo":"CALnO6CDGGiw2HJTjfhvnmLQpQYDJqQux7UdO1tv0c9-LqNXMxg@mail.gmail.com","subject":"Re: [PATCH 2/4] editor: use standard strvec API to receive environment for external editors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-21T15:20:36Z","receivedAt":"2025-05-21T15:20:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\n> Thanks; I didn't know that! (Aside: rebase --signoff seems to add SOB\n> even when it's already present. Is that a bug in rebase --signoff or a\n> misuse of the trailer on my end? Setting \"trailer.ifExists =\n> addIfDifferent\" didn't seem to affect it.)\n\nWhy do \"rebase --signoff\" in the first place? Unless you forgot to,\nthat is. \"I do not remember if I did, so I blindly add it\" is\nsomething we do not want to see, as we want to keep signing-off a\nconcious act.\n\nAnyway, as the intent of the trailer is to record what happened\nuntil the patch was finalized and sent out with your sign-off\nchronologically, if the order of events were\n\n - You wrote the patch, gave it to somebody else with your sign-off,\n to show that it is shared under DCO (a).\n\n - Somebody else may make modification, share it with their\n sign-off under DCO (b).\n\n - You find that their version is a good one, and with or without\n further change of yours, you sign-off to show that you are\n sharing this final version under DCO, either (b) or (c).\n\nit is perfectly fine for your sign-off to appear twice.\n\nThe only case Git's built-in sign-off logic omits adding a sign-off\nis when the same sign-off is sitting at the end. After the above\nevent, if you took that \"final\" version and then gave it to somebody\nelse, with or without further changes, that is still covered by the\nlast sign-off you made, so there is no point adding a duplicate.\n"},{"id":"518593","messageId":"2a8221bf-5edb-449b-a4c9-3754350466d4@gmail.com","threadId":"63489","inReplyTo":"aC3SZtwQSt0mSVuo@pks.im","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-21T15:27:48Z","receivedAt":"2025-05-21T15:27:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/05/2025 14:17, Patrick Steinhardt wrote:\n> On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:\n>>\n>> But don't we still need to change PATH so that hooks, shell aliases, git\n>> rebase --exec, git bisect run, etc. still run the same git executable that\n>> started them? For example \"/usr/bin/git -c alias.g=!git g --version\" should\n>> report the version of /usr/bin/git, not ~/.local/bin/git which comes first\n>> in my PATH if git doesn't change it.\n> \n> There's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust\n> PATH indeed to contain the location of the 'git' executable. But we also\n> add GIT_EXEC_PATH to it, which I'm less sure whether it's actually\n> needed.\n\nIf we were to add /usr/bin to beginning of PATH when the user runs \n/usr/bin/git I think that would be more surprising than adding \nGIT_EXEC_PATH which is what we do now. When we add GIT_EXEC_PATH to the \nbeginning of PATH we only affect the lookup of git's programs. If \ninstead we added /usr/bin to the beginning that would affect the lookup \nof many unrelated programs.\n\nBest Wishes\n\nPhillip\n"},{"id":"518598","messageId":"zhkp5ieg7d3itnwzo6f435snmtb6kseunwgrj2omu4gqemistg@6af5cxhswamk","threadId":"63489","inReplyTo":"aC3SZtwQSt0mSVuo@pks.im","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-05-21T15:50:03Z","receivedAt":"2025-05-21T15:54:43Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/05/21 03:17PM, Patrick Steinhardt wrote:\n> On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:\n> > On 21/05/2025 09:27, Patrick Steinhardt wrote:\n> > > I agree with what Junio mentioned in a parallel thread, especially\n> > > around Git hooks. The expectation there is that those may execute other\n> > > Git commands, and that should typically be using the same execution\n> > > environment as the original Git command that has been invoking the hook.\n> > > So refining this patch so that the mechanism is opt-in probably makes\n> > > sense.\n> > > \n> > > A slight tangent: I wonder whether it is even required nowadays to\n> > > adapt PATH at all anymore. As far as I understand this was a\n> > > requirement back when people still executed dashed binaries\n> > > directly. But nowadays scripts don't really do that anymore, but\n> > > instead use the git binary. And that one doesn't need PATH to be\n> > > adapted at all, as it knows to listen to GIT_EXEC_PATH and its\n> > > built-in path anyway.\n> > \n> > But don't we still need to change PATH so that hooks, shell aliases, git\n> > rebase --exec, git bisect run, etc. still run the same git executable that\n> > started them? For example \"/usr/bin/git -c alias.g=!git g --version\" should\n> > report the version of /usr/bin/git, not ~/.local/bin/git which comes first\n> > in my PATH if git doesn't change it.\n> \n> There's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust\n> PATH indeed to contain the location of the 'git' executable. But we also\n> add GIT_EXEC_PATH to it, which I'm less sure whether it's actually\n> needed.\n\nIn instances where GIT_EXEC_PATH is set on the parent process to\noverride the default, wouldn't we also want this configuration to\npropagate to child Git processes?\n\n-Justin\n"},{"id":"518657","messageId":"aa1f960a-e7a5-4ada-84c0-fbf86a6c873f@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"Re: [PATCH 0/4] Drop git-exec-path from non-Git child programs","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-22T13:21:38Z","receivedAt":"2025-05-22T13:21:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 20/05/2025 20:34, D. Ben Knoble wrote:\n> This has caused trouble in the past [1] [2];\n\nAnother way of looking at this is that the trouble is caused by a script \nthat makes incorrect assumptions about git.\n\nThe assumption that the scripts from contrib as installed at a fixed \nlocation relative to the git binary is false. Where they are installed \nand whether they are installed at all is down to the discretion of the \ndistribution that you're using. Looking for \"git jump\" at a fixed offset \nfrom the git binary is no more portable than looking for it in a fixed \nlocation.\n\nI think that the assumption that git should not change the environment \nwhen it runs the editor is unrealistic. \"git commit file\" will use a \ntemporary index to create the commit and sets GIT_INDEX_FILE when \nrunning the editor. This means that if the editor wants to display the \nstaged changes by running \"git diff --cached HEAD\" the diff will \naccurately represent the changes being committed. Adding GIT_EXEC_PATH \nto the beginning of PATH ensures that the diff will be created by the \nsame version of git the the user ran which avoids subtle bugs where a \nsub-process of git runs a git command using an incompatible version of \ngit. There are several other environment variables that may be set when \nrunning the editor such as GIT_DIR if the command is run from a linked \nwortree.\n\nTo create a clean environment when opening a terminal from your editor \nyou can add\n\n PATH=\"${PATH#$GIT_EXEC_PATH:}\"\n unset $(git rev-parse --local-env-vars)\n\nto your shell setup script.\n\nI think the first two patches are very welcome cleanups but I'm not \nconvinced by the rationale for patches 3 & 4.\n\nBest Wishes\n\nPhillip\n\n> after attempting to help\n> Git-for-Windows avoid that problem in Vim [3] by recommendation [4], it\n> was suggested that upstreaming the change instead would be a better\n> solution. Indeed, this should work for more uses/editors/etc.\n> \n> [1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/\n> [2]: https://benknoble.github.io/blog/2020/05/22/libexec-git-core-on-path/\n> [3]: https://github.com/git-for-windows/build-extra/pull/616\n> [4]: https://github.com/benknoble/Dotfiles/issues/143#issuecomment-2869525481\n> \n> I haven't managed to test this on Windows, so any extra eyeballs there\n> are greatly appreciated. I'd also appreciate suggestions to fix the\n> memory leak.\n> \n> Structure: patches 1 & 2 are cleanups. In particular, patch 1 is essential to\n> the tests in patch 4. I don't think patch 2 is strictly needed. Patch 3\n> refactors a little to make patch 4 take effect on all platforms. Patch 4 does\n> the real work.\n> \n> D. Ben Knoble (4):\n> t7005: sanitize test environment for subsequent tests\n> editor: use standard strvec API to receive environment for external\n> editors\n> run-command: prep_childenv on all platforms\n> drop git_exec_path() from non-Git commands' PATH\n> \n> builtin/commit.c | 2 +-\n> editor.c | 10 ++++-----\n> editor.h | 7 +++---\n> run-command.c | 55 ++++++++++++++++++++++++++++++++++++++++++-----\n> t/t7005-editor.sh | 18 +++++++++++++---\n> 5 files changed, 75 insertions(+), 17 deletions(-)\n> \n> \n> base-commit: 7a1d2bd0a596f42a8a7a68d55577967bb454fec0\n\n"},{"id":"518902","messageId":"aDQKyTOrh3yBe2ux@pks.im","threadId":"63489","inReplyTo":"zhkp5ieg7d3itnwzo6f435snmtb6kseunwgrj2omu4gqemistg@6af5cxhswamk","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-26T06:31:37Z","receivedAt":"2025-05-26T06:31:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 21, 2025 at 10:50:03AM -0500, Justin Tobler wrote:\n> On 25/05/21 03:17PM, Patrick Steinhardt wrote:\n> > On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:\n> > > On 21/05/2025 09:27, Patrick Steinhardt wrote:\n> > > > I agree with what Junio mentioned in a parallel thread, especially\n> > > > around Git hooks. The expectation there is that those may execute other\n> > > > Git commands, and that should typically be using the same execution\n> > > > environment as the original Git command that has been invoking the hook.\n> > > > So refining this patch so that the mechanism is opt-in probably makes\n> > > > sense.\n> > > > \n> > > > A slight tangent: I wonder whether it is even required nowadays to\n> > > > adapt PATH at all anymore. As far as I understand this was a\n> > > > requirement back when people still executed dashed binaries\n> > > > directly. But nowadays scripts don't really do that anymore, but\n> > > > instead use the git binary. And that one doesn't need PATH to be\n> > > > adapted at all, as it knows to listen to GIT_EXEC_PATH and its\n> > > > built-in path anyway.\n> > > \n> > > But don't we still need to change PATH so that hooks, shell aliases, git\n> > > rebase --exec, git bisect run, etc. still run the same git executable that\n> > > started them? For example \"/usr/bin/git -c alias.g=!git g --version\" should\n> > > report the version of /usr/bin/git, not ~/.local/bin/git which comes first\n> > > in my PATH if git doesn't change it.\n> > \n> > There's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust\n> > PATH indeed to contain the location of the 'git' executable. But we also\n> > add GIT_EXEC_PATH to it, which I'm less sure whether it's actually\n> > needed.\n> \n> In instances where GIT_EXEC_PATH is set on the parent process to\n> override the default, wouldn't we also want this configuration to\n> propagate to child Git processes?\n\nYes, but what we should be propagating is GIT_EXEC_PATH, not\nGIT_EXEC_PATH inserted into PATH, shouldn't we?\n\nPatrick\n"},{"id":"518903","messageId":"aDQLWkSKKO5gWbUr@pks.im","threadId":"63489","inReplyTo":"2a8221bf-5edb-449b-a4c9-3754350466d4@gmail.com","subject":"Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-26T06:34:03Z","receivedAt":"2025-05-26T06:34:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 21, 2025 at 04:27:48PM +0100, Phillip Wood wrote:\n> On 21/05/2025 14:17, Patrick Steinhardt wrote:\n> > On Wed, May 21, 2025 at 02:07:25PM +0100, Phillip Wood wrote:\n> > > \n> > > But don't we still need to change PATH so that hooks, shell aliases, git\n> > > rebase --exec, git bisect run, etc. still run the same git executable that\n> > > started them? For example \"/usr/bin/git -c alias.g=!git g --version\" should\n> > > report the version of /usr/bin/git, not ~/.local/bin/git which comes first\n> > > in my PATH if git doesn't change it.\n> > \n> > There's two parts to this: PATH and GIT_EXEC_PATH. We do have to adjust\n> > PATH indeed to contain the location of the 'git' executable. But we also\n> > add GIT_EXEC_PATH to it, which I'm less sure whether it's actually\n> > needed.\n> \n> If we were to add /usr/bin to beginning of PATH when the user runs\n> /usr/bin/git I think that would be more surprising than adding GIT_EXEC_PATH\n> which is what we do now. When we add GIT_EXEC_PATH to the beginning of PATH\n> we only affect the lookup of git's programs. If instead we added /usr/bin to\n> the beginning that would affect the lookup of many unrelated programs.\n\nThat's a valid point indeed.\n\nPatrick\n"},{"id":"523496","messageId":"CALnO6CA5LwY__chY7Di7qkZK16VhFR8MUSRbUV4C+29owsup0A@mail.gmail.com","threadId":"63489","inReplyTo":"aa1f960a-e7a5-4ada-84c0-fbf86a6c873f@gmail.com","subject":"Re: [PATCH 0/4] Drop git-exec-path from non-Git child programs","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-05T01:41:14Z","receivedAt":"2025-08-05T01:41:26Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Thu, May 22, 2025 at 9:21 AM Phillip Wood wrote:\n>\n> Hi Ben\n>\n> On 20/05/2025 20:34, D. Ben Knoble wrote:\n> > This has caused trouble in the past [1] [2];\n>\n> Another way of looking at this is that the trouble is caused by a script\n> that makes incorrect assumptions about git.\n\nI've come around on this idea since fixing my setup to not assume\ncontrib scripts are installed relative to Git (rather, I've hardcoded\nassumptions about where they get installed by my package manager).\n\n>\n> The assumption that the scripts from contrib as installed at a fixed\n> location relative to the git binary is false. Where they are installed\n> and whether they are installed at all is down to the discretion of the\n> distribution that you're using. Looking for \"git jump\" at a fixed offset\n> from the git binary is no more portable than looking for it in a fixed\n> location.\n>\n> I think that the assumption that git should not change the environment\n> when it runs the editor is unrealistic. \"git commit file\" will use a\n> temporary index to create the commit and sets GIT_INDEX_FILE when\n> running the editor. This means that if the editor wants to display the\n> staged changes by running \"git diff --cached HEAD\" the diff will\n> accurately represent the changes being committed. Adding GIT_EXEC_PATH\n> to the beginning of PATH ensures that the diff will be created by the\n> same version of git the the user ran which avoids subtle bugs where a\n> sub-process of git runs a git command using an incompatible version of\n> git. There are several other environment variables that may be set when\n> running the editor such as GIT_DIR if the command is run from a linked\n> wortree.\n>\n> To create a clean environment when opening a terminal from your editor\n> you can add\n>\n> PATH=\"${PATH#$GIT_EXEC_PATH:}\"\n> unset $(git rev-parse --local-env-vars)\n>\n> to your shell setup script.\n>\n> I think the first two patches are very welcome cleanups but I'm not\n> convinced by the rationale for patches 3 & 4.\n\nSo I'll resend the first 2 cleanups and drop the rest.\n"},{"id":"523498","messageId":"20250805024044.30024-1-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 0/2] clean up some code around editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-05T02:40:39Z","receivedAt":"2025-08-05T02:40:56Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"This \"v2\" of the previous exec-path series is simplified to contain only\nthe first 2 cleanup patches, which were largely acked by the list. Drop\nthe controversial and broken PATH munging.\n\nAlso, this version is based on a later master 112648dd6b (Merge branch\n'master' of https://github.com/j6t/git-gui, 2025-08-04) than the\noriginal from May.\n\nThese patches clean up some old code in the editor tests and subsystem\nthat does not use our modern idioms. Patrick previously argued the test\ncleanup doesn't go far enough, and he may be right, but I think the\npreserving the semantics of the test for overrides /and/ automatically\nresetting the environment is tricky, unless we can use a subshell for\nthe whole thing?\n\nv1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/\nPublished-as: https://github.com/benknoble/tree/editor-cleanup\n\nD. Ben Knoble (2):\n t7005: sanitize test environment for subsequent tests\n editor: use standard strvec API to receive environment for external\n editors\n\n builtin/commit.c | 2 +-\n editor.c | 10 +++++-----\n editor.h | 7 ++++---\n t/t7005-editor.sh | 7 +++----\n 4 files changed, 13 insertions(+), 13 deletions(-)\n\nDiff-intervalle :\n1: da4fcc237b ! 1: a37db65107 t7005: sanitize test environment for subsequent tests\n @@ Commit message\n Some of the editor tests manipulate the environment or config in ways\n that affect future tests (because they test a sequence of overrides),\n but those modifications are visible to future tests and create a footgun\n - for them. Use test_config and undo environment modifications once\n - finished.\n + for them.\n +\n + We can't make the environment-munging override tests undo their\n + modifications because they rely on editor variables overriding other\n + previously-set editor variables.\n +\n + Use test_config and undo environment modifications once finished.\n \n Signed-off-by: D. Ben Knoble \n \n2: 7b3b6b08f0 ! 2: 5450c99f59 editor: use standard strvec API to receive environment for external editors\n @@ Commit message\n Going back to the introduction of the env parameter for the editor in\n 8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as\n well., 2007-11-26), we pass a constant array of strings: as the\n - surrounding APIs evolved to use strvecs, the editor code did not.\n + surrounding APIs evolved to use strvecs (see 8d7aa4ba6a\n + (builtin/commit.c: remove the PATH_MAX limitation via dynamic\n + allocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',\n + 2020-08-10)), the editor code did not.\n \n There is only one caller of all 3 editor APIs that does not pass a NULL\n environment (the same caller for which this parameter was added), and\n it already has a strvec available to use.\n \n - Signed-off-by: D. Ben Knoble \n Helped-by: Johannes Schindelin \n + Signed-off-by: D. Ben Knoble \n \n ## builtin/commit.c ##\n @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n3: cb48533115 < -: ---------- run-command: prep_childenv on all platforms\n4: d2e54fdf75 < -: ---------- drop git_exec_path() from non-Git commands' PATH\n\nbase-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be\n-- \n2.48.1\n\n"},{"id":"523499","messageId":"20250805024044.30024-2-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 1/2] t7005: sanitize test environment for subsequent tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-05T02:40:40Z","receivedAt":"2025-08-05T02:41:06Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Some of the editor tests manipulate the environment or config in ways\nthat affect future tests (because they test a sequence of overrides),\nbut those modifications are visible to future tests and create a footgun\nfor them.\n\nWe can't make the environment-munging override tests undo their\nmodifications because they rely on editor variables overriding other\npreviously-set editor variables.\n\nUse test_config and undo environment modifications once finished.\n\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 5fcf281dfb..06fa1ecd91 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -111,6 +111,8 @@\n \t'\n done\n \n+unset EDITOR VISUAL GIT_EDITOR\n+git config --unset-all core.editor\n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n@@ -119,13 +121,10 @@\n \n '\n \n-unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\n-\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n+\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n \ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n '\n \n test_done\n-- \n2.48.1\n\n"},{"id":"523500","messageId":"20250805024044.30024-3-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 2/2] editor: use standard strvec API to receive environment for external editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-05T02:40:41Z","receivedAt":"2025-08-05T02:41:11Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Going back to the introduction of the env parameter for the editor in\n8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as\nwell., 2007-11-26), we pass a constant array of strings: as the\nsurrounding APIs evolved to use strvecs (see 8d7aa4ba6a\n(builtin/commit.c: remove the PATH_MAX limitation via dynamic\nallocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',\n2020-08-10)), the editor code did not.\n\nThere is only one caller of all 3 editor APIs that does not pass a NULL\nenvironment (the same caller for which this parameter was added), and\nit already has a strvec available to use.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: D. Ben Knoble \n---\n builtin/commit.c | 2 +-\n editor.c | 10 +++++-----\n editor.h | 7 ++++---\n 3 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b5b9608813..16cad7fb03 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1116,7 +1116,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstruct strvec env = STRVEC_INIT;\n \n \t\tstrvec_pushf(&env, \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env.v)) {\n+\t\tif (launch_editor(git_path_commit_editmsg(), NULL, &env)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n \t\t\texit(1);\ndiff --git a/editor.c b/editor.c\nindex fd174e6a03..0bc781d50c 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -58,7 +58,7 @@ const char *git_sequence_editor(void)\n }\n \n static int launch_specified_editor(const char *editor, const char *path,\n-\t\t\t\t struct strbuf *buffer, const char *const *env)\n+\t\t\t\t struct strbuf *buffer, const struct strvec *env)\n {\n \tif (!editor)\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n@@ -89,7 +89,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \n \t\tstrvec_pushl(&p.args, editor, realpath.buf, NULL);\n \t\tif (env)\n-\t\t\tstrvec_pushv(&p.env, (const char **)env);\n+\t\t\tstrvec_pushv(&p.env, env->v);\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n \t\tif (start_command(&p) < 0) {\n@@ -124,20 +124,20 @@ static int launch_specified_editor(const char *editor, const char *path,\n \treturn 0;\n }\n \n-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)\n+int launch_editor(const char *path, struct strbuf *buffer, const struct strvec *env)\n {\n \treturn launch_specified_editor(git_editor(), path, buffer, env);\n }\n \n int launch_sequence_editor(const char *path, struct strbuf *buffer,\n-\t\t\t const char *const *env)\n+\t\t\t const struct strvec *env)\n {\n \treturn launch_specified_editor(git_sequence_editor(), path, buffer, env);\n }\n \n int strbuf_edit_interactively(struct repository *r,\n \t\t\t struct strbuf *buffer, const char *path,\n-\t\t\t const char *const *env)\n+\t\t\t const struct strvec *env)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd, res = 0;\ndiff --git a/editor.h b/editor.h\nindex f1c41df378..627e992f4d 100644\n--- a/editor.h\n+++ b/editor.h\n@@ -3,6 +3,7 @@\n \n struct repository;\n struct strbuf;\n+struct strvec;\n \n const char *git_editor(void);\n const char *git_sequence_editor(void);\n@@ -16,10 +17,10 @@ int is_terminal_dumb(void);\n * file's contents are not read into the buffer upon completion.\n */\n int launch_editor(const char *path, struct strbuf *buffer,\n-\t\t const char *const *env);\n+\t\t const struct strvec *env);\n \n int launch_sequence_editor(const char *path, struct strbuf *buffer,\n-\t\t\t const char *const *env);\n+\t\t\t const struct strvec *env);\n \n /*\n * In contrast to `launch_editor()`, this function writes out the contents\n@@ -30,6 +31,6 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,\n * If `path` is relative, it refers to a file in the `.git` directory.\n */\n int strbuf_edit_interactively(struct repository *r, struct strbuf *buffer,\n-\t\t\t const char *path, const char *const *env);\n+\t\t\t const char *path, const struct strvec *env);\n \n #endif\n-- \n2.48.1\n\n"},{"id":"523640","messageId":"350146ae-b909-4d9f-91f3-64bc2ddfcd60@gmail.com","threadId":"63489","inReplyTo":"20250805024044.30024-1-ben.knoble+github@gmail.com","subject":"Re: [PATCH 0/2] clean up some code around editors","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-06T10:07:04Z","receivedAt":"2025-08-06T10:07:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 05/08/2025 03:40, D. Ben Knoble wrote:\n> \n> These patches clean up some old code in the editor tests and subsystem\n> that does not use our modern idioms. Patrick previously argued the test\n> cleanup doesn't go far enough, and he may be right, but I think the\n> preserving the semantics of the test for overrides /and/ automatically\n> resetting the environment is tricky, unless we can use a subshell for\n> the whole thing?\n\nI think we could do it if we put the loop inside a subshell that was \ninside test_expect_success(). That would mean we'd have a single test \nfor all the various combinations. That script could certainly use more \ncleanup to stop running git outside of test_expect_success() and use \nthings like test_must_fail(), write_script(), test_commit_message() etc. \nbut I think that can always come later. The first patch here is a \nwelcome improvement even if it does not fix all the issues in that test \nfile. The second patch looks good too.\n\nThanks for re-submitting these\n\nPhillip\n\n> v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/\n> Published-as: https://github.com/benknoble/tree/editor-cleanup\n> \n> D. Ben Knoble (2):\n> t7005: sanitize test environment for subsequent tests\n> editor: use standard strvec API to receive environment for external\n> editors\n> \n> builtin/commit.c | 2 +-\n> editor.c | 10 +++++-----\n> editor.h | 7 ++++---\n> t/t7005-editor.sh | 7 +++----\n> 4 files changed, 13 insertions(+), 13 deletions(-)\n> \n> Diff-intervalle :\n> 1: da4fcc237b ! 1: a37db65107 t7005: sanitize test environment for subsequent tests\n> @@ Commit message\n> Some of the editor tests manipulate the environment or config in ways\n> that affect future tests (because they test a sequence of overrides),\n> but those modifications are visible to future tests and create a footgun\n> - for them. Use test_config and undo environment modifications once\n> - finished.\n> + for them.\n> +\n> + We can't make the environment-munging override tests undo their\n> + modifications because they rely on editor variables overriding other\n> + previously-set editor variables.\n> +\n> + Use test_config and undo environment modifications once finished.\n> \n> Signed-off-by: D. Ben Knoble \n> \n> 2: 7b3b6b08f0 ! 2: 5450c99f59 editor: use standard strvec API to receive environment for external editors\n> @@ Commit message\n> Going back to the introduction of the env parameter for the editor in\n> 8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as\n> well., 2007-11-26), we pass a constant array of strings: as the\n> - surrounding APIs evolved to use strvecs, the editor code did not.\n> + surrounding APIs evolved to use strvecs (see 8d7aa4ba6a\n> + (builtin/commit.c: remove the PATH_MAX limitation via dynamic\n> + allocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',\n> + 2020-08-10)), the editor code did not.\n> \n> There is only one caller of all 3 editor APIs that does not pass a NULL\n> environment (the same caller for which this parameter was added), and\n> it already has a strvec available to use.\n> \n> - Signed-off-by: D. Ben Knoble \n> Helped-by: Johannes Schindelin \n> + Signed-off-by: D. Ben Knoble \n> \n> ## builtin/commit.c ##\n> @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n> 3: cb48533115 < -: ---------- run-command: prep_childenv on all platforms\n> 4: d2e54fdf75 < -: ---------- drop git_exec_path() from non-Git commands' PATH\n> \n> base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be\n\n"},{"id":"523890","messageId":"20250810160323.49372-1-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 0/3] clean up some code around editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-10T16:03:16Z","receivedAt":"2025-08-10T16:03:31Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Changes from v1:\n- add a prep patch with style fixes to t7005\n- rework the environment munging to use subshells, per Phillip Wood's\n suggestion\n\nThis reroll of the previous exec-path series is simplified to contain\nonly the first 2 cleanup patches, which were largely acked by the list.\nDrop the controversial and broken PATH munging.\n\nAlso, this version is (still) based on a later master 112648dd6b (Merge\nbranch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the\noriginal from May.\n\nThese patches clean up some old code in the editor tests and subsystem\nthat does not use our modern idioms.\n\nv1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/\nPublished-as: https://github.com/benknoble/tree/editor-cleanup\n\nD. Ben Knoble (3):\n t7005: use modern test style\n t7005: sanitize test environment for subsequent tests\n editor: use standard strvec API to receive environment for external\n editors\n\n builtin/commit.c | 2 +-\n editor.c | 10 +--\n editor.h | 7 ++-\n t/t7005-editor.sh | 152 ++++++++++++++++++++++++----------------------\n 4 files changed, 89 insertions(+), 82 deletions(-)\n\nDiff-intervalle :\n-: ---------- > 1: e7629202a1 t7005: use modern test style\n1: a37db65107 ! 2: 0467e33ed0 t7005: sanitize test environment for subsequent tests\n @@ Commit message\n t7005: sanitize test environment for subsequent tests\n \n Some of the editor tests manipulate the environment or config in ways\n - that affect future tests (because they test a sequence of overrides),\n - but those modifications are visible to future tests and create a footgun\n - for them.\n + that affect future tests, but those modifications are visible to future\n + tests and create a footgun for them.\n \n - We can't make the environment-munging override tests undo their\n - modifications because they rely on editor variables overriding other\n - previously-set editor variables.\n -\n - Use test_config and undo environment modifications once finished.\n + Use test_config, subshells, and test helpers to automatically undo\n + environment and config modifications once finished.\n \n + Best-viewed-with: --ignore-all-space\n Signed-off-by: D. Ben Knoble \n \n ## t/t7005-editor.sh ##\n @@\n - \t'\n - done\n + \ttest_cmp expect actual\n + '\n + \n +-TERM=dumb\n +-export TERM\n + test_expect_success 'dumb should error out when falling back on vi' '\n +-\tif git commit --amend\n +-\tthen\n +-\t\techo \"Oops?\"\n +-\t\tfalse\n +-\telse\n +-\t\t: happy\n +-\tfi\n ++\t(\n ++\t\tTERM=dumb &&\n ++\t\texport TERM &&\n ++\t\tif git commit --amend\n ++\t\tthen\n ++\t\t\techo \"Oops?\"\n ++\t\t\tfalse\n ++\t\telse\n ++\t\t\t: happy\n ++\t\tfi\n ++\t)\n + '\n + \n + test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n +-\tEDITOR=./e-EDITOR.sh &&\n +-\tVISUAL=./e-VISUAL.sh &&\n +-\texport EDITOR VISUAL &&\n +-\tgit commit --amend &&\n +-\techo \"Edited by EDITOR\" >expect &&\n +-\tgit show -s --format=%s >actual &&\n ++\t(\n ++\t\tTERM=dumb &&\n ++\t\texport TERM &&\n ++\t\tEDITOR=./e-EDITOR.sh &&\n ++\t\tVISUAL=./e-VISUAL.sh &&\n ++\t\texport EDITOR VISUAL &&\n ++\t\tgit commit --amend &&\n ++\t\techo \"Edited by EDITOR\" >expect &&\n ++\t\tgit show -s --format=%s >actual\n ++\t) &&\n + \ttest_cmp expect actual\n + '\n + \n +-TERM=vt100\n +-export TERM\n +-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n +-do\n +-\techo \"Edited by $i\" >expect\n +-\tunset EDITOR VISUAL GIT_EDITOR\n +-\tgit config --unset-all core.editor\n +-\tcase \"$i\" in\n +-\tcore_editor)\n +-\t\tgit config core.editor ./e-core_editor.sh\n +-\t\t;;\n +-\t[A-Z]*)\n +-\t\teval \"$i=./e-$i.sh\"\n +-\t\texport $i\n +-\t\t;;\n +-\tesac\n +-\ttest_expect_success \"Using $i\" '\n +-\t\tgit --exec-path=. commit --amend &&\n +-\t\tgit show -s --pretty=oneline >show &&\n +-\t\tactual &&\n +-\t\ttest_cmp expect actual\n +-\t'\n +-done\n ++test_expect_success 'Using individual editors' '\n ++\ttest_when_finished \"test_unconfig --unset-all core.editor\" &&\n ++\t(\n ++\t\tTERM=vt100 &&\n ++\t\texport TERM &&\n ++\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n ++\t\tdo\n ++\t\t\tsane_unset EDITOR VISUAL GIT_EDITOR &&\n ++\t\t\ttest_might_fail git config --unset-all core.editor &&\n ++\t\t\techo \"Edited by $i\" >expect &&\n ++\t\t\tcase \"$i\" in\n ++\t\t\tcore_editor)\n ++\t\t\t\tgit config core.editor ./e-core_editor.sh\n ++\t\t\t\t;;\n ++\t\t\t[A-Z]*)\n ++\t\t\t\teval \"$i=./e-$i.sh\" &&\n ++\t\t\t\texport $i\n ++\t\t\t\t;;\n ++\t\t\tesac &&\n ++\t\t\tgit --exec-path=. commit --amend &&\n ++\t\t\tgit show -s --pretty=oneline >show &&\n ++\t\t\tactual &&\n ++\t\t\ttest_cmp expect actual\n ++\t\tdone\n ++\t)\n ++'\n + \n +-unset EDITOR VISUAL GIT_EDITOR\n +-git config --unset-all core.editor\n +-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n +-do\n +-\techo \"Edited by $i\" >expect\n +-\tcase \"$i\" in\n +-\tcore_editor)\n +-\t\tgit config core.editor ./e-core_editor.sh\n +-\t\t;;\n +-\t[A-Z]*)\n +-\t\teval \"$i=./e-$i.sh\"\n +-\t\texport $i\n +-\t\t;;\n +-\tesac\n +-\ttest_expect_success \"Using $i (override)\" '\n +-\t\tgit --exec-path=. commit --amend &&\n +-\t\tgit show -s --pretty=oneline >show &&\n +-\t\tactual &&\n +-\t\ttest_cmp expect actual\n +-\t'\n +-done\n ++test_expect_success 'Using editors with overrides' '\n ++\t(\n ++\t\tTERM=vt100 &&\n ++\t\texport TERM &&\n ++\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n ++\t\tdo\n ++\t\t\techo \"Edited by $i\" >expect &&\n ++\t\t\tcase \"$i\" in\n ++\t\t\tcore_editor)\n ++\t\t\t\tgit config core.editor ./e-core_editor.sh\n ++\t\t\t\t;;\n ++\t\t\t[A-Z]*)\n ++\t\t\t\teval \"$i=./e-$i.sh\" &&\n ++\t\t\t\texport $i\n ++\t\t\t\t;;\n ++\t\t\tesac &&\n ++\t\t\tgit --exec-path=. commit --amend &&\n ++\t\t\tgit show -s --pretty=oneline >show &&\n ++\t\t\tactual &&\n ++\t\t\ttest_cmp expect actual\n ++\t\tdone\n ++\t)\n ++'\n \n -+unset EDITOR VISUAL GIT_EDITOR\n -+git config --unset-all core.editor\n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n - \tchmod a+x \"e space.sh\" &&\n @@\n - \n + \ttest_cmp expect actual\n '\n \n -unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n --\n -\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n +\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n - \ttest space = \"$(git show -s --pretty=format:%s)\"\n --\n - '\n - \n - test_done\n + \techo space >expect &&\n + \tgit show -s --pretty=tformat:%s >actual &&\n2: 5450c99f59 = 3: 4a9bb52470 editor: use standard strvec API to receive environment for external editors\n\nbase-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be\n-- \n2.48.1\n\n"},{"id":"523891","messageId":"20250810160323.49372-2-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 1/3] t7005: use modern test style","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-10T16:03:17Z","receivedAt":"2025-08-10T16:03:33Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Tests in t7005 mask Git error codes and do not use our nice helpers for\ncomparing results. Improve that, and drop a few old-style blank lines\nwhile at it.\n\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 32 ++++++++++++++------------------\n 1 file changed, 14 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 5fcf281dfb..2f59fc0549 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -7,10 +7,8 @@\n unset EDITOR VISUAL GIT_EDITOR\n \n test_expect_success 'determine default editor' '\n-\n \tvi=$(TERM=vt100 git var GIT_EDITOR) &&\n \ttest -n \"$vi\"\n-\n '\n \n if ! expr \"$vi\" : '[a-z]*$' >/dev/null\n@@ -33,19 +31,16 @@\n fi\n \n test_expect_success setup '\n-\n \tmsg=\"Hand-edited\" &&\n \ttest_commit \"$msg\" &&\n \techo \"$msg\" >expect &&\n-\tgit show -s --format=%s > actual &&\n+\tgit show -s --format=%s >actual &&\n \ttest_cmp expect actual\n-\n '\n \n TERM=dumb\n export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n-\n \tif git commit --amend\n \tthen\n \t\techo \"Oops?\"\n@@ -56,13 +51,13 @@\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n-\n \tEDITOR=./e-EDITOR.sh &&\n \tVISUAL=./e-VISUAL.sh &&\n \texport EDITOR VISUAL &&\n \tgit commit --amend &&\n-\ttest \"$(git show -s --format=%s)\" = \"Edited by EDITOR\"\n-\n+\techo \"Edited by EDITOR\" >expect &&\n+\tgit show -s --format=%s >actual &&\n+\ttest_cmp expect actual\n '\n \n TERM=vt100\n@@ -83,8 +78,8 @@\n \tesac\n \ttest_expect_success \"Using $i\" '\n \t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline |\n-\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n+\t\tgit show -s --pretty=oneline >show &&\n+\t\tactual &&\n \t\ttest_cmp expect actual\n \t'\n done\n@@ -105,8 +100,8 @@\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n \t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline |\n-\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n+\t\tgit show -s --pretty=oneline >show &&\n+\t\tactual &&\n \t\ttest_cmp expect actual\n \t'\n done\n@@ -115,17 +110,18 @@\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n-\ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n+\techo space >expect &&\n+\tgit show -s --pretty=tformat:%s >actual &&\n+\ttest_cmp expect actual\n '\n \n unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\n \tgit config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n-\ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n+\techo space >expect &&\n+\tgit show -s --pretty=tformat:%s >actual &&\n+\ttest_cmp expect actual\n '\n \n test_done\n-- \n2.48.1\n\n"},{"id":"523892","messageId":"20250810160323.49372-3-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 2/3] t7005: sanitize test environment for subsequent tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-10T16:03:18Z","receivedAt":"2025-08-10T16:03:38Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Some of the editor tests manipulate the environment or config in ways\nthat affect future tests, but those modifications are visible to future\ntests and create a footgun for them.\n\nUse test_config, subshells, and test helpers to automatically undo\nenvironment and config modifications once finished.\n\nBest-viewed-with: --ignore-all-space\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 132 +++++++++++++++++++++++++---------------------\n 1 file changed, 71 insertions(+), 61 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 2f59fc0549..eae76cc2f2 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -38,73 +38,84 @@\n \ttest_cmp expect actual\n '\n \n-TERM=dumb\n-export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n-\tif git commit --amend\n-\tthen\n-\t\techo \"Oops?\"\n-\t\tfalse\n-\telse\n-\t\t: happy\n-\tfi\n+\t(\n+\t\tTERM=dumb &&\n+\t\texport TERM &&\n+\t\tif git commit --amend\n+\t\tthen\n+\t\t\techo \"Oops?\"\n+\t\t\tfalse\n+\t\telse\n+\t\t\t: happy\n+\t\tfi\n+\t)\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n-\tEDITOR=./e-EDITOR.sh &&\n-\tVISUAL=./e-VISUAL.sh &&\n-\texport EDITOR VISUAL &&\n-\tgit commit --amend &&\n-\techo \"Edited by EDITOR\" >expect &&\n-\tgit show -s --format=%s >actual &&\n+\t(\n+\t\tTERM=dumb &&\n+\t\texport TERM &&\n+\t\tEDITOR=./e-EDITOR.sh &&\n+\t\tVISUAL=./e-VISUAL.sh &&\n+\t\texport EDITOR VISUAL &&\n+\t\tgit commit --amend &&\n+\t\techo \"Edited by EDITOR\" >expect &&\n+\t\tgit show -s --format=%s >actual\n+\t) &&\n \ttest_cmp expect actual\n '\n \n-TERM=vt100\n-export TERM\n-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n-do\n-\techo \"Edited by $i\" >expect\n-\tunset EDITOR VISUAL GIT_EDITOR\n-\tgit config --unset-all core.editor\n-\tcase \"$i\" in\n-\tcore_editor)\n-\t\tgit config core.editor ./e-core_editor.sh\n-\t\t;;\n-\t[A-Z]*)\n-\t\teval \"$i=./e-$i.sh\"\n-\t\texport $i\n-\t\t;;\n-\tesac\n-\ttest_expect_success \"Using $i\" '\n-\t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline >show &&\n-\t\tactual &&\n-\t\ttest_cmp expect actual\n-\t'\n-done\n+test_expect_success 'Using individual editors' '\n+\ttest_when_finished \"test_unconfig --unset-all core.editor\" &&\n+\t(\n+\t\tTERM=vt100 &&\n+\t\texport TERM &&\n+\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n+\t\tdo\n+\t\t\tsane_unset EDITOR VISUAL GIT_EDITOR &&\n+\t\t\ttest_might_fail git config --unset-all core.editor &&\n+\t\t\techo \"Edited by $i\" >expect &&\n+\t\t\tcase \"$i\" in\n+\t\t\tcore_editor)\n+\t\t\t\tgit config core.editor ./e-core_editor.sh\n+\t\t\t\t;;\n+\t\t\t[A-Z]*)\n+\t\t\t\teval \"$i=./e-$i.sh\" &&\n+\t\t\t\texport $i\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\t\t\tgit --exec-path=. commit --amend &&\n+\t\t\tgit show -s --pretty=oneline >show &&\n+\t\t\tactual &&\n+\t\t\ttest_cmp expect actual\n+\t\tdone\n+\t)\n+'\n \n-unset EDITOR VISUAL GIT_EDITOR\n-git config --unset-all core.editor\n-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n-do\n-\techo \"Edited by $i\" >expect\n-\tcase \"$i\" in\n-\tcore_editor)\n-\t\tgit config core.editor ./e-core_editor.sh\n-\t\t;;\n-\t[A-Z]*)\n-\t\teval \"$i=./e-$i.sh\"\n-\t\texport $i\n-\t\t;;\n-\tesac\n-\ttest_expect_success \"Using $i (override)\" '\n-\t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline >show &&\n-\t\tactual &&\n-\t\ttest_cmp expect actual\n-\t'\n-done\n+test_expect_success 'Using editors with overrides' '\n+\t(\n+\t\tTERM=vt100 &&\n+\t\texport TERM &&\n+\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n+\t\tdo\n+\t\t\techo \"Edited by $i\" >expect &&\n+\t\t\tcase \"$i\" in\n+\t\t\tcore_editor)\n+\t\t\t\tgit config core.editor ./e-core_editor.sh\n+\t\t\t\t;;\n+\t\t\t[A-Z]*)\n+\t\t\t\teval \"$i=./e-$i.sh\" &&\n+\t\t\t\texport $i\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\t\t\tgit --exec-path=. commit --amend &&\n+\t\t\tgit show -s --pretty=oneline >show &&\n+\t\t\tactual &&\n+\t\t\ttest_cmp expect actual\n+\t\tdone\n+\t)\n+'\n \n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n@@ -115,9 +126,8 @@\n \ttest_cmp expect actual\n '\n \n-unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n+\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n \techo space >expect &&\n \tgit show -s --pretty=tformat:%s >actual &&\n-- \n2.48.1\n\n"},{"id":"523893","messageId":"20250810160323.49372-4-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250520193506.95199-1-ben.knoble+github@gmail.com","subject":"[PATCH 3/3] editor: use standard strvec API to receive environment for external editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-10T16:03:19Z","receivedAt":"2025-08-10T16:03:42Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Going back to the introduction of the env parameter for the editor in\n8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as\nwell., 2007-11-26), we pass a constant array of strings: as the\nsurrounding APIs evolved to use strvecs (see 8d7aa4ba6a\n(builtin/commit.c: remove the PATH_MAX limitation via dynamic\nallocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',\n2020-08-10)), the editor code did not.\n\nThere is only one caller of all 3 editor APIs that does not pass a NULL\nenvironment (the same caller for which this parameter was added), and\nit already has a strvec available to use.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: D. Ben Knoble \n---\n builtin/commit.c | 2 +-\n editor.c | 10 +++++-----\n editor.h | 7 ++++---\n 3 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b5b9608813..16cad7fb03 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1116,7 +1116,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstruct strvec env = STRVEC_INIT;\n \n \t\tstrvec_pushf(&env, \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env.v)) {\n+\t\tif (launch_editor(git_path_commit_editmsg(), NULL, &env)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n \t\t\texit(1);\ndiff --git a/editor.c b/editor.c\nindex fd174e6a03..0bc781d50c 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -58,7 +58,7 @@ const char *git_sequence_editor(void)\n }\n \n static int launch_specified_editor(const char *editor, const char *path,\n-\t\t\t\t struct strbuf *buffer, const char *const *env)\n+\t\t\t\t struct strbuf *buffer, const struct strvec *env)\n {\n \tif (!editor)\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n@@ -89,7 +89,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \n \t\tstrvec_pushl(&p.args, editor, realpath.buf, NULL);\n \t\tif (env)\n-\t\t\tstrvec_pushv(&p.env, (const char **)env);\n+\t\t\tstrvec_pushv(&p.env, env->v);\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n \t\tif (start_command(&p) < 0) {\n@@ -124,20 +124,20 @@ static int launch_specified_editor(const char *editor, const char *path,\n \treturn 0;\n }\n \n-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)\n+int launch_editor(const char *path, struct strbuf *buffer, const struct strvec *env)\n {\n \treturn launch_specified_editor(git_editor(), path, buffer, env);\n }\n \n int launch_sequence_editor(const char *path, struct strbuf *buffer,\n-\t\t\t const char *const *env)\n+\t\t\t const struct strvec *env)\n {\n \treturn launch_specified_editor(git_sequence_editor(), path, buffer, env);\n }\n \n int strbuf_edit_interactively(struct repository *r,\n \t\t\t struct strbuf *buffer, const char *path,\n-\t\t\t const char *const *env)\n+\t\t\t const struct strvec *env)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd, res = 0;\ndiff --git a/editor.h b/editor.h\nindex f1c41df378..627e992f4d 100644\n--- a/editor.h\n+++ b/editor.h\n@@ -3,6 +3,7 @@\n \n struct repository;\n struct strbuf;\n+struct strvec;\n \n const char *git_editor(void);\n const char *git_sequence_editor(void);\n@@ -16,10 +17,10 @@ int is_terminal_dumb(void);\n * file's contents are not read into the buffer upon completion.\n */\n int launch_editor(const char *path, struct strbuf *buffer,\n-\t\t const char *const *env);\n+\t\t const struct strvec *env);\n \n int launch_sequence_editor(const char *path, struct strbuf *buffer,\n-\t\t\t const char *const *env);\n+\t\t\t const struct strvec *env);\n \n /*\n * In contrast to `launch_editor()`, this function writes out the contents\n@@ -30,6 +31,6 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,\n * If `path` is relative, it refers to a file in the `.git` directory.\n */\n int strbuf_edit_interactively(struct repository *r, struct strbuf *buffer,\n-\t\t\t const char *path, const char *const *env);\n+\t\t\t const char *path, const struct strvec *env);\n \n #endif\n-- \n2.48.1\n\n"},{"id":"523894","messageId":"CALnO6CAQNF2gD5BWvm50LN2D6mAHNRxdzeZ7ZQhvt-Gr3oHumg@mail.gmail.com","threadId":"63489","inReplyTo":"20250810160323.49372-1-ben.knoble+github@gmail.com","subject":"Re: [PATCH 0/3] clean up some code around editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-10T16:06:06Z","receivedAt":"2025-08-10T16:06:19Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sun, Aug 10, 2025 at 12:03 PM D. Ben Knoble\n wrote:\n>\n> Changes from v1:\n> - add a prep patch with style fixes to t7005\n> - rework the environment munging to use subshells, per Phillip Wood's\n> suggestion\n\nApologies; this should have had the \"v2\" subject.\n"},{"id":"523895","messageId":"7A0DC0BD-E0DB-4A07-B4E4-1474D58B5E75@gmail.com","threadId":"63489","inReplyTo":"CALnO6CAQNF2gD5BWvm50LN2D6mAHNRxdzeZ7ZQhvt-Gr3oHumg@mail.gmail.com","subject":"Re: [PATCH 0/3] clean up some code around editors","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-08-10T16:34:27Z","receivedAt":"2025-08-10T16:34:40Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 10 août 2025 à 12:06, D. Ben Knoble a écrit :\n> \n> On Sun, Aug 10, 2025 at 12:03 PM D. Ben Knoble\n> wrote:\n>> \n>> Changes from v1:\n>> - add a prep patch with style fixes to t7005\n>> - rework the environment munging to use subshells, per Phillip Wood's\n>> suggestion\n> \n> Apologies; this should have had the \"v2\" subject.\n\nGitHub CI caught an issue from my rebase, so I’ll send a proper v2 later. "},{"id":"523896","messageId":"01674640-21f5-44f8-8208-d6b6d88b09c5@gmail.com","threadId":"63489","inReplyTo":"20250810160323.49372-2-ben.knoble+github@gmail.com","subject":"Re: [PATCH 1/3] t7005: use modern test style","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-10T19:42:31Z","receivedAt":"2025-08-10T19:42:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nThanks for cleaning up these tests\n\nOn 10/08/2025 17:03, D. Ben Knoble wrote:\n> \n> TERM=dumb\n> export TERM\n> test_expect_success 'dumb should error out when falling back on vi' '\n> -\n> \tif git commit --amend\n\nInstead of this \"if\" we should be using test_must_fail here\n\n> \tthen\n> \t\techo \"Oops?\"\n> @@ -56,13 +51,13 @@\n> '\n> \n> test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n> -\n> \tEDITOR=./e-EDITOR.sh &&\n> \tVISUAL=./e-VISUAL.sh &&\n> \texport EDITOR VISUAL &&\n> \tgit commit --amend &&\n> -\ttest \"$(git show -s --format=%s)\" = \"Edited by EDITOR\"\n> -\n> +\techo \"Edited by EDITOR\" >expect &&\n> +\tgit show -s --format=%s >actual &&\n> +\ttest_cmp expect actual\n\nThis is a faithful conversion of the original but I'd be tempted to use \ntest_commit_message instead as I think we really should be checking the \nwhole message rather than just the subject line in all these tests.\n\n\ttest_commit_message HEAD -m \"Edited by EDITOR\"\n\nThe whitespace cleanups all look good to me\n\nThanks\n\nPhillip\n\n> '\n> \n> TERM=vt100\n> @@ -83,8 +78,8 @@\n> \tesac\n> \ttest_expect_success \"Using $i\" '\n> \t\tgit --exec-path=. commit --amend &&\n> -\t\tgit show -s --pretty=oneline |\n> -\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n> +\t\tgit show -s --pretty=oneline >show &&\n> +\t\tactual &&\n> \t\ttest_cmp expect actual\n> \t'\n> done\n> @@ -105,8 +100,8 @@\n> \tesac\n> \ttest_expect_success \"Using $i (override)\" '\n> \t\tgit --exec-path=. commit --amend &&\n> -\t\tgit show -s --pretty=oneline |\n> -\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n> +\t\tgit show -s --pretty=oneline >show &&\n> +\t\tactual &&\n> \t\ttest_cmp expect actual\n> \t'\n> done\n> @@ -115,17 +110,18 @@\n> \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n> \tchmod a+x \"e space.sh\" &&\n> \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n> -\ttest space = \"$(git show -s --pretty=format:%s)\"\n> -\n> +\techo space >expect &&\n> +\tgit show -s --pretty=tformat:%s >actual &&\n> +\ttest_cmp expect actual\n> '\n> \n> unset GIT_EDITOR\n> test_expect_success 'core.editor with a space' '\n> -\n> \tgit config core.editor \\\"./e\\ space.sh\\\" &&\n> \tgit commit --amend &&\n> -\ttest space = \"$(git show -s --pretty=format:%s)\"\n> -\n> +\techo space >expect &&\n> +\tgit show -s --pretty=tformat:%s >actual &&\n> +\ttest_cmp expect actual\n> '\n> \n> test_done\n\n"},{"id":"523897","messageId":"144b6ee4-d4b4-4843-841c-93a109e71aa9@gmail.com","threadId":"63489","inReplyTo":"20250810160323.49372-3-ben.knoble+github@gmail.com","subject":"Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-10T19:44:26Z","receivedAt":"2025-08-10T19:44:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 10/08/2025 17:03, D. Ben Knoble wrote:\n> \n> -TERM=dumb\n> -export TERM\n> test_expect_success 'dumb should error out when falling back on vi' '\n> -\tif git commit --amend\n> -\tthen\n> -\t\techo \"Oops?\"\n> -\t\tfalse\n> -\telse\n> -\t\t: happy\n> -\tfi\n> +\t(\n> +\t\tTERM=dumb &&\n> +\t\texport TERM &&\n> +\t\tif git commit --amend\n> +\t\tthen\n> +\t\t\techo \"Oops?\"\n> +\t\t\tfalse\n> +\t\telse\n> +\t\t\t: happy\n> +\t\tfi\n> +\t)\n> '\n> \n> test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n> -\tEDITOR=./e-EDITOR.sh &&\n> -\tVISUAL=./e-VISUAL.sh &&\n> -\texport EDITOR VISUAL &&\n> -\tgit commit --amend &&\n> -\techo \"Edited by EDITOR\" >expect &&\n> -\tgit show -s --format=%s >actual &&\n> +\t(\n> +\t\tTERM=dumb &&\n> +\t\texport TERM &&\n\nWe may as well export this with EDITOR and VISUAL\n\n> +\t\tEDITOR=./e-EDITOR.sh &&\n> +\t\tVISUAL=./e-VISUAL.sh &&\n> +\t\texport EDITOR VISUAL &&\n> +\t\tgit commit --amend &&\n> +\t\techo \"Edited by EDITOR\" >expect &&\n> +\t\tgit show -s --format=%s >actual\n> +\t) &&\n> \ttest_cmp expect actual\n> '\n> \n> -TERM=vt100\n> -export TERM\n> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n> -do\n> -\techo \"Edited by $i\" >expect\n> -\tunset EDITOR VISUAL GIT_EDITOR\n> -\tgit config --unset-all core.editor\n> -\tcase \"$i\" in\n> -\tcore_editor)\n> -\t\tgit config core.editor ./e-core_editor.sh\n> -\t\t;;\n> -\t[A-Z]*)\n> -\t\teval \"$i=./e-$i.sh\"\n> -\t\texport $i\n> -\t\t;;\n> -\tesac\n> -\ttest_expect_success \"Using $i\" '\n> -\t\tgit --exec-path=. commit --amend &&\n> -\t\tgit show -s --pretty=oneline >show &&\n> -\t\tactual &&\n> -\t\ttest_cmp expect actual\n> -\t'\n> -done\n> +test_expect_success 'Using individual editors' '\n> +\ttest_when_finished \"test_unconfig --unset-all core.editor\" &&\n> +\t(\n> +\t\tTERM=vt100 &&\n> +\t\texport TERM &&\n> +\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n> +\t\tdo\n> +\t\t\tsane_unset EDITOR VISUAL GIT_EDITOR &&\n> +\t\t\ttest_might_fail git config --unset-all core.editor &&\n> +\t\t\techo \"Edited by $i\" >expect &&\n> +\t\t\tcase \"$i\" in\n> +\t\t\tcore_editor)\n> +\t\t\t\tgit config core.editor ./e-core_editor.sh\n> +\t\t\t\t;;\n> +\t\t\t[A-Z]*)\n> +\t\t\t\teval \"$i=./e-$i.sh\" &&\n> +\t\t\t\texport $i\n> +\t\t\t\t;;\n> +\t\t\tesac &&\n> +\t\t\tgit --exec-path=. commit --amend &&\n\nIt would be nice to stop abusing --exec-path here and in the next test \nby adding the current directory to $PATH with\n\n\tPATH=\"$(pwd):$PATH\" git commit --amend\n\n> +\t\t\tgit show -s --pretty=oneline >show &&\n> +\t\t\tactual &&\n> +\t\t\ttest_cmp expect actual\n\nWe need to add \"|| return 1\" to the last line here and in the test below \nto reliably error out when test_cmp fails. I'd have thought that our \ntest linting should hove picked this up but maybe it is confused by the \nsubshell.\n\nThanks\n\nPhillip\n> +\t\tdone\n> +\t)\n> +'\n> \n> -unset EDITOR VISUAL GIT_EDITOR\n> -git config --unset-all core.editor\n> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n> -do\n> -\techo \"Edited by $i\" >expect\n> -\tcase \"$i\" in\n> -\tcore_editor)\n> -\t\tgit config core.editor ./e-core_editor.sh\n> -\t\t;;\n> -\t[A-Z]*)\n> -\t\teval \"$i=./e-$i.sh\"\n> -\t\texport $i\n> -\t\t;;\n> -\tesac\n> -\ttest_expect_success \"Using $i (override)\" '\n> -\t\tgit --exec-path=. commit --amend &&\n> -\t\tgit show -s --pretty=oneline >show &&\n> -\t\tactual &&\n> -\t\ttest_cmp expect actual\n> -\t'\n> -done\n> +test_expect_success 'Using editors with overrides' '\n> +\t(\n> +\t\tTERM=vt100 &&\n> +\t\texport TERM &&\n> +\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n> +\t\tdo\n> +\t\t\techo \"Edited by $i\" >expect &&\n> +\t\t\tcase \"$i\" in\n> +\t\t\tcore_editor)\n> +\t\t\t\tgit config core.editor ./e-core_editor.sh\n> +\t\t\t\t;;\n> +\t\t\t[A-Z]*)\n> +\t\t\t\teval \"$i=./e-$i.sh\" &&\n> +\t\t\t\texport $i\n> +\t\t\t\t;;\n> +\t\t\tesac &&\n> +\t\t\tgit --exec-path=. commit --amend &&\n> +\t\t\tgit show -s --pretty=oneline >show &&\n> +\t\t\tactual &&\n> +\t\t\ttest_cmp expect actual\n> +\t\tdone\n> +\t)\n> +'\n> \n> test_expect_success 'editor with a space' '\n> \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n> @@ -115,9 +126,8 @@\n> \ttest_cmp expect actual\n> '\n> \n> -unset GIT_EDITOR\n> test_expect_success 'core.editor with a space' '\n> -\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n> +\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n> \tgit commit --amend &&\n> \techo space >expect &&\n> \tgit show -s --pretty=tformat:%s >actual &&\n\n"},{"id":"523898","messageId":"9B179FBA-6695-4F8A-8EE1-A02F1236805E@gmail.com","threadId":"63489","inReplyTo":"144b6ee4-d4b4-4843-841c-93a109e71aa9@gmail.com","subject":"Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-08-10T19:53:41Z","receivedAt":"2025-08-10T19:53:54Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 10 août 2025 à 15:44, Phillip Wood a écrit :\n> \n> Hi Ben\n> \n>> On 10/08/2025 17:03, D. Ben Knoble wrote:\n>> +test_expect_success 'Using individual editors' '\n>> + test_when_finished \"test_unconfig --unset-all core.editor\" &&\n>> + (\n>> + TERM=vt100 &&\n>> + export TERM &&\n>> + for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n>> + do\n>> + sane_unset EDITOR VISUAL GIT_EDITOR &&\n>> + test_might_fail git config --unset-all core.editor &&\n>> + echo \"Edited by $i\" >expect &&\n>> + case \"$i\" in\n>> + core_editor)\n>> + git config core.editor ./e-core_editor.sh\n>> + ;;\n>> + [A-Z]*)\n>> + eval \"$i=./e-$i.sh\" &&\n>> + export $i\n>> + ;;\n>> + esac &&\n>> + git --exec-path=. commit --amend &&\n> [snip]\n>> + git show -s --pretty=oneline >show &&\n>> + actual &&\n>> + test_cmp expect actual\n> \n> 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\n\nAFK for a bit, but didn’t want anyone to think the linter was broken:\n\nThat’s what I must have lost when rebasing 1/3 from the end of the series to the beginning, good eyes!\n\nIn 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).\n\nPS I think it’a actually “exit 1” in this case?"},{"id":"523899","messageId":"CAPig+cQug9-vH5SHHuM6xa-We51td0=JigODjvise0v8XA+eEw@mail.gmail.com","threadId":"63489","inReplyTo":"9B179FBA-6695-4F8A-8EE1-A02F1236805E@gmail.com","subject":"Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-08-10T20:58:25Z","receivedAt":"2025-08-10T20:58:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 10, 2025 at 3:54 PM Ben Knoble wrote:\n> > Le 10 août 2025 à 15:44, Phillip Wood a écrit :\n> >> + git show -s --pretty=oneline >show &&\n> >> + actual &&\n> >> + test_cmp expect actual\n> >\n> > 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\n>\n> PS I think it’a actually “exit 1” in this case?\n\nIndeed, it should be `|| exit 1` since this is inside a subshell.\n\n(I was just about to respond to Phillip's email but continued reading\nthe thread and saw that you correctly mentioned `|| exit 1` already.\nThanks.)\n"},{"id":"523920","messageId":"7491b9af-adcd-431a-89e7-ff54708beca6@gmail.com","threadId":"63489","inReplyTo":"144b6ee4-d4b4-4843-841c-93a109e71aa9@gmail.com","subject":"Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-11T09:59:52Z","receivedAt":"2025-08-11T09:59:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/08/2025 20:44, Phillip Wood wrote:\n> On 10/08/2025 17:03, D. Ben Knoble wrote:\n>>   '\n>> -TERM=vt100\n>> -export TERM\n>> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n>> -do\n>> -    echo \"Edited by $i\" >expect\n>> -    unset EDITOR VISUAL GIT_EDITOR\n>> -    git config --unset-all core.editor\n>> -    case \"$i\" in\n>> -    core_editor)\n>> -        git config core.editor ./e-core_editor.sh\n>> -        ;;\n>> -    [A-Z]*)\n>> -        eval \"$i=./e-$i.sh\"\n>> -        export $i\n>> -        ;;\n>> -    esac\n>> -    test_expect_success \"Using $i\" '\n>> -        git --exec-path=. commit --amend &&\n>> -        git show -s --pretty=oneline >show &&\n>> -        actual &&\n>> -        test_cmp expect actual\n>> -    '\n>> -done\n\nThinking about it some more, for this test we can put the loop outside \nof the subshell and test_expect_success so that we don't have to worry \nabout explicitly clearing the variables set from previous iterations.\n\nfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\ndo\n\ttest_expect_success \"Using $i\" '\n\t\tif test \"$1\" = core.editor\n\t\tthen\n\t\t\ttest_config core.editor ./e-core_editor.sh\n\t\tfi &&\n\t\t(\n\t\t\tcase \"$i\" in\n\t\t\t[A-Z]*)\n\t\t\t\teval \"$i=./e-$i.sh\" &&\n\t\t\t\texport $i\n\t\t\t\t;;\n\t\t\tesac &&\n\t\t\tPATH=\"$(pwd):$PATH\" \\\n\t\t\t TERM=vt100 git commit --amend &&\n\t\t) &&\n\t\ttest_commit_message HEAD -m \"Edited by $i\"\n\t'\ndone\n\nThanks\n\nPhillip\n\n>> +test_expect_success 'Using individual editors' '\n>> +    test_when_finished \"test_unconfig --unset-all core.editor\" &&\n>> +    (\n>> +        TERM=vt100 &&\n>> +        export TERM &&\n>> +        for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n>> +        do\n>> +            sane_unset EDITOR VISUAL GIT_EDITOR &&\n>> +            test_might_fail git config --unset-all core.editor &&\n>> +            echo \"Edited by $i\" >expect &&\n>> +            case \"$i\" in\n>> +            core_editor)\n>> +                git config core.editor ./e-core_editor.sh\n>> +                ;;\n>> +            [A-Z]*)\n>> +                eval \"$i=./e-$i.sh\" &&\n>> +                export $i\n>> +                ;;\n>> +            esac &&\n>> +            git --exec-path=. commit --amend &&\n> \n> It would be nice to stop abusing --exec-path here and in the next test \n> by adding the current directory to $PATH with\n> \n>     PATH=\"$(pwd):$PATH\" git commit --amend\n> \n>> +            git show -s --pretty=oneline >show &&\n>> +            actual &&\n>> +            test_cmp expect actual\n> \n> We need to add \"|| return 1\" to the last line here and in the test below \n> to reliably error out when test_cmp fails. I'd have thought that our \n> test linting should hove picked this up but maybe it is confused by the \n> subshell.\n> \n> Thanks\n> \n> Phillip\n>> +        done\n>> +    )\n>> +'\n>> -unset EDITOR VISUAL GIT_EDITOR\n>> -git config --unset-all core.editor\n>> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n>> -do\n>> -    echo \"Edited by $i\" >expect\n>> -    case \"$i\" in\n>> -    core_editor)\n>> -        git config core.editor ./e-core_editor.sh\n>> -        ;;\n>> -    [A-Z]*)\n>> -        eval \"$i=./e-$i.sh\"\n>> -        export $i\n>> -        ;;\n>> -    esac\n>> -    test_expect_success \"Using $i (override)\" '\n>> -        git --exec-path=. commit --amend &&\n>> -        git show -s --pretty=oneline >show &&\n>> -        actual &&\n>> -        test_cmp expect actual\n>> -    '\n>> -done\n>> +test_expect_success 'Using editors with overrides' '\n>> +    (\n>> +        TERM=vt100 &&\n>> +        export TERM &&\n>> +        for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n>> +        do\n>> +            echo \"Edited by $i\" >expect &&\n>> +            case \"$i\" in\n>> +            core_editor)\n>> +                git config core.editor ./e-core_editor.sh\n>> +                ;;\n>> +            [A-Z]*)\n>> +                eval \"$i=./e-$i.sh\" &&\n>> +                export $i\n>> +                ;;\n>> +            esac &&\n>> +            git --exec-path=. commit --amend &&\n>> +            git show -s --pretty=oneline >show &&\n>> +            actual &&\n>> +            test_cmp expect actual\n>> +        done\n>> +    )\n>> +'\n>>   test_expect_success 'editor with a space' '\n>>       echo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n>> @@ -115,9 +126,8 @@\n>>       test_cmp expect actual\n>>   '\n>> -unset GIT_EDITOR\n>>   test_expect_success 'core.editor with a space' '\n>> -    git config core.editor \\\"./e\\ space.sh\\\" &&\n>> +    test_config core.editor \\\"./e\\ space.sh\\\" &&\n>>       git commit --amend &&\n>>       echo space >expect &&\n>>       git show -s --pretty=tformat:%s >actual &&\n> \n\n"},{"id":"523921","messageId":"9cccf70a-26f2-424d-8d44-3b87dab97d65@gmail.com","threadId":"63489","inReplyTo":"20250810160323.49372-2-ben.knoble+github@gmail.com","subject":"Re: [PATCH 1/3] t7005: use modern test style","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-11T10:04:34Z","receivedAt":"2025-08-11T10:04:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 10/08/2025 17:03, D. Ben Knoble wrote:\n> \n> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n> index 5fcf281dfb..2f59fc0549 100755\n> --- a/t/t7005-editor.sh\n> +++ b/t/t7005-editor.sh\n> @@ -7,10 +7,8 @@\n> unset EDITOR VISUAL GIT_EDITOR\n> \n> test_expect_success 'determine default editor' '\n> -\n> \tvi=$(TERM=vt100 git var GIT_EDITOR) &&\n> \ttest -n \"$vi\"\n> -\n> '\n> \n> if ! expr \"$vi\" : '[a-z]*$' >/dev/null\n\nAll of the code starting here that is outside of a test could usefully \nbe moved inside the \"setup\" test below it. It could also be cleaned up \nto use write_script() to create the editor scripts as well.\n\nThanks\n\nPhillip\n\n> @@ -33,19 +31,16 @@\n> fi\n> \n> test_expect_success setup '\n> -\n> \tmsg=\"Hand-edited\" &&\n> \ttest_commit \"$msg\" &&\n> \techo \"$msg\" >expect &&\n> -\tgit show -s --format=%s > actual &&\n> +\tgit show -s --format=%s >actual &&\n> \ttest_cmp expect actual\n> -\n> '\n> \n> TERM=dumb\n> export TERM\n> test_expect_success 'dumb should error out when falling back on vi' '\n> -\n> \tif git commit --amend\n> \tthen\n> \t\techo \"Oops?\"\n> @@ -56,13 +51,13 @@\n> '\n> \n> test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n> -\n> \tEDITOR=./e-EDITOR.sh &&\n> \tVISUAL=./e-VISUAL.sh &&\n> \texport EDITOR VISUAL &&\n> \tgit commit --amend &&\n> -\ttest \"$(git show -s --format=%s)\" = \"Edited by EDITOR\"\n> -\n> +\techo \"Edited by EDITOR\" >expect &&\n> +\tgit show -s --format=%s >actual &&\n> +\ttest_cmp expect actual\n> '\n> \n> TERM=vt100\n> @@ -83,8 +78,8 @@\n> \tesac\n> \ttest_expect_success \"Using $i\" '\n> \t\tgit --exec-path=. commit --amend &&\n> -\t\tgit show -s --pretty=oneline |\n> -\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n> +\t\tgit show -s --pretty=oneline >show &&\n> +\t\tactual &&\n> \t\ttest_cmp expect actual\n> \t'\n> done\n> @@ -105,8 +100,8 @@\n> \tesac\n> \ttest_expect_success \"Using $i (override)\" '\n> \t\tgit --exec-path=. commit --amend &&\n> -\t\tgit show -s --pretty=oneline |\n> -\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n> +\t\tgit show -s --pretty=oneline >show &&\n> +\t\tactual &&\n> \t\ttest_cmp expect actual\n> \t'\n> done\n> @@ -115,17 +110,18 @@\n> \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n> \tchmod a+x \"e space.sh\" &&\n> \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n> -\ttest space = \"$(git show -s --pretty=format:%s)\"\n> -\n> +\techo space >expect &&\n> +\tgit show -s --pretty=tformat:%s >actual &&\n> +\ttest_cmp expect actual\n> '\n> \n> unset GIT_EDITOR\n> test_expect_success 'core.editor with a space' '\n> -\n> \tgit config core.editor \\\"./e\\ space.sh\\\" &&\n> \tgit commit --amend &&\n> -\ttest space = \"$(git show -s --pretty=format:%s)\"\n> -\n> +\techo space >expect &&\n> +\tgit show -s --pretty=tformat:%s >actual &&\n> +\ttest_cmp expect actual\n> '\n> \n> test_done\n\n"},{"id":"523998","messageId":"20250811221706.67168-1-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250810160323.49372-1-ben.knoble+github@gmail.com","subject":"[PATCH v3 0/4] clean up some code around editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-11T22:16:51Z","receivedAt":"2025-08-11T22:17:11Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Changes from v2:\n- shuffle setup code and use more helpers in 1/4\n- insert 2/4 to stop abusing --exec-path\n- improve environment-cleansing idioms in {2 => 3}/4\n\nThanks especially to Phillip's encyclopaedic knowledge of test helpers ;)\n\nChanges from v1:\n- add a prep patch with style fixes to t7005\n- rework the environment munging to use subshells, per Phillip Wood's\n suggestion\n\nThis reroll of the previous exec-path series is simplified to contain\nonly the first 2 cleanup patches, which were largely acked by the list.\nDrop the controversial and broken PATH munging.\n\nAlso, this version is (still) based on a later master 112648dd6b (Merge\nbranch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the\noriginal from May.\n\nThese patches clean up some old code in the editor tests and subsystem\nthat does not use our modern idioms.\n\nv1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/\nv2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/\nPublished-as: https://github.com/benknoble/tree/editor-cleanup\n\nD. Ben Knoble (4):\n t7005: use modern test style\n t7005: stop abusing --exec-path\n t7005: sanitize test environment for subsequent tests\n editor: use standard strvec API to receive environment for external\n editors\n\n builtin/commit.c | 2 +-\n editor.c | 10 ++--\n editor.h | 7 ++-\n t/t7005-editor.sh | 147 +++++++++++++++++++---------------------------\n 4 files changed, 69 insertions(+), 97 deletions(-)\n\nDiff-intervalle contre v2 :\n1: e7629202a1 ! 1: 8ad2904a18 t7005: use modern test style\n @@ Metadata\n ## Commit message ##\n t7005: use modern test style\n \n - Tests in t7005 mask Git error codes and do not use our nice helpers for\n - comparing results. Improve that, and drop a few old-style blank lines\n - while at it.\n + Tests in t7005 mask Git error codes and do not use our nice test\n + helpers. Improve that, move some code into the setup test, and drop a\n + few old-style blank lines while at it.\n \n + Best-viewed-with: --ignore-all-space\n Signed-off-by: D. Ben Knoble \n \n ## t/t7005-editor.sh ##\n @@ t/t7005-editor.sh\n -\n '\n \n - if ! expr \"$vi\" : '[a-z]*$' >/dev/null\n -@@\n - fi\n - \n - test_expect_success setup '\n +-if ! expr \"$vi\" : '[a-z]*$' >/dev/null\n +-then\n +-\tvi=\n +-fi\n -\n +-for i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n +-do\n +-\tcat >e-$i.sh <<-EOF\n +-\t#!$SHELL_PATH\n +-\techo \"Edited by $i\" >\"\\$1\"\n +-\tEOF\n +-\tchmod +x e-$i.sh\n +-done\n +-\n +-if ! test -z \"$vi\"\n +-then\n +-\tmv e-$vi.sh $vi\n +-fi\n +-\n + test_expect_success setup '\n ++\tif ! expr \"$vi\" : \"[a-z]*$\" >/dev/null\n ++\tthen\n ++\t\tvi=\n ++\tfi &&\n ++\n ++\tfor i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n ++\tdo\n ++\t\twrite_script e-$i.sh <<-EOF || return 1\n ++\t\t\techo \"Edited by $i\" >\"\\$1\"\n ++\t\tEOF\n ++\tdone &&\n ++\n ++\tif ! test -z \"$vi\"\n ++\tthen\n ++\t\tmv e-$vi.sh $vi\n ++\tfi &&\n + \n \tmsg=\"Hand-edited\" &&\n \ttest_commit \"$msg\" &&\n - \techo \"$msg\" >expect &&\n +-\techo \"$msg\" >expect &&\n -\tgit show -s --format=%s > actual &&\n -+\tgit show -s --format=%s >actual &&\n - \ttest_cmp expect actual\n +-\ttest_cmp expect actual\n -\n ++\ttest_commit_message HEAD -m \"$msg\"\n '\n \n TERM=dumb\n export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n -\n - \tif git commit --amend\n - \tthen\n - \t\techo \"Oops?\"\n -@@\n +-\tif git commit --amend\n +-\tthen\n +-\t\techo \"Oops?\"\n +-\t\tfalse\n +-\telse\n +-\t\t: happy\n +-\tfi\n ++\ttest_must_fail git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n @@ t/t7005-editor.sh\n \tgit commit --amend &&\n -\ttest \"$(git show -s --format=%s)\" = \"Edited by EDITOR\"\n -\n -+\techo \"Edited by EDITOR\" >expect &&\n -+\tgit show -s --format=%s >actual &&\n -+\ttest_cmp expect actual\n ++\ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n \n TERM=vt100\n @@ t/t7005-editor.sh\n \t\tgit --exec-path=. commit --amend &&\n -\t\tgit show -s --pretty=oneline |\n -\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n -+\t\tgit show -s --pretty=oneline >show &&\n -+\t\tactual &&\n - \t\ttest_cmp expect actual\n +-\t\ttest_cmp expect actual\n ++\t\ttest_commit_message HEAD expect\n \t'\n done\n + \n @@\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n \t\tgit --exec-path=. commit --amend &&\n -\t\tgit show -s --pretty=oneline |\n -\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n -+\t\tgit show -s --pretty=oneline >show &&\n -+\t\tactual &&\n - \t\ttest_cmp expect actual\n +-\t\ttest_cmp expect actual\n ++\t\ttest_commit_message HEAD expect\n \t'\n done\n + \n @@\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n -\ttest space = \"$(git show -s --pretty=format:%s)\"\n -\n -+\techo space >expect &&\n -+\tgit show -s --pretty=tformat:%s >actual &&\n -+\ttest_cmp expect actual\n ++\ttest_commit_message HEAD -m space\n '\n \n unset GIT_EDITOR\n @@ t/t7005-editor.sh\n \tgit commit --amend &&\n -\ttest space = \"$(git show -s --pretty=format:%s)\"\n -\n -+\techo space >expect &&\n -+\tgit show -s --pretty=tformat:%s >actual &&\n -+\ttest_cmp expect actual\n ++\ttest_commit_message HEAD -m space\n '\n \n test_done\n-: ---------- > 2: 9451e4f0f6 t7005: stop abusing --exec-path\n2: 0467e33ed0 ! 3: 61cb116780 t7005: sanitize test environment for subsequent tests\n @@ Commit message\n that affect future tests, but those modifications are visible to future\n tests and create a footgun for them.\n \n - Use test_config, subshells, and test helpers to automatically undo\n - environment and config modifications once finished.\n + Use test_config, subshells, single-command environment overrides, and\n + test helpers to automatically undo environment and config modifications\n + once finished.\n \n Best-viewed-with: --ignore-all-space\n Signed-off-by: D. Ben Knoble \n \n ## t/t7005-editor.sh ##\n @@\n - \ttest_cmp expect actual\n + \ttest_commit_message HEAD -m \"$msg\"\n '\n \n -TERM=dumb\n -export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n --\tif git commit --amend\n --\tthen\n --\t\techo \"Oops?\"\n --\t\tfalse\n --\telse\n --\t\t: happy\n --\tfi\n -+\t(\n -+\t\tTERM=dumb &&\n -+\t\texport TERM &&\n -+\t\tif git commit --amend\n -+\t\tthen\n -+\t\t\techo \"Oops?\"\n -+\t\t\tfalse\n -+\t\telse\n -+\t\t\t: happy\n -+\t\tfi\n -+\t)\n +-\ttest_must_fail git commit --amend\n ++\tTERM=dumb test_must_fail git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n @@ t/t7005-editor.sh\n -\tVISUAL=./e-VISUAL.sh &&\n -\texport EDITOR VISUAL &&\n -\tgit commit --amend &&\n --\techo \"Edited by EDITOR\" >expect &&\n --\tgit show -s --format=%s >actual &&\n -+\t(\n -+\t\tTERM=dumb &&\n -+\t\texport TERM &&\n -+\t\tEDITOR=./e-EDITOR.sh &&\n -+\t\tVISUAL=./e-VISUAL.sh &&\n -+\t\texport EDITOR VISUAL &&\n ++\tTERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n +\t\tgit commit --amend &&\n -+\t\techo \"Edited by EDITOR\" >expect &&\n -+\t\tgit show -s --format=%s >actual\n -+\t) &&\n - \ttest_cmp expect actual\n + \ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n \n -TERM=vt100\n -export TERM\n --for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n --do\n + for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n + do\n -\techo \"Edited by $i\" >expect\n -\tunset EDITOR VISUAL GIT_EDITOR\n -\tgit config --unset-all core.editor\n @@ t/t7005-editor.sh\n -\t\texport $i\n -\t\t;;\n -\tesac\n --\ttest_expect_success \"Using $i\" '\n --\t\tgit --exec-path=. commit --amend &&\n --\t\tgit show -s --pretty=oneline >show &&\n --\t\tactual &&\n --\t\ttest_cmp expect actual\n --\t'\n --done\n -+test_expect_success 'Using individual editors' '\n -+\ttest_when_finished \"test_unconfig --unset-all core.editor\" &&\n -+\t(\n -+\t\tTERM=vt100 &&\n -+\t\texport TERM &&\n -+\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n -+\t\tdo\n -+\t\t\tsane_unset EDITOR VISUAL GIT_EDITOR &&\n -+\t\t\ttest_might_fail git config --unset-all core.editor &&\n -+\t\t\techo \"Edited by $i\" >expect &&\n + \ttest_expect_success \"Using $i\" '\n +-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n +-\t\ttest_commit_message HEAD expect\n ++\t\tif test \"$i\" = core_editor\n ++\t\tthen\n ++\t\t\ttest_config core.editor ./e-core_editor.sh\n ++\t\tfi &&\n ++\t\t(\n +\t\t\tcase \"$i\" in\n -+\t\t\tcore_editor)\n -+\t\t\t\tgit config core.editor ./e-core_editor.sh\n -+\t\t\t\t;;\n +\t\t\t[A-Z]*)\n +\t\t\t\teval \"$i=./e-$i.sh\" &&\n +\t\t\t\texport $i\n +\t\t\t\t;;\n +\t\t\tesac &&\n -+\t\t\tgit --exec-path=. commit --amend &&\n -+\t\t\tgit show -s --pretty=oneline >show &&\n -+\t\t\tactual &&\n -+\t\t\ttest_cmp expect actual\n -+\t\tdone\n -+\t)\n -+'\n ++\t\t\tPATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n ++\t\t) &&\n ++\t\ttest_commit_message HEAD -m \"Edited by $i\"\n + \t'\n + done\n \n -unset EDITOR VISUAL GIT_EDITOR\n -git config --unset-all core.editor\n @@ t/t7005-editor.sh\n -\t\t;;\n -\tesac\n -\ttest_expect_success \"Using $i (override)\" '\n --\t\tgit --exec-path=. commit --amend &&\n --\t\tgit show -s --pretty=oneline >show &&\n --\t\tactual &&\n --\t\ttest_cmp expect actual\n +-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n +-\t\ttest_commit_message HEAD expect\n -\t'\n -done\n +test_expect_success 'Using editors with overrides' '\n @@ t/t7005-editor.sh\n +\t\t\t\texport $i\n +\t\t\t\t;;\n +\t\t\tesac &&\n -+\t\t\tgit --exec-path=. commit --amend &&\n -+\t\t\tgit show -s --pretty=oneline >show &&\n -+\t\t\tactual &&\n -+\t\t\ttest_cmp expect actual\n ++\t\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n ++\t\t\ttest_commit_message HEAD expect || exit 1\n +\t\tdone\n +\t)\n +'\n @@ t/t7005-editor.sh\n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n @@\n - \ttest_cmp expect actual\n + \ttest_commit_message HEAD -m space\n '\n \n -unset GIT_EDITOR\n @@ t/t7005-editor.sh\n -\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n +\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n - \techo space >expect &&\n - \tgit show -s --pretty=tformat:%s >actual &&\n + \ttest_commit_message HEAD -m space\n + '\n3: 4a9bb52470 = 4: ea269f2442 editor: use standard strvec API to receive environment for external editors\n\nbase-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be\n-- \n2.48.1\n\n"},{"id":"523999","messageId":"20250811221706.67168-2-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250810160323.49372-1-ben.knoble+github@gmail.com","subject":"[PATCH v3 1/4] t7005: use modern test style","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-11T22:16:52Z","receivedAt":"2025-08-11T22:17:12Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Tests in t7005 mask Git error codes and do not use our nice test\nhelpers. Improve that, move some code into the setup test, and drop a\nfew old-style blank lines while at it.\n\nBest-viewed-with: --ignore-all-space\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 70 ++++++++++++++++-------------------------------\n 1 file changed, 23 insertions(+), 47 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 5fcf281dfb..791e2a0e74 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -7,62 +7,45 @@\n unset EDITOR VISUAL GIT_EDITOR\n \n test_expect_success 'determine default editor' '\n-\n \tvi=$(TERM=vt100 git var GIT_EDITOR) &&\n \ttest -n \"$vi\"\n-\n '\n \n-if ! expr \"$vi\" : '[a-z]*$' >/dev/null\n-then\n-\tvi=\n-fi\n-\n-for i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n-do\n-\tcat >e-$i.sh <<-EOF\n-\t#!$SHELL_PATH\n-\techo \"Edited by $i\" >\"\\$1\"\n-\tEOF\n-\tchmod +x e-$i.sh\n-done\n-\n-if ! test -z \"$vi\"\n-then\n-\tmv e-$vi.sh $vi\n-fi\n-\n test_expect_success setup '\n+\tif ! expr \"$vi\" : \"[a-z]*$\" >/dev/null\n+\tthen\n+\t\tvi=\n+\tfi &&\n+\n+\tfor i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n+\tdo\n+\t\twrite_script e-$i.sh <<-EOF || return 1\n+\t\t\techo \"Edited by $i\" >\"\\$1\"\n+\t\tEOF\n+\tdone &&\n+\n+\tif ! test -z \"$vi\"\n+\tthen\n+\t\tmv e-$vi.sh $vi\n+\tfi &&\n \n \tmsg=\"Hand-edited\" &&\n \ttest_commit \"$msg\" &&\n-\techo \"$msg\" >expect &&\n-\tgit show -s --format=%s > actual &&\n-\ttest_cmp expect actual\n-\n+\ttest_commit_message HEAD -m \"$msg\"\n '\n \n TERM=dumb\n export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n-\n-\tif git commit --amend\n-\tthen\n-\t\techo \"Oops?\"\n-\t\tfalse\n-\telse\n-\t\t: happy\n-\tfi\n+\ttest_must_fail git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n-\n \tEDITOR=./e-EDITOR.sh &&\n \tVISUAL=./e-VISUAL.sh &&\n \texport EDITOR VISUAL &&\n \tgit commit --amend &&\n-\ttest \"$(git show -s --format=%s)\" = \"Edited by EDITOR\"\n-\n+\ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n \n TERM=vt100\n@@ -83,9 +66,7 @@\n \tesac\n \ttest_expect_success \"Using $i\" '\n \t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline |\n-\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_commit_message HEAD expect\n \t'\n done\n \n@@ -105,9 +86,7 @@\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n \t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline |\n-\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_commit_message HEAD expect\n \t'\n done\n \n@@ -115,17 +94,14 @@\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n-\ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n+\ttest_commit_message HEAD -m space\n '\n \n unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\n \tgit config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n-\ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n+\ttest_commit_message HEAD -m space\n '\n \n test_done\n-- \n2.48.1\n\n"},{"id":"524000","messageId":"20250811221706.67168-3-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250810160323.49372-1-ben.knoble+github@gmail.com","subject":"[PATCH v3 2/4] t7005: stop abusing --exec-path","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-11T22:16:53Z","receivedAt":"2025-08-11T22:17:13Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"We want the editors in this test on PATH, so put them there.\n\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 791e2a0e74..0a5861b7f0 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -65,7 +65,7 @@\n \t\t;;\n \tesac\n \ttest_expect_success \"Using $i\" '\n-\t\tgit --exec-path=. commit --amend &&\n+\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n@@ -85,7 +85,7 @@\n \t\t;;\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n-\t\tgit --exec-path=. commit --amend &&\n+\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n-- \n2.48.1\n\n"},{"id":"524001","messageId":"20250811221706.67168-4-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250810160323.49372-1-ben.knoble+github@gmail.com","subject":"[PATCH v3 3/4] t7005: sanitize test environment for subsequent tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-11T22:16:54Z","receivedAt":"2025-08-11T22:17:14Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Some of the editor tests manipulate the environment or config in ways\nthat affect future tests, but those modifications are visible to future\ntests and create a footgun for them.\n\nUse test_config, subshells, single-command environment overrides, and\ntest helpers to automatically undo environment and config modifications\nonce finished.\n\nBest-viewed-with: --ignore-all-space\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 83 ++++++++++++++++++++++-------------------------\n 1 file changed, 39 insertions(+), 44 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 0a5861b7f0..3a5f4b897e 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -34,61 +34,57 @@\n \ttest_commit_message HEAD -m \"$msg\"\n '\n \n-TERM=dumb\n-export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n-\ttest_must_fail git commit --amend\n+\tTERM=dumb test_must_fail git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n-\tEDITOR=./e-EDITOR.sh &&\n-\tVISUAL=./e-VISUAL.sh &&\n-\texport EDITOR VISUAL &&\n-\tgit commit --amend &&\n+\tTERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n+\t\tgit commit --amend &&\n \ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n \n-TERM=vt100\n-export TERM\n for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n do\n-\techo \"Edited by $i\" >expect\n-\tunset EDITOR VISUAL GIT_EDITOR\n-\tgit config --unset-all core.editor\n-\tcase \"$i\" in\n-\tcore_editor)\n-\t\tgit config core.editor ./e-core_editor.sh\n-\t\t;;\n-\t[A-Z]*)\n-\t\teval \"$i=./e-$i.sh\"\n-\t\texport $i\n-\t\t;;\n-\tesac\n \ttest_expect_success \"Using $i\" '\n-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n-\t\ttest_commit_message HEAD expect\n+\t\tif test \"$i\" = core_editor\n+\t\tthen\n+\t\t\ttest_config core.editor ./e-core_editor.sh\n+\t\tfi &&\n+\t\t(\n+\t\t\tcase \"$i\" in\n+\t\t\t[A-Z]*)\n+\t\t\t\teval \"$i=./e-$i.sh\" &&\n+\t\t\t\texport $i\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\t\t\tPATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n+\t\t) &&\n+\t\ttest_commit_message HEAD -m \"Edited by $i\"\n \t'\n done\n \n-unset EDITOR VISUAL GIT_EDITOR\n-git config --unset-all core.editor\n-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n-do\n-\techo \"Edited by $i\" >expect\n-\tcase \"$i\" in\n-\tcore_editor)\n-\t\tgit config core.editor ./e-core_editor.sh\n-\t\t;;\n-\t[A-Z]*)\n-\t\teval \"$i=./e-$i.sh\"\n-\t\texport $i\n-\t\t;;\n-\tesac\n-\ttest_expect_success \"Using $i (override)\" '\n-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n-\t\ttest_commit_message HEAD expect\n-\t'\n-done\n+test_expect_success 'Using editors with overrides' '\n+\t(\n+\t\tTERM=vt100 &&\n+\t\texport TERM &&\n+\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n+\t\tdo\n+\t\t\techo \"Edited by $i\" >expect &&\n+\t\t\tcase \"$i\" in\n+\t\t\tcore_editor)\n+\t\t\t\tgit config core.editor ./e-core_editor.sh\n+\t\t\t\t;;\n+\t\t\t[A-Z]*)\n+\t\t\t\teval \"$i=./e-$i.sh\" &&\n+\t\t\t\texport $i\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\t\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n+\t\t\ttest_commit_message HEAD expect || exit 1\n+\t\tdone\n+\t)\n+'\n \n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n@@ -97,9 +93,8 @@\n \ttest_commit_message HEAD -m space\n '\n \n-unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n+\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n \ttest_commit_message HEAD -m space\n '\n-- \n2.48.1\n\n"},{"id":"524002","messageId":"20250811221706.67168-5-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250810160323.49372-1-ben.knoble+github@gmail.com","subject":"[PATCH v3 4/4] editor: use standard strvec API to receive environment for external editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-11T22:16:55Z","receivedAt":"2025-08-11T22:17:16Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Going back to the introduction of the env parameter for the editor in\n8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as\nwell., 2007-11-26), we pass a constant array of strings: as the\nsurrounding APIs evolved to use strvecs (see 8d7aa4ba6a\n(builtin/commit.c: remove the PATH_MAX limitation via dynamic\nallocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',\n2020-08-10)), the editor code did not.\n\nThere is only one caller of all 3 editor APIs that does not pass a NULL\nenvironment (the same caller for which this parameter was added), and\nit already has a strvec available to use.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: D. Ben Knoble \n---\n builtin/commit.c | 2 +-\n editor.c | 10 +++++-----\n editor.h | 7 ++++---\n 3 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b5b9608813..16cad7fb03 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1116,7 +1116,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tstruct strvec env = STRVEC_INIT;\n \n \t\tstrvec_pushf(&env, \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env.v)) {\n+\t\tif (launch_editor(git_path_commit_editmsg(), NULL, &env)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n \t\t\texit(1);\ndiff --git a/editor.c b/editor.c\nindex fd174e6a03..0bc781d50c 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -58,7 +58,7 @@ const char *git_sequence_editor(void)\n }\n \n static int launch_specified_editor(const char *editor, const char *path,\n-\t\t\t\t struct strbuf *buffer, const char *const *env)\n+\t\t\t\t struct strbuf *buffer, const struct strvec *env)\n {\n \tif (!editor)\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n@@ -89,7 +89,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \n \t\tstrvec_pushl(&p.args, editor, realpath.buf, NULL);\n \t\tif (env)\n-\t\t\tstrvec_pushv(&p.env, (const char **)env);\n+\t\t\tstrvec_pushv(&p.env, env->v);\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n \t\tif (start_command(&p) < 0) {\n@@ -124,20 +124,20 @@ static int launch_specified_editor(const char *editor, const char *path,\n \treturn 0;\n }\n \n-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)\n+int launch_editor(const char *path, struct strbuf *buffer, const struct strvec *env)\n {\n \treturn launch_specified_editor(git_editor(), path, buffer, env);\n }\n \n int launch_sequence_editor(const char *path, struct strbuf *buffer,\n-\t\t\t const char *const *env)\n+\t\t\t const struct strvec *env)\n {\n \treturn launch_specified_editor(git_sequence_editor(), path, buffer, env);\n }\n \n int strbuf_edit_interactively(struct repository *r,\n \t\t\t struct strbuf *buffer, const char *path,\n-\t\t\t const char *const *env)\n+\t\t\t const struct strvec *env)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd, res = 0;\ndiff --git a/editor.h b/editor.h\nindex f1c41df378..627e992f4d 100644\n--- a/editor.h\n+++ b/editor.h\n@@ -3,6 +3,7 @@\n \n struct repository;\n struct strbuf;\n+struct strvec;\n \n const char *git_editor(void);\n const char *git_sequence_editor(void);\n@@ -16,10 +17,10 @@ int is_terminal_dumb(void);\n * file's contents are not read into the buffer upon completion.\n */\n int launch_editor(const char *path, struct strbuf *buffer,\n-\t\t const char *const *env);\n+\t\t const struct strvec *env);\n \n int launch_sequence_editor(const char *path, struct strbuf *buffer,\n-\t\t\t const char *const *env);\n+\t\t\t const struct strvec *env);\n \n /*\n * In contrast to `launch_editor()`, this function writes out the contents\n@@ -30,6 +31,6 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,\n * If `path` is relative, it refers to a file in the `.git` directory.\n */\n int strbuf_edit_interactively(struct repository *r, struct strbuf *buffer,\n-\t\t\t const char *path, const char *const *env);\n+\t\t\t const char *path, const struct strvec *env);\n \n #endif\n-- \n2.48.1\n\n"},{"id":"524004","messageId":"CAPig+cThhm8b2vE=vAHYc5cEA9n232Lwi1c+p=LV9hK4hz=0Hw@mail.gmail.com","threadId":"63489","inReplyTo":"20250811221706.67168-4-ben.knoble+github@gmail.com","subject":"Re: [PATCH v3 3/4] t7005: sanitize test environment for subsequent tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-08-11T22:34:26Z","receivedAt":"2025-08-11T22:34:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 11, 2025 at 6:17 PM D. Ben Knoble\n wrote:\n> Some of the editor tests manipulate the environment or config in ways\n> that affect future tests, but those modifications are visible to future\n> tests and create a footgun for them.\n>\n> Use test_config, subshells, single-command environment overrides, and\n> test helpers to automatically undo environment and config modifications\n> once finished.\n>\n> Signed-off-by: D. Ben Knoble \n> ---\n> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n> @@ -34,61 +34,57 @@\n> -TERM=dumb\n> -export TERM\n> test_expect_success 'dumb should error out when falling back on vi' '\n> - test_must_fail git commit --amend\n> + TERM=dumb test_must_fail git commit --amend\n> '\n\nDon't use one-shot environment variable assignments when calling shell\nfunctions. Instead, you can do this:\n\n test_env TERM=dumb test_must_fail git commit --amend\n\nor employ the standard assignment/export boilerplate:\n\n TERM=dumb &&\n export TERM &&\n test_must_fail git commit --amend\n\nReferences:\nhttps://lore.kernel.org/git/20240727053509.34339-1-ericsunshine@charter.net/T/#u\nhttps://lore.kernel.org/git/20180713055205.32351-1-sunshine@sunshineco.com/T/#u\n"},{"id":"524006","messageId":"CAPig+cTXuTaMd1+sbSSDNbGKxxciyxAT3LMk44aSaimPco8XTQ@mail.gmail.com","threadId":"63489","inReplyTo":"20250811221706.67168-5-ben.knoble+github@gmail.com","subject":"Re: [PATCH v3 4/4] editor: use standard strvec API to receive environment for external editors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-08-11T22:46:38Z","receivedAt":"2025-08-11T22:46:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 11, 2025 at 6:17 PM D. Ben Knoble\n wrote:\n> Going back to the introduction of the env parameter for the editor in\n> 8babab95af (builtin-commit.c: export GIT_INDEX_FILE for launch_editor as\n> well., 2007-11-26), we pass a constant array of strings: as the\n> surrounding APIs evolved to use strvecs (see 8d7aa4ba6a\n> (builtin/commit.c: remove the PATH_MAX limitation via dynamic\n> allocation, 2017-01-13) and later 46b225f153 (Merge branch 'jk/strvec',\n> 2020-08-10)), the editor code did not.\n>\n> There is only one caller of all 3 editor APIs that does not pass a NULL\n> environment (the same caller for which this parameter was added), and\n> it already has a strvec available to use.\n\nI'm confused as to *why* we want to make this change, and the commit\nmessage doesn't seem to explain the motivation or why doing so is\nbeneficial.\n\n> Signed-off-by: D. Ben Knoble \n> ---\n> diff --git a/editor.h b/editor.h\n> @@ -3,6 +3,7 @@\n> int launch_editor(const char *path, struct strbuf *buffer,\n> - const char *const *env);\n> + const struct strvec *env);\n\nTypically, we would want to have the API accept the narrowest type in\norder to remain as general as possible, but this change makes it a\nrequirement that a caller must have the much wider `strvec` type at\nhand, which seems counterproductive.\n"},{"id":"524007","messageId":"6BCA1E93-7409-41FA-81FB-0CB75A3A55FB@gmail.com","threadId":"63489","inReplyTo":"20250811221706.67168-1-ben.knoble+github@gmail.com","subject":"Re: [PATCH v3 0/4] clean up some code around editors","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-08-11T22:59:08Z","receivedAt":"2025-08-11T22:59:20Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 11 août 2025 à 18:17, D. Ben Knoble a écrit :\n> \n> Changes from v2:\n> - shuffle setup code and use more helpers in 1/4\n> - insert 2/4 to stop abusing --exec-path\n> - improve environment-cleansing idioms in {2 => 3}/4\n> \n> Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)\n\nSome 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."},{"id":"524008","messageId":"xmqq8qjpz9zj.fsf@gitster.g","threadId":"63489","inReplyTo":"CAPig+cTXuTaMd1+sbSSDNbGKxxciyxAT3LMk44aSaimPco8XTQ@mail.gmail.com","subject":"Re: [PATCH v3 4/4] editor: use standard strvec API to receive environment for external editors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-11T23:58:40Z","receivedAt":"2025-08-11T23:58:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> int launch_editor(const char *path, struct strbuf *buffer,\n>> - const char *const *env);\n>> + const struct strvec *env);\n>\n> Typically, we would want to have the API accept the narrowest type in\n> order to remain as general as possible, but this change makes it a\n> requirement that a caller must have the much wider `strvec` type at\n> hand, which seems counterproductive.\n\nYup, very good point. I do not see the point of this change even\nafter looking at the whole of this patch. It is not like (being\nconst) this would make it easier for launch_editor() to further\ntweak the environment. Let's not do this.\n\n\n"},{"id":"524009","messageId":"CAPig+cTpMFfg8iTncgekq6wyfHOPidhHposLi11D3fXyfnNQRw@mail.gmail.com","threadId":"63489","inReplyTo":"6BCA1E93-7409-41FA-81FB-0CB75A3A55FB@gmail.com","subject":"Re: [PATCH v3 0/4] clean up some code around editors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-08-12T00:16:45Z","receivedAt":"2025-08-12T00:16:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 11, 2025 at 6:59 PM Ben Knoble wrote:\n> > Le 11 août 2025 à 18:17, D. Ben Knoble a écrit :\n> > Changes from v2:\n> > - shuffle setup code and use more helpers in 1/4\n> > - insert 2/4 to stop abusing --exec-path\n> > - improve environment-cleansing idioms in {2 => 3}/4\n>\n> 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.\n\nTo catch it locally, you can run:\n\n make test-lint-shell-syntax\n\nin the \"t\" directory. Alternatively, `make test-lint`, `make test` or\n`make prove` would also have caught the problem.\n"},{"id":"524064","messageId":"CALnO6CDaHZuC0=_Saccj3DuW-vaWs30PmwXf9dw1+AfsC34wWg@mail.gmail.com","threadId":"63489","inReplyTo":"CAPig+cThhm8b2vE=vAHYc5cEA9n232Lwi1c+p=LV9hK4hz=0Hw@mail.gmail.com","subject":"Re: [PATCH v3 3/4] t7005: sanitize test environment for subsequent tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-12T16:40:57Z","receivedAt":"2025-08-12T16:41:09Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Mon, Aug 11, 2025 at 6:34 PM Eric Sunshine wrote:\n>\n> On Mon, Aug 11, 2025 at 6:17 PM D. Ben Knoble\n> wrote:\n> > Some of the editor tests manipulate the environment or config in ways\n> > that affect future tests, but those modifications are visible to future\n> > tests and create a footgun for them.\n> >\n> > Use test_config, subshells, single-command environment overrides, and\n> > test helpers to automatically undo environment and config modifications\n> > once finished.\n> >\n> > Signed-off-by: D. Ben Knoble \n> > ---\n> > diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n> > @@ -34,61 +34,57 @@\n> > -TERM=dumb\n> > -export TERM\n> > test_expect_success 'dumb should error out when falling back on vi' '\n> > - test_must_fail git commit --amend\n> > + TERM=dumb test_must_fail git commit --amend\n> > '\n>\n> Don't use one-shot environment variable assignments when calling shell\n> functions. Instead, you can do this:\n>\n> test_env TERM=dumb test_must_fail git commit --amend\n>\n> or employ the standard assignment/export boilerplate:\n>\n> TERM=dumb &&\n> export TERM &&\n> test_must_fail git commit --amend\n>\n> References:\n> https://lore.kernel.org/git/20240727053509.34339-1-ericsunshine@charter.net/T/#u\n> https://lore.kernel.org/git/20180713055205.32351-1-sunshine@sunshineco.com/T/#u\n\nYep ;) I had the latter, switched (see range-diff, I think), and then\nCI caught me. Why doesn't the local test run catch it, though?\n"},{"id":"524065","messageId":"CALnO6CApnHgakYf_ihVOxkmOq9_Rph5hkK_9aC6vrecFhgjaTA@mail.gmail.com","threadId":"63489","inReplyTo":"CAPig+cTpMFfg8iTncgekq6wyfHOPidhHposLi11D3fXyfnNQRw@mail.gmail.com","subject":"Re: [PATCH v3 0/4] clean up some code around editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-08-12T16:42:28Z","receivedAt":"2025-08-12T16:42:42Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Mon, Aug 11, 2025 at 8:16 PM Eric Sunshine wrote:\n>\n> On Mon, Aug 11, 2025 at 6:59 PM Ben Knoble wrote:\n> > > Le 11 août 2025 à 18:17, D. Ben Knoble a écrit :\n> > > Changes from v2:\n> > > - shuffle setup code and use more helpers in 1/4\n> > > - insert 2/4 to stop abusing --exec-path\n> > > - improve environment-cleansing idioms in {2 => 3}/4\n> >\n> > 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.\n>\n> To catch it locally, you can run:\n>\n> make test-lint-shell-syntax\n>\n> in the \"t\" directory. Alternatively, `make test-lint`, `make test` or\n> `make prove` would also have caught the problem.\n\nAh, thanks. I was particularly confused because running the test file\ndirectly does run the chainlint check, I think, but not this lint.\n\n-- \nD. Ben Knoble\n"},{"id":"524068","messageId":"20250812170256.71751-1-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250811221706.67168-1-ben.knoble+github@gmail.com","subject":"[PATCH v4 0/3] clean up some code around editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-12T17:02:15Z","receivedAt":"2025-08-12T17:02:59Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Changes from v3:\n- drop 4/4\n- use test_env (including a case our lint does not catch when the value\n has spaces)\n\nChanges from v2:\n- shuffle setup code and use more helpers in 1/4\n- insert 2/4 to stop abusing --exec-path\n- improve environment-cleansing idioms in {2 => 3}/4\n\nThanks especially to Phillip's encyclopaedic knowledge of test helpers ;)\n\nChanges from v1:\n- add a prep patch with style fixes to t7005\n- rework the environment munging to use subshells, per Phillip Wood's\n suggestion\n\nThis reroll of the previous exec-path series is simplified to contain\nonly the first 2 cleanup patches, which were largely acked by the list.\nDrop the controversial and broken PATH munging.\n\nAlso, this version is (still) based on a later master 112648dd6b (Merge\nbranch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the\noriginal from May.\n\nThese patches clean up some old code in the editor tests and subsystem\nthat does not use our modern idioms.\n\nv1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/\nv2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/\nv3: https://lore.kernel.org/git/20250811221706.67168-1-ben.knoble+github@gmail.com/\nPublished-as: https://github.com/benknoble/tree/editor-cleanup\n\nD. Ben Knoble (3):\n t7005: use modern test style\n t7005: stop abusing --exec-path\n t7005: sanitize test environment for subsequent tests\n\n t/t7005-editor.sh | 149 +++++++++++++++++++---------------------------\n 1 file changed, 60 insertions(+), 89 deletions(-)\n\nDiff-intervalle contre v3 :\n1: 8ad2904a18 = 1: 8ad2904a18 t7005: use modern test style\n2: 9451e4f0f6 ! 2: 44a6fd8eb3 t7005: stop abusing --exec-path\n @@ t/t7005-editor.sh\n \tesac\n \ttest_expect_success \"Using $i\" '\n -\t\tgit --exec-path=. commit --amend &&\n -+\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n ++\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n @@ t/t7005-editor.sh\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n -\t\tgit --exec-path=. commit --amend &&\n -+\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n ++\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n3: 61cb116780 ! 3: 135d4368d6 t7005: sanitize test environment for subsequent tests\n @@ t/t7005-editor.sh\n -export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n -\ttest_must_fail git commit --amend\n -+\tTERM=dumb test_must_fail git commit --amend\n ++\ttest_env TERM=dumb test_must_fail git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n @@ t/t7005-editor.sh\n -\tVISUAL=./e-VISUAL.sh &&\n -\texport EDITOR VISUAL &&\n -\tgit commit --amend &&\n -+\tTERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n ++\ttest_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n +\t\tgit commit --amend &&\n \ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n @@ t/t7005-editor.sh\n -\t\t;;\n -\tesac\n \ttest_expect_success \"Using $i\" '\n --\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n +-\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n -\t\ttest_commit_message HEAD expect\n +\t\tif test \"$i\" = core_editor\n +\t\tthen\n @@ t/t7005-editor.sh\n +\t\t\t\texport $i\n +\t\t\t\t;;\n +\t\t\tesac &&\n -+\t\t\tPATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n ++\t\t\ttest_env PATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n +\t\t) &&\n +\t\ttest_commit_message HEAD -m \"Edited by $i\"\n \t'\n @@ t/t7005-editor.sh\n -\t\t;;\n -\tesac\n -\ttest_expect_success \"Using $i (override)\" '\n --\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n +-\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n -\t\ttest_commit_message HEAD expect\n -\t'\n -done\n @@ t/t7005-editor.sh\n +\t\t\t\texport $i\n +\t\t\t\t;;\n +\t\t\tesac &&\n -+\t\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n ++\t\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n +\t\t\ttest_commit_message HEAD expect || exit 1\n +\t\tdone\n +\t)\n @@ t/t7005-editor.sh\n \n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n -@@\n + \tchmod a+x \"e space.sh\" &&\n +-\tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n ++\ttest_env GIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n \ttest_commit_message HEAD -m space\n '\n \n4: ea269f2442 < -: ---------- editor: use standard strvec API to receive environment for external editors\n\nbase-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be\n-- \n2.48.1\n\n"},{"id":"524069","messageId":"20250812170256.71751-2-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250811221706.67168-1-ben.knoble+github@gmail.com","subject":"[PATCH v4 1/3] t7005: use modern test style","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-12T17:02:16Z","receivedAt":"2025-08-12T17:03:01Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Tests in t7005 mask Git error codes and do not use our nice test\nhelpers. Improve that, move some code into the setup test, and drop a\nfew old-style blank lines while at it.\n\nBest-viewed-with: --ignore-all-space\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 70 ++++++++++++++++-------------------------------\n 1 file changed, 23 insertions(+), 47 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 5fcf281dfb..791e2a0e74 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -7,62 +7,45 @@\n unset EDITOR VISUAL GIT_EDITOR\n \n test_expect_success 'determine default editor' '\n-\n \tvi=$(TERM=vt100 git var GIT_EDITOR) &&\n \ttest -n \"$vi\"\n-\n '\n \n-if ! expr \"$vi\" : '[a-z]*$' >/dev/null\n-then\n-\tvi=\n-fi\n-\n-for i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n-do\n-\tcat >e-$i.sh <<-EOF\n-\t#!$SHELL_PATH\n-\techo \"Edited by $i\" >\"\\$1\"\n-\tEOF\n-\tchmod +x e-$i.sh\n-done\n-\n-if ! test -z \"$vi\"\n-then\n-\tmv e-$vi.sh $vi\n-fi\n-\n test_expect_success setup '\n+\tif ! expr \"$vi\" : \"[a-z]*$\" >/dev/null\n+\tthen\n+\t\tvi=\n+\tfi &&\n+\n+\tfor i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n+\tdo\n+\t\twrite_script e-$i.sh <<-EOF || return 1\n+\t\t\techo \"Edited by $i\" >\"\\$1\"\n+\t\tEOF\n+\tdone &&\n+\n+\tif ! test -z \"$vi\"\n+\tthen\n+\t\tmv e-$vi.sh $vi\n+\tfi &&\n \n \tmsg=\"Hand-edited\" &&\n \ttest_commit \"$msg\" &&\n-\techo \"$msg\" >expect &&\n-\tgit show -s --format=%s > actual &&\n-\ttest_cmp expect actual\n-\n+\ttest_commit_message HEAD -m \"$msg\"\n '\n \n TERM=dumb\n export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n-\n-\tif git commit --amend\n-\tthen\n-\t\techo \"Oops?\"\n-\t\tfalse\n-\telse\n-\t\t: happy\n-\tfi\n+\ttest_must_fail git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n-\n \tEDITOR=./e-EDITOR.sh &&\n \tVISUAL=./e-VISUAL.sh &&\n \texport EDITOR VISUAL &&\n \tgit commit --amend &&\n-\ttest \"$(git show -s --format=%s)\" = \"Edited by EDITOR\"\n-\n+\ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n \n TERM=vt100\n@@ -83,9 +66,7 @@\n \tesac\n \ttest_expect_success \"Using $i\" '\n \t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline |\n-\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_commit_message HEAD expect\n \t'\n done\n \n@@ -105,9 +86,7 @@\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n \t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline |\n-\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_commit_message HEAD expect\n \t'\n done\n \n@@ -115,17 +94,14 @@\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n-\ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n+\ttest_commit_message HEAD -m space\n '\n \n unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\n \tgit config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n-\ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n+\ttest_commit_message HEAD -m space\n '\n \n test_done\n-- \n2.48.1\n\n"},{"id":"524070","messageId":"20250812170256.71751-3-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250811221706.67168-1-ben.knoble+github@gmail.com","subject":"[PATCH v4 2/3] t7005: stop abusing --exec-path","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-12T17:02:17Z","receivedAt":"2025-08-12T17:03:02Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"We want the editors in this test on PATH, so put them there.\n\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 791e2a0e74..b384eff5d8 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -65,7 +65,7 @@\n \t\t;;\n \tesac\n \ttest_expect_success \"Using $i\" '\n-\t\tgit --exec-path=. commit --amend &&\n+\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n@@ -85,7 +85,7 @@\n \t\t;;\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n-\t\tgit --exec-path=. commit --amend &&\n+\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n-- \n2.48.1\n\n"},{"id":"524071","messageId":"20250812170256.71751-4-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250811221706.67168-1-ben.knoble+github@gmail.com","subject":"[PATCH v4 3/3] t7005: sanitize test environment for subsequent tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-12T17:02:18Z","receivedAt":"2025-08-12T17:03:03Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Some of the editor tests manipulate the environment or config in ways\nthat affect future tests, but those modifications are visible to future\ntests and create a footgun for them.\n\nUse test_config, subshells, single-command environment overrides, and\ntest helpers to automatically undo environment and config modifications\nonce finished.\n\nBest-viewed-with: --ignore-all-space\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 85 ++++++++++++++++++++++-------------------------\n 1 file changed, 40 insertions(+), 45 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex b384eff5d8..6cfefa916b 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -34,72 +34,67 @@\n \ttest_commit_message HEAD -m \"$msg\"\n '\n \n-TERM=dumb\n-export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n-\ttest_must_fail git commit --amend\n+\ttest_env TERM=dumb test_must_fail git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n-\tEDITOR=./e-EDITOR.sh &&\n-\tVISUAL=./e-VISUAL.sh &&\n-\texport EDITOR VISUAL &&\n-\tgit commit --amend &&\n+\ttest_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n+\t\tgit commit --amend &&\n \ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n \n-TERM=vt100\n-export TERM\n for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n do\n-\techo \"Edited by $i\" >expect\n-\tunset EDITOR VISUAL GIT_EDITOR\n-\tgit config --unset-all core.editor\n-\tcase \"$i\" in\n-\tcore_editor)\n-\t\tgit config core.editor ./e-core_editor.sh\n-\t\t;;\n-\t[A-Z]*)\n-\t\teval \"$i=./e-$i.sh\"\n-\t\texport $i\n-\t\t;;\n-\tesac\n \ttest_expect_success \"Using $i\" '\n-\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n-\t\ttest_commit_message HEAD expect\n+\t\tif test \"$i\" = core_editor\n+\t\tthen\n+\t\t\ttest_config core.editor ./e-core_editor.sh\n+\t\tfi &&\n+\t\t(\n+\t\t\tcase \"$i\" in\n+\t\t\t[A-Z]*)\n+\t\t\t\teval \"$i=./e-$i.sh\" &&\n+\t\t\t\texport $i\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\t\t\ttest_env PATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n+\t\t) &&\n+\t\ttest_commit_message HEAD -m \"Edited by $i\"\n \t'\n done\n \n-unset EDITOR VISUAL GIT_EDITOR\n-git config --unset-all core.editor\n-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n-do\n-\techo \"Edited by $i\" >expect\n-\tcase \"$i\" in\n-\tcore_editor)\n-\t\tgit config core.editor ./e-core_editor.sh\n-\t\t;;\n-\t[A-Z]*)\n-\t\teval \"$i=./e-$i.sh\"\n-\t\texport $i\n-\t\t;;\n-\tesac\n-\ttest_expect_success \"Using $i (override)\" '\n-\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n-\t\ttest_commit_message HEAD expect\n-\t'\n-done\n+test_expect_success 'Using editors with overrides' '\n+\t(\n+\t\tTERM=vt100 &&\n+\t\texport TERM &&\n+\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n+\t\tdo\n+\t\t\techo \"Edited by $i\" >expect &&\n+\t\t\tcase \"$i\" in\n+\t\t\tcore_editor)\n+\t\t\t\tgit config core.editor ./e-core_editor.sh\n+\t\t\t\t;;\n+\t\t\t[A-Z]*)\n+\t\t\t\teval \"$i=./e-$i.sh\" &&\n+\t\t\t\texport $i\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\t\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n+\t\t\ttest_commit_message HEAD expect || exit 1\n+\t\tdone\n+\t)\n+'\n \n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n-\tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n+\ttest_env GIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n \ttest_commit_message HEAD -m space\n '\n \n-unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n+\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n \ttest_commit_message HEAD -m space\n '\n-- \n2.48.1\n\n"},{"id":"524075","messageId":"CAPig+cRRBDqewrqUZGS=EPJgSkYmX-zD_qrqbRXzXq59Ub2m_g@mail.gmail.com","threadId":"63489","inReplyTo":"CALnO6CDaHZuC0=_Saccj3DuW-vaWs30PmwXf9dw1+AfsC34wWg@mail.gmail.com","subject":"Re: [PATCH v3 3/4] t7005: sanitize test environment for subsequent tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-08-12T17:13:21Z","receivedAt":"2025-08-12T17:13:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 12, 2025 at 12:41 PM D. Ben Knoble\n wrote:\n> On Mon, Aug 11, 2025 at 6:34 PM Eric Sunshine wrote:\n> > On Mon, Aug 11, 2025 at 6:17 PM D. Ben Knoble\n> > wrote:\n> > > + TERM=dumb test_must_fail git commit --amend\n> >\n> > Don't use one-shot environment variable assignments when calling shell\n> > functions. Instead, you can do this:\n> >\n> > test_env TERM=dumb test_must_fail git commit --amend\n>\n> Yep ;) I had the latter, switched (see range-diff, I think), and then\n> CI caught me. Why doesn't the local test run catch it, though?\n\nAlthough the `chainlint` linter gets wired into each test script, thus\nis invoked automatically when manually running any individual test\nscript, the `check-non-portable-shell` linter does not get wired into\neach test script, thus does not get run automatically when manually\nrunning a test script.\n\nAlmost all of the \"lints\" performed by `check-non-portable`\ncould/should eventually be folded into `chainlint`, thus eliminating\nthis difference in behavior, but the\n\"shell-function-one-shot-variable-assignment\" lint is an outlier\nbecause it can't be easily localized. Specifically, whereas all the\nother lints can operate just by consulting local context, that one\nlint can't because it can't know what command names are in fact shell\nfunctions without looking at other test scripts, as well. Hence, the\noperation of check-non-portable-shell.sh is two-phase: (1) it first\nscans all scripts to find all shell function definitions, and then (2)\nit \"lints\" each test script individually, in the process consulting\nthe table of function names compiled in step 1.\n\nWhen you run a test script manually, the script triggers a run of\n`chainlint`, but `chainlint` only knows about that one script it's\nchecking, thus it can't compile a table of shell functions which might\nbe defined in other scripts. Obviously, there is no technical reason\nthat this couldn't be implemented directly in `chainlint`; for\ninstance, `chainlint` could detect other scripts which are pulled in\nby the script it is testing and compile a table of names of shell\nfunctions defined in those scripts. Eventually, I think it would be\nnice for `chainlint` to subsume all the checks performed by\n`check-non-portable-shell`, thus eliminating the latter script and\n(hopefully) speeding up linting altogether (at least a bit) but the\nwork to do so simply hasn't been done yet.\n"},{"id":"524076","messageId":"xmqq1ppgv3wg.fsf@gitster.g","threadId":"63489","inReplyTo":"CAPig+cTpMFfg8iTncgekq6wyfHOPidhHposLi11D3fXyfnNQRw@mail.gmail.com","subject":"Re: [PATCH v3 0/4] clean up some code around editors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-12T17:35:59Z","receivedAt":"2025-08-12T17:36:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Aug 11, 2025 at 6:59 PM Ben Knoble wrote:\n>> > Le 11 août 2025 à 18:17, D. Ben Knoble a écrit :\n>> > Changes from v2:\n>> > - shuffle setup code and use more helpers in 1/4\n>> > - insert 2/4 to stop abusing --exec-path\n>> > - improve environment-cleansing idioms in {2 => 3}/4\n>>\n>> 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.\n>\n> To catch it locally, you can run:\n>\n> make test-lint-shell-syntax\n>\n> in the \"t\" directory. Alternatively, `make test-lint`, `make test` or\n> `make prove` would also have caught the problem.\n\nAmong these three `make test` can be run from the top-level. The\nother two cannot.\n\n\"make test\" is a bit too heavy-weight to use as an initial sanity\ncheck for tests that are being newly developed, and I am wondering\nif something like this can be added as a first-line sanity checking\ntool. The idea is a simple \"make\" to build, while DEVELOPER=Yes is\nset, would trigger the common linting any developer who is working\non new things needs to pay attention to.\n\n\n\n Makefile | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git c/Makefile w/Makefile\nindex e11340c1ae..7f21afeaf9 100644\n--- c/Makefile\n+++ w/Makefile\n@@ -1471,6 +1471,7 @@ include config.mak.uname\n \n ifdef DEVELOPER\n include config.mak.dev\n+all:: check-developer\n endif\n \n GIT-VERSION-FILE: FORCE\n@@ -3350,6 +3351,10 @@ check:\n \t\texit 1; \\\n \tfi\n \n+# We may want to take over 'make check' for this, but for now...\n+.PHONY: check-developer\n+check-developer: check-docs check-tests check-builtins check-headers\n+\n COCCI_GEN_ALL = .build/contrib/coccinelle/ALL.cocci\n COCCI_GLOB = $(wildcard contrib/coccinelle/*.cocci)\n COCCI_RULES_TRACKED = $(COCCI_GLOB:%=.build/%)\n@@ -3942,6 +3947,10 @@ build-unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG)\n unit-tests: $(UNIT_TEST_PROGS) $(CLAR_TEST_PROG) t/helper/test-tool$X\n \t$(MAKE) -C t/ unit-tests\n \n+.PHONY: check-tests\n+check-tests:\n+\t$(MAKE) -C t/ test-lint\n+\n .PHONY: libgit-sys libgit-rs\n libgit-sys libgit-rs:\n \t$(QUIET)(\\\n"},{"id":"524079","messageId":"CAPig+cQMa32G0pqi1UU6hqRAB5709Zcif0pd5uWifqB8d9o6rg@mail.gmail.com","threadId":"63489","inReplyTo":"xmqq1ppgv3wg.fsf@gitster.g","subject":"Re: [PATCH v3 0/4] clean up some code around editors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-08-12T18:13:01Z","receivedAt":"2025-08-12T18:13:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 12, 2025 at 1:36 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > To catch it locally, you can run:\n> > make test-lint-shell-syntax\n> > in the \"t\" directory. Alternatively, `make test-lint`, `make test` or\n> > `make prove` would also have caught the problem.\n>\n> \"make test\" is a bit too heavy-weight to use as an initial sanity\n> check for tests that are being newly developed, and I am wondering\n> if something like this can be added as a first-line sanity checking\n> tool. The idea is a simple \"make\" to build, while DEVELOPER=Yes is\n> set, would trigger the common linting any developer who is working\n> on new things needs to pay attention to.\n>\n> ifdef DEVELOPER\n> include config.mak.dev\n> +all:: check-developer\n> endif\n>\n> +check-developer: check-docs check-tests check-builtins check-headers\n>\n> +.PHONY: check-tests\n> +check-tests:\n> + $(MAKE) -C t/ test-lint\n\nNot a bad idea, though I don't think we need to hide the target behind\nDEVELOPER.\n\nThe \"first-line sanity checking\" phrasing you used above suggested the\nname \"make check-sanity\" to me as being more meaningful and obvious\nthan \"make check-developer\". However, upon reflection, \"sanity\" is\nperhaps too generic, thus might not convey that these checks are for\ncode newly-developed (or changed) by developers, so perhaps \"make\ncheck-developer\" is indeed the better name choice.\n"},{"id":"524080","messageId":"xmqq8qjotn5s.fsf@gitster.g","threadId":"63489","inReplyTo":"CAPig+cQMa32G0pqi1UU6hqRAB5709Zcif0pd5uWifqB8d9o6rg@mail.gmail.com","subject":"Re: [PATCH v3 0/4] clean up some code around editors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-12T18:22:55Z","receivedAt":"2025-08-12T18:22:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\nEric Sunshine writes:\n\n>> ifdef DEVELOPER\n>> include config.mak.dev\n>> +all:: check-developer\n>> endif\n>> ...\n> Not a bad idea, though I don't think we need to hide the target behind\n> DEVELOPER.\n\nThis target is designed to be a collection of light-weight tests for\nyour uncooked code, so running it when somebody makes a production\nbuild should *not* be too costly, but at the same time, it is more\nor less pointless---if it catches somethig for a build engineer, it\nis way too late.\n\nOn the other hand, if you have only started to add a new command and\ntrying to see if your skeletal implementation even compiles, it may\nbe annoying to be told that you still have to write documentation.\nYou may already know you need to, but you are not ready to do so\nyet. Even though I on purpose made the checks run as part of \"all\"\nto give the target more exposure, I am not sure if limiting to\ndeveloper is still too aggressive.\n"},{"id":"524081","messageId":"CAPig+cSuHoUesmnPkQ-VBXsZFtG_jmXV_DHB_jFHd=nT_D43ng@mail.gmail.com","threadId":"63489","inReplyTo":"xmqq8qjotn5s.fsf@gitster.g","subject":"Re: [PATCH v3 0/4] clean up some code around editors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-08-12T18:30:02Z","receivedAt":"2025-08-12T18:30:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 12, 2025 at 2:22 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> >> ifdef DEVELOPER\n> >> include config.mak.dev\n> >> +all:: check-developer\n> >> endif\n> >> ...\n> > Not a bad idea, though I don't think we need to hide the target behind\n> > DEVELOPER.\n>\n> This target is designed to be a collection of light-weight tests for\n> your uncooked code, so running it when somebody makes a production\n> build should *not* be too costly, but at the same time, it is more\n> or less pointless---if it catches somethig for a build engineer, it\n> is way too late.\n\nUgh, I missed the bit where you had wired it into the \"all\" target; I\nmisread it as defining the \"check-developer\" target only if DEVELOPER\nwas set, so my response was bogus. Sorry for the noise.\n\n> On the other hand, if you have only started to add a new command and\n> trying to see if your skeletal implementation even compiles, it may\n> be annoying to be told that you still have to write documentation.\n> You may already know you need to, but you are not ready to do so\n> yet. Even though I on purpose made the checks run as part of \"all\"\n> to give the target more exposure, I am not sure if limiting to\n> developer is still too aggressive.\n\nIndeed, wiring it into \"all\" may be too aggressive. Because I missed\nthat bit, I had thought that you just meant for developers to run\n\"make check-developer\" manually. Documenting it in SubmittingPatches\nmay be the lesser evil.\n"},{"id":"524118","messageId":"2250a8fd-62b8-4218-9858-23032d9c807d@gmail.com","threadId":"63489","inReplyTo":"20250812170256.71751-1-ben.knoble+github@gmail.com","subject":"Re: [PATCH v4 0/3] clean up some code around editors","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-13T10:14:40Z","receivedAt":"2025-08-13T10:14:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 12/08/2025 18:02, D. Ben Knoble wrote:\n> Changes from v3:\n> - drop 4/4\n> - use test_env (including a case our lint does not catch when the value\n> has spaces)\n\nIt's not worth a re-roll but for future reference\n\n\ttest_env FOO=bar git commit --amend\n\nuses an extra process compared to\n\n\tFOO=bar git commit --amend\n\nwhich slows the test suite down for no real gain. We should only need to \nuse test_env to set environment variables when calling a shell function. \nIn the special case of test_must_fail it supports\n\n\ttest_must_fail env FOO=bar git commit --amend\n\nwhich is widely used in our test suite\n\n$ git grep 'test_must_fail env ' origin/master t/t\\*.sh | wc -l\n152\n$ git grep 'test_env .* test_must_fail ' origin/master t/t\\*.sh | wc -l\n1\n\nThanks for cleaning up this test file, it's looking much nicer now.\n\nPhillip\n\n> Changes from v2:\n> - shuffle setup code and use more helpers in 1/4\n> - insert 2/4 to stop abusing --exec-path\n> - improve environment-cleansing idioms in {2 => 3}/4\n> \n> Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)\n> \n> Changes from v1:\n> - add a prep patch with style fixes to t7005\n> - rework the environment munging to use subshells, per Phillip Wood's\n> suggestion\n> \n> This reroll of the previous exec-path series is simplified to contain\n> only the first 2 cleanup patches, which were largely acked by the list.\n> Drop the controversial and broken PATH munging.\n> \n> Also, this version is (still) based on a later master 112648dd6b (Merge\n> branch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the\n> original from May.\n> \n> These patches clean up some old code in the editor tests and subsystem\n> that does not use our modern idioms.\n> \n> v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/\n> v2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/\n> v3: https://lore.kernel.org/git/20250811221706.67168-1-ben.knoble+github@gmail.com/\n> Published-as: https://github.com/benknoble/tree/editor-cleanup\n> \n> D. Ben Knoble (3):\n> t7005: use modern test style\n> t7005: stop abusing --exec-path\n> t7005: sanitize test environment for subsequent tests\n> \n> t/t7005-editor.sh | 149 +++++++++++++++++++---------------------------\n> 1 file changed, 60 insertions(+), 89 deletions(-)\n> \n> Diff-intervalle contre v3 :\n> 1: 8ad2904a18 = 1: 8ad2904a18 t7005: use modern test style\n> 2: 9451e4f0f6 ! 2: 44a6fd8eb3 t7005: stop abusing --exec-path\n> @@ t/t7005-editor.sh\n> \tesac\n> \ttest_expect_success \"Using $i\" '\n> -\t\tgit --exec-path=. commit --amend &&\n> -+\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> ++\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> \t\ttest_commit_message HEAD expect\n> \t'\n> done\n> @@ t/t7005-editor.sh\n> \tesac\n> \ttest_expect_success \"Using $i (override)\" '\n> -\t\tgit --exec-path=. commit --amend &&\n> -+\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> ++\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> \t\ttest_commit_message HEAD expect\n> \t'\n> done\n> 3: 61cb116780 ! 3: 135d4368d6 t7005: sanitize test environment for subsequent tests\n> @@ t/t7005-editor.sh\n> -export TERM\n> test_expect_success 'dumb should error out when falling back on vi' '\n> -\ttest_must_fail git commit --amend\n> -+\tTERM=dumb test_must_fail git commit --amend\n> ++\ttest_env TERM=dumb test_must_fail git commit --amend\n> '\n> \n> test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n> @@ t/t7005-editor.sh\n> -\tVISUAL=./e-VISUAL.sh &&\n> -\texport EDITOR VISUAL &&\n> -\tgit commit --amend &&\n> -+\tTERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n> ++\ttest_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n> +\t\tgit commit --amend &&\n> \ttest_commit_message HEAD -m \"Edited by EDITOR\"\n> '\n> @@ t/t7005-editor.sh\n> -\t\t;;\n> -\tesac\n> \ttest_expect_success \"Using $i\" '\n> --\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> +-\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> -\t\ttest_commit_message HEAD expect\n> +\t\tif test \"$i\" = core_editor\n> +\t\tthen\n> @@ t/t7005-editor.sh\n> +\t\t\t\texport $i\n> +\t\t\t\t;;\n> +\t\t\tesac &&\n> -+\t\t\tPATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n> ++\t\t\ttest_env PATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n> +\t\t) &&\n> +\t\ttest_commit_message HEAD -m \"Edited by $i\"\n> \t'\n> @@ t/t7005-editor.sh\n> -\t\t;;\n> -\tesac\n> -\ttest_expect_success \"Using $i (override)\" '\n> --\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> +-\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> -\t\ttest_commit_message HEAD expect\n> -\t'\n> -done\n> @@ t/t7005-editor.sh\n> +\t\t\t\texport $i\n> +\t\t\t\t;;\n> +\t\t\tesac &&\n> -+\t\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> ++\t\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> +\t\t\ttest_commit_message HEAD expect || exit 1\n> +\t\tdone\n> +\t)\n> @@ t/t7005-editor.sh\n> \n> test_expect_success 'editor with a space' '\n> \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n> -@@\n> + \tchmod a+x \"e space.sh\" &&\n> +-\tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n> ++\ttest_env GIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n> \ttest_commit_message HEAD -m space\n> '\n> \n> 4: ea269f2442 < -: ---------- editor: use standard strvec API to receive environment for external editors\n> \n> base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be\n\n"},{"id":"524134","messageId":"xmqqfrdvns9d.fsf@gitster.g","threadId":"63489","inReplyTo":"2250a8fd-62b8-4218-9858-23032d9c807d@gmail.com","subject":"Re: [PATCH v4 0/3] clean up some code around editors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-13T15:41:34Z","receivedAt":"2025-08-13T15:45:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Ben\n>\n> On 12/08/2025 18:02, D. Ben Knoble wrote:\n>> Changes from v3:\n>> - drop 4/4\n>> - use test_env (including a case our lint does not catch when the value\n>> has spaces)\n>\n> It's not worth a re-roll but for future reference\n>\n> \ttest_env FOO=bar git commit --amend\n>\n> uses an extra process compared to\n>\n> \tFOO=bar git commit --amend\n>\n> which slows the test suite down for no real gain. We should only need\n> to use test_env to set environment variables when calling a shell\n> function. In the special case of test_must_fail it supports\n>\n> \ttest_must_fail env FOO=bar git commit --amend\n>\n> which is widely used in our test suite\n\nIf this were some feature series, the story may be different, but\nsince the theme of the topic is \"clean up\", the above clean-up is\nsomething that ought to be part of an update. A clean-up topic\nshould not be adding things that need to be further cleaned up ;-)\n\nThank you, both of you, for writing and reviewing the series.\n\n\n"},{"id":"524140","messageId":"CALnO6CCUAHvi3oC1A3czRLsuGNGUQMeRomB8A8sF=2ULWcmszg@mail.gmail.com","threadId":"63489","inReplyTo":"xmqqfrdvns9d.fsf@gitster.g","subject":"Re: [PATCH v4 0/3] clean up some code around editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-13T17:36:48Z","receivedAt":"2025-08-13T17:37:00Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Wed, Aug 13, 2025 at 11:45 AM Junio C Hamano wrote:\n>\n> Phillip Wood writes:\n>\n> > Hi Ben\n> >\n> > On 12/08/2025 18:02, D. Ben Knoble wrote:\n> >> Changes from v3:\n> >> - drop 4/4\n> >> - use test_env (including a case our lint does not catch when the value\n> >> has spaces)\n> >\n> > It's not worth a re-roll but for future reference\n> >\n> > test_env FOO=bar git commit --amend\n> >\n> > uses an extra process compared to\n> >\n> > FOO=bar git commit --amend\n> >\n> > which slows the test suite down for no real gain. We should only need\n> > to use test_env to set environment variables when calling a shell\n> > function. In the special case of test_must_fail it supports\n> >\n> > test_must_fail env FOO=bar git commit --amend\n> >\n> > which is widely used in our test suite\n\nAha, I learned something new (again)—although that still means my\ndiagnosis for this about the space in the value is wrong :) It's not\nflagged because it's not a shell function. Great!\n\n> If this were some feature series, the story may be different, but\n> since the theme of the topic is \"clean up\", the above clean-up is\n> something that ought to be part of an update. A clean-up topic\n> should not be adding things that need to be further cleaned up ;-)\n\nAgreed; I don't want to add more problems here ;)\n"},{"id":"524141","messageId":"20250813175112.88435-1-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250812170256.71751-1-ben.knoble+github@gmail.com","subject":"[PATCH v5 0/3] clean up some code around editors","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-13T17:50:02Z","receivedAt":"2025-08-13T17:51:20Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Changes from v4:\n- revert use of test_env: fix the one instance that needs adjusted for\n VAR=val cmd syntax by using test_must_fail env VAR=val cmd.\n\nChanges from v3:\n- drop 4/4\n- use test_env (including a case our lint does not catch when the value\n has spaces)\n\nChanges from v2:\n- shuffle setup code and use more helpers in 1/4\n- insert 2/4 to stop abusing --exec-path\n- improve environment-cleansing idioms in {2 => 3}/4\n\nThanks especially to Phillip's encyclopaedic knowledge of test helpers ;)\n\nChanges from v1:\n- add a prep patch with style fixes to t7005\n- rework the environment munging to use subshells, per Phillip Wood's\n suggestion\n\nThese patches clean up some old code in the editor tests that does not use our\nmodern idioms.\n\nAlso, this version is (still) based on a later master 112648dd6b (Merge\nbranch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the\noriginal from May.\n\nv1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/\nv2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/\nv3: https://lore.kernel.org/git/20250811221706.67168-1-ben.knoble+github@gmail.com/\nv4: https://lore.kernel.org/git/20250812170256.71751-1-ben.knoble+github@gmail.com/\nPublished-as: https://github.com/benknoble/tree/editor-cleanup\n\nD. Ben Knoble (3):\n t7005: use modern test style\n t7005: stop abusing --exec-path\n t7005: sanitize test environment for subsequent tests\n\n t/t7005-editor.sh | 147 +++++++++++++++++++---------------------------\n 1 file changed, 59 insertions(+), 88 deletions(-)\n\nDiff-intervalle contre v4 :\n1: 8ad2904a18 = 1: 8ad2904a18 t7005: use modern test style\n2: 44a6fd8eb3 ! 2: 5a35889571 t7005: stop abusing --exec-path\n @@ t/t7005-editor.sh\n \tesac\n \ttest_expect_success \"Using $i\" '\n -\t\tgit --exec-path=. commit --amend &&\n -+\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n ++\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n @@ t/t7005-editor.sh\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n -\t\tgit --exec-path=. commit --amend &&\n -+\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n ++\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n3: 135d4368d6 ! 3: e6e31ab98c t7005: sanitize test environment for subsequent tests\n @@ t/t7005-editor.sh\n -export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n -\ttest_must_fail git commit --amend\n -+\ttest_env TERM=dumb test_must_fail git commit --amend\n ++\ttest_must_fail env TERM=dumb git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n @@ t/t7005-editor.sh\n -\tVISUAL=./e-VISUAL.sh &&\n -\texport EDITOR VISUAL &&\n -\tgit commit --amend &&\n -+\ttest_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n ++\tTERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n +\t\tgit commit --amend &&\n \ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n @@ t/t7005-editor.sh\n -\t\t;;\n -\tesac\n \ttest_expect_success \"Using $i\" '\n --\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n +-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n -\t\ttest_commit_message HEAD expect\n +\t\tif test \"$i\" = core_editor\n +\t\tthen\n @@ t/t7005-editor.sh\n +\t\t\t\texport $i\n +\t\t\t\t;;\n +\t\t\tesac &&\n -+\t\t\ttest_env PATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n ++\t\t\tPATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n +\t\t) &&\n +\t\ttest_commit_message HEAD -m \"Edited by $i\"\n \t'\n @@ t/t7005-editor.sh\n -\t\t;;\n -\tesac\n -\ttest_expect_success \"Using $i (override)\" '\n --\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n +-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n -\t\ttest_commit_message HEAD expect\n -\t'\n -done\n @@ t/t7005-editor.sh\n +\t\t\t\texport $i\n +\t\t\t\t;;\n +\t\t\tesac &&\n -+\t\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n ++\t\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n +\t\t\ttest_commit_message HEAD expect || exit 1\n +\t\tdone\n +\t)\n @@ t/t7005-editor.sh\n \n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n - \tchmod a+x \"e space.sh\" &&\n --\tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n -+\ttest_env GIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n +@@\n \ttest_commit_message HEAD -m space\n '\n \n\nbase-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be\n-- \n2.48.1\n\n"},{"id":"524143","messageId":"20250813175112.88435-2-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250812170256.71751-1-ben.knoble+github@gmail.com","subject":"[PATCH v5 1/3] t7005: use modern test style","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-13T17:50:03Z","receivedAt":"2025-08-13T17:51:21Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Tests in t7005 mask Git error codes and do not use our nice test\nhelpers. Improve that, move some code into the setup test, and drop a\nfew old-style blank lines while at it.\n\nBest-viewed-with: --ignore-all-space\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 70 ++++++++++++++++-------------------------------\n 1 file changed, 23 insertions(+), 47 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 5fcf281dfb..791e2a0e74 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -7,62 +7,45 @@\n unset EDITOR VISUAL GIT_EDITOR\n \n test_expect_success 'determine default editor' '\n-\n \tvi=$(TERM=vt100 git var GIT_EDITOR) &&\n \ttest -n \"$vi\"\n-\n '\n \n-if ! expr \"$vi\" : '[a-z]*$' >/dev/null\n-then\n-\tvi=\n-fi\n-\n-for i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n-do\n-\tcat >e-$i.sh <<-EOF\n-\t#!$SHELL_PATH\n-\techo \"Edited by $i\" >\"\\$1\"\n-\tEOF\n-\tchmod +x e-$i.sh\n-done\n-\n-if ! test -z \"$vi\"\n-then\n-\tmv e-$vi.sh $vi\n-fi\n-\n test_expect_success setup '\n+\tif ! expr \"$vi\" : \"[a-z]*$\" >/dev/null\n+\tthen\n+\t\tvi=\n+\tfi &&\n+\n+\tfor i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n+\tdo\n+\t\twrite_script e-$i.sh <<-EOF || return 1\n+\t\t\techo \"Edited by $i\" >\"\\$1\"\n+\t\tEOF\n+\tdone &&\n+\n+\tif ! test -z \"$vi\"\n+\tthen\n+\t\tmv e-$vi.sh $vi\n+\tfi &&\n \n \tmsg=\"Hand-edited\" &&\n \ttest_commit \"$msg\" &&\n-\techo \"$msg\" >expect &&\n-\tgit show -s --format=%s > actual &&\n-\ttest_cmp expect actual\n-\n+\ttest_commit_message HEAD -m \"$msg\"\n '\n \n TERM=dumb\n export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n-\n-\tif git commit --amend\n-\tthen\n-\t\techo \"Oops?\"\n-\t\tfalse\n-\telse\n-\t\t: happy\n-\tfi\n+\ttest_must_fail git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n-\n \tEDITOR=./e-EDITOR.sh &&\n \tVISUAL=./e-VISUAL.sh &&\n \texport EDITOR VISUAL &&\n \tgit commit --amend &&\n-\ttest \"$(git show -s --format=%s)\" = \"Edited by EDITOR\"\n-\n+\ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n \n TERM=vt100\n@@ -83,9 +66,7 @@\n \tesac\n \ttest_expect_success \"Using $i\" '\n \t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline |\n-\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_commit_message HEAD expect\n \t'\n done\n \n@@ -105,9 +86,7 @@\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n \t\tgit --exec-path=. commit --amend &&\n-\t\tgit show -s --pretty=oneline |\n-\t\tsed -e \"s/^[0-9a-f]* //\" >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_commit_message HEAD expect\n \t'\n done\n \n@@ -115,17 +94,14 @@\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n-\ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n+\ttest_commit_message HEAD -m space\n '\n \n unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\n \tgit config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n-\ttest space = \"$(git show -s --pretty=format:%s)\"\n-\n+\ttest_commit_message HEAD -m space\n '\n \n test_done\n-- \n2.48.1\n\n"},{"id":"524142","messageId":"20250813175112.88435-3-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250812170256.71751-1-ben.knoble+github@gmail.com","subject":"[PATCH v5 2/3] t7005: stop abusing --exec-path","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-13T17:50:04Z","receivedAt":"2025-08-13T17:51:23Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"We want the editors in this test on PATH, so put them there.\n\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 791e2a0e74..0a5861b7f0 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -65,7 +65,7 @@\n \t\t;;\n \tesac\n \ttest_expect_success \"Using $i\" '\n-\t\tgit --exec-path=. commit --amend &&\n+\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n@@ -85,7 +85,7 @@\n \t\t;;\n \tesac\n \ttest_expect_success \"Using $i (override)\" '\n-\t\tgit --exec-path=. commit --amend &&\n+\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n \t\ttest_commit_message HEAD expect\n \t'\n done\n-- \n2.48.1\n\n"},{"id":"524144","messageId":"20250813175112.88435-4-ben.knoble+github@gmail.com","threadId":"63489","inReplyTo":"20250812170256.71751-1-ben.knoble+github@gmail.com","subject":"[PATCH v5 3/3] t7005: sanitize test environment for subsequent tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-13T17:50:05Z","receivedAt":"2025-08-13T17:51:24Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Some of the editor tests manipulate the environment or config in ways\nthat affect future tests, but those modifications are visible to future\ntests and create a footgun for them.\n\nUse test_config, subshells, single-command environment overrides, and\ntest helpers to automatically undo environment and config modifications\nonce finished.\n\nBest-viewed-with: --ignore-all-space\nSigned-off-by: D. Ben Knoble \n---\n t/t7005-editor.sh | 83 ++++++++++++++++++++++-------------------------\n 1 file changed, 39 insertions(+), 44 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex 0a5861b7f0..c490e5707a 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -34,61 +34,57 @@\n \ttest_commit_message HEAD -m \"$msg\"\n '\n \n-TERM=dumb\n-export TERM\n test_expect_success 'dumb should error out when falling back on vi' '\n-\ttest_must_fail git commit --amend\n+\ttest_must_fail env TERM=dumb git commit --amend\n '\n \n test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n-\tEDITOR=./e-EDITOR.sh &&\n-\tVISUAL=./e-VISUAL.sh &&\n-\texport EDITOR VISUAL &&\n-\tgit commit --amend &&\n+\tTERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n+\t\tgit commit --amend &&\n \ttest_commit_message HEAD -m \"Edited by EDITOR\"\n '\n \n-TERM=vt100\n-export TERM\n for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n do\n-\techo \"Edited by $i\" >expect\n-\tunset EDITOR VISUAL GIT_EDITOR\n-\tgit config --unset-all core.editor\n-\tcase \"$i\" in\n-\tcore_editor)\n-\t\tgit config core.editor ./e-core_editor.sh\n-\t\t;;\n-\t[A-Z]*)\n-\t\teval \"$i=./e-$i.sh\"\n-\t\texport $i\n-\t\t;;\n-\tesac\n \ttest_expect_success \"Using $i\" '\n-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n-\t\ttest_commit_message HEAD expect\n+\t\tif test \"$i\" = core_editor\n+\t\tthen\n+\t\t\ttest_config core.editor ./e-core_editor.sh\n+\t\tfi &&\n+\t\t(\n+\t\t\tcase \"$i\" in\n+\t\t\t[A-Z]*)\n+\t\t\t\teval \"$i=./e-$i.sh\" &&\n+\t\t\t\texport $i\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\t\t\tPATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n+\t\t) &&\n+\t\ttest_commit_message HEAD -m \"Edited by $i\"\n \t'\n done\n \n-unset EDITOR VISUAL GIT_EDITOR\n-git config --unset-all core.editor\n-for i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n-do\n-\techo \"Edited by $i\" >expect\n-\tcase \"$i\" in\n-\tcore_editor)\n-\t\tgit config core.editor ./e-core_editor.sh\n-\t\t;;\n-\t[A-Z]*)\n-\t\teval \"$i=./e-$i.sh\"\n-\t\texport $i\n-\t\t;;\n-\tesac\n-\ttest_expect_success \"Using $i (override)\" '\n-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n-\t\ttest_commit_message HEAD expect\n-\t'\n-done\n+test_expect_success 'Using editors with overrides' '\n+\t(\n+\t\tTERM=vt100 &&\n+\t\texport TERM &&\n+\t\tfor i in $vi EDITOR VISUAL core_editor GIT_EDITOR\n+\t\tdo\n+\t\t\techo \"Edited by $i\" >expect &&\n+\t\t\tcase \"$i\" in\n+\t\t\tcore_editor)\n+\t\t\t\tgit config core.editor ./e-core_editor.sh\n+\t\t\t\t;;\n+\t\t\t[A-Z]*)\n+\t\t\t\teval \"$i=./e-$i.sh\" &&\n+\t\t\t\texport $i\n+\t\t\t\t;;\n+\t\t\tesac &&\n+\t\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n+\t\t\ttest_commit_message HEAD expect || exit 1\n+\t\tdone\n+\t)\n+'\n \n test_expect_success 'editor with a space' '\n \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n@@ -97,9 +93,8 @@\n \ttest_commit_message HEAD -m space\n '\n \n-unset GIT_EDITOR\n test_expect_success 'core.editor with a space' '\n-\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n+\ttest_config core.editor \\\"./e\\ space.sh\\\" &&\n \tgit commit --amend &&\n \ttest_commit_message HEAD -m space\n '\n-- \n2.48.1\n\n"},{"id":"524258","messageId":"6279f266-3312-4923-8d05-c74e7f759063@gmail.com","threadId":"63489","inReplyTo":"20250813175112.88435-1-ben.knoble+github@gmail.com","subject":"Re: [PATCH v5 0/3] clean up some code around editors","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-08-15T16:00:49Z","receivedAt":"2025-08-15T16:00:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ben\n\nOn 13/08/2025 18:50, D. Ben Knoble wrote:\n> Changes from v4:\n> - revert use of test_env: fix the one instance that needs adjusted for\n> VAR=val cmd syntax by using test_must_fail env VAR=val cmd.\n\nThanks for re-rolling, the range-diff below looks good\n\nPhillip\n\n> \n> Changes from v3:\n> - drop 4/4\n> - use test_env (including a case our lint does not catch when the value\n> has spaces)\n> \n> Changes from v2:\n> - shuffle setup code and use more helpers in 1/4\n> - insert 2/4 to stop abusing --exec-path\n> - improve environment-cleansing idioms in {2 => 3}/4\n> \n> Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)\n> \n> Changes from v1:\n> - add a prep patch with style fixes to t7005\n> - rework the environment munging to use subshells, per Phillip Wood's\n> suggestion\n> \n> These patches clean up some old code in the editor tests that does not use our\n> modern idioms.\n> \n> Also, this version is (still) based on a later master 112648dd6b (Merge\n> branch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the\n> original from May.\n> \n> v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/\n> v2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/\n> v3: https://lore.kernel.org/git/20250811221706.67168-1-ben.knoble+github@gmail.com/\n> v4: https://lore.kernel.org/git/20250812170256.71751-1-ben.knoble+github@gmail.com/\n> Published-as: https://github.com/benknoble/tree/editor-cleanup\n> \n> D. Ben Knoble (3):\n> t7005: use modern test style\n> t7005: stop abusing --exec-path\n> t7005: sanitize test environment for subsequent tests\n> \n> t/t7005-editor.sh | 147 +++++++++++++++++++---------------------------\n> 1 file changed, 59 insertions(+), 88 deletions(-)\n> \n> Diff-intervalle contre v4 :\n> 1: 8ad2904a18 = 1: 8ad2904a18 t7005: use modern test style\n> 2: 44a6fd8eb3 ! 2: 5a35889571 t7005: stop abusing --exec-path\n> @@ t/t7005-editor.sh\n> \tesac\n> \ttest_expect_success \"Using $i\" '\n> -\t\tgit --exec-path=. commit --amend &&\n> -+\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> ++\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> \t\ttest_commit_message HEAD expect\n> \t'\n> done\n> @@ t/t7005-editor.sh\n> \tesac\n> \ttest_expect_success \"Using $i (override)\" '\n> -\t\tgit --exec-path=. commit --amend &&\n> -+\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> ++\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> \t\ttest_commit_message HEAD expect\n> \t'\n> done\n> 3: 135d4368d6 ! 3: e6e31ab98c t7005: sanitize test environment for subsequent tests\n> @@ t/t7005-editor.sh\n> -export TERM\n> test_expect_success 'dumb should error out when falling back on vi' '\n> -\ttest_must_fail git commit --amend\n> -+\ttest_env TERM=dumb test_must_fail git commit --amend\n> ++\ttest_must_fail env TERM=dumb git commit --amend\n> '\n> \n> test_expect_success 'dumb should prefer EDITOR to VISUAL' '\n> @@ t/t7005-editor.sh\n> -\tVISUAL=./e-VISUAL.sh &&\n> -\texport EDITOR VISUAL &&\n> -\tgit commit --amend &&\n> -+\ttest_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n> ++\tTERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \\\n> +\t\tgit commit --amend &&\n> \ttest_commit_message HEAD -m \"Edited by EDITOR\"\n> '\n> @@ t/t7005-editor.sh\n> -\t\t;;\n> -\tesac\n> \ttest_expect_success \"Using $i\" '\n> --\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> +-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> -\t\ttest_commit_message HEAD expect\n> +\t\tif test \"$i\" = core_editor\n> +\t\tthen\n> @@ t/t7005-editor.sh\n> +\t\t\t\texport $i\n> +\t\t\t\t;;\n> +\t\t\tesac &&\n> -+\t\t\ttest_env PATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n> ++\t\t\tPATH=\"$PWD:$PATH\" TERM=vt100 git commit --amend\n> +\t\t) &&\n> +\t\ttest_commit_message HEAD -m \"Edited by $i\"\n> \t'\n> @@ t/t7005-editor.sh\n> -\t\t;;\n> -\tesac\n> -\ttest_expect_success \"Using $i (override)\" '\n> --\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> +-\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> -\t\ttest_commit_message HEAD expect\n> -\t'\n> -done\n> @@ t/t7005-editor.sh\n> +\t\t\t\texport $i\n> +\t\t\t\t;;\n> +\t\t\tesac &&\n> -+\t\t\ttest_env PATH=\"$PWD:$PATH\" git commit --amend &&\n> ++\t\t\tPATH=\"$PWD:$PATH\" git commit --amend &&\n> +\t\t\ttest_commit_message HEAD expect || exit 1\n> +\t\tdone\n> +\t)\n> @@ t/t7005-editor.sh\n> \n> test_expect_success 'editor with a space' '\n> \techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n> - \tchmod a+x \"e space.sh\" &&\n> --\tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n> -+\ttest_env GIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n> +@@\n> \ttest_commit_message HEAD -m space\n> '\n> \n> \n> base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be\n\n"}]}