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

From: D. Ben Knoble <ben.knoble+github@gmail.com>
Date: May 21, 2025, 13:33 UTC
Message-ID: <CALnO6CDWoOKZ6NHNT_Xh2Jd48cWU=qHkkwYbbZHfYGvEz5LWGw@mail.gmail.com>
In-Reply-To: <e5efc450-dfe1-48fa-9dcd-7e0634957e1f@crinan.ddns.net>

On Wed, May 21, 2025 at 9:07 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 22 quoted lines

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

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

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

Thoughts?