{"thread":{"id":"17330","subject":"[RFC PATCH (GIT-GUI/CORE BUG)] git-gui: Avoid an infinite rescan loop in handle_empty_diff.","startedAt":"2009-01-23T21:52:57Z","lastAt":"2009-01-24T03:29:08Z","messageCount":3,"participants":["Alexander Gavrilov","Keith Cascio","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"101696","messageId":"200901240052.58259.angavrilov@gmail.com","threadId":"17330","inReplyTo":null,"subject":"[RFC PATCH (GIT-GUI/CORE BUG)] git-gui: Avoid an infinite rescan loop in handle_empty_diff.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2009-01-23T21:52:57Z","receivedAt":"2009-01-23T21:52:57Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"If the index update machinery and git diff happen to disagree\non whether a particular file is modified, it may cause git-gui\nto enter an infinite index rescan loop, where an empty diff\nstarts a rescan, which finds the same set of files modified,\nand tries to display the diff for the first one, which happens\nto be the empty one. A current example of a possible disagreement\npoint is the autocrlf filter.\n\nThis patch breaks the loop by using a global variable to track\nthe auto-rescans. The variable is reset whenever a non-empty\ndiff is displayed. As a way to work around the malfunctioning\nindex rescan command, it resurrects the pre-0.6.0 code that\ndirectly updates only the affected file.\n\nSigned-off-by: Alexander Gavrilov <angavrilov@gmail.com>\n---\n\n    Note that if the file is actually modified, and the bug is\n    either in git-diff or the way git-gui calls it, this patch\n    will stage the file without displaying it in the UI. Thus, it\n    may be better to simply do nothing if the loop prevention\n    logic triggers.\n\n\n    On Jan 22, 11:31 pm, kbro <kevin.broa...@googlemail.com> wrote:\n    > However, for me it was worse as the \"No differences\" message popped up\n    > as soon as I opened git-gui, and the dialog said it would run a rescan\n    > to find all files in a similar state.  The rescan caused the same\n    > error to be detected again, so I could never get the dialog box to go\n    > away.\n\n    On Friday 23 January 2009 02:56:23 Andy Davey wrote:\n    > I had the exact same problem you described running git-1.6.1-\n    > preview20081227 on Windows XP as well. (the endless loop of dialog box\n    > is very frustrating).\n\n\nP.S. Steps to reproduce the autocrlf handling mismatch on Linux:\n\n    $ git init\n    $ echo > foo\n    $ git add foo && git commit -m init\n    $ unix2dos -o foo\n    $ git config core.autocrlf true\n    $ git status\n    # On branch master\n    # Changed but not updated:\n    #   (use \"git add <file>...\" to update what will be committed)\n    #   (use \"git checkout -- <file>...\" to discard changes in working directory)\n    #\n    #       modified:   foo\n    #\n    no changes added to commit (use \"git add\" and/or \"git commit -a\")\n    $ git diff foo\n    diff --git a/foo b/foo\n\n    It happens because git-status assumes that a change in\n    file size always means that the file contents have changed.\n    Content filters like autocrlf may invalidate this assumption.\n\n\n lib/diff.tcl |   16 +++++++++++++++-\n 1 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/lib/diff.tcl b/lib/diff.tcl\nindex bbbf15c..e5abb49 100644\n--- a/lib/diff.tcl\n+++ b/lib/diff.tcl\n@@ -51,6 +51,7 @@ proc force_diff_encoding {enc} {\n \n proc handle_empty_diff {} {\n \tglobal current_diff_path file_states file_lists\n+\tglobal last_empty_diff\n \n \tset path $current_diff_path\n \tset s $file_states($path)\n@@ -66,7 +67,17 @@ A rescan will be automatically started to find other files which may have the sa\n \n \tclear_diff\n \tdisplay_file $path __\n-\trescan ui_ready 0\n+\n+\tif {![info exists last_empty_diff]} {\n+\t\tset last_empty_diff $path\n+\t\trescan ui_ready 0\n+\t} else {\n+\t\t# We already tried rescanning recently, and it failed,\n+\t\t# so resort to updating this particular file.\n+\t\tif {[catch {git update-index -- $path} err]} {\n+\t\t\terror_popup [mc \"Failed to refresh index:\\n\\n%s\" $err]\n+\t\t}\n+\t}\n }\n \n proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {\n@@ -310,6 +321,7 @@ proc read_diff {fd cont_info} {\n \tglobal ui_diff diff_active\n \tglobal is_3way_diff is_conflict_diff current_diff_header\n \tglobal current_diff_queue\n+\tglobal last_empty_diff\n \n \t$ui_diff conf -state normal\n \twhile {[gets $fd line] >= 0} {\n@@ -415,6 +427,8 @@ proc read_diff {fd cont_info} {\n \n \t\tif {[$ui_diff index end] eq {2.0}} {\n \t\t\thandle_empty_diff\n+\t\t} else {\n+\t\t\tcatch { unset last_empty_diff }\n \t\t}\n \t\tset callback [lindex $cont_info 1]\n \t\tif {$callback ne {}} {\n-- \n1.6.1.63.g950db\n"},{"id":"101719","messageId":"alpine.GSO.2.00.0901231743360.11562@kiwi.cs.ucla.edu","threadId":"17330","inReplyTo":"200901240052.58259.angavrilov@gmail.com","subject":"Re: [RFC PATCH (GIT-GUI/CORE BUG)] git-gui: Avoid an infinite rescan loop in handle_empty_diff.","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-24T01:46:57Z","receivedAt":"2009-01-24T01:46:57Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":"Teach git-gui to check diff's exit code\nin order to know whether a file actually\nchanged or not.\n\nSigned-off-by: Keith Cascio <keith@cs.ucla.edu>\n---\nAlexander,\nI encountered the same problem and I tried a different way\nto prevent it.  Could you please try this alternative patch\nand see if it works in your setup?  If so, it might be\na lower-impact solution.  Even if it doesn't solve your\nproblem, I think it is still an improvement over what\nexists and could co-exist with your patch.\n                                     -- Keith Cascio\n\n git-gui/lib/diff.tcl |    8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl\nindex bbbf15c..94faf95 100644\n--- a/git-gui/lib/diff.tcl\n+++ b/git-gui/lib/diff.tcl\n@@ -276,6 +276,7 @@ proc start_show_diff {cont_info {add_opts {}}} {\n \t}\n \n \tlappend cmd -p\n+\tlappend cmd --exit-code\n \tlappend cmd --no-color\n \tif {$repo_config(gui.diffcontext) >= 1} {\n \t\tlappend cmd \"-U$repo_config(gui.diffcontext)\"\n@@ -310,6 +311,7 @@ proc read_diff {fd cont_info} {\n \tglobal ui_diff diff_active\n \tglobal is_3way_diff is_conflict_diff current_diff_header\n \tglobal current_diff_queue\n+\tglobal errorCode\n \n \t$ui_diff conf -state normal\n \twhile {[gets $fd line] >= 0} {\n@@ -397,7 +399,9 @@ proc read_diff {fd cont_info} {\n \t$ui_diff conf -state disabled\n \n \tif {[eof $fd]} {\n-\t\tclose $fd\n+\t\tfconfigure $fd -blocking 1\n+\t\tcatch { close $fd } err\n+\t\tset diff_exit_status $errorCode\n \n \t\tif {$current_diff_queue ne {}} {\n \t\t\tadvance_diff_queue $cont_info\n@@ -413,7 +417,7 @@ proc read_diff {fd cont_info} {\n \t\t}\n \t\tui_ready\n \n-\t\tif {[$ui_diff index end] eq {2.0}} {\n+\t\tif {$diff_exit_status eq \"NONE\"} {\n \t\t\thandle_empty_diff\n \t\t}\n \t\tset callback [lindex $cont_info 1]\n-- \n1.6.1\n"},{"id":"101725","messageId":"7v7i4lpekb.fsf@gitster.siamese.dyndns.org","threadId":"17330","inReplyTo":"200901240052.58259.angavrilov@gmail.com","subject":"Re: [RFC PATCH (GIT-GUI/CORE BUG)] git-gui: Avoid an infinite rescan loop in handle_empty_diff.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-24T03:29:08Z","receivedAt":"2009-01-24T03:29:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Gavrilov <angavrilov@gmail.com> writes:\n\n>     $ git config core.autocrlf true\n\nThis operation, when done in an already populated work tree, invalidates\nall the state that is cached in the index, and you would need to adjust\nthings to the altered reality caused by this operation before doing\nanything else.  There are different reasons you would want to flip the\nconfiguration after you have files in your work tree, and depending on the\nsituation, the correct adjustment would differ.\n\nYou may have started a project in this repository, your work tree files\nall have CRLF endings that is your platform convention, and after adding\nthe files to the index (but before making a commit) you may have realized\nthat you would want to keep your project cross platform, and that may be\nthe reason you are flipping the configuration.  If that is the case, your\nindex is already contaminated with CRLF, but your files have the line\nending that is correct (for you).  You would want to remove the index and\n\"add .\" to stage everything again before proceeding, to have the autocrlf\nmechanism to correct the line endings in the repository objects.\n\nThis would be the best case in one extreme.\n\nOn the other hand, you may have cloned a cross platform project from\nelsewhere (in other words, your objects and the index have the correct\nline ending), the checkout was done without autocrlf and it does not match\nthe local filesystem convention to use CRLF, and that may be the reason\nyou are flipping the configuration.  If that is the case, before making\nany changes to the work tree files, the right adjustment would be to\nremove the index, and \"reset --hard\" to force a checkout that follows your\nautocrlf settings, so that the work tree files are corrected.\n\nThis would be the best case in the other end of the extreme.\n\nAnd there will be different cases in between these extremes.\n\nI think clueful users who flips the configuration from the command line\nwould know all of the above, know what they want and can tell what the\nbest course of action would be, but I at the same time wonder if git-gui\nshould (and if so, can) offer a simple and safe way to help this process\nfrom others.\n"}]}