git v2.49.0 - gitk regression on Cygwin

Commit Message

Mark Levedahl March 28, 2025, 12:34 p.m. UTC

referencing the upstream gitk repo at published at 
https://github.com/j6t/gitk.git:

Since commit 4cbe9e0e2 - "gitk(Windows): avoid inadvertently calling 
executables in the worktree,"

gitk no longer works on Cygwin. This commit is in Junio's tree as part 
of release v2.49.0, but I didn't trace to the specific merge commit.

The proximal issue is an endless loop caused by routine _which invoking 
exec, which is now a wrapper that invokes _which, while the builtin exec 
is renamed to real_exec.  This results in stack exhaustion.  There are 
other problems due to munging Cygwin's $PATH into Windows rather than 
Unix format, so changing _which to invoke real_exec just changes the 
failure mode on Cygwin.

Removing the Cygwin specific code so that gitk treats Cygwin as a Linux 
variant does work: e.g.,


However, the above leaves code in place affecting path search on all 
platforms without justification. The commit message references the TCL 
man page for "exec", listing a number of directories (including the 
current working directory) and file suffixes searched on the Windows 
platform that could be problematic. However, that man page does not list 
any such issues for other platforms. So, it appears the patch does not 
address a known issue on Unix platforms, which includes Cygwin.

I believe the correct fix to 4cbe9e0e2 is limiting the override of exec 
and open to Windows, and I also have a patch to do that rather than what 
I show above. Let me know.

Mark

Comments

Johannes Sixt March 28, 2025, 5:30 p.m. UTC | #1

Am 28.03.25 um 13:34 schrieb Mark Levedahl:
> gitk no longer works on Cygwin. This commit is in Junio's tree as part
> of release v2.49.0, but I didn't trace to the specific merge commit.
> 
> The proximal issue is an endless loop caused by routine _which invoking
> exec, which is now a wrapper that invokes _which, while the builtin exec
> is renamed to real_exec.  This results in stack exhaustion.

Not good. Thanks for the report.

> However, the above leaves code in place affecting path search on all
> platforms without justification. The commit message references the TCL
> man page for "exec", listing a number of directories (including the
> current working directory) and file suffixes searched on the Windows
> platform that could be problematic. However, that man page does not list
> any such issues for other platforms. So, it appears the patch does not
> address a known issue on Unix platforms, which includes Cygwin.
> 
> I believe the correct fix to 4cbe9e0e2 is limiting the override of exec
> and open to Windows, and I also have a patch to do that rather than what
> I show above. Let me know.

Indeed, it seems that this override is only needed on Windows. Dscho, is
there a non-obvious reason that 'exec' and 'open' are overridden on all
platforms?

-- Hannes

Mark Levedahl March 29, 2025, 9:49 p.m. UTC | #2

On 3/28/25 1:30 PM, Johannes Sixt wrote:
> Indeed, it seems that this override is only needed on Windows. Dscho, is
> there a non-obvious reason that 'exec' and 'open' are overridden on all
> platforms?
>
> -- Hannes
>
(resending to avoid HTML ...)

Looking at git-gui's history, I see no deliberate decision to alter the 
native path search on any platform except Windows, just no effort ever 
to leave non-Windows platforms alone. The buggy Cygwin code is part of 
what I excised from git-gui in 2023 (commit 7145c654 on the git-gui tree).

  I'll plan to send a patch for gitk to revert non-windows platforms to 
native functions in a couple of days. This idea applies also to git-gui.

Mark

Patch

diff --git a/gitk b/gitk
index bc9efa1..2c29118 100755
--- a/gitk
+++ b/gitk
@@ -49,14 +49,7 @@  proc _which {what args} {
     global env _search_exe _search_path

     if {$_search_path eq {}} {
-       if {[is_Cygwin] && [regexp {^(/|\.:)} $env(PATH)]} {
-           set _search_path [split [exec cygpath \
-               --windows \
-               --path \
-               --absolute \
-               $env(PATH)] {;}]
-           set _search_exe .exe
-       } elseif {[is_Windows]} {
+       if {[is_Windows]} {
             set gitguidir [file dirname [info script]]
             regsub -all ";" $gitguidir "\\;" gitguidir
             set env(PATH) "$gitguidir;$env(PATH)"