{"thread":{"id":"14892","subject":"[BUG/PATCH] Revert \"gitk: Arrange to kill diff-files & diff-index on quit\"","startedAt":"2008-08-08T14:41:07Z","lastAt":"2008-08-11T20:44:10Z","messageCount":8,"participants":["Christian Jaeger","Alexander Gavrilov","Paul Mackerras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"86509","messageId":"42d19ab224653b2e6988d7209a8d3e87e19858f8.1218207346.git.christian@jaeger.mine.nu","threadId":"14892","inReplyTo":null,"subject":"[BUG/PATCH] Revert \"gitk: Arrange to kill diff-files & diff-index on quit\"","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-08-08T14:41:07Z","receivedAt":"2008-08-08T14:41:07Z","isPatch":true,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"This reverts commit e439e092b8ee5248e92ed4cb4400f9dbed70f689.\n\ngitk would not show diffs (or trees when choosing tree view) about\nhalf of the times it is started, it would only show the commit\nmessages. Sometimes it took dozens of times to get it to show a diff\nagain with 3 starts, then the next 2 starts not, then the next 2\nstarts would show it again, and so on.\n\nConflicts:\n\n\tgitk-git/gitk\n\nSigned-off-by: Christian Jaeger <christian@jaeger.mine.nu>\n---\n\nThis is fixing the problem for 1.6.0-rc2.\n\nI've found the culprit with bisect, running gitk directly from the\nsource tree witout installation (meaning that it probably used the\n1.6.0-rc2 git tools throughout the whole bisect run).\n\nMy system environment:\n  Debian Lenny (testing)\n  tk8.4 8.4.19-2\n\nThanks,\nChristian.\n\n\n gitk-git/gitk |   41 +++++++++++++++++------------------------\n 1 files changed, 17 insertions(+), 24 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex d093a39..5b6ab7e 100644\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -90,15 +90,6 @@ proc dorunq {} {\n     }\n }\n \n-proc reg_instance {fd} {\n-    global commfd leftover loginstance\n-\n-    set i [incr loginstance]\n-    set commfd($i) $fd\n-    set leftover($i) {}\n-    return $i\n-}\n-\n proc unmerged_files {files} {\n     global nr_unmerged\n \n@@ -303,11 +294,11 @@ proc parseviewrevs {view revs} {\n # Start off a git log process and arrange to read its output\n proc start_rev_list {view} {\n     global startmsecs commitidx viewcomplete curview\n-    global tclencoding\n+    global commfd leftover tclencoding\n     global viewargs viewargscmd viewfiles vfilelimit\n     global showlocalchanges commitinterest\n-    global viewactive viewinstances vmergeonly\n-    global mainheadid\n+    global viewactive loginstance viewinstances vmergeonly\n+    global pending_select mainheadid\n     global vcanopt vflags vrevs vorigargs\n \n     set startmsecs [clock clicks -milliseconds]\n@@ -363,8 +354,10 @@ proc start_rev_list {view} {\n \terror_popup \"[mc \"Error executing git log:\"] $err\"\n \treturn 0\n     }\n-    set i [reg_instance $fd]\n+    set i [incr loginstance]\n     set viewinstances($view) [list $i]\n+    set commfd($i) $fd\n+    set leftover($i) {}\n     if {$showlocalchanges && $mainheadid ne {}} {\n \tlappend commitinterest($mainheadid) {dodiffindex}\n     }\n@@ -440,8 +433,8 @@ proc getcommits {selid} {\n \n proc updatecommits {} {\n     global curview vcanopt vorigargs vfilelimit viewinstances\n-    global viewactive viewcomplete tclencoding\n-    global startmsecs showneartags showlocalchanges\n+    global viewactive viewcomplete loginstance tclencoding\n+    global startmsecs commfd showneartags showlocalchanges leftover\n     global mainheadid pending_select\n     global isworktree\n     global varcid vposids vnegids vflags vrevs\n@@ -502,8 +495,10 @@ proc updatecommits {} {\n     if {$viewactive($view) == 0} {\n \tset startmsecs [clock clicks -milliseconds]\n     }\n-    set i [reg_instance $fd]\n+    set i [incr loginstance]\n     lappend viewinstances($view) $i\n+    set commfd($i) $fd\n+    set leftover($i) {}\n     fconfigure $fd -blocking 0 -translation lf -eofchar {}\n     if {$tclencoding != {}} {\n \tfconfigure $fd -encoding $tclencoding\n@@ -4091,11 +4086,10 @@ proc dodiffindex {} {\n     incr lserial\n     set fd [open \"|git diff-index --cached HEAD\" r]\n     fconfigure $fd -blocking 0\n-    set i [reg_instance $fd]\n-    filerun $fd [list readdiffindex $fd $lserial $i]\n+    filerun $fd [list readdiffindex $fd $lserial]\n }\n \n-proc readdiffindex {fd serial inst} {\n+proc readdiffindex {fd serial} {\n     global mainheadid nullid nullid2 curview commitinfo commitdata lserial\n \n     set isdiff 1\n@@ -4106,7 +4100,7 @@ proc readdiffindex {fd serial inst} {\n \tset isdiff 0\n     }\n     # we only need to see one line and we don't really care what it says...\n-    stop_instance $inst\n+    close $fd\n \n     if {$serial != $lserial} {\n \treturn 0\n@@ -4115,8 +4109,7 @@ proc readdiffindex {fd serial inst} {\n     # now see if there are any local changes not checked in to the index\n     set fd [open \"|git diff-files\" r]\n     fconfigure $fd -blocking 0\n-    set i [reg_instance $fd]\n-    filerun $fd [list readdifffiles $fd $serial $i]\n+    filerun $fd [list readdifffiles $fd $serial]\n \n     if {$isdiff && ![commitinview $nullid2 $curview]} {\n \t# add the line for the changes in the index to the graph\n@@ -4133,7 +4126,7 @@ proc readdiffindex {fd serial inst} {\n     return 0\n }\n \n-proc readdifffiles {fd serial inst} {\n+proc readdifffiles {fd serial} {\n     global mainheadid nullid nullid2 curview\n     global commitinfo commitdata lserial\n \n@@ -4145,7 +4138,7 @@ proc readdifffiles {fd serial inst} {\n \tset isdiff 0\n     }\n     # we only need to see one line and we don't really care what it says...\n-    stop_instance $inst\n+    close $fd\n \n     if {$serial != $lserial} {\n \treturn 0\n-- \n1.6.0.rc2.1.g42d19\n"},{"id":"86588","messageId":"200808091313.52528.angavrilov@gmail.com","threadId":"14892","inReplyTo":"42d19ab224653b2e6988d7209a8d3e87e19858f8.1218207346.git.christian@jaeger.mine.nu","subject":"Re: [BUG/PATCH] Revert \"gitk: Arrange to kill diff-files & diff-index on quit\"","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-08-09T09:13:52Z","receivedAt":"2008-08-09T09:13:52Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"On Friday 08 August 2008 18:41:07 Christian Jaeger wrote:\n> gitk would not show diffs (or trees when choosing tree view) about\n> half of the times it is started, it would only show the commit\n> messages. Sometimes it took dozens of times to get it to show a diff\n> again with 3 starts, then the next 2 starts not, then the next 2\n> starts would show it again, and so on.\n\n> My system environment:\n>   Debian Lenny (testing)\n>   tk8.4 8.4.19-2\n\nI cannot reproduce this on my Fedora 7 system with tk8.4.13 no matter how I try.\nCould you please try to isolate which hunk causes the problem? You should be\nable to remove them from bottom to top without any serious problems.\n\n\nBy the way, I experienced a similar problem while working on another patch\n(although it required a more elaborate sequence of actions to trigger it), and\nmade a fix for it:\n\nhttp://repo.or.cz/w/git.git?a=commitdiff;h=7272131b3e49879d3a7bedacad3cdb12ae678ee8\n\nFor some reason, now I cannot reproduce that either.\n\n-- Alexander\n"},{"id":"86594","messageId":"217ad8e755d8d51e2ec0f06b4bffa0864976f7e4.1218277122.git.christian@jaeger.mine.nu","threadId":"14892","inReplyTo":"200808091313.52528.angavrilov@gmail.com","subject":"[PATCH] gitk: make diff and tree display work reliably again","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-08-09T10:04:43Z","receivedAt":"2008-08-09T10:04:43Z","isPatch":true,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"This reverts parts of the commit e439e092b8ee5248e92ed4cb4400f9dbed70f689.\n\ngitk would not show diffs (or trees when choosing tree view) about\nhalf of the times it is started, it would only show the commit\nmessages. Sometimes it took dozens of times to get it to show a diff\nagain, then show it again the next 3 starts, then the next 2 starts\nnot, then the next 2 starts would show it again, and so on.\n\nThe problem has been observed on Linux 2.6.26 x86_64 (Core 2 Duo),\nDebian lenny, tk8.4 8.4.19-2. (Playing with frequency scaling settings\ndidn't show a difference; switching off one core made it more\nreproducible.)\n\nSigned-off-by: Christian Jaeger <christian@jaeger.mine.nu>\n---\n gitk-git/gitk |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex d093a39..216a3ce 100644\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -4145,7 +4145,7 @@ proc readdifffiles {fd serial inst} {\n \tset isdiff 0\n     }\n     # we only need to see one line and we don't really care what it says...\n-    stop_instance $inst\n+    close $fd\n \n     if {$serial != $lserial} {\n \treturn 0\n-- \n1.6.0.rc2.1.g42d19\n"},{"id":"86595","messageId":"200808091441.50444.angavrilov@gmail.com","threadId":"14892","inReplyTo":"217ad8e755d8d51e2ec0f06b4bffa0864976f7e4.1218277122.git.christian@jaeger.mine.nu","subject":"[PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-08-09T10:41:50Z","receivedAt":"2008-08-09T10:41:50Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"Originally dorunq assumed that the queue entry remained first\nin the queue after the script eval, and blindly removed it.\nHowever, if the handler calls nukefile, it may not be the\ncase anymore, and a random queue entry gets dropped instead.\n\nThis patch makes dorunq remove the entry before calling the\nscript, and adds a global variable to allow other functions\nto determine if they are called from within a dorunq handler.\n\nSigned-off-by: Alexander Gavrilov <angavrilov@gmail.com>\n---\n\n\tOn Saturday 09 August 2008 14:04:43 Christian Jaeger wrote:\n\t> gitk would not show diffs (or trees when choosing tree view) about\n\t> half of the times it is started, it would only show the commit\n\t> messages. Sometimes it took dozens of times to get it to show a diff\n\t> again, then show it again the next 3 starts, then the next 2 starts\n\t> not, then the next 2 starts would show it again, and so on.\n\t\n\tI think I guessed the cause of this bug: if two or more files\n\tbecome ready for reading at once, and the first one in the queue\n\tcalls nukefile on itself, the next one will get silently dropped from\n\tthe queue. If the second one was a diff pipe, the diff system gets\n\twedged until gitk is restarted.\n \n\tPlease test if this patch fixes it.\n\n\t-- Alexander\n\n\n gitk |   14 ++++++++------\n 1 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex b523c98..18d000c 100755\n--- a/gitk\n+++ b/gitk\n@@ -22,11 +22,11 @@ proc gitdir {} {\n # run before X event handlers, so reading from a fast source can\n # make the GUI completely unresponsive.\n proc run args {\n-    global isonrunq runq\n+    global isonrunq runq currunq\n \n     set script $args\n     if {[info exists isonrunq($script)]} return\n-    if {$runq eq {}} {\n+    if {$runq eq {} && ![info exists currunq]} {\n \tafter idle dorunq\n     }\n     lappend runq [list {} $script]\n@@ -38,10 +38,10 @@ proc filerun {fd script} {\n }\n \n proc filereadable {fd script} {\n-    global runq\n+    global runq currunq\n \n     fileevent $fd readable {}\n-    if {$runq eq {}} {\n+    if {$runq eq {} && ![info exists currunq]} {\n \tafter idle dorunq\n     }\n     lappend runq [list $fd $script]\n@@ -60,17 +60,19 @@ proc nukefile {fd} {\n }\n \n proc dorunq {} {\n-    global isonrunq runq\n+    global isonrunq runq currunq\n \n     set tstart [clock clicks -milliseconds]\n     set t0 $tstart\n     while {[llength $runq] > 0} {\n \tset fd [lindex $runq 0 0]\n \tset script [lindex $runq 0 1]\n+\tset currunq [lindex $runq 0]\n+\tset runq [lrange $runq 1 end]\n \tset repeat [eval $script]\n+\tunset currunq\n \tset t1 [clock clicks -milliseconds]\n \tset t [expr {$t1 - $t0}]\n-\tset runq [lrange $runq 1 end]\n \tif {$repeat ne {} && $repeat} {\n \t    if {$fd eq {} || $repeat == 2} {\n \t\t# script returns 1 if it wants to be readded\n-- \n1.6.0.rc2\n"},{"id":"86597","messageId":"489D7E67.5050205@jaeger.mine.nu","threadId":"14892","inReplyTo":"200808091441.50444.angavrilov@gmail.com","subject":"Re: [PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-08-09T11:24:23Z","receivedAt":"2008-08-09T11:24:23Z","isPatch":true,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"Alexander Gavrilov wrote:\n> \tPlease test if this patch fixes it.\n>   \n\nYes, with that patch it works reliably.\n\nBTW here's a patch to your patch to make it apply on top of 1.6.0.rc2:\n\n--- patch.eml.1\t2008-08-09 13:17:30.000000000 +0200\n+++ patch.eml\t2008-08-09 13:14:22.000000000 +0200\n@@ -96,10 +96,10 @@\n  gitk |   14 ++++++++------\n  1 files changed, 8 insertions(+), 6 deletions(-)\n \n-diff --git a/gitk b/gitk\n-index b523c98..18d000c 100755\n---- a/gitk\n-+++ b/gitk\n+diff --git a/gitk-git/gitk b/gitk-git/gitk\n+index b523c98..18d000c 100644\n+--- a/gitk-git/gitk\n++++ b/gitk-git/gitk\n @@ -22,11 +22,11 @@ proc gitdir {} {\n  # run before X event handlers, so reading from a fast source can\n  # make the GUI completely unresponsive.\n\n\n\nChristian.\n"},{"id":"86738","messageId":"18591.33943.93992.177637@cargo.ozlabs.ibm.com","threadId":"14892","inReplyTo":"200808091441.50444.angavrilov@gmail.com","subject":"Re: [PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2008-08-11T00:15:19Z","receivedAt":"2008-08-11T00:15:19Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Alexander Gavrilov writes:\n\n> Originally dorunq assumed that the queue entry remained first\n> in the queue after the script eval, and blindly removed it.\n> However, if the handler calls nukefile, it may not be the\n> case anymore, and a random queue entry gets dropped instead.\n> \n> This patch makes dorunq remove the entry before calling the\n> script, and adds a global variable to allow other functions\n> to determine if they are called from within a dorunq handler.\n> \n> Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>\n\nThanks, applied.\n"},{"id":"86739","messageId":"489F8FAE.2040201@jaeger.mine.nu","threadId":"14892","inReplyTo":"489D7E67.5050205@jaeger.mine.nu","subject":"Re: [PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.","fromName":"Christian Jaeger","fromEmail":"christian@jaeger.mine.nu","sentAt":"2008-08-11T01:02:38Z","receivedAt":"2008-08-11T01:02:38Z","isPatch":true,"sender":{"key":"christian@jaeger.mine.nu","avatar":null},"body":"I wrote:\n> BTW here's a patch to your patch to make it apply on top of 1.6.0.rc2:\n\nNote to self: how could I be so naive to assume Git didn't offer a \nsolution to that. I've missed the -3 option to git am.\n\nChristian.\n"},{"id":"86828","messageId":"7vd4kfwacl.fsf@gitster.siamese.dyndns.org","threadId":"14892","inReplyTo":"489F8FAE.2040201@jaeger.mine.nu","subject":"Re: [PATCH (GITK BUGFIX)] gitk: Allow safely calling nukefile from a run queue handler.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-11T20:44:10Z","receivedAt":"2008-08-11T20:44:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Jaeger <christian@jaeger.mine.nu> writes:\n\n> I wrote:\n>> BTW here's a patch to your patch to make it apply on top of 1.6.0.rc2:\n>\n> Note to self: how could I be so naive to assume Git didn't offer a\n> solution to that. I've missed the -3 option to git am.\n\nNot only that, it is customary to offer gitk and git-gui patches to the\nupstream (i.e. not against git.git).  I essentially pull from them and\nwithout ever modifying their parts inside git.git.\n"}]}