threads / bug / 31672

Git diff-file bug?

Subject: Git diff-file bug?

## tl;dr

18 messages between Sep 28, 2012 and Oct 3, 2012.

replies: 17people: 7as markdown or json

Scott Batchelor· Sep 28, 2012, 18:55 UTC · lore

Hello. I'm fairly new to git and am witnessing some strange behavior with git that I suspect may be a bug. Can anyone set my mind at rest.

Every so often (I've not quite figured out the exact set of circumstances yet) "git diff-files" shows that every file in my repo is modified, when I expect none to be. If I type "git status" (which shows what I expect - no files modified) and then "git diff-files" again, git now shows that no files have been modified (which is what I expect). It's like "git status" is resetting something that "git diff-files" uses.

I'm trying to figure out what the problem with "git diff-files" is because gitk uses it under the hood, and I think that gitk is reporting erroneous changes (which are also reset by performing a "git status" in the repo) in the "patch" files list.

Any thoughts? I'm using git 1.7.11.4 on SLES 10.3.

Thanks, Scott.

Jeff King· Sep 28, 2012, 20:23 UTC · re: Scott Batchelor · lore

Re: Git diff-file bug?

On Fri, Sep 28, 2012 at 07:55:10PM +0100, Scott Batchelor wrote:
> I'm fairly new to git and am witnessing some strange behavior with git
> that I suspect may be a bug. Can anyone set my mind at rest.
It's not a bug.
> Every so often (I've not quite figured out the exact set of
> circumstances yet)  "git diff-files" shows that every file in my repo
> is modified, when I expect none to be.

Diff-files only looks at the cached sha1 in the index. It will not re-read the file to update its index entry if its stat information appears out of date. So what is happening is that another program[1] is updating the timestamp or other information about each file, but not the content. Diff-files does not re-read the content to find out there is in fact no change.

If you are using plumbing like diff-files, you are expected to run "git update-index --refresh" yourself first. The reasoning is that for performance reasons, a script that issues many git commands would only want to pay the price to refresh the index once, at the beginning of the script.

If you use the "git diff" porcelain, it will automatically refresh the index for you.

> If I type "git status" (which shows what I expect - no files modified)
> and then "git diff-files" again, git now shows that no files have been
> modified (which is what I expect). It's like "git status" is resetting
> something that "git diff-files" uses.

Exactly. "git status" automatically refreshes the index, and the result is saved.

> I'm trying to figure out what the problem with "git diff-files" is
> because gitk uses it under the hood, and I think that gitk is
> reporting  erroneous changes (which are also reset by performing a
> "git status" in the repo) in the "patch" files list.

gitk should probably be calling "update-index --refresh" on startup. If it already is, then it may be that whatever is updating the files is doing it while gitk is running.

-Peff
[1] Usually stat updates are caused by another program. But we have had
    cases where unusual filesystems yielded inconsistent results from
    stat().
Jeff King· Sep 28, 2012, 20:50 UTC · re: Jeff King · lore

[PATCH] gitk: refresh the index before running diff-files

On Fri, Sep 28, 2012 at 04:23:30PM -0400, Jeff King wrote:
Show 8 quoted lines
> > I'm trying to figure out what the problem with "git diff-files" is
> > because gitk uses it under the hood, and I think that gitk is
> > reporting  erroneous changes (which are also reset by performing a
> > "git status" in the repo) in the "patch" files list.
> 
> gitk should probably be calling "update-index --refresh" on startup. If
> it already is, then it may be that whatever is updating the files is
> doing it while gitk is running.

Ah, it is not refreshing the index at all. Try this (substitute Makefile with some file in your repository):

  $ touch Makefile
  $ gitk  <-- reports uncommitted changes
  $ git update-index --refresh
  $ gitk  <-- changes go away
I am not a tcl hacker, but the patch below seems to fix it for me.
-- >8 --
Subject: [PATCH] gitk: refresh the index before running diff-files

If you have index entries that are stat-dirty but otherwise unchanged, gitk will erroneously report that "local uncommited changes" are present. This is because we call the plumbing diff-files, which expects us to have updated the index ourselves. So let's start doing so.

We only run this once per invocation of gitk, though. The point of diff-files not running the update is that it does not need to be run for every command, but generally only once per high-level operation.

Signed-off-by: Jeff King <peff@peff.net>
---
Potentially the "reload" command should reset the need_index_refresh
flag, too.
 gitk | 10 ++++++++++
 1 file changed, 10 insertions(+)
diff --git a/gitk b/gitk
index 379582a..561be23 100755
--- a/gitk
+++ b/gitk
@@ -5112,6 +5112,14 @@ proc dodiffindex {} {
     filerun $fd [list readdiffindex $fd $lserial $i]
 }
 
+proc refresh_index {} {
+    global need_index_refresh
+    if { $need_index_refresh } {
+	exec sh -c "git update-index --refresh >/dev/null 2>&1 || true"
+	set need_index_refresh false
+    }
+}
+
 proc readdiffindex {fd serial inst} {
     global viewmainheadid nullid nullid2 curview commitinfo commitdata lserial
     global vfilelimit
@@ -5131,6 +5139,7 @@ proc readdiffindex {fd serial inst} {
     }
 
     # now see if there are any local changes not checked in to the index
+    refresh_index
     set cmd "|git diff-files"
     if {$vfilelimit($curview) ne {}} {
 	set cmd [concat $cmd -- $vfilelimit($curview)]
@@ -11670,6 +11679,7 @@ set want_ttk 1
 set autosellen 40
 set perfile_attrs 0
 set want_ttk 1
+set need_index_refresh true
 
 if {[tk windowingsystem] eq "aqua"} {
     set extdifftool "opendiff"
-- 
1.7.12.42.g8bdb6d3
Junio C Hamano· Sep 28, 2012, 21:40 UTC · re: Jeff King · lore

Re: [PATCH] gitk: refresh the index before running diff-files

Jeff King <peff@peff.net> writes:
> Potentially the "reload" command should reset the need_index_refresh
> flag, too.
Yeah, I think that is a sane enhancement to think about.
Show 38 quoted lines
>  gitk | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/gitk b/gitk
> index 379582a..561be23 100755
> --- a/gitk
> +++ b/gitk
> @@ -5112,6 +5112,14 @@ proc dodiffindex {} {
>      filerun $fd [list readdiffindex $fd $lserial $i]
>  }
>  
> +proc refresh_index {} {
> +    global need_index_refresh
> +    if { $need_index_refresh } {
> +	exec sh -c "git update-index --refresh >/dev/null 2>&1 || true"
> +	set need_index_refresh false
> +    }
> +}
> +
>  proc readdiffindex {fd serial inst} {
>      global viewmainheadid nullid nullid2 curview commitinfo commitdata lserial
>      global vfilelimit
> @@ -5131,6 +5139,7 @@ proc readdiffindex {fd serial inst} {
>      }
>  
>      # now see if there are any local changes not checked in to the index
> +    refresh_index
>      set cmd "|git diff-files"
>      if {$vfilelimit($curview) ne {}} {
>  	set cmd [concat $cmd -- $vfilelimit($curview)]
> @@ -11670,6 +11679,7 @@ set want_ttk 1
>  set autosellen 40
>  set perfile_attrs 0
>  set want_ttk 1
> +set need_index_refresh true
>  
>  if {[tk windowingsystem] eq "aqua"} {
>      set extdifftool "opendiff"
Andreas Schwab· Sep 28, 2012, 22:11 UTC · re: Jeff King · lore

Re: [PATCH] gitk: refresh the index before running diff-files

Jeff King <peff@peff.net> writes:
> +proc refresh_index {} {
> +    global need_index_refresh
> +    if { $need_index_refresh } {
> +	exec sh -c "git update-index --refresh >/dev/null 2>&1 || true"

I think the usual idiom for ignoring errors is to use catch around exec, avoiding the extra shell wrapper:

        catch { exec git update-index --refresh }
Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Jeff King· Sep 28, 2012, 22:31 UTC · re: Andreas Schwab · lore

Re: [PATCH] gitk: refresh the index before running diff-files

On Sat, Sep 29, 2012 at 12:11:58AM +0200, Andreas Schwab wrote:
Show 11 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > +proc refresh_index {} {
> > +    global need_index_refresh
> > +    if { $need_index_refresh } {
> > +	exec sh -c "git update-index --refresh >/dev/null 2>&1 || true"
> 
> I think the usual idiom for ignoring errors is to use catch around exec,
> avoiding the extra shell wrapper:
> 
>         catch { exec git update-index --refresh }

Thanks. I don't speak tcl at all, but your version makes much more sense.

-Peff
Junio C Hamano· Sep 28, 2012, 23:02 UTC · re: Jeff King · lore

Re: [PATCH] gitk: refresh the index before running diff-files

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> On Sat, Sep 29, 2012 at 12:11:58AM +0200, Andreas Schwab wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > +proc refresh_index {} {
>> > +    global need_index_refresh
>> > +    if { $need_index_refresh } {
>> > +	exec sh -c "git update-index --refresh >/dev/null 2>&1 || true"
>> 
>> I think the usual idiom for ignoring errors is to use catch around exec,
>> avoiding the extra shell wrapper:
>> 
>>         catch { exec git update-index --refresh }
>
> Thanks. I don't speak tcl at all, but your version makes much more
> sense.

But isn't the redirection still needed? Otherwise the "Needs update" messages will go to the terminal, no?

Jeff King· Sep 28, 2012, 23:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitk: refresh the index before running diff-files

On Fri, Sep 28, 2012 at 04:02:32PM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> >> Jeff King <peff@peff.net> writes:
> >> 
> >> > +proc refresh_index {} {
> >> > +    global need_index_refresh
> >> > +    if { $need_index_refresh } {
> >> > +	exec sh -c "git update-index --refresh >/dev/null 2>&1 || true"
> >> 
> >> I think the usual idiom for ignoring errors is to use catch around exec,
> >> avoiding the extra shell wrapper:
> >> 
> >>         catch { exec git update-index --refresh }
> >
> > Thanks. I don't speak tcl at all, but your version makes much more
> > sense.
> 
> But isn't the redirection still needed?  Otherwise the "Needs
> update" messages will go to the terminal, no?

I think the weird tcl-catches-stderr thing kicks in (at least it did for me in a simple experiment). But like I said, I am not an expert.

-Peff
Junio C Hamano· Sep 28, 2012, 23:18 UTC · re: Jeff King · lore

Re: [PATCH] gitk: refresh the index before running diff-files

Jeff King <peff@peff.net> writes:
> I think the weird tcl-catches-stderr thing kicks in (at least it did for
> me in a simple experiment). But like I said, I am not an expert.
OK, I'll believe you ;-)
Andreas Schwab· Sep 28, 2012, 23:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] gitk: refresh the index before running diff-files

Junio C Hamano <gitster@pobox.com> writes:
Show 21 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> On Sat, Sep 29, 2012 at 12:11:58AM +0200, Andreas Schwab wrote:
>>
>>> Jeff King <peff@peff.net> writes:
>>> 
>>> > +proc refresh_index {} {
>>> > +    global need_index_refresh
>>> > +    if { $need_index_refresh } {
>>> > +	exec sh -c "git update-index --refresh >/dev/null 2>&1 || true"
>>> 
>>> I think the usual idiom for ignoring errors is to use catch around exec,
>>> avoiding the extra shell wrapper:
>>> 
>>>         catch { exec git update-index --refresh }
>>
>> Thanks. I don't speak tcl at all, but your version makes much more
>> sense.
>
> But isn't the redirection still needed?  Otherwise the "Needs
> update" messages will go to the terminal, no?

The exec command captures both stdout and stderr and returns it as its value, and catch ignores it.

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Paul Mackerras· Sep 30, 2012, 00:05 UTC · re: Jeff King · lore

Re: [PATCH] gitk: refresh the index before running diff-files

On Fri, Sep 28, 2012 at 04:50:54PM -0400, Jeff King wrote:
Show 7 quoted lines
> +proc refresh_index {} {
> +    global need_index_refresh
> +    if { $need_index_refresh } {
> +	exec sh -c "git update-index --refresh >/dev/null 2>&1 || true"
> +	set need_index_refresh false
> +    }
> +}

Unfortunately this will wait for the git update-index command to complete, making the GUI unresponsive while it executes, and that can take minutes on a large repository (e.g. the linux kernel) on a machine with a slow disk and a cold disk cache. We will need to make the git update-index execute asynchronously.

Paul.
Jeff King· Sep 30, 2012, 01:42 UTC · re: Paul Mackerras · lore

Re: [PATCH] gitk: refresh the index before running diff-files

On Sun, Sep 30, 2012 at 10:05:27AM +1000, Paul Mackerras wrote:
Show 15 quoted lines
> On Fri, Sep 28, 2012 at 04:50:54PM -0400, Jeff King wrote:
> 
> > +proc refresh_index {} {
> > +    global need_index_refresh
> > +    if { $need_index_refresh } {
> > +	exec sh -c "git update-index --refresh >/dev/null 2>&1 || true"
> > +	set need_index_refresh false
> > +    }
> > +}
> 
> Unfortunately this will wait for the git update-index command to
> complete, making the GUI unresponsive while it executes, and that can
> take minutes on a large repository (e.g. the linux kernel) on a
> machine with a slow disk and a cold disk cache.  We will need to make
> the git update-index execute asynchronously.

Good point. We're getting out of my very limited tcl cargo-culting skills now, so I'll let somebody more clueful do that fix.

-Peff
Jonathan Nieder· Sep 30, 2012, 20:34 UTC · re: Jeff King · lore

Re: [PATCH] gitk: refresh the index before running diff-files

Hi Jeff,
Jeff King wrote:
> On Sun, Sep 30, 2012 at 10:05:27AM +1000, Paul Mackerras wrote:
Show 8 quoted lines
>> Unfortunately this will wait for the git update-index command to
>> complete, making the GUI unresponsive while it executes, and that can
>> take minutes on a large repository (e.g. the linux kernel) on a
>> machine with a slow disk and a cold disk cache.  We will need to make
>> the git update-index execute asynchronously.
>
> Good point. We're getting out of my very limited tcl cargo-culting
> skills now, so I'll let somebody more clueful do that fix.
You might find the following patch and discussion entertaining:
  http://thread.gmane.org/gmane.comp.version-control.git/144182
Not my itch, but it was fun to write back then. ;-)

Hope that helps, Jonathan

Jeff King· Oct 1, 2012, 22:32 UTC · re: Jonathan Nieder · lore

Re: [PATCH] gitk: refresh the index before running diff-files

On Sun, Sep 30, 2012 at 01:34:58PM -0700, Jonathan Nieder wrote:
Show 17 quoted lines
> Jeff King wrote:
> > On Sun, Sep 30, 2012 at 10:05:27AM +1000, Paul Mackerras wrote:
> 
> >> Unfortunately this will wait for the git update-index command to
> >> complete, making the GUI unresponsive while it executes, and that can
> >> take minutes on a large repository (e.g. the linux kernel) on a
> >> machine with a slow disk and a cold disk cache.  We will need to make
> >> the git update-index execute asynchronously.
> >
> > Good point. We're getting out of my very limited tcl cargo-culting
> > skills now, so I'll let somebody more clueful do that fix.
> 
> You might find the following patch and discussion entertaining:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/144182
> 
> Not my itch, but it was fun to write back then. ;-)

Not really my itch either, which is why I'm trying to dump the tcl work on somebody else. :) For what it's worth, your patch from that thread looks like a sane approach.

I don't buy the "gitk should be read-only" argument from that thread. I think we decided a while back[1] that the stat cache is not really a user-visible modification; you're not updating the index in any meaningful way that impacts the user's workflow, but merely refreshing our cache of what is in the filesystem. That may have performance implications (which is why we do not do it in all commands by default), but it really is a cache, and updating it should be no different than updating gitk.cache (i.e., do it if we can, but do not complain if we are in a read-only repository).

-Peff
[1] I did not track down the threads, but I consider this resolved by
    all of the discussion that led to "git diff" refreshing (and saving)
    the index rather than printing patch-less stat-dirty entries. The
    original argument against refreshing was that the stat-dirtiness was
    somehow interesting to users, but I think experience has shown that
    people are happy to have it happen magically behind the scenes.
Junio C Hamano· Oct 1, 2012, 22:54 UTC · re: Jeff King · lore

Re: [PATCH] gitk: refresh the index before running diff-files

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> I don't buy the "gitk should be read-only" argument from that thread. I
> think we decided a while back[1] that the stat cache is not really a
> user-visible modification; you're not updating the index in any
> meaningful way that impacts the user's workflow, but merely refreshing
> our cache of what is in the filesystem.

I think it is OK as long as a failure to "update-index --refresh" does not lead to a user-visible error (I think we made sure that the the opportunistic refresh "git diff" and "git status" does won't report an error, when you are trying to help your co-worker by visiting her repository and running these commands, only to peek).

Junio C Hamano· Sep 28, 2012, 20:40 UTC · re: Scott Batchelor · lore

Re: Git diff-file bug?

Scott Batchelor <scott.batchelor@gmail.com> writes:
Show 5 quoted lines
> I'm fairly new to git and am witnessing some strange behavior with git
> that I suspect may be a bug. Can anyone set my mind at rest.
>
> Every so often (I've not quite figured out the exact set of
> circumstances yet) 

Figure that circumstances out. That is the key to the issue. Something in your workflow is futzing with the inode data of the files in your working tree behind your back. It sometimes is a virus scanner.

"git diff-*" plumbing commands are meant to be used after running "git update-index --refresh" once in the program and when the caller of these commands (in your case, gitk) knows that any change in the information returned by lstat(2) on the paths in the working tree files since that call indicate real changes to the files.

"git status" internally runs an equivalent of "--refresh" before it goes to find changes, so after running it, until that something smudges the inode data behind your back, "gitk" will not be confused.

Scott Batchelor· Oct 3, 2012, 08:04 UTC · re: Junio C Hamano · lore

Re: Git diff-file bug?

Many thanks to all who have responded to my question. I have found that something is, indeed, modifying the inodes for all the files in my repository. Our systems administrator executes a backup using "tar" with the "--atime-preserve" flag. It is this flag that modifies the "changed time" in the inode, and causes gitk to show that all my files have changed. Thanks, Scott.

On 28 September 2012 21:40, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Scott Batchelor <scott.batchelor@gmail.com> writes:
>
>> I'm fairly new to git and am witnessing some strange behavior with git
>> that I suspect may be a bug. Can anyone set my mind at rest.
>>
>> Every so often (I've not quite figured out the exact set of
>> circumstances yet)
>
> Figure that circumstances out.  That is the key to the issue.
> Something in your workflow is futzing with the inode data of the
> files in your working tree behind your back.  It sometimes is a
> virus scanner.
>
> "git diff-*" plumbing commands are meant to be used after running
> "git update-index --refresh" once in the program and when the caller
> of these commands (in your case, gitk) knows that any change in the
> information returned by lstat(2) on the paths in the working tree
> files since that call indicate real changes to the files.
>
> "git status" internally runs an equivalent of "--refresh" before it
> goes to find changes, so after running it, until that something
> smudges the inode data behind your back, "gitk" will not be
> confused.
>
Drew Northup· Oct 3, 2012, 11:58 UTC · re: Scott Batchelor · lore

Re: Git diff-file bug?

On Wed, Oct 3, 2012 at 4:04 AM, Scott Batchelor <scott.batchelor@gmail.com> wrote:

Show 8 quoted lines
> Many thanks to all who have responded to my question.
> I have found that something is, indeed, modifying the inodes for all
> the files in my repository. Our systems administrator executes a
> backup using "tar" with the "--atime-preserve" flag. It is this flag
> that modifies the "changed time" in the inode, and causes gitk to show
> that all my files have changed.
> Thanks,
> Scott.

Scott, We do that in our office @work. Perhaps this will help:

"core.trustctime If false, the ctime differences between the index and the working tree are ignored; useful when the inode change time is regularly modified by something outside Git (file system crawlers and some backup systems). See git-update-index(1). True by default."

(Quoted via http://git-scm.com/docs/git-config )
When/if I have problems I set that false.
(CC list reconstructed)
Show 24 quoted lines
> On 28 September 2012 21:40, Junio C Hamano <gitster@pobox.com> wrote:
>> Scott Batchelor <scott.batchelor@gmail.com> writes:
>>
>>> I'm fairly new to git and am witnessing some strange behavior with git
>>> that I suspect may be a bug. Can anyone set my mind at rest.
>>>
>>> Every so often (I've not quite figured out the exact set of
>>> circumstances yet)
>>
>> Figure that circumstances out.  That is the key to the issue.
>> Something in your workflow is futzing with the inode data of the
>> files in your working tree behind your back.  It sometimes is a
>> virus scanner.
>>
>> "git diff-*" plumbing commands are meant to be used after running
>> "git update-index --refresh" once in the program and when the caller
>> of these commands (in your case, gitk) knows that any change in the
>> information returned by lstat(2) on the paths in the working tree
>> files since that call indicate real changes to the files.
>>
>> "git status" internally runs an equivalent of "--refresh" before it
>> goes to find changes, so after running it, until that something
>> smudges the inode data behind your back, "gitk" will not be
>> confused.
-- 
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

← back to recent threads