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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 6, 2025, 10:07 UTC
Message-ID: <350146ae-b909-4d9f-91f3-64bc2ddfcd60@gmail.com>
In-Reply-To: <20250805024044.30024-1-ben.knoble+github@gmail.com>

Hi Ben

On 05/08/2025 03:40, D. Ben Knoble wrote:

Show 7 quoted lines

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

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

Thanks for re-submitting these

Phillip

Show 57 quoted lines

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