git-gui: Consolidate hook execution code into a single function

The code we use to test if a hook is executable or not differs on
Cygwin from the normal POSIX case.  Rather then repeating that for
all three hooks we call in our commit code path we can place the
common logic into a global procedure and invoke it when necessary.

This also lets us get rid of the ugly "|& cat" we were using before
as we can now rely on the Tcl 8.4 feature of "2>@1" or fallback to
the "|& cat" when necessary.

The post-commit hook is now run through the same API, but its outcome
does not influence the commit status.  As a result we now show any of
the errors from the post-commit hook in a dialog window, instead of on
the user's tty that was used to launch git-gui.  This resolves a long
standing bug related to not getting errors out of the post-commit hook
when launched under git-gui.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

diff --git a/git-gui.sh b/git-gui.sh
index fcb2ab2fb7447ec2c3fec625afea292fc8a59e92..f42e461fd42ad37075d433f3509d6bd415a0b97a 100755 (executable)
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -438,6 +438,34 @@ proc git_write {args} {
        return [open [concat $opt $cmdp $args] w]
 }
 
+proc githook_read {hook_name args} {
+       set pchook [gitdir hooks $hook_name]
+       lappend args 2>@1
+
+       # On Cygwin [file executable] might lie so we need to ask
+       # the shell if the hook is executable.  Yes that's annoying.
+       #
+       if {[is_Cygwin]} {
+               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 {}
+}
+
 proc sq {value} {
        regsub -all ' $value "'\\''" value
        return "'$value'"

diff --git a/lib/commit.tcl b/lib/commit.tcl
index 73e18bf9825d7ce7c5dfab4a1a35082333864bd0..947b201c328eef044cfe21b463e8228031f6f8cd 100644 (file)
--- a/lib/commit.tcl
+++ b/lib/commit.tcl
@@ -212,26 +212,14 @@ A good commit message has the following format:
 
        # -- Run the pre-commit hook.
        #
-       set pchook [gitdir hooks pre-commit]
-
-       # On Cygwin [file executable] might lie so we need to ask
-       # the shell if the hook is executable.  Yes that's annoying.
-       #
-       if {[is_Cygwin] && [file isfile $pchook]} {
-               set pchook [list sh -c [concat \
-                       "if test -x \"$pchook\";" \
-                       "then exec \"$pchook\" 2>&1;" \
-                       "fi"]]
-       } elseif {[file executable $pchook]} {
-               set pchook [list $pchook |& cat]
-       } else {
+       set fd_ph [githook_read pre-commit]
+       if {$fd_ph eq {}} {
                commit_commitmsg $curHEAD $msg_p
                return
        }
 
        ui_status {Calling pre-commit hook...}
        set pch_error {}
-       set fd_ph [open "| $pchook" r]
        fconfigure $fd_ph -blocking 0 -translation binary -eofchar {}
        fileevent $fd_ph readable \
                [list commit_prehook_wait $fd_ph $curHEAD $msg_p]
@@ -262,26 +250,14 @@ proc commit_commitmsg {curHEAD msg_p} {
 
        # -- Run the commit-msg hook.
        #
-       set pchook [gitdir hooks commit-msg]
-
-       # On Cygwin [file executable] might lie so we need to ask
-       # the shell if the hook is executable.  Yes that's annoying.
-       #
-       if {[is_Cygwin] && [file isfile $pchook]} {
-               set pchook [list sh -c [concat \
-                       "if test -x \"$pchook\";" \
-                       "then exec \"$pchook\" \"$msg_p\" 2>&1;" \
-                       "fi"]]
-       } elseif {[file executable $pchook]} {
-               set pchook [list $pchook $msg_p |& cat]
-       } else {
+       set fd_ph [githook_read commit-msg $msg_p]
+       if {$fd_ph eq {}} {
                commit_writetree $curHEAD $msg_p
                return
        }
 
        ui_status {Calling commit-msg hook...}
        set pch_error {}
-       set fd_ph [open "| $pchook" r]
        fconfigure $fd_ph -blocking 0 -translation binary -eofchar {}
        fileevent $fd_ph readable \
                [list commit_commitmsg_wait $fd_ph $curHEAD $msg_p]
@@ -415,17 +391,13 @@ A rescan will be automatically started now.
 
        # -- Run the post-commit hook.
        #
-       set pchook [gitdir hooks post-commit]
-       if {[is_Cygwin] && [file isfile $pchook]} {
-               set pchook [list sh -c [concat \
-                       "if test -x \"$pchook\";" \
-                       "then exec \"$pchook\";" \
-                       "fi"]]
-       } elseif {![file executable $pchook]} {
-               set pchook {}
-       }
-       if {$pchook ne {}} {
-               catch {exec $pchook &}
+       set fd_ph [githook_read post-commit]
+       if {$fd_ph ne {}} {
+               upvar #0 pch_error$cmt_id pc_err
+               set pc_err {}
+               fconfigure $fd_ph -blocking 0 -translation binary -eofchar {}
+               fileevent $fd_ph readable \
+                       [list commit_postcommit_wait $fd_ph $cmt_id]
        }
 
        $ui_comm delete 0.0 end
@@ -481,3 +453,18 @@ A rescan will be automatically started now.
        reshow_diff
        ui_status [mc "Created commit %s: %s" [string range $cmt_id 0 7] $subject]
 }
+
+proc commit_postcommit_wait {fd_ph cmt_id} {
+       upvar #0 pch_error$cmt_id pch_error
+
+       append pch_error [read $fd_ph]
+       fconfigure $fd_ph -blocking 1
+       if {[eof $fd_ph]} {
+               if {[catch {close $fd_ph}]} {
+                       hook_failed_popup post-commit $pch_error 0
+               }
+               unset pch_error
+               return
+       }
+       fconfigure $fd_ph -blocking 0
+}

diff --git a/lib/error.tcl b/lib/error.tcl
index 712d21755317d879ea89996edc12e58c3df411d3..0fdd7531dac07e68315cebc2efa87e99947821b6 100644 (file)
--- a/lib/error.tcl
+++ b/lib/error.tcl
@@ -62,7 +62,7 @@ proc ask_popup {msg} {
        eval $cmd
 }
 
-proc hook_failed_popup {hook msg} {
+proc hook_failed_popup {hook msg {is_fatal 1}} {
        set w .hookfail
        toplevel $w
 
@@ -77,14 +77,16 @@ proc hook_failed_popup {hook msg} {
                -width 80 -height 10 \
                -font font_diff \
                -yscrollcommand [list $w.m.sby set]
-       label $w.m.l2 \
-               -text [mc "You must correct the above errors before committing."] \
-               -anchor w \
-               -justify left \
-               -font font_uibold
        scrollbar $w.m.sby -command [list $w.m.t yview]
        pack $w.m.l1 -side top -fill x
-       pack $w.m.l2 -side bottom -fill x
+       if {$is_fatal} {
+               label $w.m.l2 \
+                       -text [mc "You must correct the above errors before committing."] \
+                       -anchor w \
+                       -justify left \
+                       -font font_uibold
+               pack $w.m.l2 -side bottom -fill x
+       }
        pack $w.m.sby -side right -fill y
        pack $w.m.t -side left -fill both -expand 1
        pack $w.m -side top -fill both -expand 1 -padx 5 -pady 10