[RFC PATCH (GIT-GUI/CORE BUG)] git-gui: Avoid an infinite rescan loop in handle_empty_diff.

[RFC PATCH (GIT-GUI/CORE BUG)] git-gui: Avoid an infinite rescan loop in handle_empty_diff.

[Alexander Gavrilov]

If the index update machinery and git diff happen to disagree
on whether a particular file is modified, it may cause git-gui
to enter an infinite index rescan loop, where an empty diff
starts a rescan, which finds the same set of files modified,
and tries to display the diff for the first one, which happens
to be the empty one. A current example of a possible disagreement
point is the autocrlf filter.

This patch breaks the loop by using a global variable to track
the auto-rescans. The variable is reset whenever a non-empty
diff is displayed. As a way to work around the malfunctioning
index rescan command, it resurrects the pre-0.6.0 code that
directly updates only the affected file.

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

    Note that if the file is actually modified, and the bug is
    either in git-diff or the way git-gui calls it, this patch
    will stage the file without displaying it in the UI. Thus, it
    may be better to simply do nothing if the loop prevention
    logic triggers.


    On Jan 22, 11:31 pm, kbro <kevin.broa...@googlemail.com> wrote:
    > However, for me it was worse as the "No differences" message popped up
    > as soon as I opened git-gui, and the dialog said it would run a rescan
    > to find all files in a similar state.  The rescan caused the same
    > error to be detected again, so I could never get the dialog box to go
    > away.

    On Friday 23 January 2009 02:56:23 Andy Davey wrote:
    > I had the exact same problem you described running git-1.6.1-
    > preview20081227 on Windows XP as well. (the endless loop of dialog box
    > is very frustrating).


P.S. Steps to reproduce the autocrlf handling mismatch on Linux:

    $ git init
    $ echo > foo
    $ git add foo && git commit -m init
    $ unix2dos -o foo
    $ git config core.autocrlf true
    $ git status
    # On branch master
    # Changed but not updated:
    #   (use "git add <file>..." to update what will be committed)
    #   (use "git checkout -- <file>..." to discard changes in working directory)
    #
    #       modified:   foo
    #
    no changes added to commit (use "git add" and/or "git commit -a")
    $ git diff foo
    diff --git a/foo b/foo

    It happens because git-status assumes that a change in
    file size always means that the file contents have changed.
    Content filters like autocrlf may invalidate this assumption.


 lib/diff.tcl |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/lib/diff.tcl b/lib/diff.tcl
index bbbf15c..e5abb49 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -51,6 +51,7 @@ proc force_diff_encoding {enc} {
 
 proc handle_empty_diff {} {
 	global current_diff_path file_states file_lists
+	global last_empty_diff
 
 	set path $current_diff_path
 	set s $file_states($path)
@@ -66,7 +67,17 @@ A rescan will be automatically started to find other files which may have the sa
 
 	clear_diff
 	display_file $path __
-	rescan ui_ready 0
+
+	if {![info exists last_empty_diff]} {
+		set last_empty_diff $path
+		rescan ui_ready 0
+	} else {
+		# We already tried rescanning recently, and it failed,
+		# so resort to updating this particular file.
+		if {[catch {git update-index -- $path} err]} {
+			error_popup [mc "Failed to refresh index:\n\n%s" $err]
+		}
+	}
 }
 
 proc show_diff {path w {lno {}} {scroll_pos {}} {callback {}}} {
@@ -310,6 +321,7 @@ proc read_diff {fd cont_info} {
 	global ui_diff diff_active
 	global is_3way_diff is_conflict_diff current_diff_header
 	global current_diff_queue
+	global last_empty_diff
 
 	$ui_diff conf -state normal
 	while {[gets $fd line] >= 0} {
@@ -415,6 +427,8 @@ proc read_diff {fd cont_info} {
 
 		if {[$ui_diff index end] eq {2.0}} {
 			handle_empty_diff
+		} else {
+			catch { unset last_empty_diff }
 		}
 		set callback [lindex $cont_info 1]
 		if {$callback ne {}} {
-- 
1.6.1.63.g950db

Re: [RFC PATCH (GIT-GUI/CORE BUG)] git-gui: Avoid an infinite rescan loop in handle_empty_diff.

[Keith Cascio]

Teach git-gui to check diff's exit code
in order to know whether a file actually
changed or not.

Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
Alexander,
I encountered the same problem and I tried a different way
to prevent it.  Could you please try this alternative patch
and see if it works in your setup?  If so, it might be
a lower-impact solution.  Even if it doesn't solve your
problem, I think it is still an improvement over what
exists and could co-exist with your patch.
                                     -- Keith Cascio

 git-gui/lib/diff.tcl |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index bbbf15c..94faf95 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -276,6 +276,7 @@ proc start_show_diff {cont_info {add_opts {}}} {
 	}
 
 	lappend cmd -p
+	lappend cmd --exit-code
 	lappend cmd --no-color
 	if {$repo_config(gui.diffcontext) >= 1} {
 		lappend cmd "-U$repo_config(gui.diffcontext)"
@@ -310,6 +311,7 @@ proc read_diff {fd cont_info} {
 	global ui_diff diff_active
 	global is_3way_diff is_conflict_diff current_diff_header
 	global current_diff_queue
+	global errorCode
 
 	$ui_diff conf -state normal
 	while {[gets $fd line] >= 0} {
@@ -397,7 +399,9 @@ proc read_diff {fd cont_info} {
 	$ui_diff conf -state disabled
 
 	if {[eof $fd]} {
-		close $fd
+		fconfigure $fd -blocking 1
+		catch { close $fd } err
+		set diff_exit_status $errorCode
 
 		if {$current_diff_queue ne {}} {
 			advance_diff_queue $cont_info
@@ -413,7 +417,7 @@ proc read_diff {fd cont_info} {
 		}
 		ui_ready
 
-		if {[$ui_diff index end] eq {2.0}} {
+		if {$diff_exit_status eq "NONE"} {
 			handle_empty_diff
 		}
 		set callback [lindex $cont_info 1]
-- 
1.6.1

Re: [RFC PATCH (GIT-GUI/CORE BUG)] git-gui: Avoid an infinite rescan loop in handle_empty_diff.

[Junio C Hamano]

Alexander Gavrilov <angavrilov@gmail.com> writes:

>     $ git config core.autocrlf true

This operation, when done in an already populated work tree, invalidates
all the state that is cached in the index, and you would need to adjust
things to the altered reality caused by this operation before doing
anything else.  There are different reasons you would want to flip the
configuration after you have files in your work tree, and depending on the
situation, the correct adjustment would differ.

You may have started a project in this repository, your work tree files
all have CRLF endings that is your platform convention, and after adding
the files to the index (but before making a commit) you may have realized
that you would want to keep your project cross platform, and that may be
the reason you are flipping the configuration.  If that is the case, your
index is already contaminated with CRLF, but your files have the line
ending that is correct (for you).  You would want to remove the index and
"add ." to stage everything again before proceeding, to have the autocrlf
mechanism to correct the line endings in the repository objects.

This would be the best case in one extreme.

On the other hand, you may have cloned a cross platform project from
elsewhere (in other words, your objects and the index have the correct
line ending), the checkout was done without autocrlf and it does not match
the local filesystem convention to use CRLF, and that may be the reason
you are flipping the configuration.  If that is the case, before making
any changes to the work tree files, the right adjustment would be to
remove the index, and "reset --hard" to force a checkout that follows your
autocrlf settings, so that the work tree files are corrected.

This would be the best case in the other end of the extreme.

And there will be different cases in between these extremes.

I think clueful users who flips the configuration from the command line
would know all of the above, know what they want and can tell what the
best course of action would be, but I at the same time wonder if git-gui
should (and if so, can) offer a simple and safe way to help this process
from others.