* Re: [PATCH v2] git-gui - re-enable use of hook scripts
2023-09-16 21:01 ` [PATCH v2] " Mark Levedahl
@ 2023-09-16 21:51 ` Junio C Hamano
2023-09-17 19:22 ` Mark Levedahl
2023-09-18 15:26 ` [PATCH v2] git-gui - re-enable use of hook scripts Johannes Schindelin
2023-09-20 13:27 ` Pratyush Yadav
2 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2023-09-16 21:51 UTC (permalink / raw)
To: Mark Levedahl; +Cc: johannes.schindelin, me, git
Mark Levedahl <mlevedahl@gmail.com> writes:
> uses tcl's "file split" command. Experiments on Linux and Windows, using
> tclsh, show that command names with relative and absolute paths always
> give at least two components, while a bare command gives only one.
>
> Linux: puts [file split {foo}] ==> foo
> Linux: puts [file split {/foo}] ==> / foo
> Linux: puts [file split {.git/foo}] ==> .git foo
> Windows: puts [file split {foo}] ==> foo
> Windows: puts [file split {c:\foo}] ==> c:/ foo
> Windows: puts [file split {.git\foo}] ==> .git foo
;-) Nice documentation of what you found out.
> 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"
Nice. Now we need to find a replacement maintainer for Git-gui ;-)
In the meantime, I can queue this patch on top of what I updated
git-gui part the last time with and merge it in.
Thanks.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2] git-gui - re-enable use of hook scripts
2023-09-16 21:51 ` Junio C Hamano
@ 2023-09-17 19:22 ` Mark Levedahl
2023-09-17 19:24 ` [PATCH] git-gui - use git-hook, honor core.hooksPath Mark Levedahl
0 siblings, 1 reply; 25+ messages in thread
From: Mark Levedahl @ 2023-09-17 19:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: johannes.schindelin, me, git
On 9/16/23 17:51, Junio C Hamano wrote:
>
> Nice. Now we need to find a replacement maintainer for Git-gui ;-)
> In the meantime, I can queue this patch on top of what I updated
> git-gui part the last time with and merge it in.
>
> Thanks.
Thank you for help on this too. I retired some time ago, and stopped
using git much a decade ago. My popping up on the list was inspired by
cleaning out an old laptop and finding some old patches I thought would
be useful, especially as I'd helped Shawn create some of that old
git-gui/Cygwin code. My interest is unlikely to endure so I'm definitely
not a good candidate to maintain git-gui.
On this hook execution problem, looking further, I find using git-hook
run will fix some other issues in git-gui's hook handling, and that
would actually also patch around the problem we just fixed. So, another
patch follows, the commit message presumes the one fixing relative path
search remains. I would suggest keeping the one fixing the relative path
search regardless.
Mark
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] git-gui - use git-hook, honor core.hooksPath
2023-09-17 19:22 ` Mark Levedahl
@ 2023-09-17 19:24 ` Mark Levedahl
2023-09-18 15:27 ` Johannes Schindelin
2023-09-20 13:05 ` Pratyush Yadav
0 siblings, 2 replies; 25+ messages in thread
From: Mark Levedahl @ 2023-09-17 19:24 UTC (permalink / raw)
To: gitster, johannes.schindelin, me, git; +Cc: Mark Levedahl
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]
}
proc kill_file_process {fd} {
--
2.41.0.99.19
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
2023-09-17 19:24 ` [PATCH] git-gui - use git-hook, honor core.hooksPath Mark Levedahl
@ 2023-09-18 15:27 ` Johannes Schindelin
2023-09-18 15:58 ` Junio C Hamano
2023-09-20 13:05 ` Pratyush Yadav
1 sibling, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2023-09-18 15:27 UTC (permalink / raw)
To: Mark Levedahl; +Cc: gitster, me, git
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
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
2023-09-18 15:27 ` Johannes Schindelin
@ 2023-09-18 15:58 ` Junio C Hamano
2023-09-18 16:25 ` Mark Levedahl
0 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2023-09-18 15:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Mark Levedahl, me, git
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
2023-09-18 15:58 ` Junio C Hamano
@ 2023-09-18 16:25 ` Mark Levedahl
2023-09-18 17:53 ` Junio C Hamano
0 siblings, 1 reply; 25+ messages in thread
From: Mark Levedahl @ 2023-09-18 16:25 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin; +Cc: me, git
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
2023-09-18 16:25 ` Mark Levedahl
@ 2023-09-18 17:53 ` Junio C Hamano
0 siblings, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2023-09-18 17:53 UTC (permalink / raw)
To: Mark Levedahl; +Cc: Johannes Schindelin, me, git
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
2023-09-17 19:24 ` [PATCH] git-gui - use git-hook, honor core.hooksPath Mark Levedahl
2023-09-18 15:27 ` Johannes Schindelin
@ 2023-09-20 13:05 ` Pratyush Yadav
2023-09-20 15:30 ` Mark Levedahl
2023-09-20 15:49 ` Junio C Hamano
1 sibling, 2 replies; 25+ messages in thread
From: Pratyush Yadav @ 2023-09-20 13:05 UTC (permalink / raw)
To: Mark Levedahl; +Cc: gitster, johannes.schindelin, me, git
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} {
--
Regards,
Pratyush Yadav
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
2023-09-20 13:05 ` Pratyush Yadav
@ 2023-09-20 15:30 ` Mark Levedahl
2023-09-20 16:58 ` Junio C Hamano
2023-09-20 15:49 ` Junio C Hamano
1 sibling, 1 reply; 25+ messages in thread
From: Mark Levedahl @ 2023-09-20 15:30 UTC (permalink / raw)
To: Pratyush Yadav; +Cc: gitster, johannes.schindelin, git
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
2023-09-20 15:30 ` Mark Levedahl
@ 2023-09-20 16:58 ` Junio C Hamano
0 siblings, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2023-09-20 16:58 UTC (permalink / raw)
To: Mark Levedahl; +Cc: Pratyush Yadav, johannes.schindelin, git
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] git-gui - use git-hook, honor core.hooksPath
2023-09-20 13:05 ` Pratyush Yadav
2023-09-20 15:30 ` Mark Levedahl
@ 2023-09-20 15:49 ` Junio C Hamano
1 sibling, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2023-09-20 15:49 UTC (permalink / raw)
To: Pratyush Yadav; +Cc: Mark Levedahl, johannes.schindelin, git
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] git-gui - re-enable use of hook scripts
2023-09-16 21:01 ` [PATCH v2] " Mark Levedahl
2023-09-16 21:51 ` Junio C Hamano
@ 2023-09-18 15:26 ` Johannes Schindelin
2023-09-18 16:04 ` Junio C Hamano
2023-09-20 13:27 ` Pratyush Yadav
2 siblings, 1 reply; 25+ messages in thread
From: Johannes Schindelin @ 2023-09-18 15:26 UTC (permalink / raw)
To: Mark Levedahl; +Cc: gitster, me, git
Hi,
On Sat, 16 Sep 2023, Mark Levedahl wrote:
> Earlier, 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
> in a directory in $PATH. This search is intended to find an absolute
> path for a bare executable ( e.g, a function "foo") by finding the first
> instance of "foo" in a directory given in $PATH, and this search works
> correctly. The search is explicitly avoided for an executable named
> with an absolute path (e.g., /bin/sh), and that works as well.
>
> Unfortunately, the search is also applied to commands named with a
> relative path. A hook script (or executable) $HOOK is usually located
> relative to the project directory as .git/hooks/$HOOK. The search for
> this will generally fail as that relative path will (probably) not exist
> on any directory in $PATH. This means that git hooks in general now fail
> to run. Considerable mayhem could occur should a directory on $PATH be
> git controlled. If such a directory includes .git/hooks/$HOOK, that
> repository's $HOOK will be substituted for the one in the current
> project, with unknown consequences.
>
> This lookup failure also occurs in worktrees linked to a remote .git
> directory using git-new-workdir. However, a worktree using a .git file
> pointing to a separate git directory apparently avoids this: in that
> case the hook command is resolved to an absolute path before being
> passed down to the code introduced in aae9560a.
>
> Fix this by replacing the test for an "absolute" pathname to a check for
> a command name having more than one pathname component. This limits the
> search and absolute pathname resolution to bare commands. The new test
> uses tcl's "file split" command. Experiments on Linux and Windows, using
> tclsh, show that command names with relative and absolute paths always
> give at least two components, while a bare command gives only one.
>
> Linux: puts [file split {foo}] ==> foo
> Linux: puts [file split {/foo}] ==> / foo
> Linux: puts [file split {.git/foo}] ==> .git foo
> Windows: puts [file split {foo}] ==> foo
> Windows: puts [file split {c:\foo}] ==> c:/ foo
> Windows: puts [file split {.git\foo}] ==> .git foo
>
> The above results show the new test limits search and replacement
> to bare commands on both Linux and Windows.
Sounds good. FWIW I ran a couple experiments here, too:
% file pathtype "C:/foo"
absolute
% file pathtype ".git/hooks"
relative
% file pathtype ".git\\hooks"
relative
% file pathtype "/foo"
volumerelative
% file pathtype "foo"
relative
The problem, therefore, is that `file pathtype` does not discern between a
bare file name and a relative path. The proposed patch looks correct to
me.
Thank you,
Johannes
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
> 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"
> --
> 2.41.0.99.19
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2] git-gui - re-enable use of hook scripts
2023-09-18 15:26 ` [PATCH v2] git-gui - re-enable use of hook scripts Johannes Schindelin
@ 2023-09-18 16:04 ` Junio C Hamano
0 siblings, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2023-09-18 16:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Mark Levedahl, me, git
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Sounds good. FWIW I ran a couple experiments here, too:
>
> % file pathtype "C:/foo"
> absolute
> % file pathtype ".git/hooks"
> relative
> % file pathtype ".git\\hooks"
> relative
> % file pathtype "/foo"
> volumerelative
> % file pathtype "foo"
> relative
>
> The problem, therefore, is that `file pathtype` does not discern between a
> bare file name and a relative path. The proposed patch looks correct to
> me.
>
> Thank you,
> Johannes
Yup, the other "run hooks in a more modern way using 'git hook'"
patch is the right solution for the immediate breakage, but it still
cannot remove this sanitize_command_line proc as we have other users
and use cases where we want to use the sanitized $PATH search, so
this fix is still needed.
Thanks for a quick review on both patches.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] git-gui - re-enable use of hook scripts
2023-09-16 21:01 ` [PATCH v2] " Mark Levedahl
2023-09-16 21:51 ` Junio C Hamano
2023-09-18 15:26 ` [PATCH v2] git-gui - re-enable use of hook scripts Johannes Schindelin
@ 2023-09-20 13:27 ` Pratyush Yadav
2 siblings, 0 replies; 25+ messages in thread
From: Pratyush Yadav @ 2023-09-20 13:27 UTC (permalink / raw)
To: Mark Levedahl; +Cc: gitster, johannes.schindelin, me, git
Hi,
On Sat, Sep 16 2023, Mark Levedahl wrote:
> Earlier, 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
> in a directory in $PATH. This search is intended to find an absolute
> path for a bare executable ( e.g, a function "foo") by finding the first
> instance of "foo" in a directory given in $PATH, and this search works
> correctly. The search is explicitly avoided for an executable named
> with an absolute path (e.g., /bin/sh), and that works as well.
>
> Unfortunately, the search is also applied to commands named with a
> relative path. A hook script (or executable) $HOOK is usually located
> relative to the project directory as .git/hooks/$HOOK. The search for
> this will generally fail as that relative path will (probably) not exist
> on any directory in $PATH. This means that git hooks in general now fail
> to run. Considerable mayhem could occur should a directory on $PATH be
> git controlled. If such a directory includes .git/hooks/$HOOK, that
> repository's $HOOK will be substituted for the one in the current
> project, with unknown consequences.
>
> This lookup failure also occurs in worktrees linked to a remote .git
> directory using git-new-workdir. However, a worktree using a .git file
> pointing to a separate git directory apparently avoids this: in that
> case the hook command is resolved to an absolute path before being
> passed down to the code introduced in aae9560a.
>
> Fix this by replacing the test for an "absolute" pathname to a check for
> a command name having more than one pathname component. This limits the
> search and absolute pathname resolution to bare commands. The new test
> uses tcl's "file split" command. Experiments on Linux and Windows, using
> tclsh, show that command names with relative and absolute paths always
> give at least two components, while a bare command gives only one.
>
> Linux: puts [file split {foo}] ==> foo
> Linux: puts [file split {/foo}] ==> / foo
> Linux: puts [file split {.git/foo}] ==> .git foo
> Windows: puts [file split {foo}] ==> foo
> Windows: puts [file split {c:\foo}] ==> c:/ foo
> Windows: puts [file split {.git\foo}] ==> .git foo
>
> The above results show the new test limits search and replacement
> to bare commands on both Linux and Windows.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
Looks good. Thanks.
Reviewed-by: Pratyush Yadav <me@yadavpratyush.com>
> ---
> 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"
--
Regards,
Pratyush Yadav
^ permalink raw reply [flat|nested] 25+ messages in thread