Re: [PATCH v2 0/3] gitk: override PATH search only on Windows

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 1, 2025, 16:10 UTC
Message-ID: <3c42af3d-556f-9593-b715-ea689d7b508c@gmx.de>
In-Reply-To: <20250401030102.297272-1-mlevedahl@gmail.com>

Hi Mark,

On Mon, 31 Mar 2025, Mark Levedahl wrote:

Show 6 quoted lines

> Restrict overrides of exec/open to Windows only, as
> the need for this is Tcl adding the current working directory
> to $PATH on Windows. Recent modifications to this render
> gitk unusable on Cygwin, isolating these overrides to Windows only
> both fixes that breakage andk reduces the liklihood of similar
> issues in the future.

I agree that this is the right thing to do, and apologize for the breakage by copying the code from Git GUI to gitk and then contributing it _years_ later without double-checking whether it is still needed for Cygwin.

Show 14 quoted lines

> patch summary:
> 	1 - modifies the existing code to restrict the overrides
> 	   to Windows, restoring other platorms to native exec/open.
> 	2 - remove now superflous variable _search_exe.
> 	3 - fix the override code to avoid path search given a
> 	    relative path like foo/bar.
>
> ---
> Changes since v1 - fixed commit ID reference for git-gui, otherwise
>                    improved commit message in patch 1.
> 		   Added patches 2 and 3.
>
> Mark Levedahl (3):
>   gitk: override $PATH search only on Windows

I really wish that the reviewing process offered better tools than a fixed diff for this patch; Inspecting it with `-w` would probably make it much more obvious what it does (and make it substantially easier to verify that it does not do anything inadvertently).

In any case, these patches look good to me, thank you for working on them so carefully.

Ciao, Johannes

Show 8 quoted lines

>   gitk: _search_exe is no longer needed
>   gitk: limit PATH search to bare executable names
>
>  gitk | 147 +++++++++++++++++++++++------------------------------------
>  1 file changed, 58 insertions(+), 89 deletions(-)
>
> --
> 2.49.0.99.31

I want that version. It probably has fixed all the bugs.