threads / patch / 14892

patchRevert "gitk: Arrange to kill diff-files & diff-index on quit"

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

## tl;dr

8 messages between Aug 8, 2008 and Aug 11, 2008. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Christian Jaeger· Aug 8, 2008, 14:41 UTC · lore
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(-)
Show changes to gitk-git/gitk +17 −24
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
Alexander Gavrilov· Aug 9, 2008, 09:13 UTC · re: Christian Jaeger · lore

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

On Friday 08 August 2008 18:41:07 Christian Jaeger wrote:
Show 5 quoted lines
> 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
Christian Jaeger· Aug 9, 2008, 10:04 UTC · re: Alexander Gavrilov · lore

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

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(-)
Show changes to gitk-git/gitk +1 −1
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
Alexander Gavrilov· Aug 9, 2008, 10:41 UTC · re: Christian Jaeger · lore

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

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(-)
Show changes to gitk +8 −6
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
Christian Jaeger· Aug 9, 2008, 11:24 UTC · re: Alexander Gavrilov · lore

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

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

Show changes to diff +3 −3
@@ -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.
Christian Jaeger· Aug 11, 2008, 01:02 UTC · re: Christian Jaeger · lore

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

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.
Junio C Hamano· Aug 11, 2008, 20:44 UTC · re: Christian Jaeger · lore

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

Christian Jaeger <christian@jaeger.mine.nu> writes:
Show 5 quoted lines
> 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.

Paul Mackerras· Aug 11, 2008, 00:15 UTC · re: Alexander Gavrilov · lore

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

Alexander Gavrilov writes:
Show 10 quoted lines
> 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.

← back to recent threads