On Wed, May 21, 2025 at 9:07 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > On 21/05/2025 08:56, Patrick Steinhardt wrote: > > On Tue, May 20, 2025 at 03:34:57PM -0400, D. Ben Knoble wrote: > >> We only prepare the child environment on non-Windows platforms, but > >> prep_childenv is the natural interposition point for our subprocess > >> system to adjust the environment as needed. Use it for Windows > >> platforms, also. In subsequent commits we'll use this interposition > >> point to modify the environment on all platforms. > > > > What is the consequence though of calling `prep_childenv()` on Windows > > now? Why didn't we call it before this change? Details like this should > > definitely go into the commit message to explain why it's safe to add > > the call now. > > The environment prep for windows is currently implemented in > compat/mingw.c:make_environment_block(). That function is careful to > handle unicode characters correctly, it is hard to see how that is > compatible this change. > > Best Wishes > > Phillip Ah, thanks both—I missed that it got special handling later (since run-command throws the char **env over to mingw_spawnvpe without modification). This patch is probably "obviously wrong" with that context. That makes me wonder: per Junio's comments in another spot, we could be more targeted at the launch_editor family (which also requires a separate patch to contrib/git-jump to scratch my itch); or, we could try to push these modifications into both environment handling blocks. I lack the knowledge needed to do the right thing on the Windows side, though, so I'd probably leave it alone. Thoughts?
