{"thread":{"id":"60234","subject":"BUG: git-gui no longer executes hook scripts","startedAt":"2023-09-15T16:46:14Z","lastAt":"2023-09-20T16:58:36Z","messageCount":25,"participants":["Mark Levedahl","Junio C Hamano","Johannes Schindelin","Pratyush Yadav"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"481875","messageId":"bd510f6d-6613-413b-6d64-c3d2fd01d8a9@gmail.com","threadId":"60234","inReplyTo":null,"subject":"BUG: git-gui no longer executes hook scripts","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-15T16:45:31Z","receivedAt":"2023-09-15T16:46:14Z","isPatch":false,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"\nThe commit titled \"Work around Tcl's default |PATH| lookup\",|aae9560, \nadds checking on all commands to be executed to assure these are on the \nPATH. Any script in .git/hooks is rejected as .git/hooks is not (in \ngeneral) on the PATH, even if the entry in .git/hooks is a symlink to a \nfile on the PATH. Instead, git-gui throws and error without completing \nthe operation. This is easily demonstrated by say, enabling the \ncommit-msg script (hooks-commit-msg.sample templates) and attempting a \ncommit.\n|\n\n|I don't have a suggested solution to this: reverting the above commit \nwill fix this problem, but that commit  was made to mitigate a security \nissue.  Perhaps anything in .git/hooks should be accepted without \nfurther checks?\n|\n\n|\n|\n\n|Mark\n|\n\n"},{"id":"481876","messageId":"xmqqa5tngynh.fsf@gitster.g","threadId":"60234","inReplyTo":"bd510f6d-6613-413b-6d64-c3d2fd01d8a9@gmail.com","subject":"Re: BUG: git-gui no longer executes hook scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-15T17:00:02Z","receivedAt":"2023-09-15T17:01:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> The commit titled \"Work around Tcl's default |PATH| lookup\",|aae9560,\n> adds checking on all commands to be executed to assure these are on\n> the PATH.\n\ncommit aae9560a355d4ab91385e49eae62fade2ddd27ef\nAuthor: Johannes Schindelin \nDate: Wed Nov 23 09:31:06 2022 +0100\n\n Work around Tcl's default `PATH` lookup\n \n As per https://www.tcl.tk/man/tcl8.6/TclCmd/exec.html#M23, Tcl's `exec`\n function goes out of its way to imitate the highly dangerous path lookup\n of `cmd.exe`, but _of course_ only on Windows:\n \n If a directory name was not specified as part of the application\n name, the following directories are automatically searched in\n order when attempting to locate the application:\n\nIn other words, if somebody tries to run \".git/hooks/pre-commit\",\nbecause a directory name _is_ given (i.e. \".git/hooks/\" in this case),\nthe path lookup is *not* done. Which is what I would expect, and then\n\"oh, only on Windows to match what cmd.exe does, the current directory\nis early in the search order\" should not be a problem.\n\n To avoid that, Git GUI already has the `_which` function that does not\n imitate that dangerous practice when looking up executables in the\n search path.\n \nSounds good, but ...\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex b0eb5a6ae4..cb92bba1c4 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -121,6 +121,62 @@ proc _which {what args} {\n \treturn {}\n }\n \n+proc sanitize_command_line {command_line from_index} {\n+\tset i $from_index\n+\twhile {$i < [llength $command_line]} {\n+\t\tset cmd [lindex $command_line $i]\n+\t\tif {[file pathtype $cmd] ne \"absolute\"} {\n+\t\t\tset fullpath [_which $cmd]\n+\t\t\tif {$fullpath eq \"\"} {\n+\t\t\t\tthrow {NOT-FOUND} \"$cmd not found in PATH\"\n+\t\t\t}\n+\t\t\tlset command_line $i $fullpath\n\nShouldn't this \"is it absolute\" check with \"$cmd\" also check if $cmd\nhas either forward or backward slash in it? I do not know about the\nWindows cmd.exe convention, but with Unix background, I would be\nsurprised if dir/cmd gave by end users ran \"C:\\program\nfiles\\dir\\cmd\" (unless I happened to be in the \"C:\\program files\\\"\nfolder, that is).\n\nChecking the use of _which with fixed arguments, it is used to spawn\ngit, gitk, nice, sh; and _which finding where they appear on the\nsearch path does sound sane. But _which does not seem to have the \"if\ngiven a command with directory separator, the search path does not\nmatter. The caller means it is relative to the $cwd\" logic at all,\nso it seems it is the callers responsibility to make sure it does\nnot pass things like \".git/hooks/pre-commit\" to it.\n\n+\t\t}\n+\n+\t\t# handle piped commands, e.g. `exec A | B`\n+\t\tfor {incr i} {$i < [llength $command_line]} {incr i} {\n+\t\t\tif {[lindex $command_line $i] eq \"|\"} {\n+\t\t\t\tincr i\n+\t\t\t\tbreak\n+\t\t\t}\n+\t\t}\n+\t}\n+\treturn $command_line\n+}\n+\n+# Override `exec` to avoid unsafe PATH lookup\n+\n+rename exec real_exec\n+\n+proc exec {args} {\n+\t# skip options\n+\tfor {set i 0} {$i < [llength $args]} {incr i} {\n+\t\tset arg [lindex $args $i]\n+\t\tif {$arg eq \"--\"} {\n+\t\t\tincr i\n+\t\t\tbreak\n+\t\t}\n+\t\tif {[string range $arg 0 0] ne \"-\"} {\n+\t\t\tbreak\n+\t\t}\n+\t}\n+\tset args [sanitize_command_line $args $i]\n+\tuplevel 1 real_exec $args\n+}\n+\n+# Override `open` to avoid unsafe PATH lookup\n+\n+rename open real_open\n+\n+proc open {args} {\n+\tset arg0 [lindex $args 0]\n+\tif {[string range $arg0 0 0] eq \"|\"} {\n+\t\tset command_line [string trim [string range $arg0 1 end]]\n+\t\tlset args 0 \"| [sanitize_command_line $command_line 0]\"\n+\t}\n+\tuplevel 1 real_open $args\n+}\n+\n ######################################################################\n ##\n ## locate our library\n"},{"id":"481877","messageId":"xmqq5y4bgxy1.fsf@gitster.g","threadId":"60234","inReplyTo":"xmqqa5tngynh.fsf@gitster.g","subject":"Re: BUG: git-gui no longer executes hook scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-15T17:15:18Z","receivedAt":"2023-09-15T17:16:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Shouldn't this \"is it absolute\" check with \"$cmd\" also check if $cmd\n> has either forward or backward slash in it?\n>\n> Checking the use of _which with fixed arguments, it is used to spawn\n> git, gitk, nice, sh; and _which finding where they appear on the\n> search path does sound sane. But _which does not seem to have the \"if\n> given a command with directory separator, the search path does not\n> matter. The caller means it is relative to the $cwd\" logic at all,\n> so it seems it is the callers responsibility to make sure it does\n> not pass things like \".git/hooks/pre-commit\" to it.\n\nIn other words, something along this line may go in the right\ndirection (I no longer speak Tcl, and this is done with manual in\none hand, while typing with the other hand).\n\n git-gui.sh | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git c/git-gui.sh w/git-gui.sh\nindex 8bc8892c40..45d8f48b39 100755\n--- c/git-gui/git-gui.sh\n+++ w/git-gui/git-gui.sh\n@@ -119,11 +119,15 @@ proc sanitize_command_line {command_line from_index} {\n \twhile {$i < [llength $command_line]} {\n \t\tset cmd [lindex $command_line $i]\n \t\tif {[file pathtype $cmd] ne \"absolute\"} {\n-\t\t\tset fullpath [_which $cmd]\n-\t\t\tif {$fullpath eq \"\"} {\n-\t\t\t\tthrow {NOT-FOUND} \"$cmd not found in PATH\"\n+\t\t\tif {1 < [llength [file split $cmd]]]} {\n+\t\t\t set cmdpath [_which $cmd]\n+\t\t\t if {$cmdpath eq \"\"} {\n+\t\t\t\t throw {NOT-FOUND} \"$cmd not found in PATH\"\n+\t\t\t }\n+\t\t\t} else {\n+\t\t\t\tset cmdpath $cmd\n \t\t\t}\n-\t\t\tlset command_line $i $fullpath\n+\t\t\tlset command_line $i $cmdpath\n \t\t}\n \n \t\t# handle piped commands, e.g. `exec A | B`\n"},{"id":"481898","messageId":"454d8b7b-96df-ec8f-2285-e022de66c66c@gmail.com","threadId":"60234","inReplyTo":"xmqq5y4bgxy1.fsf@gitster.g","subject":"Re: BUG: git-gui no longer executes hook scripts","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-15T23:33:55Z","receivedAt":"2023-09-15T23:34:32Z","isPatch":false,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"\nOn 9/15/23 13:15, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Shouldn't this \"is it absolute\" check with \"$cmd\" also check if $cmd\n>> has either forward or backward slash in it?\n>>\n>> Checking the use of _which with fixed arguments, it is used to spawn\n>> git, gitk, nice, sh; and _which finding where they appear on the\n>> search path does sound sane. But _which does not seem to have the \"if\n>> given a command with directory separator, the search path does not\n>> matter. The caller means it is relative to the $cwd\" logic at all,\n>> so it seems it is the callers responsibility to make sure it does\n>> not pass things like \".git/hooks/pre-commit\" to it.\n> In other words, something along this line may go in the right\n> direction (I no longer speak Tcl, and this is done with manual in\n> one hand, while typing with the other hand).\n>\nI think a simpler fix is just to examine the number of path components - \nmore than one means a relative or absolute command (/foo splits into two \nparts). The below works for me on Linux.\n\ndiff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh\nindex 277a2b1c8c..0c39d9fa26 100755\n--- a/git-gui/git-gui.sh\n+++ b/git-gui/git-gui.sh\n@@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {\n     set i $from_index\n     while {$i < [llength $command_line]} {\n         set cmd [lindex $command_line $i]\n-       if {[file pathtype $cmd] ne \"absolute\"} {\n+       if {[llength [file split $cmd]] < 2} {\n             set fullpath [_which $cmd]\n             if {$fullpath eq \"\"} {\n                 throw {NOT-FOUND} \"$cmd not found in PATH\"\n\n\nWe could also wrap the entirety of commit aae9560a in\n\n     if {[is_Windows]} { ... }\n\nas all of this code is fixing a Windows specific vulnerability, though a \nfix like the above is needed regardless.\n\nMark\n\n"},{"id":"481899","messageId":"20230916003516.51053-1-mlevedahl@gmail.com","threadId":"60234","inReplyTo":"454d8b7b-96df-ec8f-2285-e022de66c66c@gmail.com","subject":"[PATCH] git-gui - re-enable use of hook scripts","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-16T00:35:16Z","receivedAt":"2023-09-16T00:39:08Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"Commit aae9560a introduced search in $PATH to find executables before\nrunning them, avoiding an issue where on Windows a same named file in\nthe current directory can be executed in preference to anything on the\npath. The updated search excludes files given with an absolute path (e.g.,\n/bin/sh). However this change precludes operation of hook scripts as these\nare named with a relative path (.git/hooks/$HOOK), while a search on $PATH\ncan succeed only for bare file names, not relative paths. Furthermore,\nthe current repository's .git/hooks directory is in general not listed\nin PATH.\n\nFix this by changing the \"absolute\" check to a check for more than one\ncomponent in the pathname, thereby avoiding the PATH check for anything\ngiven with a relative path as well. Bare \"git\" has one component, \"/sh\"\nhas two components, and .git/hooks/$HOOK has more than two, so relative\nand absolute pathnames avoid the check.\n\nSigned-off-by: Mark Levedahl \n---\n git-gui.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex 8bc8892..8603437 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {\n \tset i $from_index\n \twhile {$i < [llength $command_line]} {\n \t\tset cmd [lindex $command_line $i]\n-\t\tif {[file pathtype $cmd] ne \"absolute\"} {\n+\t\tif {[llength [file split $cmd]] < 2} {\n \t\t\tset fullpath [_which $cmd]\n \t\t\tif {$fullpath eq \"\"} {\n \t\t\t\tthrow {NOT-FOUND} \"$cmd not found in PATH\"\n-- \n2.41.0.99.19\n\n"},{"id":"481914","messageId":"xmqqil8ad8un.fsf@gitster.g","threadId":"60234","inReplyTo":"454d8b7b-96df-ec8f-2285-e022de66c66c@gmail.com","subject":"Re: BUG: git-gui no longer executes hook scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-16T04:45:36Z","receivedAt":"2023-09-16T04:49:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> I think a simpler fix is just to examine the number of path components\n> - more than one means a relative or absolute command (/foo splits into\n> two parts). The below works for me on Linux.\n\nThat is clever, but I cannot convince myself that it is not too\nclever for its own sake. The \"pathtype\" thing Dscho used in his\noriginal is documented to be aware of things like \"C:\\path\\name\",\nbut I didn't re-read the Tcl manual page too carefully to know what\n\"file split\" does for such pathname to be certain.\n\n"},{"id":"481926","messageId":"ffd5e1dc-bad7-2b1d-3344-76ffeb2858f5@gmail.com","threadId":"60234","inReplyTo":"xmqqil8ad8un.fsf@gitster.g","subject":"Re: BUG: git-gui no longer executes hook scripts","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-16T12:56:28Z","receivedAt":"2023-09-16T12:57:21Z","isPatch":false,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"\nOn 9/16/23 00:45, Junio C Hamano wrote:\n> Mark Levedahl writes:\n>\n>> I think a simpler fix is just to examine the number of path components\n>> - more than one means a relative or absolute command (/foo splits into\n>> two parts). The below works for me on Linux.\n> That is clever, but I cannot convince myself that it is not too\n> clever for its own sake. The \"pathtype\" thing Dscho used in his\n> original is documented to be aware of things like \"C:\\path\\name\",\n> but I didn't re-read the Tcl manual page too carefully to know what\n> \"file split\" does for such pathname to be certain.\n>\n\nThe manual does not talk about Windows explicitly. From \nhttps://www.tcl.tk/man/tcl/TclCmd/file.html#M35\n\n*file split */name/\n Returns a list whose elements are the path components in /name/. The\n first element of the list will have the same path type as /name/.\n All other elements will be relative. Path separators will be\n discarded unless they are needed to ensure that an element is\n unambiguously relative. For example, under Unix\n\n *file split* /foo/~bar/baz\n\n returns “*/ foo ./~bar baz*” to ensure that later commands that use\n the third component do not attempt to perform tilde substitution.\n\nSo, there is hope c:\\foo will split into c: foo, or c:\\ foo, but testing \non Windows is needed. Really need Dscho or someone else from g4w to help \nout here.\n\n\n"},{"id":"481928","messageId":"2ce41212-41e7-fe5f-cc9f-bcfaa2641e59@gmail.com","threadId":"60234","inReplyTo":"ffd5e1dc-bad7-2b1d-3344-76ffeb2858f5@gmail.com","subject":"Re: BUG: git-gui no longer executes hook scripts","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-16T14:49:12Z","receivedAt":"2023-09-16T14:49:49Z","isPatch":false,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"\nOn 9/16/23 08:56, Mark Levedahl wrote:\n>\n>\n>\n> So, there is hope c:\\foo will split into c: foo, or c:\\ foo, but \n> testing on Windows is needed. Really need Dscho or someone else from \n> g4w to help out here.\n>\n>\nI did install git for windows into a bare VM, running tclsh.exe on that\n\n\nputs [file split {c:\\foo}]\nc:/ foo\n\nputs [llength [file split {c:\\foo}]]\n2\n\nputs [file split {hooks\\foo}]\nhooks foo\n\nputs [llength [file split {hooks\\foo}]]\n2\n\nputs [file split {foo}]\nfoo\n\nputs [llength [file split {foo}]]\n1\n\nSo, file split seems to work as needed on Windows.\n\n"},{"id":"481929","messageId":"xmqqo7i2autz.fsf@gitster.g","threadId":"60234","inReplyTo":"2ce41212-41e7-fe5f-cc9f-bcfaa2641e59@gmail.com","subject":"Re: BUG: git-gui no longer executes hook scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-16T17:31:20Z","receivedAt":"2023-09-16T17:31:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> On 9/16/23 08:56, Mark Levedahl wrote:\n>>\n>>\n>>\n>> So, there is hope c:\\foo will split into c: foo, or c:\\ foo, but\n>> testing on Windows is needed. Really need Dscho or someone else from\n>> g4w to help out here.\n>>\n>>\n> I did install git for windows into a bare VM, running tclsh.exe on that\n>\n>\n> puts [file split {c:\\foo}]\n> c:/ foo\n\nGreat. That is exactly what we want to see. Thanks.\n"},{"id":"481930","messageId":"xmqqy1h6auy7.fsf@gitster.g","threadId":"60234","inReplyTo":"20230916003516.51053-1-mlevedahl@gmail.com","subject":"Re: [PATCH] git-gui - re-enable use of hook scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-16T17:28:48Z","receivedAt":"2023-09-16T17:31:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> Commit aae9560a introduced search in $PATH to find executables before\n> running them, avoiding an issue where on Windows a same named file in\n> the current directory can be executed in preference to anything on the\n> path. The updated search excludes files given with an absolute path (e.g.,\n> /bin/sh). However this change precludes operation of hook scripts as these\n> are named with a relative path (.git/hooks/$HOOK), while a search on $PATH\n> can succeed only for bare file names, not relative paths. Furthermore,\n> the current repository's .git/hooks directory is in general not listed\n> in PATH.\n>\n> Fix this by changing the \"absolute\" check to a check for more than one\n> component in the pathname, thereby avoiding the PATH check for anything\n> given with a relative path as well. Bare \"git\" has one component, \"/sh\"\n> has two components, and .git/hooks/$HOOK has more than two, so relative\n> and absolute pathnames avoid the check.\n>\n> Signed-off-by: Mark Levedahl \n> ---\n\nWith your experiments in the other thread, I think this is quite a\nreasonable fix to the problem. I'd prefer a few updates to the\nproposed log message above, though.\n\n * Refer the older commit like so:\n\n Earlier, aae9560a (Work around Tcl's default `PATH` lookup,\n 2022-11-23) introduced ...\n\n * It would help readers if you clarify that \"The updated search\n excludes ...\" and the rest of that paragraph of the log gives a\n bug/problem/undesirable behaviour of the current code introduced\n by the earlier change. Perhaps something along the lines of ...\n\n\tThe updated search excludes commands given as an absolute\n\tpath (e.g., /bin/sh), which is good, but it also tries to\n\tfind commands given as a path relative to the current\n\tdirectory with directory separator (e.g.,\n\t.git/hooks/pre-commit), which makes the hooks from running\n\tat all. We only want to apply the $PATH logic to a token\n\twithout any directory separator in it.\n\n * Mention that we already know the new logic works for absolute\n paths even on Windows by tweaking the sentence starting with\n \"Bare 'git' has ...\". Something like:\n\n\tBare \"git\" has one component (which we want to use $PATH),\n\t\"/bin/sh\", \"C:\\some\\command\", and \".git/hooks/$HOOK\" all\n\tsplit into 2 or more (which we want to use as-is). The only\n\tcase we want to use $PATH is when result of [file split] has\n\tonly one element.\n\nBut other than that it looks good.\n\nDscho?\n\n> git-gui.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-gui.sh b/git-gui.sh\n> index 8bc8892..8603437 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {\n> \tset i $from_index\n> \twhile {$i < [llength $command_line]} {\n> \t\tset cmd [lindex $command_line $i]\n> -\t\tif {[file pathtype $cmd] ne \"absolute\"} {\n> +\t\tif {[llength [file split $cmd]] < 2} {\n> \t\t\tset fullpath [_which $cmd]\n> \t\t\tif {$fullpath eq \"\"} {\n> \t\t\t\tthrow {NOT-FOUND} \"$cmd not found in PATH\"\n"},{"id":"481934","messageId":"20230916210131.78593-1-mlevedahl@gmail.com","threadId":"60234","inReplyTo":"xmqqy1h6auy7.fsf@gitster.g","subject":"[PATCH v2] git-gui - re-enable use of hook scripts","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-16T21:01:31Z","receivedAt":"2023-09-16T21:02:34Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"Earlier, commit aae9560a introduced search in $PATH to find executables\nbefore running them, avoiding an issue where on Windows a same named\nfile in the current directory can be executed in preference to anything\nin a directory in $PATH. This search is intended to find an absolute\npath for a bare executable ( e.g, a function \"foo\") by finding the first\ninstance of \"foo\" in a directory given in $PATH, and this search works\ncorrectly. The search is explicitly avoided for an executable named\nwith an absolute path (e.g., /bin/sh), and that works as well.\n\nUnfortunately, the search is also applied to commands named with a\nrelative path. A hook script (or executable) $HOOK is usually located\nrelative to the project directory as .git/hooks/$HOOK. The search for\nthis will generally fail as that relative path will (probably) not exist\non any directory in $PATH. This means that git hooks in general now fail\nto run. Considerable mayhem could occur should a directory on $PATH be\ngit controlled. If such a directory includes .git/hooks/$HOOK, that\nrepository's $HOOK will be substituted for the one in the current\nproject, with unknown consequences.\n\nThis lookup failure also occurs in worktrees linked to a remote .git\ndirectory using git-new-workdir. However, a worktree using a .git file\npointing to a separate git directory apparently avoids this: in that\ncase the hook command is resolved to an absolute path before being\npassed down to the code introduced in aae9560a.\n\nFix this by replacing the test for an \"absolute\" pathname to a check for\na command name having more than one pathname component. This limits the\nsearch and absolute pathname resolution to bare commands. The new test\nuses tcl's \"file split\" command. Experiments on Linux and Windows, using\ntclsh, show that command names with relative and absolute paths always\ngive at least two components, while a bare command gives only one.\n\n\t Linux: puts [file split {foo}] ==> foo\n\t Linux: puts [file split {/foo}] ==> / foo\n\t Linux: puts [file split {.git/foo}] ==> .git foo\n\t Windows: puts [file split {foo}] ==> foo\n\t Windows: puts [file split {c:\\foo}] ==> c:/ foo\n\t Windows: puts [file split {.git\\foo}] ==> .git foo\n\nThe above results show the new test limits search and replacement\nto bare commands on both Linux and Windows.\n\nSigned-off-by: Mark Levedahl \n---\n git-gui.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex 8bc8892..8603437 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {\n \tset i $from_index\n \twhile {$i < [llength $command_line]} {\n \t\tset cmd [lindex $command_line $i]\n-\t\tif {[file pathtype $cmd] ne \"absolute\"} {\n+\t\tif {[llength [file split $cmd]] < 2} {\n \t\t\tset fullpath [_which $cmd]\n \t\t\tif {$fullpath eq \"\"} {\n \t\t\t\tthrow {NOT-FOUND} \"$cmd not found in PATH\"\n-- \n2.41.0.99.19\n\n"},{"id":"481936","messageId":"xmqqy1h5aisw.fsf@gitster.g","threadId":"60234","inReplyTo":"20230916210131.78593-1-mlevedahl@gmail.com","subject":"Re: [PATCH v2] git-gui - re-enable use of hook scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-16T21:51:11Z","receivedAt":"2023-09-16T22:01:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> uses tcl's \"file split\" command. Experiments on Linux and Windows, using\n> tclsh, show that command names with relative and absolute paths always\n> give at least two components, while a bare command gives only one.\n>\n> \t Linux: puts [file split {foo}] ==> foo\n> \t Linux: puts [file split {/foo}] ==> / foo\n> \t Linux: puts [file split {.git/foo}] ==> .git foo\n> \t Windows: puts [file split {foo}] ==> foo\n> \t Windows: puts [file split {c:\\foo}] ==> c:/ foo\n> \t Windows: puts [file split {.git\\foo}] ==> .git foo\n\n;-) Nice documentation of what you found out.\n\n> diff --git a/git-gui.sh b/git-gui.sh\n> index 8bc8892..8603437 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {\n> \tset i $from_index\n> \twhile {$i < [llength $command_line]} {\n> \t\tset cmd [lindex $command_line $i]\n> -\t\tif {[file pathtype $cmd] ne \"absolute\"} {\n> +\t\tif {[llength [file split $cmd]] < 2} {\n> \t\t\tset fullpath [_which $cmd]\n> \t\t\tif {$fullpath eq \"\"} {\n> \t\t\t\tthrow {NOT-FOUND} \"$cmd not found in PATH\"\n\nNice. Now we need to find a replacement maintainer for Git-gui ;-)\nIn the meantime, I can queue this patch on top of what I updated\ngit-gui part the last time with and merge it in.\n\nThanks.\n"},{"id":"481942","messageId":"fa876f80-02dc-2c04-0db3-bf3f6269b427@gmail.com","threadId":"60234","inReplyTo":"xmqqy1h5aisw.fsf@gitster.g","subject":"Re: [PATCH v2] git-gui - re-enable use of hook scripts","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-17T19:22:39Z","receivedAt":"2023-09-17T19:23:38Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"\nOn 9/16/23 17:51, Junio C Hamano wrote:\n>\n> Nice. Now we need to find a replacement maintainer for Git-gui ;-)\n> In the meantime, I can queue this patch on top of what I updated\n> git-gui part the last time with and merge it in.\n>\n> Thanks.\n\nThank you for help on this too. I retired some time ago, and stopped \nusing git much a decade ago. My popping up on the list was inspired by \ncleaning out an old laptop and finding some old patches I thought would \nbe useful, especially as I'd helped Shawn create some of that old \ngit-gui/Cygwin code. My interest is unlikely to endure so I'm definitely \nnot a good candidate to maintain git-gui.\n\nOn this hook execution problem, looking further, I find using git-hook \nrun will fix some other issues in git-gui's hook handling, and that \nwould actually also patch around the problem we just fixed. So, another \npatch follows, the commit message presumes the one fixing relative path \nsearch remains. I would suggest keeping the one fixing the relative path \nsearch regardless.\n\nMark\n\n"},{"id":"481943","messageId":"20230917192431.101775-1-mlevedahl@gmail.com","threadId":"60234","inReplyTo":"fa876f80-02dc-2c04-0db3-bf3f6269b427@gmail.com","subject":"[PATCH] git-gui - use git-hook, honor core.hooksPath","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-17T19:24:31Z","receivedAt":"2023-09-17T19:25:46Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"git-gui currently runs some hooks directly using its own code written\nbefore 2010, long predating git v2.9 that added the core.hooksPath\nconfiguration to override the assumed location at $GIT_DIR/hooks. Thus,\ngit-gui looks for and runs hooks including prepare-commit-msg,\ncommit-msg, pre-commit, post-commit, and post-checkout from\n$GIT_DIR/hooks, regardless of configuration. Commands (e.g., git-merge)\nthat git-gui invokes directly do honor core.hooksPath, meaning the\noverall behaviour is inconsistent.\n\nFurthermore, since v2.36 git exposes its hook exection machinery via\ngit-hook run, eliminating the need for others to maintain code\nduplicating that functionality. Using git-hook will both fix git-gui's\ncurrent issues on hook configuration and (presumably) reduce the\nmaintenance burden going forward. So, teach git-gui to use git-hook.\n\nSigned-off-by: Mark Levedahl \n---\n git-gui.sh | 27 ++-------------------------\n 1 file changed, 2 insertions(+), 25 deletions(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex 8603437..3e5907a 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -661,31 +661,8 @@ proc git_write {args} {\n }\n \n proc githook_read {hook_name args} {\n-\tset pchook [gitdir hooks $hook_name]\n-\tlappend args 2>@1\n-\n-\t# On Windows [file executable] might lie so we need to ask\n-\t# the shell if the hook is executable. Yes that's annoying.\n-\t#\n-\tif {[is_Windows]} {\n-\t\tupvar #0 _sh interp\n-\t\tif {![info exists interp]} {\n-\t\t\tset interp [_which sh]\n-\t\t}\n-\t\tif {$interp eq {}} {\n-\t\t\terror \"hook execution requires sh (not in PATH)\"\n-\t\t}\n-\n-\t\tset scr {if test -x \"$1\";then exec \"$@\";fi}\n-\t\tset sh_c [list $interp -c $scr $interp $pchook]\n-\t\treturn [_open_stdout_stderr [concat $sh_c $args]]\n-\t}\n-\n-\tif {[file executable $pchook]} {\n-\t\treturn [_open_stdout_stderr [concat [list $pchook] $args]]\n-\t}\n-\n-\treturn {}\n+\tset cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]\n+\treturn [_open_stdout_stderr $cmd]\n }\n \n proc kill_file_process {fd} {\n-- \n2.41.0.99.19\n\n"},{"id":"481952","messageId":"260b62dc-d45b-58fa-85c4-ffbd981a1c84@gmx.de","threadId":"60234","inReplyTo":"20230916210131.78593-1-mlevedahl@gmail.com","subject":"Re: [PATCH v2] git-gui - re-enable use of hook scripts","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-09-18T15:26:21Z","receivedAt":"2023-09-18T15:37:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 16 Sep 2023, Mark Levedahl wrote:\n\n> Earlier, commit aae9560a introduced search in $PATH to find executables\n> before running them, avoiding an issue where on Windows a same named\n> file in the current directory can be executed in preference to anything\n> in a directory in $PATH. This search is intended to find an absolute\n> path for a bare executable ( e.g, a function \"foo\") by finding the first\n> instance of \"foo\" in a directory given in $PATH, and this search works\n> correctly. The search is explicitly avoided for an executable named\n> with an absolute path (e.g., /bin/sh), and that works as well.\n>\n> Unfortunately, the search is also applied to commands named with a\n> relative path. A hook script (or executable) $HOOK is usually located\n> relative to the project directory as .git/hooks/$HOOK. The search for\n> this will generally fail as that relative path will (probably) not exist\n> on any directory in $PATH. This means that git hooks in general now fail\n> to run. Considerable mayhem could occur should a directory on $PATH be\n> git controlled. If such a directory includes .git/hooks/$HOOK, that\n> repository's $HOOK will be substituted for the one in the current\n> project, with unknown consequences.\n>\n> This lookup failure also occurs in worktrees linked to a remote .git\n> directory using git-new-workdir. However, a worktree using a .git file\n> pointing to a separate git directory apparently avoids this: in that\n> case the hook command is resolved to an absolute path before being\n> passed down to the code introduced in aae9560a.\n>\n> Fix this by replacing the test for an \"absolute\" pathname to a check for\n> a command name having more than one pathname component. This limits the\n> search and absolute pathname resolution to bare commands. The new test\n> uses tcl's \"file split\" command. Experiments on Linux and Windows, using\n> tclsh, show that command names with relative and absolute paths always\n> give at least two components, while a bare command gives only one.\n>\n> \t Linux: puts [file split {foo}] ==> foo\n> \t Linux: puts [file split {/foo}] ==> / foo\n> \t Linux: puts [file split {.git/foo}] ==> .git foo\n> \t Windows: puts [file split {foo}] ==> foo\n> \t Windows: puts [file split {c:\\foo}] ==> c:/ foo\n> \t Windows: puts [file split {.git\\foo}] ==> .git foo\n>\n> The above results show the new test limits search and replacement\n> to bare commands on both Linux and Windows.\n\nSounds good. FWIW I ran a couple experiments here, too:\n\n\t% file pathtype \"C:/foo\"\n\tabsolute\n\t% file pathtype \".git/hooks\"\n\trelative\n\t% file pathtype \".git\\\\hooks\"\n\trelative\n\t% file pathtype \"/foo\"\n\tvolumerelative\n\t% file pathtype \"foo\"\n\trelative\n\nThe problem, therefore, is that `file pathtype` does not discern between a\nbare file name and a relative path. The proposed patch looks correct to\nme.\n\nThank you,\nJohannes\n\n>\n> Signed-off-by: Mark Levedahl \n> ---\n> git-gui.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-gui.sh b/git-gui.sh\n> index 8bc8892..8603437 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {\n> \tset i $from_index\n> \twhile {$i < [llength $command_line]} {\n> \t\tset cmd [lindex $command_line $i]\n> -\t\tif {[file pathtype $cmd] ne \"absolute\"} {\n> +\t\tif {[llength [file split $cmd]] < 2} {\n> \t\t\tset fullpath [_which $cmd]\n> \t\t\tif {$fullpath eq \"\"} {\n> \t\t\t\tthrow {NOT-FOUND} \"$cmd not found in PATH\"\n> --\n> 2.41.0.99.19\n>\n>\n"},{"id":"481953","messageId":"a6998d64-32a7-80b6-f75c-d983ac6130dd@gmx.de","threadId":"60234","inReplyTo":"20230917192431.101775-1-mlevedahl@gmail.com","subject":"Re: [PATCH] git-gui - use git-hook, honor core.hooksPath","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-09-18T15:27:44Z","receivedAt":"2023-09-18T15:46:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Mark,\n\nOn Sun, 17 Sep 2023, Mark Levedahl wrote:\n\n> git-gui currently runs some hooks directly using its own code written\n> before 2010, long predating git v2.9 that added the core.hooksPath\n> configuration to override the assumed location at $GIT_DIR/hooks. Thus,\n> git-gui looks for and runs hooks including prepare-commit-msg,\n> commit-msg, pre-commit, post-commit, and post-checkout from\n> $GIT_DIR/hooks, regardless of configuration. Commands (e.g., git-merge)\n> that git-gui invokes directly do honor core.hooksPath, meaning the\n> overall behaviour is inconsistent.\n>\n> Furthermore, since v2.36 git exposes its hook exection machinery via\n> git-hook run, eliminating the need for others to maintain code\n> duplicating that functionality. Using git-hook will both fix git-gui's\n> current issues on hook configuration and (presumably) reduce the\n> maintenance burden going forward. So, teach git-gui to use git-hook.\n>\n> Signed-off-by: Mark Levedahl \n> ---\n> git-gui.sh | 27 ++-------------------------\n> 1 file changed, 2 insertions(+), 25 deletions(-)\n>\n> diff --git a/git-gui.sh b/git-gui.sh\n> index 8603437..3e5907a 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -661,31 +661,8 @@ proc git_write {args} {\n> }\n>\n> proc githook_read {hook_name args} {\n> -\tset pchook [gitdir hooks $hook_name]\n> -\tlappend args 2>@1\n> -\n> -\t# On Windows [file executable] might lie so we need to ask\n> -\t# the shell if the hook is executable. Yes that's annoying.\n> -\t#\n> -\tif {[is_Windows]} {\n> -\t\tupvar #0 _sh interp\n> -\t\tif {![info exists interp]} {\n> -\t\t\tset interp [_which sh]\n> -\t\t}\n> -\t\tif {$interp eq {}} {\n> -\t\t\terror \"hook execution requires sh (not in PATH)\"\n> -\t\t}\n> -\n> -\t\tset scr {if test -x \"$1\";then exec \"$@\";fi}\n> -\t\tset sh_c [list $interp -c $scr $interp $pchook]\n> -\t\treturn [_open_stdout_stderr [concat $sh_c $args]]\n> -\t}\n> -\n> -\tif {[file executable $pchook]} {\n> -\t\treturn [_open_stdout_stderr [concat [list $pchook] $args]]\n> -\t}\n> -\n> -\treturn {}\n> +\tset cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]\n> +\treturn [_open_stdout_stderr $cmd]\n\nThis looks so much nicer than the original code.\n\nThank you,\nJohannes\n\n> }\n>\n> proc kill_file_process {fd} {\n> --\n> 2.41.0.99.19\n>\n>\n"},{"id":"481954","messageId":"xmqqpm2fmq2d.fsf@gitster.g","threadId":"60234","inReplyTo":"a6998d64-32a7-80b6-f75c-d983ac6130dd@gmx.de","subject":"Re: [PATCH] git-gui - use git-hook, honor core.hooksPath","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-18T15:58:02Z","receivedAt":"2023-09-18T16:04:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> +\tset cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]\n>> +\treturn [_open_stdout_stderr $cmd]\n>\n> This looks so much nicer than the original code.\n>\n> Thank you,\n> Johannes\n\nYup, looking good.\n"},{"id":"481955","messageId":"xmqqjzsnmpr0.fsf@gitster.g","threadId":"60234","inReplyTo":"260b62dc-d45b-58fa-85c4-ffbd981a1c84@gmx.de","subject":"Re: [PATCH v2] git-gui - re-enable use of hook scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-18T16:04:51Z","receivedAt":"2023-09-18T16:06:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Sounds good. FWIW I ran a couple experiments here, too:\n>\n> \t% file pathtype \"C:/foo\"\n> \tabsolute\n> \t% file pathtype \".git/hooks\"\n> \trelative\n> \t% file pathtype \".git\\\\hooks\"\n> \trelative\n> \t% file pathtype \"/foo\"\n> \tvolumerelative\n> \t% file pathtype \"foo\"\n> \trelative\n>\n> The problem, therefore, is that `file pathtype` does not discern between a\n> bare file name and a relative path. The proposed patch looks correct to\n> me.\n>\n> Thank you,\n> Johannes\n\nYup, the other \"run hooks in a more modern way using 'git hook'\"\npatch is the right solution for the immediate breakage, but it still\ncannot remove this sanitize_command_line proc as we have other users\nand use cases where we want to use the sanitized $PATH search, so\nthis fix is still needed.\n\nThanks for a quick review on both patches.\n"},{"id":"481960","messageId":"a4765b59-1953-695b-4f5e-686bef0a3a50@gmail.com","threadId":"60234","inReplyTo":"xmqqpm2fmq2d.fsf@gitster.g","subject":"Re: [PATCH] git-gui - use git-hook, honor core.hooksPath","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-18T16:25:02Z","receivedAt":"2023-09-18T16:27:45Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"\nOn 9/18/23 11:58, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n>\n>>> +\tset cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]\n>>> +\treturn [_open_stdout_stderr $cmd]\n>> This looks so much nicer than the original code.\n>>\n>> Thank you,\n>> Johannes\n> Yup, looking good.\n\nThanks. BTW, my commit message at \"Furthermore, since v2.36 git exposes \nits hook exection machinery via\" needs\n\n     s/exection/execution/\n\nShould I resend?\n\nMark\n\n"},{"id":"481966","messageId":"xmqqa5tjl65x.fsf@gitster.g","threadId":"60234","inReplyTo":"a4765b59-1953-695b-4f5e-686bef0a3a50@gmail.com","subject":"Re: [PATCH] git-gui - use git-hook, honor core.hooksPath","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-18T17:53:14Z","receivedAt":"2023-09-18T17:53:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> On 9/18/23 11:58, Junio C Hamano wrote:\n>> Johannes Schindelin writes:\n>>\n>>>> +\tset cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]\n>>>> +\treturn [_open_stdout_stderr $cmd]\n>>> This looks so much nicer than the original code.\n>>>\n>>> Thank you,\n>>> Johannes\n>> Yup, looking good.\n>\n> Thanks. BTW, my commit message at \"Furthermore, since v2.36 git\n> exposes its hook exection machinery via\" needs\n>\n>     s/exection/execution/\n>\n> Should I resend?\n\nNah, I'll just fix it up locally before merging.\n"},{"id":"482062","messageId":"mafs01qetq9kk.fsf@yadavpratyush.com","threadId":"60234","inReplyTo":"20230917192431.101775-1-mlevedahl@gmail.com","subject":"Re: [PATCH] git-gui - use git-hook, honor core.hooksPath","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2023-09-20T13:05:15Z","receivedAt":"2023-09-20T13:05:22Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"Hi,\n\nThanks for the patch.\n\nOn Sun, Sep 17 2023, Mark Levedahl wrote:\n\n> git-gui currently runs some hooks directly using its own code written\n> before 2010, long predating git v2.9 that added the core.hooksPath\n> configuration to override the assumed location at $GIT_DIR/hooks. Thus,\n> git-gui looks for and runs hooks including prepare-commit-msg,\n> commit-msg, pre-commit, post-commit, and post-checkout from\n> $GIT_DIR/hooks, regardless of configuration. Commands (e.g., git-merge)\n> that git-gui invokes directly do honor core.hooksPath, meaning the\n> overall behaviour is inconsistent.\n>\n> Furthermore, since v2.36 git exposes its hook exection machinery via\n> git-hook run, eliminating the need for others to maintain code\n> duplicating that functionality. Using git-hook will both fix git-gui's\n> current issues on hook configuration and (presumably) reduce the\n> maintenance burden going forward. So, teach git-gui to use git-hook.\n\nIn the past, git-gui has tried to keep backward compatibility with all\nversions of Git, not just the latest ones. v2.36 is relatively new and\nthis code would not work for anyone using an older version of Git.\n\nI have largely followed this practice for all the code I have written\nbut I am not sure if it is a good idea to insist on it -- especially if\nit would end up adding some more complexity. I would be interested to\nhear what other people think about this.\n\nJunio, I was under the impression that I would keep maintaining the tree\nuntil we found a replacement maintainer. If you are okay with being the\ninterim maintainer, that sounds good to me. Let me know what works best.\n\nI have applied another patch since my last pull request. So I can apply\nthis one, send you a new one and sync our trees.\n\n>\n> Signed-off-by: Mark Levedahl \n> ---\n> git-gui.sh | 27 ++-------------------------\n> 1 file changed, 2 insertions(+), 25 deletions(-)\n>\n> diff --git a/git-gui.sh b/git-gui.sh\n> index 8603437..3e5907a 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -661,31 +661,8 @@ proc git_write {args} {\n> }\n> \n> proc githook_read {hook_name args} {\n> -\tset pchook [gitdir hooks $hook_name]\n> -\tlappend args 2>@1\n> -\n> -\t# On Windows [file executable] might lie so we need to ask\n> -\t# the shell if the hook is executable. Yes that's annoying.\n> -\t#\n> -\tif {[is_Windows]} {\n> -\t\tupvar #0 _sh interp\n> -\t\tif {![info exists interp]} {\n> -\t\t\tset interp [_which sh]\n> -\t\t}\n> -\t\tif {$interp eq {}} {\n> -\t\t\terror \"hook execution requires sh (not in PATH)\"\n> -\t\t}\n> -\n> -\t\tset scr {if test -x \"$1\";then exec \"$@\";fi}\n> -\t\tset sh_c [list $interp -c $scr $interp $pchook]\n> -\t\treturn [_open_stdout_stderr [concat $sh_c $args]]\n> -\t}\n> -\n> -\tif {[file executable $pchook]} {\n> -\t\treturn [_open_stdout_stderr [concat [list $pchook] $args]]\n> -\t}\n> -\n> -\treturn {}\n> +\tset cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1]\n> +\treturn [_open_stdout_stderr $cmd]\n\nLGTM, other than my concerns with backward compatibility.\n\n> }\n> \n> proc kill_file_process {fd} {\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"482065","messageId":"mafs0wmwlotya.fsf@yadavpratyush.com","threadId":"60234","inReplyTo":"20230916210131.78593-1-mlevedahl@gmail.com","subject":"Re: [PATCH v2] git-gui - re-enable use of hook scripts","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2023-09-20T13:27:57Z","receivedAt":"2023-09-20T13:28:10Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"\nHi,\n\nOn Sat, Sep 16 2023, Mark Levedahl wrote:\n\n> Earlier, commit aae9560a introduced search in $PATH to find executables\n> before running them, avoiding an issue where on Windows a same named\n> file in the current directory can be executed in preference to anything\n> in a directory in $PATH. This search is intended to find an absolute\n> path for a bare executable ( e.g, a function \"foo\") by finding the first\n> instance of \"foo\" in a directory given in $PATH, and this search works\n> correctly. The search is explicitly avoided for an executable named\n> with an absolute path (e.g., /bin/sh), and that works as well.\n>\n> Unfortunately, the search is also applied to commands named with a\n> relative path. A hook script (or executable) $HOOK is usually located\n> relative to the project directory as .git/hooks/$HOOK. The search for\n> this will generally fail as that relative path will (probably) not exist\n> on any directory in $PATH. This means that git hooks in general now fail\n> to run. Considerable mayhem could occur should a directory on $PATH be\n> git controlled. If such a directory includes .git/hooks/$HOOK, that\n> repository's $HOOK will be substituted for the one in the current\n> project, with unknown consequences.\n>\n> This lookup failure also occurs in worktrees linked to a remote .git\n> directory using git-new-workdir. However, a worktree using a .git file\n> pointing to a separate git directory apparently avoids this: in that\n> case the hook command is resolved to an absolute path before being\n> passed down to the code introduced in aae9560a.\n>\n> Fix this by replacing the test for an \"absolute\" pathname to a check for\n> a command name having more than one pathname component. This limits the\n> search and absolute pathname resolution to bare commands. The new test\n> uses tcl's \"file split\" command. Experiments on Linux and Windows, using\n> tclsh, show that command names with relative and absolute paths always\n> give at least two components, while a bare command gives only one.\n>\n> \t Linux: puts [file split {foo}] ==> foo\n> \t Linux: puts [file split {/foo}] ==> / foo\n> \t Linux: puts [file split {.git/foo}] ==> .git foo\n> \t Windows: puts [file split {foo}] ==> foo\n> \t Windows: puts [file split {c:\\foo}] ==> c:/ foo\n> \t Windows: puts [file split {.git\\foo}] ==> .git foo\n>\n> The above results show the new test limits search and replacement\n> to bare commands on both Linux and Windows.\n>\n> Signed-off-by: Mark Levedahl \n\nLooks good. Thanks.\n\nReviewed-by: Pratyush Yadav \n\n> ---\n> git-gui.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-gui.sh b/git-gui.sh\n> index 8bc8892..8603437 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -118,7 +118,7 @@ proc sanitize_command_line {command_line from_index} {\n> \tset i $from_index\n> \twhile {$i < [llength $command_line]} {\n> \t\tset cmd [lindex $command_line $i]\n> -\t\tif {[file pathtype $cmd] ne \"absolute\"} {\n> +\t\tif {[llength [file split $cmd]] < 2} {\n> \t\t\tset fullpath [_which $cmd]\n> \t\t\tif {$fullpath eq \"\"} {\n> \t\t\t\tthrow {NOT-FOUND} \"$cmd not found in PATH\"\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"482072","messageId":"573c6dc5-2102-cb65-8f71-dea37fff0c9b@gmail.com","threadId":"60234","inReplyTo":"mafs01qetq9kk.fsf@yadavpratyush.com","subject":"Re: [PATCH] git-gui - use git-hook, honor core.hooksPath","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2023-09-20T15:30:08Z","receivedAt":"2023-09-20T15:30:15Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"\nOn 9/20/23 09:05, Pratyush Yadav wrote:\n> In the past, git-gui has tried to keep backward compatibility with all\n> versions of Git, not just the latest ones. v2.36 is relatively new and\n> this code would not work for anyone using an older version of Git.\n>\n> I have largely followed this practice for all the code I have written\n> but I am not sure if it is a good idea to insist on it -- especially if\n> it would end up adding some more complexity. I would be interested to\n> hear what other people think about this.\n>\nI am not aware of any distribution (Linux, g4w, Mac) shipping anything \nexcept the git-gui in Junio's tree, which is specific to the git-core \nversion, and the git-gui packages require (or are a part of) the same \nversion git-core package: no cross-version compatibility of git \ncomponents is assumed. Certainly, folks rolling their own can pull from \nupstream git-gui, but they take the risk of incompatibility with an \noutdated git. Other tools in Junio's tree have already made the switch \nto git-hook (send-email, git-p4) even though they are usually packaged \nseparately from git-core, but also version locked to matching git-core.\n\nUpdating git-gui's hook execution to match git internals would be more \ncomplex than what I implemented or what was there before.  For instance, \nI never looked at what git-hook's g4w compatibility code uses to test if \na hook is present and executable, it wouldn't surprise me to find \ngit-gui was missing something there, but who wants to bother? Also, the \ncommit language surrounding addition of git-hook is strongly suggestive \nof other changes in configuration coming, meaning more changes to hook \nexecution code would be needed that are avoided by using git-hook. Note: \nI have one more patch to send, removing yet another work-around for \nearly Cygwin tcl/tk, as more evidence of how many years it takes to \nclean some of this stuff out and the difficulty of keeping git-gui up to \ndate.\n\nI had considered the above when creating the patch, and I believe what I \ndid is the right approach.\n\nMark\n\n"},{"id":"482076","messageId":"xmqqttroalqm.fsf@gitster.g","threadId":"60234","inReplyTo":"mafs01qetq9kk.fsf@yadavpratyush.com","subject":"Re: [PATCH] git-gui - use git-hook, honor core.hooksPath","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-20T15:49:05Z","receivedAt":"2023-09-20T15:49:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pratyush Yadav writes:\n\n> In the past, git-gui has tried to keep backward compatibility with all\n> versions of Git, not just the latest ones. v2.36 is relatively new and\n> this code would not work for anyone using an older version of Git.\n>\n> I have largely followed this practice for all the code I have written\n> but I am not sure if it is a good idea to insist on it -- especially if\n> it would end up adding some more complexity. I would be interested to\n> hear what other people think about this.\n\nGood point.\n\n> Junio, I was under the impression that I would keep maintaining the tree\n> until we found a replacement maintainer. If you are okay with being the\n> interim maintainer, that sounds good to me. Let me know what works best.\n\nI am actually not OK ;-).\n\nI prefer to see somebody who does use git-gui, or at least somebody\nwho uses Git in a graphical environment in their daily work, to be\nmaintaining it. I am disqualified on both counts.\n\n> I have applied another patch since my last pull request. So I can apply\n> this one, send you a new one and sync our trees.\n\nOK. I'll drop the copy I have on my end when it happens, then.\n\nThanks.\n\n"},{"id":"482079","messageId":"xmqq7cokaij2.fsf@gitster.g","threadId":"60234","inReplyTo":"573c6dc5-2102-cb65-8f71-dea37fff0c9b@gmail.com","subject":"Re: [PATCH] git-gui - use git-hook, honor core.hooksPath","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-20T16:58:25Z","receivedAt":"2023-09-20T16:58:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> Certainly, folks rolling their own can pull\n> from upstream git-gui, but they take the risk of incompatibility with\n> an outdated git. Other tools in Junio's tree have already made the\n> switch to git-hook (send-email, git-p4) even though they are usually\n> packaged separately from git-core, but also version locked to matching\n> git-core.\n\nThe cross-version compatibility story is the same for \"gitk\" (which\nI believe \"git-gui\" took the \"do not too deeply depend on the\nmatching version of git\" mantra from). I can understand the desire\nand being able to aim for wider compatibility may be an advantage\nfor these tools that are not tightly bundled with the rest of the\nsystem. It allowed them to evolve without waiting for Git to catch\nup, for example.\n\nBut at this point in their history where these tools are very\nmature, it may be fair to say that the cross-version compatibility\nis becoming a lost cause.\n"}]}