| Message ID | 20230916003516.51053-1-mlevedahl@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git-gui - re-enable use of hook scripts | expand |
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"
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"
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(-)