Git diff-file bug?

Git diff-file bug?

[Scott Batchelor]

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.

Re: Git diff-file bug?

[Jeff King]

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().

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

[Jeff King]

On Fri, Sep 28, 2012 at 04:23:30PM -0400, Jeff King wrote:

> > 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

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

[Junio C Hamano]

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.

>  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"

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

[Andreas Schwab]

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."

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

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?

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

[Jeff King]

On Fri, Sep 28, 2012 at 04:02:32PM -0700, Junio C Hamano 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?

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

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

[Junio C Hamano]

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 ;-)

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

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> 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."

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

[Paul Mackerras]

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.

Paul.

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

[Jeff King]

On Sun, Sep 30, 2012 at 10:05:27AM +1000, Paul Mackerras wrote:

> 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

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

[Jonathan Nieder]

Hi Jeff,

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. ;-)

Hope that helps,
Jonathan

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

[Jeff King]

On Sun, Sep 30, 2012 at 01:34:58PM -0700, Jonathan Nieder wrote:

> 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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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).

Re: Git diff-file bug?

[Junio C Hamano]

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.

Re: Git diff-file bug?

[Scott Batchelor]

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:
> 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.
>

Re: Git diff-file bug?

[Drew Northup]

On Wed, Oct 3, 2012 at 4:04 AM, Scott Batchelor
<scott.batchelor@gmail.com> wrote:
> 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)

> 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