threads / discuss / 23363

gitk pays too much attention to file timestamps

Subject: gitk pays too much attention to file timestamps

## tl;dr

15 messages between Apr 6, 2010 and Apr 7, 2010.

replies: 14people: 7as markdown or json

Alexander Gladysh· Apr 6, 2010, 22:57 UTC · lore
Hi, list!

OS X 10.6.3 Git 1.7.0.4

When I "touch" a file, gitk lists it in "local uncommitted changes, not checked in to index" (without a difference, just a name). I believe that it should not.

Git status, and git commit do ignore such files.
After git reset --hard, gitk stops seeing changes as expected.
See steps to reproduce below.

HTH, Alexander.

$ mkdir touch $ cd touch $ git init $ echo "A" > alpha $ git add alpha $ git commit -m "alpha" $ touch alpha $ git status # On branch master nothing to commit (working directory clean) $ gitk --all

Observe "local uncommitted changes, not checked in to index"
Markus Heidelberg· Apr 6, 2010, 23:15 UTC · re: Alexander Gladysh · lore

Re: gitk pays too much attention to file timestamps

Alexander Gladysh, 2010-04-07 00:57:
Show 12 quoted lines
> Hi, list!
> 
> OS X 10.6.3
> Git 1.7.0.4
> 
> When I "touch" a file, gitk lists it in "local uncommitted changes,
> not checked in to index" (without a difference, just a name). I
> believe that it should not.
> 
> Git status, and git commit do ignore such files.
> 
> After git reset --hard, gitk stops seeing changes as expected.

The problem is that gitk doesn't invoke "git update-index --refresh", I guess it should on Update (F5) and Reload (Ctrl-F5).

Markus
Jonathan Nieder· Apr 6, 2010, 23:36 UTC · re: Alexander Gladysh · lore

Re: gitk pays too much attention to file timestamps

Hi!
Alexander Gladysh wrote:
> When I "touch" a file, gitk lists it in "local uncommitted changes,
> not checked in to index" (without a difference, just a name). I
> believe that it should not.
First, an explanation:

In general, git keeps some stat(2) information to tell whether an entry in the index is "dirty". This way, low-level commands can compare that to the metadata in the file to avoid a costly comparison of actual objects to the content of files on disk.

Before starting work, the high-level commands like ‘git commit’ will generally update this stat(2) information in one pass before doing anything else. This turns any "dirty" entries without actually different content in the index into clean entries, so the user doesn’t have to worry about anything except content for these commands. You can request this update at any time yourself with the following command:

  git update-index --refresh -q

Unlike ‘git checkout HEAD -- .’ this does not touch the files in your work tree, and unlike ‘git reset HEAD’, it does not affect the content registered in the index for your files.

Okay, on to your topic:

gitk is something of a passive observer of the index, which is actually something I like about it. This keeps it relatively fast and can be useful when trying to understand other commands.

I am not sure how other people use gitk, though. Maybe this would be worth changing. For a reference point, another command in a very similar situation is ‘git diff’: people who want the speedup from avoiding refreshing the index with that command use

	[diff]
		autoRefreshIndex = false

in their configuration file, so the rest of us don’t have to suffer from the confusing behavior.

As some kind of evil compromise, it might be worth teaching gitk to check the same configuration and run update-index --refresh in getcommits{} if and only if it is unset or set to true.

Thoughts? Jonathan

Alexander Gladysh· Apr 6, 2010, 23:47 UTC · re: Jonathan Nieder · lore

Re: gitk pays too much attention to file timestamps

Jonathan,
thank you for the explanation!
<...>
> gitk is something of a passive observer of the index, which is
> actually something I like about it.  This keeps it relatively fast
> and can be useful when trying to understand other commands.
I *think* that 1.6.x didn't have this issue. (Sorry, can't check now.)
> I am not sure how other people use gitk, though.  Maybe this would
> be worth changing.  For a reference point, another command in a
> very similar situation is ‘git diff’: people who want the speedup
> from avoiding refreshing the index with that command use
>        [diff]
>                autoRefreshIndex = false
> in their configuration file, so the rest of us don’t have to suffer
> from the confusing behavior.
> As some kind of evil compromise, it might be worth teaching gitk
> to check the same configuration and run update-index --refresh in
> getcommits{} if and only if it is unset or set to true.
> Thoughts?
That's fine as far as I'm concerned.
The current behaviour is really annoying.

If I want something fast, I do not use GUI tools. Gitk starts up rather slowly on my box anyway.

Alexander.
Jonathan Nieder· Apr 7, 2010, 00:43 UTC · re: Alexander Gladysh · lore

[PATCH/RFC] gitk: refresh index before checking for local changes

Most git porcelain silently refreshes stat-dirty index entries. Teach gitk to, too; this will make the behavior easier to understand when a person makes a change to a file and then changes mind and restores the old version in her editor of choice.

This patch does not change the ‘checkout’ code path, since it is assumed that the index is already being cleaned in that case.

Testing is needed to check if this breaks operation with read-only access to a repository.

Requested-by: Alexander Gladysh <agladysh@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Alexander Gladysh wrote:
> Jonathan Nieder wrote:
[some nonsense about configurability]
> That's fine as far as I'm concerned.
> 
> The current behaviour is really annoying.

Maybe that could be added in the future. From testing this out, refreshing unconditionally seems fast enough, at least.

 gitk |   50 +++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/gitk b/gitk
index 1f36a3e..2753446 100755
--- a/gitk
+++ b/gitk
@@ -375,7 +375,7 @@ proc start_rev_list {view} {
 	get_viewmainhead $view
     }
     if {$showlocalchanges && $viewmainheadid($view) ne {}} {
-	interestedin $viewmainheadid($view) dodiffindex
+	interestedin $viewmainheadid($view) dorefreshindex
     }
     fconfigure $fd -blocking 0 -translation lf -eofchar {}
     if {$tclencoding != {}} {
@@ -4974,9 +4974,9 @@ proc doshowlocalchanges {} {
 
     if {$viewmainheadid($curview) eq {}} return
     if {[commitinview $viewmainheadid($curview) $curview]} {
-	dodiffindex
+	dorefreshindex
     } else {
-	interestedin $viewmainheadid($curview) dodiffindex
+	interestedin $viewmainheadid($curview) dorefreshindex
     }
 }
 
@@ -4992,6 +4992,42 @@ proc dohidelocalchanges {} {
     incr lserial
 }
 
+# spawn off a process to refresh the index
+proc dorefreshindex {} {
+    global lserial showlocalchanges isworktree vfilelimit curview
+
+    if {!$showlocalchanges || !$isworktree} return
+    incr lserial
+    set cmd "|git update-index --refresh -q"
+    if {$vfilelimit($curview) ne {}} {
+	set cmd [concat $cmd -- $vfilelimit($curview)]
+    }
+    set fd [open $cmd r]
+    fconfigure $fd -blocking 0
+    set i [reg_instance $fd]
+    filerun $fd [list readrefreshindex $fd $lserial $i]
+}
+
+# update-index --refresh -q finished?
+proc readrefreshindex {fd serial inst} {
+    global lserial
+
+    if {$serial != $lserial} {
+	stop_instance $inst
+	return 0
+    }
+    if {[gets $fd line] == 0} {
+	# ignore output
+	return 0
+    }
+    if {![eof $fd]} {
+        return 1
+    }
+    stop_instance $inst
+    dodiffindex
+    return 0
+}
+
 # spawn off a process to do git diff-index --cached HEAD
 proc dodiffindex {} {
     global lserial showlocalchanges vfilelimit curview
@@ -7504,13 +7540,9 @@ proc getblobdiffs {ids} {
     global git_version
 
     set textconv {}
-    if {[package vcompare $git_version "1.6.1"] >= 0} {
-	set textconv "--textconv"
-    }
+    set textconv "--textconv"
     set submodule {}
-    if {[package vcompare $git_version "1.6.6"] >= 0} {
-	set submodule "--submodule"
-    }
+    set submodule "--submodule"
     set cmd [diffcmd $ids "-p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext"]
     if {$ignorespace} {
 	append cmd " -w"
-- 
debian.1.7.0.3.1.469.g398f8
Alexander Gladysh· Apr 7, 2010, 01:07 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC] gitk: refresh index before checking for local changes

On Wed, Apr 7, 2010 at 04:43, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Most git porcelain silently refreshes stat-dirty index entries.  Teach
> gitk to, too; this will make the behavior easier to understand when a
> person makes a change to a file and then changes mind and restores the
> old version in her editor of choice.
This patch fixes my problem, thank you!
Alexander.
Jonathan Nieder· Apr 7, 2010, 01:16 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC] gitk: refresh index before checking for local changes

Hi,
I’ll start off the reviewing, too:
Jonathan Nieder wrote:
> +    if {![eof $fd]} {
> +        return 1
Whitespace damage.
Show 16 quoted lines
> @@ -7504,13 +7540,9 @@ proc getblobdiffs {ids} {
>      global git_version
>  
>      set textconv {}
> -    if {[package vcompare $git_version "1.6.1"] >= 0} {
> -	set textconv "--textconv"
> -    }
> +    set textconv "--textconv"
>      set submodule {}
> -    if {[package vcompare $git_version "1.6.6"] >= 0} {
> -	set submodule "--submodule"
> -    }
> +    set submodule "--submodule"
>      set cmd [diffcmd $ids "-p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext"]
>      if {$ignorespace} {
>  	append cmd " -w"

What does this have to do with the topic at hand? (Sorry, I lumped in a local fix that still needs to be generalized and submitted separately.)

So this patch is in good enough shape to try out, but please ping me for a new one before it is time to send something like it on to the masses.

Sorry for the noise, Jonathan

A Large Angry SCM· Apr 7, 2010, 02:21 UTC · re: Jonathan Nieder · lore

Re: [PATCH/RFC] gitk: refresh index before checking for local changes

Jonathan Nieder wrote:
Show 11 quoted lines
> Most git porcelain silently refreshes stat-dirty index entries.  Teach
> gitk to, too; this will make the behavior easier to understand when a
> person makes a change to a file and then changes mind and restores the
> old version in her editor of choice.
> 
> This patch does not change the ‘checkout’ code path, since it is
> assumed that the index is already being cleaned in that case.
> 
> Testing is needed to check if this breaks operation with read-only
> access to a repository.
> 

NAK - gitk should not modify a repository and/or working dir unless _explicitly_ prompted to by the user.

If you want a new _non-default_ option setting for gitk, that fine also.
Jonathan Nieder· Apr 7, 2010, 02:57 UTC · re: A Large Angry SCM · lore

Re: [PATCH/RFC] gitk: refresh index before checking for local changes

A Large Angry SCM wrote:
[context: should gitk run ‘gitk update-index --refresh -q’ transparently?]
> NAK - gitk should not modify a repository and/or working dir unless
> _explicitly_ prompted to by the user.
>
> If you want a new _non-default_ option setting for gitk, that fine also.
I have some sympathy for this point of view.
Does the same principle apply to ‘git diff’?  If not, why not?

It should be possible to suppress files with no changes from the display without refreshing the index (or not to suppress them and to still refresh the index, nor that matter; the issues are orthogonal), but it would be nice to come up with a clear rationale first.

Jonathan
Junio C Hamano· Apr 7, 2010, 05:47 UTC · re: A Large Angry SCM · lore

Re: [PATCH/RFC] gitk: refresh index before checking for local changes

A Large Angry SCM <gitzilla@gmail.com> writes:
> NAK - gitk should not modify a repository and/or working dir unless
> _explicitly_ prompted to by the user.

I used to think that way, until I realized that gitk has operations like "Tag this commit" that does write into the repository.

A Large Angry SCM· Apr 7, 2010, 11:21 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] gitk: refresh index before checking for local changes

Junio C Hamano wrote:
Show 8 quoted lines
> A Large Angry SCM <gitzilla@gmail.com> writes:
> 
>> NAK - gitk should not modify a repository and/or working dir unless
>> _explicitly_ prompted to by the user.
> 
> I used to think that way, until I realized that gitk has operations like
> "Tag this commit" that does write into the repository.
> 

Does that happen every time you run gitk or only when the gitk user instructs gitk to?

Avery Pennarun· Apr 7, 2010, 16:48 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] gitk: refresh index before checking for local changes

On Wed, Apr 7, 2010 at 1:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> A Large Angry SCM <gitzilla@gmail.com> writes:
>> NAK - gitk should not modify a repository and/or working dir unless
>> _explicitly_ prompted to by the user.
>
> I used to think that way, until I realized that gitk has operations like
> "Tag this commit" that does write into the repository.

Also 'git status' does the same "modify a repository" operation as we're currently discussing, so calling it modification is a bit of an exaggeration.

Avery
Jon Seymour· Apr 7, 2010, 14:36 UTC · re: A Large Angry SCM · lore

Re: [PATCH/RFC] gitk: refresh index before checking for local changes

On Wed, Apr 7, 2010 at 2:21 PM, A Large Angry SCM <gitzilla@gmail.com> wrote:
Show 5 quoted lines
>
> NAK - gitk should not modify a repository and/or working dir unless
> _explicitly_ prompted to by the user.
>
> If you want a new _non-default_ option setting for gitk, that fine also.
gitk and git gui currently behave differently in this regard.
git gui updates the indexes view of the working tree on start, gitk does not.
gitk's current behaviour is somewhat mysterious to the uninitiated -

user: "what? I have local changes?" tool: "relax dear user what you see is dirty laundry. if you want to see the actual state, start git gui then come back to me and refresh me when you are done"

gitk is effectively lying to the user about the state of their working tree. git gui does not.

Needless to say, I support this change.
jon.
Show 5 quoted lines
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Avery Pennarun· Apr 6, 2010, 23:58 UTC · re: Jonathan Nieder · lore

Re: gitk pays too much attention to file timestamps

On Tue, Apr 6, 2010 at 7:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 8 quoted lines
> gitk is something of a passive observer of the index, which is
> actually something I like about it.  This keeps it relatively fast
> and can be useful when trying to understand other commands.
>
> I am not sure how other people use gitk, though.  Maybe this would
> be worth changing.  For a reference point, another command in a
> very similar situation is ‘git diff’: people who want the speedup
> from avoiding refreshing the index with that command use

Isn't it kind of weird, then, that it bothers to check the stat information at all? I'm not sure why I'd want to know accurately whether or not my file matches the index, but then have gitk fail to tell me about *what* doesn't match. If it has to check the stat() information anyway, isn't it already being slow?

Avery
Jonathan Nieder· Apr 7, 2010, 01:01 UTC · re: Avery Pennarun · lore

Re: gitk pays too much attention to file timestamps

Avery Pennarun wrote:
> I'm not sure why I'd want to know accurately
> whether or not my file matches the index, but then have gitk fail to
> tell me about *what* doesn't match.
Ah, yes, I am not thinking very well.  Please forget what I said before.

v1.5.3-rc5~10 (git-diff: squelch "empty" diffs, 2007-08-03) has the answer for your question. I am not sure I agree with its conclusion.

v1.5.3~8 (git-diff: resurrect the traditional empty "diff --git" behaviour, 2007-08-31) tweaked the porcelain more by dropping the warning message and adding the diff.autorefreshindex variable.

What is missing is a --refresh-index option for diff-files and diff-index, to make it easy for porcelain, especially long-running porcelain like gitk.

Jonathan

← back to recent threads