git v2.49.0 - gitk regression on Cygwin

git v2.49.0 - gitk regression on Cygwin

[Mark Levedahl]

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.,

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)"

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

Re: git v2.49.0 - gitk regression on Cygwin

[Johannes Sixt]

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

Re: git v2.49.0 - gitk regression on Cygwin

[Mark Levedahl]

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] gitk - override $PATH search only on Windows

[Mark Levedahl]

Commit 4cbe9e0e2 was written to address problems that result from Tcl's
documented behavior on Windows where the current working directory and a
number of Windows system directories are automatically prepended to
$PATH when searching for executables [1].  This basic Windows behavior
has resulted in more than one CVE against git for Windows:
CVE-2023-23618, CVE-2022-41953 are listed on the git for Windows github
website for the Tcl components of git (gitk, git-gui).

4cbe9e0e2 is intended to restrict the search to looking only in
directories given in $PATH and in the given order, which is exactly the
Tcl behavior documented to exist on non-Windows platforms [1]. Thus,
this change could have been written to affect only Windows, leaving
other platforms alone.

However, 4cbe9e0e2 implements the override for all platforms.  and
includes specialized code for Cygwin, copied copied from git-gui prior
to commit 6d2f9d90 on https://github.com/j6t/git-gui.git), so targets a
long retired Cygwin port of the Windows Tcl/Tk using Windows pathnames.
Since 2012, Cygwin uses a Unix/X11 port requiring Unix pathnames,
meaning 4cbe9e0e2 is incompatible. The patch also induces an infinite
recursion as _which now invokes the exec wrapper that invokes _which.
As this is part of git v2.49.0, gitk on Cygwin is broken in that
release.

Rather than fix the unnecessary override code for Cygwin, let's just
limit the override of exec/open to Windows, leaving all other platforms
using their native exec/open as they did prior to 4cbe9e0e2. This patch
wraps the override code in an "if {[is_Windows]} { ... }" block while
removing the non-Windows code added in 4cbe9e0e2.

[1] see https://www.tcl-lang.org/man/tcl8.6/TclCmd/exec.htm

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 gitk | 146 ++++++++++++++++++++++++-----------------------------------
 1 file changed, 58 insertions(+), 88 deletions(-)

diff --git a/gitk b/gitk
index bc9efa1..a101b07 100755
--- a/gitk
+++ b/gitk
@@ -13,13 +13,6 @@ package require Tk
 ##
 ## Enabling platform-specific code paths
 
-proc is_MacOSX {} {
-	if {[tk windowingsystem] eq {aqua}} {
-		return 1
-	}
-	return 0
-}
-
 proc is_Windows {} {
 	if {$::tcl_platform(platform) eq {windows}} {
 		return 1
@@ -27,36 +20,16 @@ proc is_Windows {} {
 	return 0
 }
 
-set _iscygwin {}
-proc is_Cygwin {} {
-	global _iscygwin
-	if {$_iscygwin eq {}} {
-		if {[string match "CYGWIN_*" $::tcl_platform(os)]} {
-			set _iscygwin 1
-		} else {
-			set _iscygwin 0
-		}
-	}
-	return $_iscygwin
-}
-
 ######################################################################
 ##
 ## PATH lookup
 
-set _search_path {}
-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 _search_path {}
+	proc _which {what args} {
+		global env _search_exe _search_path
+
+		if {$_search_path eq {}} {
 			set gitguidir [file dirname [info script]]
 			regsub -all ";" $gitguidir "\\;" gitguidir
 			set env(PATH) "$gitguidir;$env(PATH)"
@@ -65,81 +38,78 @@ proc _which {what args} {
 			set _search_path [lsearch -all -inline -not -exact \
 				$_search_path ""]
 			set _search_exe .exe
-		} else {
-			set _search_path [split $env(PATH) :]
-			set _search_exe {}
 		}
-	}
 
-	if {[is_Windows] && [lsearch -exact $args -script] >= 0} {
-		set suffix {}
-	} else {
-		set suffix $_search_exe
-	}
+		if {[lsearch -exact $args -script] >= 0} {
+			set suffix {}
+		} else {
+			set suffix $_search_exe
+		}
 
-	foreach p $_search_path {
-		set p [file join $p $what$suffix]
-		if {[file exists $p]} {
-			return [file normalize $p]
+		foreach p $_search_path {
+			set p [file join $p $what$suffix]
+			if {[file exists $p]} {
+				return [file normalize $p]
+			}
 		}
+		return {}
 	}
-	return {}
-}
 
-proc sanitize_command_line {command_line from_index} {
-	set i $from_index
-	while {$i < [llength $command_line]} {
-		set cmd [lindex $command_line $i]
-		if {[file pathtype $cmd] ne "absolute"} {
-			set fullpath [_which $cmd]
-			if {$fullpath eq ""} {
-				throw {NOT-FOUND} "$cmd not found in PATH"
+	proc sanitize_command_line {command_line from_index} {
+		set i $from_index
+		while {$i < [llength $command_line]} {
+			set cmd [lindex $command_line $i]
+			if {[file pathtype $cmd] ne "absolute"} {
+				set fullpath [_which $cmd]
+				if {$fullpath eq ""} {
+					throw {NOT-FOUND} "$cmd not found in PATH"
+				}
+				lset command_line $i $fullpath
 			}
-			lset command_line $i $fullpath
-		}
 
-		# handle piped commands, e.g. `exec A | B`
-		for {incr i} {$i < [llength $command_line]} {incr i} {
-			if {[lindex $command_line $i] eq "|"} {
-				incr i
-				break
+			# handle piped commands, e.g. `exec A | B`
+			for {incr i} {$i < [llength $command_line]} {incr i} {
+				if {[lindex $command_line $i] eq "|"} {
+					incr i
+					break
+				}
 			}
 		}
+		return $command_line
 	}
-	return $command_line
-}
 
-# Override `exec` to avoid unsafe PATH lookup
+	# Override `exec` to avoid unsafe PATH lookup
 
-rename exec real_exec
+	rename exec real_exec
 
-proc exec {args} {
-	# skip options
-	for {set i 0} {$i < [llength $args]} {incr i} {
-		set arg [lindex $args $i]
-		if {$arg eq "--"} {
-			incr i
-			break
-		}
-		if {[string range $arg 0 0] ne "-"} {
-			break
+	proc exec {args} {
+		# skip options
+		for {set i 0} {$i < [llength $args]} {incr i} {
+			set arg [lindex $args $i]
+			if {$arg eq "--"} {
+				incr i
+				break
+			}
+			if {[string range $arg 0 0] ne "-"} {
+				break
+			}
 		}
+		set args [sanitize_command_line $args $i]
+		uplevel 1 real_exec $args
 	}
-	set args [sanitize_command_line $args $i]
-	uplevel 1 real_exec $args
-}
 
-# Override `open` to avoid unsafe PATH lookup
+	# Override `open` to avoid unsafe PATH lookup
 
-rename open real_open
+	rename open real_open
 
-proc open {args} {
-	set arg0 [lindex $args 0]
-	if {[string range $arg0 0 0] eq "|"} {
-		set command_line [string trim [string range $arg0 1 end]]
-		lset args 0 "| [sanitize_command_line $command_line 0]"
+	proc open {args} {
+		set arg0 [lindex $args 0]
+		if {[string range $arg0 0 0] eq "|"} {
+			set command_line [string trim [string range $arg0 1 end]]
+			lset args 0 "| [sanitize_command_line $command_line 0]"
+		}
+		uplevel 1 real_open $args
 	}
-	uplevel 1 real_open $args
 }
 
 # End of safe PATH lookup stuff
-- 
2.49.0.99.31

Re: [PATCH] gitk - override $PATH search only on Windows

[Johannes Sixt]

Am 31.03.25 um 17:12 schrieb Mark Levedahl:
> Commit 4cbe9e0e2 was written to address problems that result from Tcl's
> documented behavior on Windows where the current working directory and a
> number of Windows system directories are automatically prepended to
> $PATH when searching for executables [1].  This basic Windows behavior
> has resulted in more than one CVE against git for Windows:
> CVE-2023-23618, CVE-2022-41953 are listed on the git for Windows github
> website for the Tcl components of git (gitk, git-gui).
> 
> 4cbe9e0e2 is intended to restrict the search to looking only in
> directories given in $PATH and in the given order, which is exactly the
> Tcl behavior documented to exist on non-Windows platforms [1]. Thus,
> this change could have been written to affect only Windows, leaving
> other platforms alone.
> 
> However, 4cbe9e0e2 implements the override for all platforms.  and
> includes specialized code for Cygwin, copied copied from git-gui prior
> to commit 6d2f9d90 on https://github.com/j6t/git-gui.git), so targets a

I can't find 6d2f9d90 anywhere. Do you have a URL?

> long retired Cygwin port of the Windows Tcl/Tk using Windows pathnames.
> Since 2012, Cygwin uses a Unix/X11 port requiring Unix pathnames,
> meaning 4cbe9e0e2 is incompatible. The patch also induces an infinite
> recursion as _which now invokes the exec wrapper that invokes _which.
> As this is part of git v2.49.0, gitk on Cygwin is broken in that
> release.
> 
> Rather than fix the unnecessary override code for Cygwin, let's just
> limit the override of exec/open to Windows, leaving all other platforms
> using their native exec/open as they did prior to 4cbe9e0e2. This patch
> wraps the override code in an "if {[is_Windows]} { ... }" block while
> removing the non-Windows code added in 4cbe9e0e2.
> 
> [1] see https://www.tcl-lang.org/man/tcl8.6/TclCmd/exec.htm

Thanks, nicely summarized.

> 
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  gitk | 146 ++++++++++++++++++++++++-----------------------------------
>  1 file changed, 58 insertions(+), 88 deletions(-)
> 
> diff --git a/gitk b/gitk
> index bc9efa1..a101b07 100755
> --- a/gitk
> +++ b/gitk
> @@ -13,13 +13,6 @@ package require Tk
>  ##
>  ## Enabling platform-specific code paths
>  
> -proc is_MacOSX {} {
> -	if {[tk windowingsystem] eq {aqua}} {
> -		return 1
> -	}
> -	return 0
> -}
> -
>  proc is_Windows {} {
>  	if {$::tcl_platform(platform) eq {windows}} {
>  		return 1
> @@ -27,36 +20,16 @@ proc is_Windows {} {
>  	return 0
>  }
>  
> -set _iscygwin {}
> -proc is_Cygwin {} {
> -	global _iscygwin
> -	if {$_iscygwin eq {}} {
> -		if {[string match "CYGWIN_*" $::tcl_platform(os)]} {
> -			set _iscygwin 1
> -		} else {
> -			set _iscygwin 0
> -		}
> -	}
> -	return $_iscygwin
> -}
> -
>  ######################################################################
>  ##
>  ## PATH lookup
>  
> -set _search_path {}
> -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 _search_path {}
> +	proc _which {what args} {
> +		global env _search_exe _search_path
> +
> +		if {$_search_path eq {}} {
>  			set gitguidir [file dirname [info script]]
>  			regsub -all ";" $gitguidir "\\;" gitguidir
>  			set env(PATH) "$gitguidir;$env(PATH)"
> @@ -65,81 +38,78 @@ proc _which {what args} {
>  			set _search_path [lsearch -all -inline -not -exact \
>  				$_search_path ""]
>  			set _search_exe .exe
> -		} else {
> -			set _search_path [split $env(PATH) :]
> -			set _search_exe {}
>  		}
> -	}
>  
> -	if {[is_Windows] && [lsearch -exact $args -script] >= 0} {
> -		set suffix {}
> -	} else {
> -		set suffix $_search_exe
> -	}
> +		if {[lsearch -exact $args -script] >= 0} {
> +			set suffix {}
> +		} else {
> +			set suffix $_search_exe
> +		}

Now that this code is only about Windows, _search_exe is always ".exe".
It would be great if we could remove it as well.

>  
> -	foreach p $_search_path {
> -		set p [file join $p $what$suffix]
> -		if {[file exists $p]} {
> -			return [file normalize $p]
> +		foreach p $_search_path {
> +			set p [file join $p $what$suffix]
> +			if {[file exists $p]} {
> +				return [file normalize $p]
> +			}
>  		}
> +		return {}
>  	}
> -	return {}
> -}
>  
> -proc sanitize_command_line {command_line from_index} {
> -	set i $from_index
> -	while {$i < [llength $command_line]} {
> -		set cmd [lindex $command_line $i]
> -		if {[file pathtype $cmd] ne "absolute"} {
> -			set fullpath [_which $cmd]
> -			if {$fullpath eq ""} {
> -				throw {NOT-FOUND} "$cmd not found in PATH"
> +	proc sanitize_command_line {command_line from_index} {
> +		set i $from_index
> +		while {$i < [llength $command_line]} {
> +			set cmd [lindex $command_line $i]
> +			if {[file pathtype $cmd] ne "absolute"} {
> +				set fullpath [_which $cmd]
> +				if {$fullpath eq ""} {
> +					throw {NOT-FOUND} "$cmd not found in PATH"
> +				}
> +				lset command_line $i $fullpath
>  			}
> -			lset command_line $i $fullpath
> -		}
>  
> -		# handle piped commands, e.g. `exec A | B`
> -		for {incr i} {$i < [llength $command_line]} {incr i} {
> -			if {[lindex $command_line $i] eq "|"} {
> -				incr i
> -				break
> +			# handle piped commands, e.g. `exec A | B`
> +			for {incr i} {$i < [llength $command_line]} {incr i} {
> +				if {[lindex $command_line $i] eq "|"} {
> +					incr i
> +					break
> +				}
>  			}
>  		}
> +		return $command_line
>  	}
> -	return $command_line
> -}
>  
> -# Override `exec` to avoid unsafe PATH lookup
> +	# Override `exec` to avoid unsafe PATH lookup
>  
> -rename exec real_exec
> +	rename exec real_exec
>  
> -proc exec {args} {
> -	# skip options
> -	for {set i 0} {$i < [llength $args]} {incr i} {
> -		set arg [lindex $args $i]
> -		if {$arg eq "--"} {
> -			incr i
> -			break
> -		}
> -		if {[string range $arg 0 0] ne "-"} {
> -			break
> +	proc exec {args} {
> +		# skip options
> +		for {set i 0} {$i < [llength $args]} {incr i} {
> +			set arg [lindex $args $i]
> +			if {$arg eq "--"} {
> +				incr i
> +				break
> +			}
> +			if {[string range $arg 0 0] ne "-"} {
> +				break
> +			}
>  		}
> +		set args [sanitize_command_line $args $i]
> +		uplevel 1 real_exec $args
>  	}
> -	set args [sanitize_command_line $args $i]
> -	uplevel 1 real_exec $args
> -}
>  
> -# Override `open` to avoid unsafe PATH lookup
> +	# Override `open` to avoid unsafe PATH lookup
>  
> -rename open real_open
> +	rename open real_open
>  
> -proc open {args} {
> -	set arg0 [lindex $args 0]
> -	if {[string range $arg0 0 0] eq "|"} {
> -		set command_line [string trim [string range $arg0 1 end]]
> -		lset args 0 "| [sanitize_command_line $command_line 0]"
> +	proc open {args} {
> +		set arg0 [lindex $args 0]
> +		if {[string range $arg0 0 0] eq "|"} {
> +			set command_line [string trim [string range $arg0 1 end]]
> +			lset args 0 "| [sanitize_command_line $command_line 0]"
> +		}
> +		uplevel 1 real_open $args
>  	}
> -	uplevel 1 real_open $args
>  }
>  
>  # End of safe PATH lookup stuff

-- Hannes

Re: [PATCH] gitk - override $PATH search only on Windows

[Mark Levedahl]

On 3/31/25 1:12 PM, Johannes Sixt wrote:
> Am 31.03.25 um 17:12 schrieb Mark Levedahl:
>> Commit 4cbe9e0e2 was written to address problems that result from Tcl's
>> documented behavior on Windows where the current working directory and a
>> number of Windows system directories are automatically prepended to
>> $PATH when searching for executables [1].  This basic Windows behavior
>> has resulted in more than one CVE against git for Windows:
>> CVE-2023-23618, CVE-2022-41953 are listed on the git for Windows github
>> website for the Tcl components of git (gitk, git-gui).
>>
>> 4cbe9e0e2 is intended to restrict the search to looking only in
>> directories given in $PATH and in the given order, which is exactly the
>> Tcl behavior documented to exist on non-Windows platforms [1]. Thus,
>> this change could have been written to affect only Windows, leaving
>> other platforms alone.
>>
>> However, 4cbe9e0e2 implements the override for all platforms.  and
>> includes specialized code for Cygwin, copied copied from git-gui prior
>> to commit 6d2f9d90 on https://github.com/j6t/git-gui.git), so targets a
> I can't find 6d2f9d90 anywhere. Do you have a URL?

Sorry about that (bad copy / paste). Should be 7145c654

https://github.com/j6t/git-gui/commit/7145c654fffecd1f3d4a2b8bf05755ce262903e8

> Now that this code is only about Windows, _search_exe is always ".exe".
> It would be great if we could remove it as well.
>
Will do for v2.

Mark

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

[Mark Levedahl]

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.

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
  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

[PATCH v2 1/3] gitk: override $PATH search only on Windows

[Mark Levedahl]

Commit 4cbe9e0e2 was written to address problems that result from Tcl's
documented behavior on Windows where the current working directory and a
number of Windows system directories are automatically prepended to
$PATH when searching for executables [1].  This basic Windows behavior
has resulted in more than one CVE against git for Windows:
CVE-2023-23618, CVE-2022-41953 are listed on the git for Windows github
website for the Tcl components of git (gitk, git-gui).

4cbe9e0e2 is intended to restrict the search to looking only in
directories given in $PATH and in the given order, which is exactly the
Tcl behavior documented to exist on non-Windows platforms [1]. Thus,
this change could have been written to affect only Windows, leaving
other platforms alone.

However, 4cbe9e0e2 implements the override for all platforms. This
includes specialized code for Cygwin, copied from git-gui prior to
commit 7145c654 on https://github.com/j6t/git-gui, so targets a
long retired Cygwin port of the Windows Tcl/Tk using Windows pathnames.
Since 2012, Cygwin uses a Unix/X11 port requiring Unix pathnames,
meaning 4cbe9e0e2 is incompatible.  4cbe9e0e2 also induces an infinite
recursion as _which now invokes the exec wrapper that invokes _which.
This is part of git v2.49.0, so gitk on Cygwin is broken in that
release.

Rather than fix the unnecessary override code for Cygwin, let's just
limit the override of exec/open to Windows, leaving all other platforms
using their native exec/open as they did prior to 4cbe9e0e2. This patch
wraps the override code in an "if {[is_Windows]} { ... }" block while
removing the non-Windows code added in 4cbe9e0e2.

[1] see https://www.tcl-lang.org/man/tcl8.6/TclCmd/exec.htm

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 gitk | 146 ++++++++++++++++++++++++-----------------------------------
 1 file changed, 58 insertions(+), 88 deletions(-)

diff --git a/gitk b/gitk
index bc9efa1..a101b07 100755
--- a/gitk
+++ b/gitk
@@ -13,13 +13,6 @@ package require Tk
 ##
 ## Enabling platform-specific code paths
 
-proc is_MacOSX {} {
-	if {[tk windowingsystem] eq {aqua}} {
-		return 1
-	}
-	return 0
-}
-
 proc is_Windows {} {
 	if {$::tcl_platform(platform) eq {windows}} {
 		return 1
@@ -27,36 +20,16 @@ proc is_Windows {} {
 	return 0
 }
 
-set _iscygwin {}
-proc is_Cygwin {} {
-	global _iscygwin
-	if {$_iscygwin eq {}} {
-		if {[string match "CYGWIN_*" $::tcl_platform(os)]} {
-			set _iscygwin 1
-		} else {
-			set _iscygwin 0
-		}
-	}
-	return $_iscygwin
-}
-
 ######################################################################
 ##
 ## PATH lookup
 
-set _search_path {}
-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 _search_path {}
+	proc _which {what args} {
+		global env _search_exe _search_path
+
+		if {$_search_path eq {}} {
 			set gitguidir [file dirname [info script]]
 			regsub -all ";" $gitguidir "\\;" gitguidir
 			set env(PATH) "$gitguidir;$env(PATH)"
@@ -65,81 +38,78 @@ proc _which {what args} {
 			set _search_path [lsearch -all -inline -not -exact \
 				$_search_path ""]
 			set _search_exe .exe
-		} else {
-			set _search_path [split $env(PATH) :]
-			set _search_exe {}
 		}
-	}
 
-	if {[is_Windows] && [lsearch -exact $args -script] >= 0} {
-		set suffix {}
-	} else {
-		set suffix $_search_exe
-	}
+		if {[lsearch -exact $args -script] >= 0} {
+			set suffix {}
+		} else {
+			set suffix $_search_exe
+		}
 
-	foreach p $_search_path {
-		set p [file join $p $what$suffix]
-		if {[file exists $p]} {
-			return [file normalize $p]
+		foreach p $_search_path {
+			set p [file join $p $what$suffix]
+			if {[file exists $p]} {
+				return [file normalize $p]
+			}
 		}
+		return {}
 	}
-	return {}
-}
 
-proc sanitize_command_line {command_line from_index} {
-	set i $from_index
-	while {$i < [llength $command_line]} {
-		set cmd [lindex $command_line $i]
-		if {[file pathtype $cmd] ne "absolute"} {
-			set fullpath [_which $cmd]
-			if {$fullpath eq ""} {
-				throw {NOT-FOUND} "$cmd not found in PATH"
+	proc sanitize_command_line {command_line from_index} {
+		set i $from_index
+		while {$i < [llength $command_line]} {
+			set cmd [lindex $command_line $i]
+			if {[file pathtype $cmd] ne "absolute"} {
+				set fullpath [_which $cmd]
+				if {$fullpath eq ""} {
+					throw {NOT-FOUND} "$cmd not found in PATH"
+				}
+				lset command_line $i $fullpath
 			}
-			lset command_line $i $fullpath
-		}
 
-		# handle piped commands, e.g. `exec A | B`
-		for {incr i} {$i < [llength $command_line]} {incr i} {
-			if {[lindex $command_line $i] eq "|"} {
-				incr i
-				break
+			# handle piped commands, e.g. `exec A | B`
+			for {incr i} {$i < [llength $command_line]} {incr i} {
+				if {[lindex $command_line $i] eq "|"} {
+					incr i
+					break
+				}
 			}
 		}
+		return $command_line
 	}
-	return $command_line
-}
 
-# Override `exec` to avoid unsafe PATH lookup
+	# Override `exec` to avoid unsafe PATH lookup
 
-rename exec real_exec
+	rename exec real_exec
 
-proc exec {args} {
-	# skip options
-	for {set i 0} {$i < [llength $args]} {incr i} {
-		set arg [lindex $args $i]
-		if {$arg eq "--"} {
-			incr i
-			break
-		}
-		if {[string range $arg 0 0] ne "-"} {
-			break
+	proc exec {args} {
+		# skip options
+		for {set i 0} {$i < [llength $args]} {incr i} {
+			set arg [lindex $args $i]
+			if {$arg eq "--"} {
+				incr i
+				break
+			}
+			if {[string range $arg 0 0] ne "-"} {
+				break
+			}
 		}
+		set args [sanitize_command_line $args $i]
+		uplevel 1 real_exec $args
 	}
-	set args [sanitize_command_line $args $i]
-	uplevel 1 real_exec $args
-}
 
-# Override `open` to avoid unsafe PATH lookup
+	# Override `open` to avoid unsafe PATH lookup
 
-rename open real_open
+	rename open real_open
 
-proc open {args} {
-	set arg0 [lindex $args 0]
-	if {[string range $arg0 0 0] eq "|"} {
-		set command_line [string trim [string range $arg0 1 end]]
-		lset args 0 "| [sanitize_command_line $command_line 0]"
+	proc open {args} {
+		set arg0 [lindex $args 0]
+		if {[string range $arg0 0 0] eq "|"} {
+			set command_line [string trim [string range $arg0 1 end]]
+			lset args 0 "| [sanitize_command_line $command_line 0]"
+		}
+		uplevel 1 real_open $args
 	}
-	uplevel 1 real_open $args
 }
 
 # End of safe PATH lookup stuff
-- 
2.49.0.99.31

[PATCH v2 2/3] gitk: _search_exe is no longer needed

[Mark Levedahl]

The _search_exe variable allows specifying the suffix used for executables,
typically {} on unix, .exe on Windows. But, the override code is now
used only on Windows, so _search_exe is no longer needed. Eliminate it.

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 gitk | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gitk b/gitk
index a101b07..e4d0500 100755
--- a/gitk
+++ b/gitk
@@ -27,7 +27,7 @@ proc is_Windows {} {
 if {[is_Windows]} {
 	set _search_path {}
 	proc _which {what args} {
-		global env _search_exe _search_path
+		global env _search_path
 
 		if {$_search_path eq {}} {
 			set gitguidir [file dirname [info script]]
@@ -37,13 +37,12 @@ if {[is_Windows]} {
 			# Skip empty `PATH` elements
 			set _search_path [lsearch -all -inline -not -exact \
 				$_search_path ""]
-			set _search_exe .exe
 		}
 
 		if {[lsearch -exact $args -script] >= 0} {
 			set suffix {}
 		} else {
-			set suffix $_search_exe
+			set suffix .exe
 		}
 
 		foreach p $_search_path {
-- 
2.49.0.99.31

[PATCH v2 3/3] gitk: limit PATH search to bare executable names

[Mark Levedahl]

The path search overrides used by gitk on Windows are applied to any
executable whose name is not 'absolute', meaning that
	[exec foo/bar ...]
will search each element of $PATH to find one with subdirectory foo
containing bar. But, per POSIX, and Tcl implementation on all platforms,
foo/bar is taken as $(pwd)/foo/bar, and is not searched on $PATH.

Fix this descrepency using the same approach applied to git-gui in
commit 3f71c97e. The key is that the executable name must have no path
component, indicated by [file split $exename] having array length 1.

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitk b/gitk
index e4d0500..e38e071 100755
--- a/gitk
+++ b/gitk
@@ -58,7 +58,7 @@ if {[is_Windows]} {
 		set i $from_index
 		while {$i < [llength $command_line]} {
 			set cmd [lindex $command_line $i]
-			if {[file pathtype $cmd] ne "absolute"} {
+			if {[llength [file split $cmd]] < 2} {
 				set fullpath [_which $cmd]
 				if {$fullpath eq ""} {
 					throw {NOT-FOUND} "$cmd not found in PATH"
-- 
2.49.0.99.31

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

[Johannes Schindelin]

Hi Mark,

On Mon, 31 Mar 2025, Mark Levedahl wrote:

> 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.

> 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

>   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.

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

[Mark Levedahl]

On 4/1/25 12:10 PM, Johannes Schindelin wrote:
> 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).

Agreed. The first patch is nasty due to indentation changes, git diff -w 
is indespensible, perhaps that should be available similar to a diffstat.

Thanks for the review.

Mark

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

[Johannes Sixt]

Am 01.04.25 um 05:00 schrieb Mark Levedahl:
> 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.
> 
> 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
>   gitk: _search_exe is no longer needed
>   gitk: limit PATH search to bare executable names

Thank you for following up. These all look good. Patch 3 is a good find!

I've inserted a patch 0.5/3 that replaces the existing TAB characters
with four SP in the indentation.

Thanks,
-- Hannes