BUG: git-gui no longer executes hook scripts

BUG: git-gui no longer executes hook scripts

[Mark Levedahl]

The commit titled "Work around Tcl's default |PATH| lookup",|aae9560, 
adds checking on all commands to be executed to assure these are on the 
PATH. Any script in .git/hooks is rejected as .git/hooks is not (in 
general) on the PATH, even if the entry in .git/hooks is a symlink to a 
file on the PATH. Instead, git-gui throws and error without completing 
the operation. This is easily demonstrated by say, enabling the 
commit-msg script (hooks-commit-msg.sample templates) and attempting a 
commit.
|

|I don't have a suggested solution to this: reverting the above commit 
will fix this problem, but that commit  was made to mitigate a security 
issue.  Perhaps anything in .git/hooks should be accepted without 
further checks?
|

|
|

|Mark
|

Re: BUG: git-gui no longer executes hook scripts

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> The commit titled "Work around Tcl's default |PATH| lookup",|aae9560,
> adds checking on all commands to be executed to assure these are on
> the PATH.

commit aae9560a355d4ab91385e49eae62fade2ddd27ef
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Wed Nov 23 09:31:06 2022 +0100

    Work around Tcl's default `PATH` lookup
    
    As per https://www.tcl.tk/man/tcl8.6/TclCmd/exec.html#M23, Tcl's `exec`
    function goes out of its way to imitate the highly dangerous path lookup
    of `cmd.exe`, but _of course_ only on Windows:
    
            If a directory name was not specified as part of the application
            name, the following directories are automatically searched in
            order when attempting to locate the application:

In other words, if somebody tries to run ".git/hooks/pre-commit",
because a directory name _is_ given (i.e. ".git/hooks/" in this case),
the path lookup is *not* done.  Which is what I would expect, and then
"oh, only on Windows to match what cmd.exe does, the current directory
is early in the search order" should not be a problem.

    To avoid that, Git GUI already has the `_which` function that does not
    imitate that dangerous practice when looking up executables in the
    search path.
    
Sounds good, but ...

diff --git a/git-gui.sh b/git-gui.sh
index b0eb5a6ae4..cb92bba1c4 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -121,6 +121,62 @@ proc _which {what args} {
 	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"
+			}
+			lset command_line $i $fullpath

Shouldn't this "is it absolute" check with "$cmd" also check if $cmd
has either forward or backward slash in it?  I do not know about the
Windows cmd.exe convention, but with Unix background, I would be
surprised if dir/cmd gave by end users ran "C:\program
files\dir\cmd" (unless I happened to be in the "C:\program files\"
folder, that is).

Checking the use of _which with fixed arguments, it is used to spawn
git, gitk, nice, sh; and _which finding where they appear on the
search path does sound sane.  But _which does not seem to have the "if
given a command with directory separator, the search path does not
matter.  The caller means it is relative to the $cwd" logic at all,
so it seems it is the callers responsibility to make sure it does
not pass things like ".git/hooks/pre-commit" to it.

+		}
+
+		# 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
+}
+
+# Override `exec` to avoid unsafe PATH lookup
+
+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
+		}
+	}
+	set args [sanitize_command_line $args $i]
+	uplevel 1 real_exec $args
+}
+
+# Override `open` to avoid unsafe PATH lookup
+
+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]"
+	}
+	uplevel 1 real_open $args
+}
+
 ######################################################################
 ##
 ## locate our library

Re: BUG: git-gui no longer executes hook scripts

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Shouldn't this "is it absolute" check with "$cmd" also check if $cmd
> has either forward or backward slash in it?
>
> Checking the use of _which with fixed arguments, it is used to spawn
> git, gitk, nice, sh; and _which finding where they appear on the
> search path does sound sane.  But _which does not seem to have the "if
> given a command with directory separator, the search path does not
> matter.  The caller means it is relative to the $cwd" logic at all,
> so it seems it is the callers responsibility to make sure it does
> not pass things like ".git/hooks/pre-commit" to it.

In other words, something along this line may go in the right
direction (I no longer speak Tcl, and this is done with manual in
one hand, while typing with the other hand).

 git-gui.sh | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git c/git-gui.sh w/git-gui.sh
index 8bc8892c40..45d8f48b39 100755
--- c/git-gui/git-gui.sh
+++ w/git-gui/git-gui.sh
@@ -119,11 +119,15 @@ proc sanitize_command_line {command_line 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"
+			if {1 < [llength [file split $cmd]]]} {
+			    set cmdpath [_which $cmd]
+			    if {$cmdpath eq ""} {
+				    throw {NOT-FOUND} "$cmd not found in PATH"
+			    }
+			} else {
+				set cmdpath $cmd
 			}
-			lset command_line $i $fullpath
+			lset command_line $i $cmdpath
 		}
 
 		# handle piped commands, e.g. `exec A | B`

Re: BUG: git-gui no longer executes hook scripts

[Mark Levedahl]

On 9/15/23 13:15, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Shouldn't this "is it absolute" check with "$cmd" also check if $cmd
>> has either forward or backward slash in it?
>>
>> Checking the use of _which with fixed arguments, it is used to spawn
>> git, gitk, nice, sh; and _which finding where they appear on the
>> search path does sound sane.  But _which does not seem to have the "if
>> given a command with directory separator, the search path does not
>> matter.  The caller means it is relative to the $cwd" logic at all,
>> so it seems it is the callers responsibility to make sure it does
>> not pass things like ".git/hooks/pre-commit" to it.
> In other words, something along this line may go in the right
> direction (I no longer speak Tcl, and this is done with manual in
> one hand, while typing with the other hand).
>
I think a simpler fix is just to examine the number of path components - 
more than one means a relative or absolute command (/foo splits into two 
parts). The below works for me on Linux.

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 277a2b1c8c..0c39d9fa26 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -118,7 +118,7 @@ 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"} {
+       if {[llength [file split $cmd]] < 2} {
             set fullpath [_which $cmd]
             if {$fullpath eq ""} {
                 throw {NOT-FOUND} "$cmd not found in PATH"


We could also wrap the entirety of commit aae9560a in

     if {[is_Windows]} { ... }

as all of this code is fixing a Windows specific vulnerability, though a 
fix like the above is needed regardless.

Mark

[PATCH] git-gui - re-enable use of hook scripts

[Mark Levedahl]

Commit aae9560a introduced search in $PATH to find executables before
running them, avoiding an issue where on Windows a same named file in
the current directory can be executed in preference to anything on the
path. The updated search excludes files given with an absolute path (e.g.,
/bin/sh). However this change precludes operation of hook scripts as these
are named with a relative path (.git/hooks/$HOOK), while a search on $PATH
can succeed only for bare file names, not relative paths. Furthermore,
the current repository's .git/hooks directory is in general not listed
in PATH.

Fix this by changing the "absolute" check to a check for more than one
component in the pathname, thereby avoiding the PATH check for anything
given with a relative path as well. Bare "git" has one component, "/sh"
has two components, and .git/hooks/$HOOK has more than two, so relative
and absolute pathnames avoid the check.

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

diff --git a/git-gui.sh b/git-gui.sh
index 8bc8892..8603437 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -118,7 +118,7 @@ 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"} {
+		if {[llength [file split $cmd]] < 2} {
 			set fullpath [_which $cmd]
 			if {$fullpath eq ""} {
 				throw {NOT-FOUND} "$cmd not found in PATH"
-- 
2.41.0.99.19

Re: BUG: git-gui no longer executes hook scripts

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> I think a simpler fix is just to examine the number of path components
> - more than one means a relative or absolute command (/foo splits into
> two parts). The below works for me on Linux.

That is clever, but I cannot convince myself that it is not too
clever for its own sake.  The "pathtype" thing Dscho used in his
original is documented to be aware of things like "C:\path\name",
but I didn't re-read the Tcl manual page too carefully to know what
"file split" does for such pathname to be certain.

Re: BUG: git-gui no longer executes hook scripts

[Mark Levedahl]

On 9/16/23 00:45, Junio C Hamano wrote:
> Mark Levedahl <mlevedahl@gmail.com> writes:
>
>> I think a simpler fix is just to examine the number of path components
>> - more than one means a relative or absolute command (/foo splits into
>> two parts). The below works for me on Linux.
> That is clever, but I cannot convince myself that it is not too
> clever for its own sake.  The "pathtype" thing Dscho used in his
> original is documented to be aware of things like "C:\path\name",
> but I didn't re-read the Tcl manual page too carefully to know what
> "file split" does for such pathname to be certain.
>

The manual does not talk about Windows explicitly. From 
https://www.tcl.tk/man/tcl/TclCmd/file.html#M35

*file split */name/
    Returns a list whose elements are the path components in /name/. The
    first element of the list will have the same path type as /name/.
    All other elements will be relative. Path separators will be
    discarded unless they are needed to ensure that an element is
    unambiguously relative. For example, under Unix

    *file split*  /foo/~bar/baz

    returns “*/ foo ./~bar baz*” to ensure that later commands that use
    the third component do not attempt to perform tilde substitution.

So, there is hope c:\foo will split into c: foo, or c:\ foo, but testing 
on Windows is needed. Really need Dscho or someone else from g4w to help 
out here.

Re: BUG: git-gui no longer executes hook scripts

[Mark Levedahl]

On 9/16/23 08:56, Mark Levedahl wrote:
>
>
>
> So, there is hope c:\foo will split into c: foo, or c:\ foo, but 
> testing on Windows is needed. Really need Dscho or someone else from 
> g4w to help out here.
>
>
I did install git for windows into a bare VM, running tclsh.exe on that


puts [file split {c:\foo}]
c:/ foo

puts [llength [file split {c:\foo}]]
2

puts [file split {hooks\foo}]
hooks foo

puts [llength [file split {hooks\foo}]]
2

puts [file split {foo}]
foo

puts [llength [file split {foo}]]
1

So, file split seems to work as needed on Windows.

Re: [PATCH] git-gui - re-enable use of hook scripts

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Commit aae9560a introduced search in $PATH to find executables before
> running them, avoiding an issue where on Windows a same named file in
> the current directory can be executed in preference to anything on the
> path. The updated search excludes files given with an absolute path (e.g.,
> /bin/sh). However this change precludes operation of hook scripts as these
> are named with a relative path (.git/hooks/$HOOK), while a search on $PATH
> can succeed only for bare file names, not relative paths. Furthermore,
> the current repository's .git/hooks directory is in general not listed
> in PATH.
>
> Fix this by changing the "absolute" check to a check for more than one
> component in the pathname, thereby avoiding the PATH check for anything
> given with a relative path as well. Bare "git" has one component, "/sh"
> has two components, and .git/hooks/$HOOK has more than two, so relative
> and absolute pathnames avoid the check.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---

With your experiments in the other thread, I think this is quite a
reasonable fix to the problem.  I'd prefer a few updates to the
proposed log message above, though.

 * Refer the older commit like so:

        Earlier, aae9560a (Work around Tcl's default `PATH` lookup,
        2022-11-23) introduced ...

 * It would help readers if you clarify that "The updated search
   excludes ..." and the rest of that paragraph of the log gives a
   bug/problem/undesirable behaviour of the current code introduced
   by the earlier change.  Perhaps something along the lines of ...

	The updated search excludes commands given as an absolute
	path (e.g., /bin/sh), which is good, but it also tries to
	find commands given as a path relative to the current
	directory with directory separator (e.g.,
	.git/hooks/pre-commit), which makes the hooks from running
	at all.  We only want to apply the $PATH logic to a token
	without any directory separator in it.

 * Mention that we already know the new logic works for absolute
   paths even on Windows by tweaking the sentence starting with
   "Bare 'git' has ...".  Something like:

	Bare "git" has one component (which we want to use $PATH),
	"/bin/sh", "C:\some\command", and ".git/hooks/$HOOK" all
	split into 2 or more (which we want to use as-is).  The only
	case we want to use $PATH is when result of [file split] has
	only one element.

But other than that it looks good.

Dscho?

>  git-gui.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 8bc8892..8603437 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -118,7 +118,7 @@ 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"} {
> +		if {[llength [file split $cmd]] < 2} {
>  			set fullpath [_which $cmd]
>  			if {$fullpath eq ""} {
>  				throw {NOT-FOUND} "$cmd not found in PATH"

Re: BUG: git-gui no longer executes hook scripts

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> On 9/16/23 08:56, Mark Levedahl wrote:
>>
>>
>>
>> So, there is hope c:\foo will split into c: foo, or c:\ foo, but
>> testing on Windows is needed. Really need Dscho or someone else from
>> g4w to help out here.
>>
>>
> I did install git for windows into a bare VM, running tclsh.exe on that
>
>
> puts [file split {c:\foo}]
> c:/ foo

Great.  That is exactly what we want to see.  Thanks.

[PATCH v2] git-gui - re-enable use of hook scripts

[Mark Levedahl]

Earlier, commit aae9560a introduced search in $PATH to find executables
before running them, avoiding an issue where on Windows a same named
file in the current directory can be executed in preference to anything
in a directory in $PATH. This search is intended to find an absolute
path for a bare executable ( e.g, a function "foo") by finding the first
instance of "foo" in a directory given in $PATH, and this search works
correctly.  The search is explicitly avoided for an executable named
with an absolute path (e.g., /bin/sh), and that works as well.

Unfortunately, the search is also applied to commands named with a
relative path. A hook script (or executable) $HOOK is usually located
relative to the project directory as .git/hooks/$HOOK. The search for
this will generally fail as that relative path will (probably) not exist
on any directory in $PATH. This means that git hooks in general now fail
to run. Considerable mayhem could occur should a directory on $PATH be
git controlled. If such a directory includes .git/hooks/$HOOK, that
repository's $HOOK will be substituted for the one in the current
project, with unknown consequences.

This lookup failure also occurs in worktrees linked to a remote .git
directory using git-new-workdir. However, a worktree using a .git file
pointing to a separate git directory apparently avoids this: in that
case the hook command is resolved to an absolute path before being
passed down to the code introduced in aae9560a.

Fix this by replacing the test for an "absolute" pathname to a check for
a command name having more than one pathname component. This limits the
search and absolute pathname resolution to bare commands. The new test
uses tcl's "file split" command. Experiments on Linux and Windows, using
tclsh, show that command names with relative and absolute paths always
give at least two components, while a bare command gives only one.

	  Linux:   puts [file split {foo}]       ==>  foo
	  Linux:   puts [file split {/foo}]      ==>  / foo
	  Linux:   puts [file split {.git/foo}]  ==> .git foo
	  Windows: puts [file split {foo}]       ==>  foo
	  Windows: puts [file split {c:\foo}]    ==>  c:/ foo
	  Windows: puts [file split {.git\foo}]  ==> .git foo

The above results show the new test limits search and replacement
to bare commands on both Linux and Windows.

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

diff --git a/git-gui.sh b/git-gui.sh
index 8bc8892..8603437 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -118,7 +118,7 @@ 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"} {
+		if {[llength [file split $cmd]] < 2} {
 			set fullpath [_which $cmd]
 			if {$fullpath eq ""} {
 				throw {NOT-FOUND} "$cmd not found in PATH"
-- 
2.41.0.99.19

Re: [PATCH v2] git-gui - re-enable use of hook scripts

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> uses tcl's "file split" command. Experiments on Linux and Windows, using
> tclsh, show that command names with relative and absolute paths always
> give at least two components, while a bare command gives only one.
>
> 	  Linux:   puts [file split {foo}]       ==>  foo
> 	  Linux:   puts [file split {/foo}]      ==>  / foo
> 	  Linux:   puts [file split {.git/foo}]  ==> .git foo
> 	  Windows: puts [file split {foo}]       ==>  foo
> 	  Windows: puts [file split {c:\foo}]    ==>  c:/ foo
> 	  Windows: puts [file split {.git\foo}]  ==> .git foo

;-)  Nice documentation of what you found out.

> diff --git a/git-gui.sh b/git-gui.sh
> index 8bc8892..8603437 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -118,7 +118,7 @@ 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"} {
> +		if {[llength [file split $cmd]] < 2} {
>  			set fullpath [_which $cmd]
>  			if {$fullpath eq ""} {
>  				throw {NOT-FOUND} "$cmd not found in PATH"

Nice.  Now we need to find a replacement maintainer for Git-gui ;-)
In the meantime, I can queue this patch on top of what I updated
git-gui part the last time with and merge it in.

Thanks.

Re: [PATCH v2] git-gui - re-enable use of hook scripts

[Mark Levedahl]

On 9/16/23 17:51, Junio C Hamano wrote:
>
> Nice.  Now we need to find a replacement maintainer for Git-gui ;-)
> In the meantime, I can queue this patch on top of what I updated
> git-gui part the last time with and merge it in.
>
> Thanks.

Thank you for help on this too. I retired some time ago, and stopped 
using git much a decade ago. My popping up on the list was inspired by 
cleaning out an old laptop and finding some old patches I thought would 
be useful, especially as I'd helped Shawn create some of that old 
git-gui/Cygwin code. My interest is unlikely to endure so I'm definitely 
not a good candidate to maintain git-gui.

On this hook execution problem, looking further, I find using git-hook 
run will fix some other issues in git-gui's hook handling, and that 
would actually also patch around the problem we just fixed. So, another 
patch follows, the commit message presumes the one fixing relative path 
search remains. I would suggest keeping the one fixing the relative path 
search regardless.

Mark

[PATCH] git-gui - use git-hook, honor core.hooksPath

[Mark Levedahl]

git-gui currently runs some hooks directly using its own code written
before 2010, long predating git v2.9 that added the core.hooksPath
configuration to override the assumed location at $GIT_DIR/hooks.  Thus,
git-gui looks for and runs hooks including prepare-commit-msg,
commit-msg, pre-commit, post-commit, and post-checkout from
$GIT_DIR/hooks, regardless of configuration. Commands (e.g., git-merge)
that git-gui invokes directly do honor core.hooksPath, meaning the
overall behaviour is inconsistent.

Furthermore, since v2.36 git exposes its hook exection machinery via
git-hook run, eliminating the need for others to maintain code
duplicating that functionality.  Using git-hook will both fix git-gui's
current issues on hook configuration and (presumably) reduce the
maintenance burden going forward. So, teach git-gui to use git-hook.

Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
 git-gui.sh | 27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index 8603437..3e5907a 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -661,31 +661,8 @@ proc git_write {args} {
 }
 
 proc githook_read {hook_name args} {
-	set pchook [gitdir hooks $hook_name]
-	lappend args 2>@1
-
-	# On Windows [file executable] might lie so we need to ask
-	# the shell if the hook is executable.  Yes that's annoying.
-	#
-	if {[is_Windows]} {
-		upvar #0 _sh interp
-		if {![info exists interp]} {
-			set interp [_which sh]
-		}
-		if {$interp eq {}} {
-			error "hook execution requires sh (not in PATH)"
-		}
-
-		set scr {if test -x "$1";then exec "$@";fi}
-		set sh_c [list $interp -c $scr $interp $pchook]
-		return [_open_stdout_stderr [concat $sh_c $args]]
-	}
-
-	if {[file executable $pchook]} {
-		return [_open_stdout_stderr [concat [list $pchook] $args]]
-	}
-
-	return {}
+	set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
+	return [_open_stdout_stderr $cmd]
 }
 
 proc kill_file_process {fd} {
-- 
2.41.0.99.19

Re: [PATCH v2] git-gui - re-enable use of hook scripts

[Johannes Schindelin]

Hi,

On Sat, 16 Sep 2023, Mark Levedahl wrote:

> Earlier, commit aae9560a introduced search in $PATH to find executables
> before running them, avoiding an issue where on Windows a same named
> file in the current directory can be executed in preference to anything
> in a directory in $PATH. This search is intended to find an absolute
> path for a bare executable ( e.g, a function "foo") by finding the first
> instance of "foo" in a directory given in $PATH, and this search works
> correctly.  The search is explicitly avoided for an executable named
> with an absolute path (e.g., /bin/sh), and that works as well.
>
> Unfortunately, the search is also applied to commands named with a
> relative path. A hook script (or executable) $HOOK is usually located
> relative to the project directory as .git/hooks/$HOOK. The search for
> this will generally fail as that relative path will (probably) not exist
> on any directory in $PATH. This means that git hooks in general now fail
> to run. Considerable mayhem could occur should a directory on $PATH be
> git controlled. If such a directory includes .git/hooks/$HOOK, that
> repository's $HOOK will be substituted for the one in the current
> project, with unknown consequences.
>
> This lookup failure also occurs in worktrees linked to a remote .git
> directory using git-new-workdir. However, a worktree using a .git file
> pointing to a separate git directory apparently avoids this: in that
> case the hook command is resolved to an absolute path before being
> passed down to the code introduced in aae9560a.
>
> Fix this by replacing the test for an "absolute" pathname to a check for
> a command name having more than one pathname component. This limits the
> search and absolute pathname resolution to bare commands. The new test
> uses tcl's "file split" command. Experiments on Linux and Windows, using
> tclsh, show that command names with relative and absolute paths always
> give at least two components, while a bare command gives only one.
>
> 	  Linux:   puts [file split {foo}]       ==>  foo
> 	  Linux:   puts [file split {/foo}]      ==>  / foo
> 	  Linux:   puts [file split {.git/foo}]  ==> .git foo
> 	  Windows: puts [file split {foo}]       ==>  foo
> 	  Windows: puts [file split {c:\foo}]    ==>  c:/ foo
> 	  Windows: puts [file split {.git\foo}]  ==> .git foo
>
> The above results show the new test limits search and replacement
> to bare commands on both Linux and Windows.

Sounds good. FWIW I ran a couple experiments here, too:

	% file pathtype "C:/foo"
	absolute
	% file pathtype ".git/hooks"
	relative
	% file pathtype ".git\\hooks"
	relative
	% file pathtype "/foo"
	volumerelative
	% file pathtype "foo"
	relative

The problem, therefore, is that `file pathtype` does not discern between a
bare file name and a relative path. The proposed patch looks correct to
me.

Thank you,
Johannes

>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 8bc8892..8603437 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -118,7 +118,7 @@ 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"} {
> +		if {[llength [file split $cmd]] < 2} {
>  			set fullpath [_which $cmd]
>  			if {$fullpath eq ""} {
>  				throw {NOT-FOUND} "$cmd not found in PATH"
> --
> 2.41.0.99.19
>
>

Re: [PATCH] git-gui - use git-hook, honor core.hooksPath

[Johannes Schindelin]

Hi Mark,

On Sun, 17 Sep 2023, Mark Levedahl wrote:

> git-gui currently runs some hooks directly using its own code written
> before 2010, long predating git v2.9 that added the core.hooksPath
> configuration to override the assumed location at $GIT_DIR/hooks.  Thus,
> git-gui looks for and runs hooks including prepare-commit-msg,
> commit-msg, pre-commit, post-commit, and post-checkout from
> $GIT_DIR/hooks, regardless of configuration. Commands (e.g., git-merge)
> that git-gui invokes directly do honor core.hooksPath, meaning the
> overall behaviour is inconsistent.
>
> Furthermore, since v2.36 git exposes its hook exection machinery via
> git-hook run, eliminating the need for others to maintain code
> duplicating that functionality.  Using git-hook will both fix git-gui's
> current issues on hook configuration and (presumably) reduce the
> maintenance burden going forward. So, teach git-gui to use git-hook.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 8603437..3e5907a 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -661,31 +661,8 @@ proc git_write {args} {
>  }
>
>  proc githook_read {hook_name args} {
> -	set pchook [gitdir hooks $hook_name]
> -	lappend args 2>@1
> -
> -	# On Windows [file executable] might lie so we need to ask
> -	# the shell if the hook is executable.  Yes that's annoying.
> -	#
> -	if {[is_Windows]} {
> -		upvar #0 _sh interp
> -		if {![info exists interp]} {
> -			set interp [_which sh]
> -		}
> -		if {$interp eq {}} {
> -			error "hook execution requires sh (not in PATH)"
> -		}
> -
> -		set scr {if test -x "$1";then exec "$@";fi}
> -		set sh_c [list $interp -c $scr $interp $pchook]
> -		return [_open_stdout_stderr [concat $sh_c $args]]
> -	}
> -
> -	if {[file executable $pchook]} {
> -		return [_open_stdout_stderr [concat [list $pchook] $args]]
> -	}
> -
> -	return {}
> +	set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
> +	return [_open_stdout_stderr $cmd]

This looks so much nicer than the original code.

Thank you,
Johannes

>  }
>
>  proc kill_file_process {fd} {
> --
> 2.41.0.99.19
>
>

Re: [PATCH] git-gui - use git-hook, honor core.hooksPath

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +	set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
>> +	return [_open_stdout_stderr $cmd]
>
> This looks so much nicer than the original code.
>
> Thank you,
> Johannes

Yup, looking good.

Re: [PATCH v2] git-gui - re-enable use of hook scripts

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Sounds good. FWIW I ran a couple experiments here, too:
>
> 	% file pathtype "C:/foo"
> 	absolute
> 	% file pathtype ".git/hooks"
> 	relative
> 	% file pathtype ".git\\hooks"
> 	relative
> 	% file pathtype "/foo"
> 	volumerelative
> 	% file pathtype "foo"
> 	relative
>
> The problem, therefore, is that `file pathtype` does not discern between a
> bare file name and a relative path. The proposed patch looks correct to
> me.
>
> Thank you,
> Johannes

Yup, the other "run hooks in a more modern way using 'git hook'"
patch is the right solution for the immediate breakage, but it still
cannot remove this sanitize_command_line proc as we have other users
and use cases where we want to use the sanitized $PATH search, so
this fix is still needed.

Thanks for a quick review on both patches.

Re: [PATCH] git-gui - use git-hook, honor core.hooksPath

[Mark Levedahl]

On 9/18/23 11:58, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> +	set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
>>> +	return [_open_stdout_stderr $cmd]
>> This looks so much nicer than the original code.
>>
>> Thank you,
>> Johannes
> Yup, looking good.

Thanks. BTW, my commit message at "Furthermore, since v2.36 git exposes 
its hook exection machinery via" needs

     s/exection/execution/

Should I resend?

Mark

Re: [PATCH] git-gui - use git-hook, honor core.hooksPath

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> On 9/18/23 11:58, Junio C Hamano wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>>> +	set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
>>>> +	return [_open_stdout_stderr $cmd]
>>> This looks so much nicer than the original code.
>>>
>>> Thank you,
>>> Johannes
>> Yup, looking good.
>
> Thanks. BTW, my commit message at "Furthermore, since v2.36 git
> exposes its hook exection machinery via" needs
>
>     s/exection/execution/
>
> Should I resend?

Nah, I'll just fix it up locally before merging.

Re: [PATCH] git-gui - use git-hook, honor core.hooksPath

[Pratyush Yadav]

Hi,

Thanks for the patch.

On Sun, Sep 17 2023, Mark Levedahl wrote:

> git-gui currently runs some hooks directly using its own code written
> before 2010, long predating git v2.9 that added the core.hooksPath
> configuration to override the assumed location at $GIT_DIR/hooks.  Thus,
> git-gui looks for and runs hooks including prepare-commit-msg,
> commit-msg, pre-commit, post-commit, and post-checkout from
> $GIT_DIR/hooks, regardless of configuration. Commands (e.g., git-merge)
> that git-gui invokes directly do honor core.hooksPath, meaning the
> overall behaviour is inconsistent.
>
> Furthermore, since v2.36 git exposes its hook exection machinery via
> git-hook run, eliminating the need for others to maintain code
> duplicating that functionality.  Using git-hook will both fix git-gui's
> current issues on hook configuration and (presumably) reduce the
> maintenance burden going forward. So, teach git-gui to use git-hook.

In the past, git-gui has tried to keep backward compatibility with all
versions of Git, not just the latest ones. v2.36 is relatively new and
this code would not work for anyone using an older version of Git.

I have largely followed this practice for all the code I have written
but I am not sure if it is a good idea to insist on it -- especially if
it would end up adding some more complexity. I would be interested to
hear what other people think about this.

Junio, I was under the impression that I would keep maintaining the tree
until we found a replacement maintainer. If you are okay with being the
interim maintainer, that sounds good to me. Let me know what works best.

I have applied another patch since my last pull request. So I can apply
this one, send you a new one and sync our trees.

>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
>  git-gui.sh | 27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 8603437..3e5907a 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -661,31 +661,8 @@ proc git_write {args} {
>  }
>  
>  proc githook_read {hook_name args} {
> -	set pchook [gitdir hooks $hook_name]
> -	lappend args 2>@1
> -
> -	# On Windows [file executable] might lie so we need to ask
> -	# the shell if the hook is executable.  Yes that's annoying.
> -	#
> -	if {[is_Windows]} {
> -		upvar #0 _sh interp
> -		if {![info exists interp]} {
> -			set interp [_which sh]
> -		}
> -		if {$interp eq {}} {
> -			error "hook execution requires sh (not in PATH)"
> -		}
> -
> -		set scr {if test -x "$1";then exec "$@";fi}
> -		set sh_c [list $interp -c $scr $interp $pchook]
> -		return [_open_stdout_stderr [concat $sh_c $args]]
> -	}
> -
> -	if {[file executable $pchook]} {
> -		return [_open_stdout_stderr [concat [list $pchook] $args]]
> -	}
> -
> -	return {}
> +	set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]
> +	return [_open_stdout_stderr $cmd]

LGTM, other than my concerns with backward compatibility.

>  }
>  
>  proc kill_file_process {fd} {

-- 
Regards,
Pratyush Yadav

Re: [PATCH v2] git-gui - re-enable use of hook scripts

[Pratyush Yadav]

Hi,

On Sat, Sep 16 2023, Mark Levedahl wrote:

> Earlier, commit aae9560a introduced search in $PATH to find executables
> before running them, avoiding an issue where on Windows a same named
> file in the current directory can be executed in preference to anything
> in a directory in $PATH. This search is intended to find an absolute
> path for a bare executable ( e.g, a function "foo") by finding the first
> instance of "foo" in a directory given in $PATH, and this search works
> correctly.  The search is explicitly avoided for an executable named
> with an absolute path (e.g., /bin/sh), and that works as well.
>
> Unfortunately, the search is also applied to commands named with a
> relative path. A hook script (or executable) $HOOK is usually located
> relative to the project directory as .git/hooks/$HOOK. The search for
> this will generally fail as that relative path will (probably) not exist
> on any directory in $PATH. This means that git hooks in general now fail
> to run. Considerable mayhem could occur should a directory on $PATH be
> git controlled. If such a directory includes .git/hooks/$HOOK, that
> repository's $HOOK will be substituted for the one in the current
> project, with unknown consequences.
>
> This lookup failure also occurs in worktrees linked to a remote .git
> directory using git-new-workdir. However, a worktree using a .git file
> pointing to a separate git directory apparently avoids this: in that
> case the hook command is resolved to an absolute path before being
> passed down to the code introduced in aae9560a.
>
> Fix this by replacing the test for an "absolute" pathname to a check for
> a command name having more than one pathname component. This limits the
> search and absolute pathname resolution to bare commands. The new test
> uses tcl's "file split" command. Experiments on Linux and Windows, using
> tclsh, show that command names with relative and absolute paths always
> give at least two components, while a bare command gives only one.
>
> 	  Linux:   puts [file split {foo}]       ==>  foo
> 	  Linux:   puts [file split {/foo}]      ==>  / foo
> 	  Linux:   puts [file split {.git/foo}]  ==> .git foo
> 	  Windows: puts [file split {foo}]       ==>  foo
> 	  Windows: puts [file split {c:\foo}]    ==>  c:/ foo
> 	  Windows: puts [file split {.git\foo}]  ==> .git foo
>
> The above results show the new test limits search and replacement
> to bare commands on both Linux and Windows.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>

Looks good. Thanks.

Reviewed-by: Pratyush Yadav <me@yadavpratyush.com>

> ---
>  git-gui.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 8bc8892..8603437 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -118,7 +118,7 @@ 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"} {
> +		if {[llength [file split $cmd]] < 2} {
>  			set fullpath [_which $cmd]
>  			if {$fullpath eq ""} {
>  				throw {NOT-FOUND} "$cmd not found in PATH"

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui - use git-hook, honor core.hooksPath

[Mark Levedahl]

On 9/20/23 09:05, Pratyush Yadav wrote:
> In the past, git-gui has tried to keep backward compatibility with all
> versions of Git, not just the latest ones. v2.36 is relatively new and
> this code would not work for anyone using an older version of Git.
>
> I have largely followed this practice for all the code I have written
> but I am not sure if it is a good idea to insist on it -- especially if
> it would end up adding some more complexity. I would be interested to
> hear what other people think about this.
>
I am not aware of any distribution (Linux, g4w, Mac) shipping anything 
except the git-gui in Junio's tree, which is specific to the git-core 
version, and the git-gui packages require (or are a part of) the same 
version git-core package: no cross-version compatibility of git 
components is assumed. Certainly, folks rolling their own can pull from 
upstream git-gui, but they take the risk of incompatibility with an 
outdated git. Other tools in Junio's tree have already made the switch 
to git-hook (send-email, git-p4) even though they are usually packaged 
separately from git-core, but also version locked to matching git-core.

Updating git-gui's hook execution to match git internals would be more 
complex than what I implemented or what was there before.  For instance, 
I never looked at what git-hook's g4w compatibility code uses to test if 
a hook is present and executable, it wouldn't surprise me to find 
git-gui was missing something there, but who wants to bother? Also, the 
commit language surrounding addition of git-hook is strongly suggestive 
of other changes in configuration coming, meaning more changes to hook 
execution code would be needed that are avoided by using git-hook. Note: 
I have one more patch to send, removing yet another work-around for 
early Cygwin tcl/tk, as more evidence of how many years it takes to 
clean some of this stuff out and the difficulty of keeping git-gui up to 
date.

I had considered the above when creating the patch, and I believe what I 
did is the right approach.

Mark

Re: [PATCH] git-gui - use git-hook, honor core.hooksPath

[Junio C Hamano]

Pratyush Yadav <me@yadavpratyush.com> writes:

> In the past, git-gui has tried to keep backward compatibility with all
> versions of Git, not just the latest ones. v2.36 is relatively new and
> this code would not work for anyone using an older version of Git.
>
> I have largely followed this practice for all the code I have written
> but I am not sure if it is a good idea to insist on it -- especially if
> it would end up adding some more complexity. I would be interested to
> hear what other people think about this.

Good point.

> Junio, I was under the impression that I would keep maintaining the tree
> until we found a replacement maintainer. If you are okay with being the
> interim maintainer, that sounds good to me. Let me know what works best.

I am actually not OK ;-).

I prefer to see somebody who does use git-gui, or at least somebody
who uses Git in a graphical environment in their daily work, to be
maintaining it.  I am disqualified on both counts.

> I have applied another patch since my last pull request. So I can apply
> this one, send you a new one and sync our trees.

OK.  I'll drop the copy I have on my end when it happens, then.

Thanks.

Re: [PATCH] git-gui - use git-hook, honor core.hooksPath

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> Certainly, folks rolling their own can pull
> from upstream git-gui, but they take the risk of incompatibility with
> an outdated git. Other tools in Junio's tree have already made the
> switch to git-hook (send-email, git-p4) even though they are usually
> packaged separately from git-core, but also version locked to matching
> git-core.

The cross-version compatibility story is the same for "gitk" (which
I believe "git-gui" took the "do not too deeply depend on the
matching version of git" mantra from).  I can understand the desire
and being able to aim for wider compatibility may be an advantage
for these tools that are not tightly bundled with the rest of the
system.  It allowed them to evolve without waiting for Git to catch
up, for example.

But at this point in their history where these tools are very
mature, it may be fair to say that the cross-version compatibility
is becoming a lost cause.