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

From: Pratyush Yadav <me@yadavpratyush.com>
Date: Sep 20, 2023, 13:05 UTC
Message-ID: <mafs01qetq9kk.fsf@yadavpratyush.com>
In-Reply-To: <20230917192431.101775-1-mlevedahl@gmail.com>

Hi,

Thanks for the patch.

On Sun, Sep 17 2023, Mark Levedahl wrote:

Show 14 quoted lines

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

Show 41 quoted lines

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