[PATCH (GITK) 0/6] Runaway process and commit selection fixes

[PATCH (GITK) 0/6] Runaway process and commit selection fixes

[Alexander Gavrilov]

This series includes three patches that I sent 8 days ago,
because I haven't received any reply so far.

These patches address the following problems:

1) Runaway processes (resend)

	As in the 'git gui blame' case, gitk back-end processes can sometimes
	run for a while without producing any output, e.g. diff-files on a slow
	filesystem.

	These patches make gitk explicitly kill its back-end processes.

2) Gitk stopping showing any diffs under random conditions.

3) Broken commit selection on view reload: gitk tried to preserve
  the selected commit, but usually failed because of code rot.

	I added selection preservation on Reload and Edit View.
	Update still should reset the selection to HEAD if anything changed.

	Also, if the previously selected commit was not found in the new view,
	gitk should fall back to selecting HEAD.


Alexander Gavrilov (6):
      gitk: Kill back-end processes on window close.
      gitk: Register diff-files & diff-index in commfd, to ensure kill.
      gitk: On Windows use a Cygwin-specific flag for kill.

      gitk: Fixed broken exception handling in diff.
      gitk: Fixed automatic row selection during load.
      gitk: Fallback to selecting the head commit upon load.

 gitk |  148 +++++++++++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 97 insertions(+), 51 deletions(-)

[PATCH (GITK) 1/6] gitk: Kill back-end processes on window close.

[Alexander Gavrilov]

Date: Sat, 12 Jul 2008 16:09:28 +0400

When collecting commits for a rarely changed, or recently
created file or directory, rev-list may work for a noticeable
period of time without producing any output. Such processes
don't receive SIGPIPE for a while after gitk is closed, thus
becoming runaway CPU hogs.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
 gitk |   35 +++++++++++++++++++++++++----------
 1 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/gitk b/gitk
index fddcb45..29d79d6 100755
--- a/gitk
+++ b/gitk
@@ -375,19 +375,33 @@ proc start_rev_list {view} {
     return 1
 }
 
+proc stop_instance {inst} {
+    global commfd leftover
+
+    set fd $commfd($inst)
+    catch {
+	set pid [pid $fd]
+	exec kill $pid
+    }
+    catch {close $fd}
+    nukefile $fd
+    unset commfd($inst)
+    unset leftover($inst)
+}
+
+proc stop_backends {} {
+    global commfd
+
+    foreach inst [array names commfd] {
+	stop_instance $inst
+    }
+}
+
 proc stop_rev_list {view} {
-    global commfd viewinstances leftover
+    global viewinstances
 
     foreach inst $viewinstances($view) {
-	set fd $commfd($inst)
-	catch {
-	    set pid [pid $fd]
-	    exec kill $pid
-	}
-	catch {close $fd}
-	nukefile $fd
-	unset commfd($inst)
-	unset leftover($inst)
+	stop_instance $inst
     }
     set viewinstances($view) {}
 }
@@ -2103,6 +2117,7 @@ proc makewindow {} {
     bind . <$M1B-minus> {incrfont -1}
     bind . <$M1B-KP_Subtract> {incrfont -1}
     wm protocol . WM_DELETE_WINDOW doquit
+    bind . <Destroy> {stop_backends}
     bind . <Button-1> "click %W"
     bind $fstring <Key-Return> {dofind 1 1}
     bind $sha1entry <Key-Return> gotocommit
-- 
1.5.6.3.18.gfe82

[PATCH (GITK) 2/6] gitk: Register diff-files & diff-index in commfd, to ensure kill.

[Alexander Gavrilov]

Date: Sun, 13 Jul 2008 16:40:47 +0400

Local change analysis can take a noticeable amount of time on large
file sets, and produce no output if there are no changes. Register
the back-ends in commfd, so that they get properly killed on window
close.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
 gitk |   39 +++++++++++++++++++++++----------------
 1 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/gitk b/gitk
index 29d79d6..b523c98 100755
--- a/gitk
+++ b/gitk
@@ -90,6 +90,15 @@ proc dorunq {} {
     }
 }
 
+proc reg_instance {fd} {
+    global commfd leftover loginstance
+
+    set i [incr loginstance]
+    set commfd($i) $fd
+    set leftover($i) {}
+    return $i
+}
+
 proc unmerged_files {files} {
     global nr_unmerged
 
@@ -294,10 +303,10 @@ proc parseviewrevs {view revs} {
 # Start off a git log process and arrange to read its output
 proc start_rev_list {view} {
     global startmsecs commitidx viewcomplete curview
-    global commfd leftover tclencoding
+    global tclencoding
     global viewargs viewargscmd viewfiles vfilelimit
     global showlocalchanges commitinterest
-    global viewactive loginstance viewinstances vmergeonly
+    global viewactive viewinstances vmergeonly
     global pending_select mainheadid
     global vcanopt vflags vrevs vorigargs
 
@@ -354,10 +363,8 @@ proc start_rev_list {view} {
 	error_popup "[mc "Error executing git log:"] $err"
 	return 0
     }
-    set i [incr loginstance]
+    set i [reg_instance $fd]
     set viewinstances($view) [list $i]
-    set commfd($i) $fd
-    set leftover($i) {}
     if {$showlocalchanges && $mainheadid ne {}} {
 	lappend commitinterest($mainheadid) {dodiffindex}
     }
@@ -420,8 +427,8 @@ proc getcommits {} {
 
 proc updatecommits {} {
     global curview vcanopt vorigargs vfilelimit viewinstances
-    global viewactive viewcomplete loginstance tclencoding
-    global startmsecs commfd showneartags showlocalchanges leftover
+    global viewactive viewcomplete tclencoding
+    global startmsecs showneartags showlocalchanges
     global mainheadid pending_select
     global isworktree
     global varcid vposids vnegids vflags vrevs
@@ -482,10 +489,8 @@ proc updatecommits {} {
     if {$viewactive($view) == 0} {
 	set startmsecs [clock clicks -milliseconds]
     }
-    set i [incr loginstance]
+    set i [reg_instance $fd]
     lappend viewinstances($view) $i
-    set commfd($i) $fd
-    set leftover($i) {}
     fconfigure $fd -blocking 0 -translation lf -eofchar {}
     if {$tclencoding != {}} {
 	fconfigure $fd -encoding $tclencoding
@@ -4063,10 +4068,11 @@ proc dodiffindex {} {
     incr lserial
     set fd [open "|git diff-index --cached HEAD" r]
     fconfigure $fd -blocking 0
-    filerun $fd [list readdiffindex $fd $lserial]
+    set i [reg_instance $fd]
+    filerun $fd [list readdiffindex $fd $lserial $i]
 }
 
-proc readdiffindex {fd serial} {
+proc readdiffindex {fd serial inst} {
     global mainheadid nullid nullid2 curview commitinfo commitdata lserial
 
     set isdiff 1
@@ -4077,7 +4083,7 @@ proc readdiffindex {fd serial} {
 	set isdiff 0
     }
     # we only need to see one line and we don't really care what it says...
-    close $fd
+    stop_instance $inst
 
     if {$serial != $lserial} {
 	return 0
@@ -4086,7 +4092,8 @@ proc readdiffindex {fd serial} {
     # now see if there are any local changes not checked in to the index
     set fd [open "|git diff-files" r]
     fconfigure $fd -blocking 0
-    filerun $fd [list readdifffiles $fd $serial]
+    set i [reg_instance $fd]
+    filerun $fd [list readdifffiles $fd $serial $i]
 
     if {$isdiff && ![commitinview $nullid2 $curview]} {
 	# add the line for the changes in the index to the graph
@@ -4103,7 +4110,7 @@ proc readdiffindex {fd serial} {
     return 0
 }
 
-proc readdifffiles {fd serial} {
+proc readdifffiles {fd serial inst} {
     global mainheadid nullid nullid2 curview
     global commitinfo commitdata lserial
 
@@ -4115,7 +4122,7 @@ proc readdifffiles {fd serial} {
 	set isdiff 0
     }
     # we only need to see one line and we don't really care what it says...
-    close $fd
+    stop_instance $inst
 
     if {$serial != $lserial} {
 	return 0
-- 
1.5.6.3.18.gfe82

[PATCH (GITK) 3/6] gitk: On Windows use a Cygwin-specific flag for kill.

[Alexander Gavrilov]

Date: Tue, 15 Jul 2008 00:35:42 +0400

MSysGit compiles git binaries as native Windows executables,
so they cannot be killed unless a special flag is specified.

This flag is implemented by the Cygwin version of kill,
which is also included in MSysGit.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
 gitk |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitk b/gitk
index b523c98..d7fea26 100755
--- a/gitk
+++ b/gitk
@@ -388,7 +388,12 @@ proc stop_instance {inst} {
     set fd $commfd($inst)
     catch {
 	set pid [pid $fd]
-	exec kill $pid
+
+	if {$::tcl_platform(platform) eq {windows}} {
+	    exec kill -f $pid
+	} else {
+	    exec kill $pid
+	}
     }
     catch {close $fd}
     nukefile $fd
-- 
1.5.6.3.18.gfe82

[PATCH (GITK) 4/6] gitk: Fixed broken exception handling in diff.

[Alexander Gavrilov]

Date: Sat, 26 Jul 2008 18:48:41 +0400

If the tree diff command failed to start for some
random reason, treepending remained set, and thus
no more diffs were shown after that.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
 gitk |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gitk b/gitk
index d7fea26..abb6542 100755
--- a/gitk
+++ b/gitk
@@ -6457,9 +6457,10 @@ proc diffcmd {ids flags} {
 proc gettreediffs {ids} {
     global treediff treepending
 
+    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id}] r]}]} return
+
     set treepending $ids
     set treediff {}
-    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id}] r]}]} return
     fconfigure $gdtf -blocking 0
     filerun $gdtf [list gettreediffline $gdtf $ids]
 }
-- 
1.5.6.3.18.gfe82

[PATCH (GITK) 5/6] gitk: Fixed automatic row selection during load.

[Alexander Gavrilov]

Date: Sat, 26 Jul 2008 20:13:45 +0400

- Switching views now actually preserves the selected commit.
- Reloading (also Edit View) preserves the currently selected commit.
- Initial selection does not produce weird scrolling.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
 gitk |   41 ++++++++++++++++++++++++-----------------
 1 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/gitk b/gitk
index abb6542..5021437 100755
--- a/gitk
+++ b/gitk
@@ -307,7 +307,7 @@ proc start_rev_list {view} {
     global viewargs viewargscmd viewfiles vfilelimit
     global showlocalchanges commitinterest
     global viewactive viewinstances vmergeonly
-    global pending_select mainheadid
+    global mainheadid
     global vcanopt vflags vrevs vorigargs
 
     set startmsecs [clock clicks -milliseconds]
@@ -374,9 +374,6 @@ proc start_rev_list {view} {
     }
     filerun $fd [list getcommitlines $fd $i $view 0]
     nowbusy $view [mc "Reading"]
-    if {$view == $curview} {
-	set pending_select $mainheadid
-    }
     set viewcomplete($view) 0
     set viewactive($view) 1
     return 1
@@ -418,11 +415,22 @@ proc stop_rev_list {view} {
     set viewinstances($view) {}
 }
 
-proc getcommits {} {
+proc reset_pending_select {selid} {
+    global pending_select mainheadid
+
+    if {$selid ne {}} {
+	set pending_select $selid
+    } else {
+	set pending_select $mainheadid
+    }
+}
+
+proc getcommits {selid} {
     global canv curview need_redisplay viewactive
 
     initlayout
     if {[start_rev_list $curview]} {
+	reset_pending_select $selid
 	show_status [mc "Reading commits..."]
 	set need_redisplay 1
     } else {
@@ -503,7 +511,7 @@ proc updatecommits {} {
     filerun $fd [list getcommitlines $fd $i $view 1]
     incr viewactive($view)
     set viewcomplete($view) 0
-    set pending_select $mainheadid
+    reset_pending_select {}
     nowbusy $view "Reading"
     if {$showneartags} {
 	getallcommits
@@ -515,6 +523,11 @@ proc reloadcommits {} {
     global showneartags treediffs commitinterest cached_commitrow
     global targetid
 
+    set selid {}
+    if {$selectedline ne {}} {
+	set selid $currentid
+    }
+
     if {!$viewcomplete($curview)} {
 	stop_rev_list $curview
     }
@@ -533,7 +546,7 @@ proc reloadcommits {} {
     catch {unset cached_commitrow}
     catch {unset targetid}
     setcanvscroll
-    getcommits
+    getcommits $selid
     return 0
 }
 
@@ -3325,10 +3338,7 @@ proc showview {n} {
 
     run refill_reflist
     if {![info exists viewcomplete($n)]} {
-	if {$selid ne {}} {
-	    set pending_select $selid
-	}
-	getcommits
+	getcommits $selid
 	return
     }
 
@@ -3365,11 +3375,7 @@ proc showview {n} {
     } elseif {$mainheadid ne {} && [commitinview $mainheadid $curview]} {
 	selectline [rowofcommit $mainheadid] 1
     } elseif {!$viewcomplete($n)} {
-	if {$selid ne {}} {
-	    set pending_select $selid
-	} else {
-	    set pending_select $mainheadid
-	}
+	reset_pending_select $selid
     } else {
 	set row [first_real_row]
 	if {$row < $numcommits} {
@@ -4036,6 +4042,7 @@ proc layoutmore {} {
     }
     if {[info exists pending_select] &&
 	[commitinview $pending_select $curview]} {
+	update
 	selectline [rowofcommit $pending_select] 1
     }
     drawvisible
@@ -9973,4 +9980,4 @@ if {[info exists permviews]} {
 	addviewmenu $n
     }
 }
-getcommits
+getcommits {}
-- 
1.5.6.3.18.gfe82

[PATCH (GITK) 6/6] gitk: Fallback to selecting the head commit upon load.

[Alexander Gavrilov]

Date: Sat, 26 Jul 2008 20:15:54 +0400

Try selecting the head, if the previously selected commit
is not available in the new view.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
 gitk |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/gitk b/gitk
index 5021437..d093a39 100755
--- a/gitk
+++ b/gitk
@@ -1506,8 +1506,15 @@ proc chewcommits {} {
 	global numcommits startmsecs
 
 	if {[info exists pending_select]} {
-	    set row [first_real_row]
-	    selectline $row 1
+	    update
+	    reset_pending_select {}
+
+	    if {[commitinview $pending_select $curview]} {
+		selectline [rowofcommit $pending_select] 1
+	    } else {
+		set row [first_real_row]
+		selectline $row 1
+	    }
 	}
 	if {$commitidx($curview) > 0} {
 	    #set ms [expr {[clock clicks -milliseconds] - $startmsecs}]
@@ -3372,14 +3379,18 @@ proc showview {n} {
     drawvisible
     if {$row ne {}} {
 	selectline $row 0
-    } elseif {$mainheadid ne {} && [commitinview $mainheadid $curview]} {
-	selectline [rowofcommit $mainheadid] 1
     } elseif {!$viewcomplete($n)} {
 	reset_pending_select $selid
     } else {
-	set row [first_real_row]
-	if {$row < $numcommits} {
-	    selectline $row 0
+	reset_pending_select {}
+
+	if {[commitinview $pending_select $curview]} {
+	    selectline [rowofcommit $pending_select] 1
+	} else {
+	    set row [first_real_row]
+	    if {$row < $numcommits} {
+		selectline $row 0
+	    }
 	}
     }
     if {!$viewcomplete($n)} {
-- 
1.5.6.3.18.gfe82

Re: [PATCH (GITK) 5/6] gitk: Fixed automatic row selection during load.

[Paul Mackerras]

Alexander Gavrilov writes:

> - Switching views now actually preserves the selected commit.
> - Reloading (also Edit View) preserves the currently selected commit.
> - Initial selection does not produce weird scrolling.
> 
> Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>

I need a more detailed explanation of the rationale for the specific
changes you have made in the changelog.

As for the patch, it mostly looks good, but I have a few comments
below.

> +proc getcommits {selid} {
>      global canv curview need_redisplay viewactive
>  
>      initlayout
>      if {[start_rev_list $curview]} {
> +	reset_pending_select $selid

Is there any significance to having the call to reset_pending_select
after the start_rev_list call (other than not setting pending_select
if start_rev_list fails)?  I couldn't see any.  If there is then it
should be noted in a comment and/or the patch description.

> @@ -503,7 +511,7 @@ proc updatecommits {} {
>      filerun $fd [list getcommitlines $fd $i $view 1]
>      incr viewactive($view)
>      set viewcomplete($view) 0
> -    set pending_select $mainheadid
> +    reset_pending_select {}

This doesn't actually change anything, right?  If so then I'd prefer
the simple, direct assignment to calling a procedure that does the
assignment.

> @@ -4036,6 +4042,7 @@ proc layoutmore {} {
>      }
>      if {[info exists pending_select] &&
>  	[commitinview $pending_select $curview]} {
> +	update
>  	selectline [rowofcommit $pending_select] 1

What does that update do?  Would update idletasks be better?

Thanks,
Paul.

Re: [PATCH (GITK) 5/6] gitk: Fixed automatic row selection during load.

[Alexander Gavrilov]

On Thu, Jul 31, 2008 at 3:25 PM, Paul Mackerras <paulus@samba.org> wrote:
> Alexander Gavrilov writes:
>
>> - Switching views now actually preserves the selected commit.
>> - Reloading (also Edit View) preserves the currently selected commit.
>> - Initial selection does not produce weird scrolling.
>>
>> Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
>
> I need a more detailed explanation of the rationale for the specific
> changes you have made in the changelog.

The rationale is that preserving the currently selected commit is more
intelligent behavior than always resetting to a preset position, and
it makes the UI feel more smooth. Also, although it is possible to
restore selection by clicking the 'back' button, it is not immediately
obvious; in fact I realized it only after reading the code.

Gitk already tried to preserve the commit in many cases, but failed
because of a conflicting change that made it select the head. Such
behavior is clearly a bug.

The Reload case is arguable, but I think that Edit View should
preserve the selection, and it uses Reload internally. It can be
resolved by adding a parameter to the function implementing Reload.

> As for the patch, it mostly looks good, but I have a few comments
> below.
>
>> +proc getcommits {selid} {
>>      global canv curview need_redisplay viewactive
>>
>>      initlayout
>>      if {[start_rev_list $curview]} {
>> +     reset_pending_select $selid
>
> Is there any significance to having the call to reset_pending_select
> after the start_rev_list call (other than not setting pending_select
> if start_rev_list fails)?  I couldn't see any.  If there is then it
> should be noted in a comment and/or the patch description.

It simply tries to preserve the original behavior.

>> @@ -503,7 +511,7 @@ proc updatecommits {} {
>>      filerun $fd [list getcommitlines $fd $i $view 1]
>>      incr viewactive($view)
>>      set viewcomplete($view) 0
>> -    set pending_select $mainheadid
>> +    reset_pending_select {}
>
> This doesn't actually change anything, right?  If so then I'd prefer
> the simple, direct assignment to calling a procedure that does the
> assignment.

I have a patch WIP that allows specifying a commit on the command line
to select instead of the head (I need it to enhance the git gui blame
UI). It makes the function somewhat more intelligent. I'll submit it
as soon as this series is sorted out.

>> @@ -4036,6 +4042,7 @@ proc layoutmore {} {
>>      }
>>      if {[info exists pending_select] &&
>>       [commitinview $pending_select $curview]} {
>> +     update
>>       selectline [rowofcommit $pending_select] 1
>
> What does that update do?  Would update idletasks be better?

That update forces Tk to recompute the widget dimensions. Otherwise
selectline sometimes gets all zeroes from yview, which makes it
compute really weird scrolling settings. Git-gui always does an update
before scrolling computations.

Alexander

[RFC PATCH (GITK)] gitk: Allow overriding the default commit.

[Alexander Gavrilov]

Date: Sun, 27 Jul 2008 08:18:27 +0400

Other GUI tools may occasionally need to start
gitk and make it automatically select a certain
commit. This patch supports doing it through the
environment or command line.

Using the environment allows graceful degradation of
the tool when used with an old version of gitk:
unsupported command line options cause it to die.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---

	On Thursday 31 July 2008 16:41:20 Alexander Gavrilov wrote:
	> I have a patch WIP that allows specifying a commit on the command line
	> to select instead of the head (I need it to enhance the git gui blame
	> UI). It makes the function somewhat more intelligent. I'll submit it
	> as soon as this series is sorted out.

	I decided to send it now.

	-- Alexander

 gitk |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/gitk b/gitk
index d093a39..2da885d 100755
--- a/gitk
+++ b/gitk
@@ -416,10 +416,12 @@ proc stop_rev_list {view} {
 }
 
 proc reset_pending_select {selid} {
-    global pending_select mainheadid
+    global pending_select mainheadid selectheadid
 
     if {$selid ne {}} {
 	set pending_select $selid
+    } elseif {$selectheadid ne {}} {
+	set pending_select $selectheadid
     } else {
 	set pending_select $mainheadid
     }
@@ -1607,6 +1609,7 @@ proc getcommit {id} {
 proc readrefs {} {
     global tagids idtags headids idheads tagobjid
     global otherrefids idotherrefs mainhead mainheadid
+    global selecthead selectheadid
 
     foreach v {tagids idtags headids idheads otherrefids idotherrefs} {
 	catch {unset $v}
@@ -1653,6 +1656,12 @@ proc readrefs {} {
 	    set mainhead [string range $thehead 11 end]
 	}
     }
+    set selectheadid {}
+    if {$selecthead ne {}} {
+	catch {
+	    set selectheadid [exec git rev-parse --verify $selecthead]
+	}
+    }
 }
 
 # skip over fake commits
@@ -9863,6 +9872,13 @@ if {![file isdirectory $gitdir]} {
     exit 1
 }
 
+set selecthead {}
+set selectheadid {}
+
+if {[info exists env(GITK_SELECT_ID)]} {
+    set selecthead $env(GITK_SELECT_ID)
+}
+
 set revtreeargs {}
 set cmdline_files {}
 set i 0
@@ -9874,6 +9890,9 @@ foreach arg $argv {
 	    set cmdline_files [lrange $argv [expr {$i + 1}] end]
 	    break
 	}
+	"--select-commit=*" {
+	    set selecthead [string range $arg 16 end]
+	}
 	"--argscmd=*" {
 	    set revtreeargscmd [string range $arg 10 end]
 	}
@@ -9884,6 +9903,10 @@ foreach arg $argv {
     incr i
 }
 
+if {$selecthead eq "HEAD"} {
+    set selecthead {}
+}
+
 if {$i >= [llength $argv] && $revtreeargs ne {}} {
     # no -- on command line, but some arguments (other than --argscmd)
     if {[catch {
-- 
1.5.6.3.18.gfe82