gitk pays too much attention to file timestamps

gitk pays too much attention to file timestamps

[Alexander Gladysh]

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"

Re: gitk pays too much attention to file timestamps

[Markus Heidelberg]

Alexander Gladysh, 2010-04-07 00:57:
> 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

Re: gitk pays too much attention to file timestamps

[Jonathan Nieder]

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

Re: gitk pays too much attention to file timestamps

[Alexander Gladysh]

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.

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

[Jonathan Nieder]

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

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

[Alexander Gladysh]

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.

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

[Jonathan Nieder]

Hi,

I’ll start off the reviewing, too:

Jonathan Nieder wrote:

> +    if {![eof $fd]} {
> +        return 1

Whitespace damage.

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

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

[A Large Angry SCM]

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

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

[Jonathan Nieder]

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

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

[Junio C Hamano]

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.

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

[A Large Angry SCM]

Junio C Hamano wrote:
> 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?

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

[Avery Pennarun]

On Wed, Apr 7, 2010 at 1:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Jon Seymour]

On Wed, Apr 7, 2010 at 2:21 PM, A Large Angry SCM <gitzilla@gmail.com> wrote:
>
> 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.


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

Re: gitk pays too much attention to file timestamps

[Avery Pennarun]

On Tue, Apr 6, 2010 at 7:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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

Re: gitk pays too much attention to file timestamps

[Jonathan Nieder]

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