[PATCH] git-gui: run post-checkout hook on checkout

[PATCH] git-gui: run post-checkout hook on checkout

[Jens Lehmann]

git-gui is using "git-read-tree -u" for checkout which doesn't
invoke the post-checkout hook as a plain git-checkout would.
So git-gui must call the hook itself.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

It's been a while since i deeply touched tcl/tk code ... i basically
adapted the code for calling the post-commit hook here.

I renamed the former local variable "curHEAD" from the method
_start_checkout to a field named "old_hash" to be able to remember
the hash of the tree *before* the checkout started. This hash has
to be given as first argument to the post-checkout hook *after* the
checkout, where it is not the *current* HEAD anymore.


 git-gui/lib/checkout_op.tcl |   39 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/git-gui/lib/checkout_op.tcl b/git-gui/lib/checkout_op.tcl
index caca888..31863a5 100644
--- a/git-gui/lib/checkout_op.tcl
+++ b/git-gui/lib/checkout_op.tcl
@@ -9,6 +9,7 @@ field w_cons   {}; # embedded console window object
 field new_expr   ; # expression the user saw/thinks this is
 field new_hash   ; # commit SHA-1 we are switching to
 field new_ref    ; # ref we are updating/creating
+field old_hash   ; # commit SHA-1 that was checked when we started
 
 field parent_w      .; # window that started us
 field merge_type none; # type of merge to apply to existing branch
@@ -280,11 +281,11 @@ method _start_checkout {} {
 
 	# -- Our in memory state should match the repository.
 	#
-	repository_state curType curHEAD curMERGE_HEAD
+	repository_state curType old_hash curMERGE_HEAD
 	if {[string match amend* $commit_type]
 		&& $curType eq {normal}
-		&& $curHEAD eq $HEAD} {
-	} elseif {$commit_type ne $curType || $HEAD ne $curHEAD} {
+		&& $old_hash eq $HEAD} {
+	} elseif {$commit_type ne $curType || $HEAD ne $old_hash} {
 		info_popup [mc "Last scanned state does not match repository state.
 
 Another Git program has modified this repository since the last scan.  A rescan must be performed before the current branch can be changed.
@@ -297,7 +298,7 @@ The rescan will be automatically started now.
 		return
 	}
 
-	if {$curHEAD eq $new_hash} {
+	if {$old_hash eq $new_hash} {
 		_after_readtree $this
 	} elseif {[is_config_true gui.trustmtime]} {
 		_readtree $this
@@ -471,7 +472,19 @@ If you wanted to be on a branch, create one now starting from 'This Detached Che
 		set PARENT $HEAD
 		ui_status [mc "Checked out '%s'." $name]
 	}
-	delete_this
+
+	# -- Run the post-checkout hook.
+	#
+	set fd_ph [githook_read post-checkout $old_hash $new_hash 1]
+	if {$fd_ph ne {}} {
+		upvar #0 pch_error pc_err
+		set pc_err {}
+		fconfigure $fd_ph -blocking 0 -translation binary -eofchar {}
+		fileevent $fd_ph readable \
+			[list checkout_postcheckout_wait $fd_ph $this]
+	} else {
+		delete_this
+	}
 }
 
 git-version proc _detach_HEAD {log new} {
@@ -608,3 +621,19 @@ $err"]
 }
 
 }
+
+proc checkout_postcheckout_wait {fd_ph t} {
+	upvar #0 pch_error 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-checkout $pch_error 0
+		}
+		unset pch_error
+		delete_this $t
+		return
+	}
+	fconfigure $fd_ph -blocking 0
+}
-- 
1.6.2.1.307.g91408

Re: [PATCH] git-gui: run post-checkout hook on checkout

[Shawn O. Pearce]

Jens Lehmann <Jens.Lehmann@web.de> wrote:
> It's been a while since i deeply touched tcl/tk code ... i basically
> adapted the code for calling the post-commit hook here.

Not bad for not having touched tcl/tk in a long time... :-)
 
> +	# -- Run the post-checkout hook.
> +	#
> +	set fd_ph [githook_read post-checkout $old_hash $new_hash 1]
> +	if {$fd_ph ne {}} {
> +		upvar #0 pch_error pc_err

I'd rather spell this "global pch_error".

> +		set pc_err {}
> +		fconfigure $fd_ph -blocking 0 -translation binary -eofchar {}
> +		fileevent $fd_ph readable \
> +			[list checkout_postcheckout_wait $fd_ph $this]

The callback should be "[cb _postcheckout_wait $fd_ph]".  This is
a git-gui macro which returns a handle to invoke a "method" named
"_postcheckout_wait", passing in $this as the first parameter.

> +proc checkout_postcheckout_wait {fd_ph t} {

This should be "method _postcheckout_wait {fd_ph} {" to use the
cb macro above, and have $this automatically carry through.

> +	upvar #0 pch_error pch_error

"global 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-checkout $pch_error 0
> +		}
> +		unset pch_error
> +		delete_this $t
> +		return
> +	}
> +	fconfigure $fd_ph -blocking 0

Also, I'm wondering about the UI state semantics here.  Back on
line 462 (in the preimage) we unlock_index, which means that the
user can start to push buttons again, and then later on around
line 474 you start up the post-checkout hook.

I think we'd want to defer the unlock_index call until after the
hook has terminated, if we are going to run a hook at all.  In fact,
we probably need to move that entire block 462-474 (the unlock_index
... delete_this block) to a new method which we invoke if there is
no hook, or after the hook subprocess terminates.

By waiting until after the hook to do the rescan, we also ensure
that the hook has a chance to update the working directory, perhaps
by dirtying a file, and the user seeing the result of that dirtying
immediately.

-- 
Shawn.

Re: [PATCH] git-gui: run post-checkout hook on checkout

[Jens Lehmann]

Shawn O. Pearce schrieb:
> Jens Lehmann <Jens.Lehmann@web.de> wrote:
>> +	# -- Run the post-checkout hook.
>> +	#
>> +	set fd_ph [githook_read post-checkout $old_hash $new_hash 1]
>> +	if {$fd_ph ne {}} {
>> +		upvar #0 pch_error pc_err
> 
> I'd rather spell this "global pch_error".
> 
>> +		set pc_err {}

What i noticed when fixing this issue is that i copied this upvar
statement from the calling of the post-commit hook in commit.tcl
with only a minor change (ommitting the "$cmt_id" behind
"pch_error").

It does seem to be incorrect there too, as i couldn't find any use
of the variable "pc_err" or "pch_error$cmt_id" anywhere in git-gui.
So setting "pc_err" to empty seems pretty pointless, as everywhere
else in commit.tcl "pch_error" is used instead.

Or am i overlooking something? If not, the patch below should correct
that.

Jens

--------------------- 8>< ---------------------

>From 5ddd7e8c2d52fc99e496ed3bc96358cc07e538f1 Mon Sep 17 00:00:00 2001
From: Jens Lehmann <Jens.Lehmann@web.de>
Date: Mon, 30 Mar 2009 20:35:57 +0200
Subject: [PATCH] git-gui: When calling post-commit hook wrong variable was cleared.

Before calling the post-commit hook, the variable "pc_err" is cleared
while later only "pch_error" is used. "pch_error$cmt_id" only appeared in
"upvar"-Statements (which were changed to "global") and was removed.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 git-gui/lib/commit.tcl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 9cc8410..7255efb 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -398,8 +398,8 @@ A rescan will be automatically started now.
 	#
 	set fd_ph [githook_read post-commit]
 	if {$fd_ph ne {}} {
-		upvar #0 pch_error$cmt_id pc_err
-		set pc_err {}
+		global pch_error
+		set pch_error {}
 		fconfigure $fd_ph -blocking 0 -translation binary -eofchar {}
 		fileevent $fd_ph readable \
 			[list commit_postcommit_wait $fd_ph $cmt_id]
@@ -461,7 +461,7 @@ A rescan will be automatically started now.
 }
 
 proc commit_postcommit_wait {fd_ph cmt_id} {
-	upvar #0 pch_error$cmt_id pch_error
+	global pch_error
 
 	append pch_error [read $fd_ph]
 	fconfigure $fd_ph -blocking 1
-- 
1.6.2.1.414.g2daa3