Mark Levedahl <mlevedahl@xxxxxxxxx> 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@xxxxxxxxx>
> ---
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"
