| Message ID | 20230917192431.101775-1-mlevedahl@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0730a5a3a5e69e4b5fa0fbf6edd7fcbd7a08c992 |
| Headers | show |
| Series | git-gui - use git-hook, honor core.hooksPath | expand |
Hi Mark, On Sun, 17 Sep 2023, Mark Levedahl wrote: > 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. > > 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] This looks so much nicer than the original code. Thank you, Johannes > } > > proc kill_file_process {fd} { > -- > 2.41.0.99.19 > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> + set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1] >> + return [_open_stdout_stderr $cmd] > > This looks so much nicer than the original code. > > Thank you, > Johannes Yup, looking good.
On 9/18/23 11:58, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >>> + set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1] >>> + return [_open_stdout_stderr $cmd] >> This looks so much nicer than the original code. >> >> Thank you, >> Johannes > Yup, looking good. Thanks. BTW, my commit message at "Furthermore, since v2.36 git exposes its hook exection machinery via" needs s/exection/execution/ Should I resend? Mark
Mark Levedahl <mlevedahl@gmail.com> writes: > On 9/18/23 11:58, Junio C Hamano wrote: >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> >>>> + set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1] >>>> + return [_open_stdout_stderr $cmd] >>> This looks so much nicer than the original code. >>> >>> Thank you, >>> Johannes >> Yup, looking good. > > Thanks. BTW, my commit message at "Furthermore, since v2.36 git > exposes its hook exection machinery via" needs > > s/exection/execution/ > > Should I resend? Nah, I'll just fix it up locally before merging.
Hi, Thanks for the patch. On Sun, Sep 17 2023, Mark Levedahl wrote: > 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. > > 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} {
On 9/20/23 09:05, Pratyush Yadav wrote: > 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. > I am not aware of any distribution (Linux, g4w, Mac) shipping anything except the git-gui in Junio's tree, which is specific to the git-core version, and the git-gui packages require (or are a part of) the same version git-core package: no cross-version compatibility of git components is assumed. Certainly, folks rolling their own can pull from upstream git-gui, but they take the risk of incompatibility with an outdated git. Other tools in Junio's tree have already made the switch to git-hook (send-email, git-p4) even though they are usually packaged separately from git-core, but also version locked to matching git-core. Updating git-gui's hook execution to match git internals would be more complex than what I implemented or what was there before. For instance, I never looked at what git-hook's g4w compatibility code uses to test if a hook is present and executable, it wouldn't surprise me to find git-gui was missing something there, but who wants to bother? Also, the commit language surrounding addition of git-hook is strongly suggestive of other changes in configuration coming, meaning more changes to hook execution code would be needed that are avoided by using git-hook. Note: I have one more patch to send, removing yet another work-around for early Cygwin tcl/tk, as more evidence of how many years it takes to clean some of this stuff out and the difficulty of keeping git-gui up to date. I had considered the above when creating the patch, and I believe what I did is the right approach. Mark
Pratyush Yadav <me@yadavpratyush.com> writes: > 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. Good point. > 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 am actually not OK ;-). I prefer to see somebody who does use git-gui, or at least somebody who uses Git in a graphical environment in their daily work, to be maintaining it. I am disqualified on both counts. > 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. OK. I'll drop the copy I have on my end when it happens, then. Thanks.
Mark Levedahl <mlevedahl@gmail.com> writes: > Certainly, folks rolling their own can pull > from upstream git-gui, but they take the risk of incompatibility with > an outdated git. Other tools in Junio's tree have already made the > switch to git-hook (send-email, git-p4) even though they are usually > packaged separately from git-core, but also version locked to matching > git-core. The cross-version compatibility story is the same for "gitk" (which I believe "git-gui" took the "do not too deeply depend on the matching version of git" mantra from). I can understand the desire and being able to aim for wider compatibility may be an advantage for these tools that are not tightly bundled with the rest of the system. It allowed them to evolve without waiting for Git to catch up, for example. But at this point in their history where these tools are very mature, it may be fair to say that the cross-version compatibility is becoming a lost cause.
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] } proc kill_file_process {fd} {
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. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- git-gui.sh | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-)