[BUG/PATCH] Revert "gitk: Arrange to kill diff-files & diff-index on quit"

[BUG/PATCH] Revert "gitk: Arrange to kill diff-files & diff-index on quit"

[Christian Jaeger]

This reverts commit e439e092b8ee5248e92ed4cb4400f9dbed70f689.

gitk would not show diffs (or trees when choosing tree view) about
half of the times it is started, it would only show the commit
messages. Sometimes it took dozens of times to get it to show a diff
again with 3 starts, then the next 2 starts not, then the next 2
starts would show it again, and so on.

Conflicts:

	gitk-git/gitk

Signed-off-by: Christian Jaeger <christian@jaeger.mine.nu>
---

This is fixing the problem for 1.6.0-rc2.

I've found the culprit with bisect, running gitk directly from the
source tree witout installation (meaning that it probably used the
1.6.0-rc2 git tools throughout the whole bisect run).

My system environment:
  Debian Lenny (testing)
  tk8.4 8.4.19-2

Thanks,
Christian.


 gitk-git/gitk |   41 +++++++++++++++++------------------------
 1 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index d093a39..5b6ab7e 100644
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -90,15 +90,6 @@ 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
 
@@ -303,11 +294,11 @@ 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 tclencoding
+    global commfd leftover tclencoding
     global viewargs viewargscmd viewfiles vfilelimit
     global showlocalchanges commitinterest
-    global viewactive viewinstances vmergeonly
-    global mainheadid
+    global viewactive loginstance viewinstances vmergeonly
+    global pending_select mainheadid
     global vcanopt vflags vrevs vorigargs
 
     set startmsecs [clock clicks -milliseconds]
@@ -363,8 +354,10 @@ proc start_rev_list {view} {
 	error_popup "[mc "Error executing git log:"] $err"
 	return 0
     }
-    set i [reg_instance $fd]
+    set i [incr loginstance]
     set viewinstances($view) [list $i]
+    set commfd($i) $fd
+    set leftover($i) {}
     if {$showlocalchanges && $mainheadid ne {}} {
 	lappend commitinterest($mainheadid) {dodiffindex}
     }
@@ -440,8 +433,8 @@ proc getcommits {selid} {
 
 proc updatecommits {} {
     global curview vcanopt vorigargs vfilelimit viewinstances
-    global viewactive viewcomplete tclencoding
-    global startmsecs showneartags showlocalchanges
+    global viewactive viewcomplete loginstance tclencoding
+    global startmsecs commfd showneartags showlocalchanges leftover
     global mainheadid pending_select
     global isworktree
     global varcid vposids vnegids vflags vrevs
@@ -502,8 +495,10 @@ proc updatecommits {} {
     if {$viewactive($view) == 0} {
 	set startmsecs [clock clicks -milliseconds]
     }
-    set i [reg_instance $fd]
+    set i [incr loginstance]
     lappend viewinstances($view) $i
+    set commfd($i) $fd
+    set leftover($i) {}
     fconfigure $fd -blocking 0 -translation lf -eofchar {}
     if {$tclencoding != {}} {
 	fconfigure $fd -encoding $tclencoding
@@ -4091,11 +4086,10 @@ proc dodiffindex {} {
     incr lserial
     set fd [open "|git diff-index --cached HEAD" r]
     fconfigure $fd -blocking 0
-    set i [reg_instance $fd]
-    filerun $fd [list readdiffindex $fd $lserial $i]
+    filerun $fd [list readdiffindex $fd $lserial]
 }
 
-proc readdiffindex {fd serial inst} {
+proc readdiffindex {fd serial} {
     global mainheadid nullid nullid2 curview commitinfo commitdata lserial
 
     set isdiff 1
@@ -4106,7 +4100,7 @@ proc readdiffindex {fd serial inst} {
 	set isdiff 0
     }
     # we only need to see one line and we don't really care what it says...
-    stop_instance $inst
+    close $fd
 
     if {$serial != $lserial} {
 	return 0
@@ -4115,8 +4109,7 @@ proc readdiffindex {fd serial inst} {
     # 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
-    set i [reg_instance $fd]
-    filerun $fd [list readdifffiles $fd $serial $i]
+    filerun $fd [list readdifffiles $fd $serial]
 
     if {$isdiff && ![commitinview $nullid2 $curview]} {
 	# add the line for the changes in the index to the graph
@@ -4133,7 +4126,7 @@ proc readdiffindex {fd serial inst} {
     return 0
 }
 
-proc readdifffiles {fd serial inst} {
+proc readdifffiles {fd serial} {
     global mainheadid nullid nullid2 curview
     global commitinfo commitdata lserial
 
@@ -4145,7 +4138,7 @@ proc readdifffiles {fd serial inst} {
 	set isdiff 0
     }
     # we only need to see one line and we don't really care what it says...
-    stop_instance $inst
+    close $fd
 
     if {$serial != $lserial} {
 	return 0
-- 
1.6.0.rc2.1.g42d19

Re: [BUG/PATCH] Revert "gitk: Arrange to kill diff-files & diff-index on quit"

[Alexander Gavrilov]

On Friday 08 August 2008 18:41:07 Christian Jaeger wrote:
> gitk would not show diffs (or trees when choosing tree view) about
> half of the times it is started, it would only show the commit
> messages. Sometimes it took dozens of times to get it to show a diff
> again with 3 starts, then the next 2 starts not, then the next 2
> starts would show it again, and so on.

> My system environment:
>   Debian Lenny (testing)
>   tk8.4 8.4.19-2

I cannot reproduce this on my Fedora 7 system with tk8.4.13 no matter how I try.
Could you please try to isolate which hunk causes the problem? You should be
able to remove them from bottom to top without any serious problems.


By the way, I experienced a similar problem while working on another patch
(although it required a more elaborate sequence of actions to trigger it), and
made a fix for it:

http://repo.or.cz/w/git.git?a=commitdiff;h=7272131b3e49879d3a7bedacad3cdb12ae678ee8

For some reason, now I cannot reproduce that either.

-- Alexander

[PATCH] gitk: make diff and tree display work reliably again

[Christian Jaeger]

This reverts parts of the commit e439e092b8ee5248e92ed4cb4400f9dbed70f689.

gitk would not show diffs (or trees when choosing tree view) about
half of the times it is started, it would only show the commit
messages. Sometimes it took dozens of times to get it to show a diff
again, then show it again the next 3 starts, then the next 2 starts
not, then the next 2 starts would show it again, and so on.

The problem has been observed on Linux 2.6.26 x86_64 (Core 2 Duo),
Debian lenny, tk8.4 8.4.19-2. (Playing with frequency scaling settings
didn't show a difference; switching off one core made it more
reproducible.)

Signed-off-by: Christian Jaeger <christian@jaeger.mine.nu>
---
 gitk-git/gitk |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index d093a39..216a3ce 100644
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -4145,7 +4145,7 @@ proc readdifffiles {fd serial inst} {
 	set isdiff 0
     }
     # we only need to see one line and we don't really care what it says...
-    stop_instance $inst
+    close $fd
 
     if {$serial != $lserial} {
 	return 0
-- 
1.6.0.rc2.1.g42d19

[PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.

[Alexander Gavrilov]

Originally dorunq assumed that the queue entry remained first
in the queue after the script eval, and blindly removed it.
However, if the handler calls nukefile, it may not be the
case anymore, and a random queue entry gets dropped instead.

This patch makes dorunq remove the entry before calling the
script, and adds a global variable to allow other functions
to determine if they are called from within a dorunq handler.

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

	On Saturday 09 August 2008 14:04:43 Christian Jaeger wrote:
	> gitk would not show diffs (or trees when choosing tree view) about
	> half of the times it is started, it would only show the commit
	> messages. Sometimes it took dozens of times to get it to show a diff
	> again, then show it again the next 3 starts, then the next 2 starts
	> not, then the next 2 starts would show it again, and so on.
	
	I think I guessed the cause of this bug: if two or more files
	become ready for reading at once, and the first one in the queue
	calls nukefile on itself, the next one will get silently dropped from
	the queue. If the second one was a diff pipe, the diff system gets
	wedged until gitk is restarted.
 
	Please test if this patch fixes it.

	-- Alexander


 gitk |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/gitk b/gitk
index b523c98..18d000c 100755
--- a/gitk
+++ b/gitk
@@ -22,11 +22,11 @@ proc gitdir {} {
 # run before X event handlers, so reading from a fast source can
 # make the GUI completely unresponsive.
 proc run args {
-    global isonrunq runq
+    global isonrunq runq currunq
 
     set script $args
     if {[info exists isonrunq($script)]} return
-    if {$runq eq {}} {
+    if {$runq eq {} && ![info exists currunq]} {
 	after idle dorunq
     }
     lappend runq [list {} $script]
@@ -38,10 +38,10 @@ proc filerun {fd script} {
 }
 
 proc filereadable {fd script} {
-    global runq
+    global runq currunq
 
     fileevent $fd readable {}
-    if {$runq eq {}} {
+    if {$runq eq {} && ![info exists currunq]} {
 	after idle dorunq
     }
     lappend runq [list $fd $script]
@@ -60,17 +60,19 @@ proc nukefile {fd} {
 }
 
 proc dorunq {} {
-    global isonrunq runq
+    global isonrunq runq currunq
 
     set tstart [clock clicks -milliseconds]
     set t0 $tstart
     while {[llength $runq] > 0} {
 	set fd [lindex $runq 0 0]
 	set script [lindex $runq 0 1]
+	set currunq [lindex $runq 0]
+	set runq [lrange $runq 1 end]
 	set repeat [eval $script]
+	unset currunq
 	set t1 [clock clicks -milliseconds]
 	set t [expr {$t1 - $t0}]
-	set runq [lrange $runq 1 end]
 	if {$repeat ne {} && $repeat} {
 	    if {$fd eq {} || $repeat == 2} {
 		# script returns 1 if it wants to be readded
-- 
1.6.0.rc2

Re: [PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.

[Christian Jaeger]

Alexander Gavrilov wrote:
> 	Please test if this patch fixes it.
>   

Yes, with that patch it works reliably.

BTW here's a patch to your patch to make it apply on top of 1.6.0.rc2:

--- patch.eml.1	2008-08-09 13:17:30.000000000 +0200
+++ patch.eml	2008-08-09 13:14:22.000000000 +0200
@@ -96,10 +96,10 @@
  gitk |   14 ++++++++------
  1 files changed, 8 insertions(+), 6 deletions(-)
 
-diff --git a/gitk b/gitk
-index b523c98..18d000c 100755
---- a/gitk
-+++ b/gitk
+diff --git a/gitk-git/gitk b/gitk-git/gitk
+index b523c98..18d000c 100644
+--- a/gitk-git/gitk
++++ b/gitk-git/gitk
 @@ -22,11 +22,11 @@ proc gitdir {} {
  # run before X event handlers, so reading from a fast source can
  # make the GUI completely unresponsive.



Christian.

Re: [PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.

[Christian Jaeger]

I wrote:
> BTW here's a patch to your patch to make it apply on top of 1.6.0.rc2:

Note to self: how could I be so naive to assume Git didn't offer a 
solution to that. I've missed the -3 option to git am.

Christian.

Re: [PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.

[Junio C Hamano]

Christian Jaeger <christian@jaeger.mine.nu> writes:

> I wrote:
>> BTW here's a patch to your patch to make it apply on top of 1.6.0.rc2:
>
> Note to self: how could I be so naive to assume Git didn't offer a
> solution to that. I've missed the -3 option to git am.

Not only that, it is customary to offer gitk and git-gui patches to the
upstream (i.e. not against git.git).  I essentially pull from them and
without ever modifying their parts inside git.git.

Re: [PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.

[Paul Mackerras]

Alexander Gavrilov writes:

> Originally dorunq assumed that the queue entry remained first
> in the queue after the script eval, and blindly removed it.
> However, if the handler calls nukefile, it may not be the
> case anymore, and a random queue entry gets dropped instead.
> 
> This patch makes dorunq remove the entry before calling the
> script, and adds a global variable to allow other functions
> to determine if they are called from within a dorunq handler.
> 
> Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>

Thanks, applied.