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

["Shawn O. Pearce" <spearce@spearce.org>]

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.