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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 16, 2023, 17:28 UTC
Message-ID: <xmqqy1h6auy7.fsf@gitster.g>
In-Reply-To: <20230916003516.51053-1-mlevedahl@gmail.com>

Mark Levedahl <mlevedahl@gmail.com> writes:

Show 18 quoted lines

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

Show 16 quoted lines

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