git-diff on touched files: bug or feature?

Hi,

When a file is "touched" (ie. stat information not matching the index, but the content still matching), git-status doesn't report the file as modified (as expected), but git-diff does (with an empty diff):

$ git st # On branch master nothing to commit (working directory clean) $ ls bar $ touch bar $ git diff

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

Yes. Very much so, intentionally, from very early days of git. This serves as a reminder to the user that he started editing but changed his mind to end up with the same contents as the original, until the next "update-index --refresh" (which is internally invoked from "status").

If the feature still makes sense in the modern world is a different story, but I do find it useful.

Not the made-up "touch" example, but often in real life, I explore a solution by first making changes to one part of the system, realizing a better way is to change the caller of what I initially thought should be changed, edit the file back and modify the caller which is in another file. The former file will show that empty "header-only" diff as the reminder of what I did.

After that, when I reach the point to run "git status", because I have been reminded, I already know about these "tried but discarded" changes, and I find the fact that the they are forgot by that operation very convenient.

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

It would be nice to have a way to refresh a single file though. For instance in vc-git.el the workfile-unchanged-p function currently has to rehash the file every time to see if it really changed, because we can't afford to refresh the whole project at that point.

Alexandre Julliard <julliard@winehq.org> writes:

Maybe I am missing something.  Why can't you "afford to"?

"update-index --refresh" looks at only the files whose cached stat information does indicate there might be chanegs. It does not rehash already up-to-date ones.

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

No, but it goes through the tree and stats every single file. On a large project that can be slow, especially if you don't have enough RAM to keep it all in cache, so it's not something we can do while the user is interacting with a file.

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

I understand that it can be usefull, but I really don't like having it by default (is there a way to deactivate it BTW?):

I've hit this while working on a project, doing a lot of modifications through scripting (some regexp substitutions and such kinds of things). Then, git-diff shows me pages of "diff --git ...", and a few relevant entries in the middle of it. That's very bad from the usability point of view (I actually had some ~20 lines diff surrounded by 100+ irrelevant lines), and also kills performance: if a script touches a lot of files, I expect the next "diff" or "status" to be slow, but not the second next. Here, diff will be slow until I run git-status again.

And I find the "reminder" feature very fragile. That means git-status is no longer a read-only operation for the user. As a user, I expect to be able to run git-status without changing the behavior of subsequent git commands, which is not the case here. That means for example that someone used to running git-diff /before/ git-status will get the reminder, while someone used to running git-diff /after/ git-status (which I find sensible, get an overview before getting the details of what you did) won't get it. Note also that this makes a difference between git-status (which updates the stat in the index) and git-status -a (which doesn't). That's an implementation detail that shouldn't be exposed to the user.

Since I don't see any mention of this in the man pages for git-diff or git-status (I might have missed it), I wonder how many user actually ever used this as a feature.

I'd be in favor of disabling this by default, and providing a configuration option and/or a command line option to diff to enable it. I can try writting a patch for this if people agree on the specification.

Hi,

On Thu, 2 Aug 2007, Matthieu Moy wrote:

Yes.  Just call "git status" and be done with it.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

That's not what I mean (my original message mentionned that already BTW). By "deactivate", I mean "make git-diff never show empty diffs". I don't want to run two commands where I need only one.

Hi,

On Thu, 2 Aug 2007, Matthieu Moy wrote:

Then don't touch the files you do not want to touch! Or if you want to have it convenient, and have a script that touches everything, even if it does not change the contents, just add "git add -u" at the end of the script". Not that difficult.

Ciao, Dscho

I find comments like this to be counter productive. Admin it, git porcelain still has some work to be done. We can't expect new users to know the git internals workflow before they can use git effectively. We can expect new users to read the man pages, but not necessarely expect them to understand all the plumbing implied by what they read. Here I think M.Moy understand the plumbing and could silently deal with it. But instead he decided to help improve git and decided to raise a flag about inconsistencies he faced. We should never answer request for improvement with ' Just do X and be done with it'. This is a 'geek' answer to a legitime comment.

I know the goal of git is not to reign over the world of vcs, but it's not a reason to refuse to improve it when constructive comments are made about it.

- jfv

Le 07-08-02 à 06:48, Johannes Schindelin a écrit :

Hi,

[please do not top-post.  Either comment on what you quote, or delete it.]

On Thu, 2 Aug 2007, Jean-Fran?ois Veillette wrote:

No need to argue there, I admit it.

This use case has not much to do with new users. A new user _has_ to know that updating all files, even if their content does not change, is not right.

At least we do not commit empty changes like CVS did all too happily.

Ciao, Dscho

Johannes Schindelin wrote:

Someone who has used, say, Subversion might have a perfectly reasonable expectation that "git diff" will show differences in content, and when there are no differences in content, will not mention a file at all. Other version control systems have "diff" commands that ignore touched files.

I admit I also thought the empty diffs were a bug (albeit a minor one not worth making noise about) until this thread. Now I understand why it happens, though I still think we'd be better off just not displaying the filename in git-diff until we know there's an actual diff to display.

I certainly don't think the "it's a feature: it reminds you when you've edited a file without changing it" argument holds any water at all. If that were truly the intent, if we truly considered that to be useful information a developer would want to get at after the fact, then why would git-status throw away that information? If I check to see what files I've modified/added (for which I run git-status) why does that automatically imply I am no longer interested in being reminded that I have saved a file without making changes, especially given that such files *don't* show up in the git-status output? git-status is silently losing information here; it gives you no indication that it has refreshed the index for those touched-but-not-edited files.

Now, I happen to think throwing away that information is just fine, because I don't think I have ever once cared to know that I touched a file but didn't change it. But fundamentally it's either a piece of information we care about (in which case we shouldn't go silently discarding it) or not (in which case it is just clutter in git-diff).

In the meantime, though, it's trivial enough to put a wrapper around git-diff to filter out the diffless files. I haven't cared enough to bother, but if I did it'd be just a few lines of Perl, no big deal.

-Steve

Hi,

On Thu, 2 Aug 2007, Steven Grimm wrote:

Okay, I'll answer just this one, instead of pointing you to the thread that I've been pointing to twice now (because your ideas about how git should work are usually similar to mine, and by way of saying thanks for your contributions):

When is the time to say "git status"?

It is just before committing. I.e when you really think that you're done editing, and want to have the end picture. "git status" only gives you names, and therefore it _has_ to update the index if it got out of sync, to show meaningful results.

When is the time to say "git diff"?

Much more often. In the middle of your work. And there it would be _disruptive_ if it updated the index all the time, especially if you have a quite large working tree.

But then, normal users do not touch all the files.  They don't.

So I doubt that in the common case the subject we are discussing matters at all.

Yes, "perl -pi" is something I used myself. Yes, I think it is a bug that it writes new files when it does not really change anything. And yes, I had a script lying somewhere on my backup hard disk which uses some evil "git diff --name-only | xargs bla" mantra (it does not even use the --quiet option, since that was not invented back then) to actually _undo_ the effects by setting the timestamps back, since the full compilation time in that project _hurt_.

But it is hardly an operation that I use daily. Hardly even twice a year.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

The link was probably not the right one since I saw only two [PATCH] message, but yes, I remember a thread pointing out why git-status had to update the index. I'm not arguing against that, I'm happy with the current git-status behavior, but I find the git-diff one inconsistant with git-status, and still don't understand any reason why a normal user would want a difference.

That's your point of view, but I do not share it. Depending on the kind of things I'm doing, I usually run status regularly, because it's short to read, and it shows me both staged and unstaged changes. git-status tells me if I did something obviously totally wrong (changing a file on which I was not working, deleting something important ...), and after that, git-diff gives me a finer-grained vision of what I did.

Does this really sound so much irreasonable to you?

On Thu, Aug 02, 2007 at 05:45:20PM +0200, Matthieu Moy wrote:

Yeah, ditto for me. When I return to a project after having been away a few minutes, the first things I do are

	git branch	# remind me which topic I was working on
	git status	# remind me if I was in the middle of something.

So I end up running it a lot. I only do a git-diff if I need some details.

--b.

The default is now to not show the diff --git header line if the file's timestamp has changed but the contents and/or file mode haven't.

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---

	Okay, enough arguing about whether the empty diff lines are
	useful or not -- here's a patch to get rid of them.

	This passes all the existing "diff" tests, with one minor tweak
	to the symlink test (since it expected the old behavior.)
	If someone can find a case where this will spit out an actual
	diff but not the "diff --git" line, please tell me how to make
	that happen. The code *looks* like it has such a path, but I was
	unable to make it happen in my ad-hoc testing and it doesn't
	happen in any of the existing diff test cases.

	Personally I'm in favor of doing away with the option altogether
	and having the code always work the way it works by default with
	this patch, but if some people find the old behavior useful they
	can still get at it with the new option.

	My xmalloc() call allocates a few more bytes than strictly
	needed, but I found it was less readable to subtract out the
	space taken by the "%s" tokens in the format string.

 Documentation/diff-options.txt |    4 ++
 diff.c                         |   46 ++++++++++++++++++++++++++----
 diff.h                         |    3 +-
 t/t4011-diff-symlink.sh        |    2 +-
 t/t4021-diff-untouched.sh      |   61 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 108 insertions(+), 8 deletions(-)
 create mode 100755 t/t4021-diff-untouched.sh

Steven Grimm <koreth@midwinter.com> writes:

I do not think this addresses anything but -p (i.e. textual diff) output. If we _were_ to really do this, I think the patch I sent earlier today, with possible improvements I suggested, would be a better direction to go.

Hi,

On Thu, 2 Aug 2007, Junio C Hamano wrote:

But I'd really think that what should be done (if anything has to be done at all) is to introduce a config variable which triggers the same logic in git-diff as was introduced in 2b5f9a8c0cff511f2bb0833b1ee02645b79323f4.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Sorry, I don't follow at all. The diff toolchain works all inside core without having to write a temporary index out, which was the issue the commit you are quoting was about.

In any case, enough discussion. Here is an updated patch, which I _could_ be pursuaded to consider for inclusion after v1.5.3 happens, if there are enough agreements and Acks.

-- >8 -- git-diff: --stat-unmatch

Traditionally, git-diff with the working tree files showed files whose lstat(2) information did not match with what were recorded in the index, even if the actual contents did not have any differences. This squelches such output from git-diff by default.

A new option, --stat-unmatch, is introduced to restore the traditional behaviour. This is useful to see if you want to know you have too many files you only touch(1)ed without modifying. Having many such paths hurts performance, and you can run "git-update-index --refresh" to update the lstat(2) information recorded in the index in such a case.

The low level git-diff-files command is not affected.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Documentation/git-diff.txt |   13 ++++++++++
 builtin-diff.c             |   13 ++++++++++
 diff.c                     |   54 ++++++++++++++++++++++++++++++++++++++++++++
 diff.h                     |    1 +
 4 files changed, 81 insertions(+), 0 deletions(-)

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

To me, that patch makes sense, yes.

That said, a configuration option would probably be better than a command-line option. The expected behavior seems to depend on user, but not much on use-cases. So, people who like the old behavior could set the option and forget about it, and other would not be distracted about it.

Also, is there any particular reason not to update the index stat information when files are found to be identical? Well, we've discussed that quite much in this thread, but this is what status is doing, and I still fail to see why diff shouldn't. (I can update the patch myself if you agree it should be done. I don't know the git codebase well, so it takes time for me, but the exercise is fun ;-) )

Side performance note: if I understand your patch correctly, you're comparing the file content twice for actually modified changes. But that probably doesn't matter much since the expansive thing is to load the files and to compute the diff.

Thanks,

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

Very much --- diff is a read-only operation.

"git-status $args" on the other hand is a preview of "what would happen if I say 'git-commit $args'", and in order to compute that, you would fundamentally need to be able to write into the object store. In a special case of giving empty $args it can be read-only. The commit Dscho quoted earlier was to hack that around so that "git-status" can pretend to be a read-only operation in a repository you do not have write permission to.

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

Can you give an example where it _could_ not be read-only?

I really, really, really, don't understand your argument.

There are two different concepts:

* Should git-diff be "read-only for the user"?
  (i.e. external specification)

* Should git-diff be actually read-only for the filesystem?
  (i.e. implementation)

"read-only for the user" is a user-interface thing. It just means that running

$ git-diff >& /dev/null; whatever

will give the same result as

$ whatever

In another context, "cat", for example, is a read-only operation for the user. I can run "cat whatever-file" without influencing the behavior of subsequent operations. Now, somewhere in the kernel of my OS, I do hope that reading this file will have side-effects (putting the file in the cache, and why not decide to physically move the file on disk).

Here, obviously, git-diff is a read-only operation for the user. I don't expect git-diff to modify the behavior of subsequent commands, but I appreciate if git-diff can improve the speed of subsequent commands.

Now, both of us agree that git-status should not be read-only for the filesystem, both of us agree that git-diff should be read-only for the user, but we disagree on the two other cases.

In the same way, I expect git-status to be read-only for the user. You say "what _would_ happen _if_ I say commit $args". But you don't commit, the sentence is conditionnal. I don't expect any tool to have visible side-effects when I say "what would happen if ...".

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

Think of what "git commit -a" would have to do. It needs to hash and deposit a new object for blobs that have been modified. Where do those new blob object go?

By running git-status the user is asking to get the overall picture, discarding the "touched but not modified" information. Once you _HAVE TO_ refresh the index for whatever reason, it is better to keep the result of the effort and cycles spent for that refresh operation for obvious performance reasons in practice, and that is what we have now in git-status. IOW, we are practical bunch.

Maybe in a theoretical ideal world, you might prefer to reverting back to the stat-dirty original index to make git-status appear a read-only operation, with continued degraded performance. You are welcome to reimplement it that way, and the patch should be trivial (while git-commit.sh is still a script, at least) but that is not what we did.

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

I don't know whether it was a typo, but we're not talking about "commit", but "status".

git-status _does_ hash and deposit new objects, but it doesn't _need_ to. It can very well show you what "commit -a" would do without actually doing it.

A trivial (and very stupid, yes) way to do this would be

cp -r . /tmp/git/ cd /tmp/git git-status -a

There's no visible side-effects for the user.

IIRC, git-status -a does actually "git-add" the modified objects, but does so in a temporary index, so I believe the objects you leave in the objects database are not pointed to by anyone (indeed, I just checked, git-fsck --unreachable shows the dangling blob), and are not really useful (but will probably be used later when you run commit or add).

You still didn't understand my point about the difference between user-specification and internal behavior. I'm very happy with git-status updating the stat information in the index, since it is not suppose to have user-visible side effects (it has with the current empty-diff-for-touched-files behavior of git-diff).

Now, at that point, if I still didn't manage to show you the difference between user-visible behavior and implementation, I believe I have no better thing to do than giving up.

Hi,

On Sun, 5 Aug 2007, Matthieu Moy wrote:

No typo. "git status" is literally "git commit --dry-run". Why? Because people expected it to be called "git status".

And even if I think about it over and over again, it makes sense.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I was surprised of that when looking at the source, but yes, it makes sense.

But the message I was replying to claimed that "git status-or-commit -a" _needed_ to put objects in the object database. That's true of "git commit -a" but not of "git status -a", even if you call it "git commit --dry-run -a", precisely because of the --dry-run thing.

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

I like this new behaviour but I don't see the old one too often either.

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

Ack, ack, ack. The current default behavior is plainly unusable. For example, I've rsynced -a a tree including .git, and suddenly git-diff goes out of kilter. And stops doing so when running git-status once.

This is the worst kind of "unpredictable", and I don't care one bit that there are conceivable use cases.

I don't even think it prudent to _offer_ the --show-touched option in a porcelain such as git-diff as long as purportedly read-only porcelain commands like git-status can trash the state: what is reported is not actually "touched" but something internal to the operation of git.

At least not without a notice in the manual that this option might or might not work, depending on what one did previously.

David Kastrup <dak@gnu.org> writes:

Proposal: if this option is to stay, call it rather --show-stale since
that corresponds better with what the option actually does: show
whether git's inode cache went stale.  It does _not_ show whether the
file has been touched (git-status does not touch files, for example).

David Kastrup <dak@gnu.org> writes:

Unfortunately, the patch solves the "large and irrelevant output" of git-diff, but not the performance problem (see the rest of the thread, I failed to convince Junio that updating the index was a performance improvement while keeping the same user semantics).

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

That's what update-index --refresh (or status if you insist) are for, and the coalmine canary you are so dead set to kill are helping you realize the need for running.

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

That does not convince me. Cache staleness should be a problem of git, not of the user. In particular if the user is just using porcelain. If letting the cache get stale impacts performance, then git should clean up its act on its own without barfing when using unrelated commands. If it notices this during diff (presumably by overstepping some staleness ratio), then it can set a "regenerate on next opportunity" flag on the index, and then the next command wanting to process the index from the start can rewrite a refreshed version.

On Tue, Aug 07, 2007 at 08:32:55AM +0200, David Kastrup wrote:

The last time I had a serious problem with "cache staleness", it was with Beagle, which modifies the files it indexes (by writing some extended attributes). I figured out what was happening when I noticed that the list of touched files was growing each time I did a diff (implying the something was working on them right then), so I ran top, noticed beagled, eventually thought to query the extended attributes, and finally turned off beagled's indexing to solve the problem.

So, in this case:

	- If git had fixed up the problem silently, I probably would
	  have just assumed git was slow and not found the problem.

	- Seeing the actual list of files for which the index was dirty
	  helped me identify the problem.  I probably would have
	  eventually figured it out even if all I'd had was a single
	  "index is stale" message, but I suspect it would have taken
	  longer.

Draw whatever moral you'd like....

--b.

On Thu, 2 Aug 2007, Steven Grimm wrote:

I don't mind this per se, but I'd *really* want some kind of warning that the index is not up-to-date.

Otherwise, git usage can be horrendously slow, and you're never even told why. The diffs just take lots of time (because it reads each file), but the output is empty.

It's not that the old output is "useful" in itself, but it's important for people to know that the index is clean. So I'd suggest just setting a flag when the header isn't printed, and then printing out a single line at the end about "git index not up-to-date" or something.

Doing a "git diff" cannot actually update the index (since it very much has to work on a read-only setup too), which is why the index _stays_ stale unless something is done (eg "git status") to refresh it. And it's that stale index that continues to make for bad performance without any indication of why that is a problem.

			Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

That's essentially the patch I sent out in another thread allows you to do, and some of these people even Acked them, but there is one minor issue. "git diff" output is paged, and that "not up to date" warning, if it is given to stderr, would not be usually seen.

Indeed.

At least, I am now glad to know that somebody else is of the same opinion as I am.

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

I don't want a system to tell me when it is shooting itself in the foot. It should not be doing this in the first place.

File systems have automatic fsck procedures enforced regularly, too, to keep them operative. If git finds that it is getting inefficient, it should just mark the index as "regenerate at next access". And then do it.

[ Slow at responding to email, sorry ]

On Mon, 6 Aug 2007, Junio C Hamano wrote:

I agree. I wouldn't send it to stderr, I'd literally make it part of the output.

Everybody who takes patches will accept crud afterwards, since the normal thing is to email them around, so there's no real downside to adding some status output at the end. It shouldn't screw anything up, but people will hopefully notice (sure, if you exit the pager without looking at it all you wouldn't notice, but that's _already_ true, so..)

		Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Well, at least "hundreds of empty diff" has a value of getting attention for even such a use case, so you _could_ argue this patch is a regression ;-)

In any case, this will go in as part of the first batch after 1.5.3, I would guess.

Hi,

On Tue, 7 Aug 2007, Linus Torvalds wrote:

But then, you could output the message _twice_: first possibly in-between patches ("WARNING: ...") and then at the end.

However, I have the slight suspicion that people will not even notice. I mean, we had bug reports on merge-recursive, where the reporter failed to even acknowledge the fact that merge-recursive said that there were conflicts, and even listed them.

So I have the slight suspicion that all this will accomplish is "shut the darn thing up", and old-timers will have a harder time, since they no longer spot easily when they did a Dumb Thing and left the index out of sync.

Slightly negative.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

The hardest hit would be old-timers who try to be friendly by trying to help new people, who has much less chance to notice and report these much less prominent warnings, over e-mail or irc.

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

"Dude, you got a stale index hanging out of your trousers."

I find it ridiculous to parade local problems in patches sent out to the world rather than fixing them, so that old-timers have a chance to get karma points.

Really: if stale indexes are considered a problem, git should silently
mark the staleness in the file, and the next time (or after three more
times or whatever) the index is used, it is silently regenerated.

Old-timers can get an option disabling this so that they can proud themselves on cleaning up after themselves consciously, but for the normal user, this is a bother he can do without.

Hi,

On Wed, 8 Aug 2007, Junio C Hamano wrote:

True. It is even bigger than that annoyance to people who know how git works.

Ciao, Dscho

Johannes Schindelin wrote:

Perhaps config variable, by default old behaviour if not set?

Linus Torvalds wrote:

Or even a count of the number of files whose index data is unclean. I'd be fine with that as a suffix to the diff output.

I totally agree that there needs to be a way to tell if the index is clean or not. I do wonder if the default output of "git diff" is the right place for that information, but if the notification can be collapsed to a line or two (rather than the unbounded number of lines that it potentially outputs now) then that's probably good enough.

Actually, though this will probably make people roll their eyes, before this discussion I would have guessed that "git status" would be the command that would tell you the index was out of date, and that there'd be a separate command (say, "git update-index"?) that you could then use to sync things up again. The fact that "git status" is really "git update index a little bit then show status" was not something I expected; it presents itself as a query utility, not an update utility, so I would have expected it to be read-only. Its index-modifying behavior is not even hinted at in the documentation (a patch for which follows.)

-Steve

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---
 Documentation/git-status.txt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---
	This is based on (and includes) Junio's patch. This should
	hopefully address the "I want to know when my index is very
	stale" problem with both his original patch and mine.

	If we are running a pager, I output the warning to standard
	output so it doesn't get immediately scrolled off the screen by
	the paged diff output. Otherwise I output to standard error
	which is really the more appropriate place for the warning.
	Obviously that is no good if the user is running his own pager,
	but I'm not sure how to detect that and not cause problems for
	diffs that are piped into other programs.

 diff.c     |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 diffcore.h |    1 +
 2 files changed, 57 insertions(+), 3 deletions(-)

Steven Grimm <koreth@midwinter.com> writes:

Hmph. One way to avoid causing problems for diffs that are piped into other programs and still give the "index of sync" warning is to emit "diff --git" line and no patch body fot textual diffs, or 0{40} SHA-1 on the right hand side for --raw format diffs.

Jokes aside...

For textual diffs, I think we can always spit out the warning message at the beginning of at the end on the standard output without harming any of the patch based toolchain.

So how about...

 - If and only if the output format asks for textual diff
   (DIFF_FORMAT_PATCH), we do this "stat-dirty-removal";
   otherwise we do not spend extra cycles and keep the current
   behaviour.

 - At the end of patch text, show "stat-dirty-removal" warning
   on stdout.

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---
	Modified as suggested by Junio.

 diff.c     |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 diffcore.h |    1 +
 2 files changed, 53 insertions(+), 3 deletions(-)

Steven Grimm <koreth@midwinter.com> writes:

Okay.

Assuming that other people are happy with this version (I have to warn you that I haven't even attempted to apply this patch, let alone compiling yet), I'd prefer to keep our combined thought process in the commit log, so that we do not have to rehash this later, over and over again.

Something along the following lines, perhaps...?

  After starting to edit a working tree file but later when your
  edit ends up identical to the original (this can also happen
  when you ran a wholesale regexp replace with something like
  "perl -i" that does not touch many of the paths), "git diff"
  between the index and the working tree outputs many "empty"
  diffs that show "diff --git" header and nothing else, because
  these paths are stat dirty.  While it was _a_ way to warn the
  user that the earlier action of the user made the index
  ineffective as an optimization mechanism, it was felt too loud
  for the purpose of warning even to experienced users, and also
  resulted in confusing people new to git.

  This replaces the "empty" diffs with a single warning message
  at the end.  When you see such a message, you know you did
  something suboptimal to your index; you can optimize the index
  again by running "git-update-index --refresh".

  The change affects only "git diff" that outputs patch text,
  because that is where the annoyance of too many "empty" diff
  is most strongly felt, and because the warning message can be
  safely ignored by downstream tools without getting mistaken as
  part of the patch.  For the low-level "git diff-files", the
  traditional behaviour is retained.

Junio C Hamano wrote:

That seems like a fine commit message to me. My one-liner definitely assumed too much context, coming in the middle of this discussion as it did -- much better to be explicit about the reasoning for posterity's sake.

-Steve

Junio C Hamano wrote:

Does touch the file (makes file stat-dirty, changes file mtime), but doesn't change it.

Actually this is wrong in two points:

 * The filtering should be done upfront at the beginning of the
   diffcore_std(), not before the end.  Otherwise, unchanged but
   cache dirty file could be subject to copy detection.

 * I do not think it should affect the low-level git-diff-* (or
   you should update the tests, documentations and perhaps
   whatever people can find from google).

By the way, I had an updated version of my patch to fix the first point on 'pu' for a while.

On Tue, 7 Aug 2007, Steven Grimm wrote:

Well, historically, you literally would just do

	git update-index --refresh

to do that.

"git status" is fairly newfangled, and is purely because users from other SCM's expected that kind of command to exist. The fact that as part of it running it does that update-index is really just a side effect.

		Linus

Linus Torvalds wrote:

Another thought: How about if git-diff *tries* to update the index if needed, but failure to do so is not treated as an error condition? That seems like the best of both worlds to me: git would self-correct a potential performance problem without user intervention, while still working properly in a read-only environment.

-Steve

Linus Torvalds <torvalds@linux-foundation.org> writes:

Yes. Junio's patch has this as a comment, it's probably good to uncomment it, and perhaps print it directly on stdout so that you see it even with a pager.

Err, what's the relationship between the two parts of your sentence? You can't be sure that git-diff will update the index (because you may be working on a read-only setup, yes), but git-diff can at least _try_ to, and fall-back to the read-only behavior if updating the index fails.

That's not a highly original idea since this is what git already does with "status".

Once more, I'm willing to write the code for that if it has a chance to be accepted.

Jean-François Veillette <jean_francois_veillette@yahoo.ca> writes:

Yes, but this certainly is not an area that needs changing.

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

You said it yourself below --- run git-status (or update-index --refresh) first.

I have to say that you are quite mistaken.

Scripted style bulk modification that indiscriminately touch everbody but actually only modifies some, e.g. "perl -p -i", is a fine component of people's workflow, but that is *NOT* the norm. If it were, then you are not programming nor editing -- your script is doing the work. But as you know, after such a bulk operation, you can always...

... refresh away the cache-dirtiness.

The default should be tuned for users who perform manual editing with status checks. And power users like yourself who run "bulk touch indiscriminately but modify only some" scripts should learn to run git-status (or "update-index --refresh") after such operation. Swapping the defaults to optimize for the abnormal case is madness.

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

Having said that, there is another lesson to take home from this.

Quite honestly, a script that indiscriminately touches everybody but only modifies a few is simply broken. Think about "make". "git diff" reporting many cache-dirty files is simply reminding you the brokenness of such a script.

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

I wouldn't call this "broken", but clearly suboptimal, yes. But for an occasionnal one-liner (perl -pi -e ... or so), I lose less time recompiling extra-files than I would writting a cleaner script. "make" has no way to detect the absence of modification, while git has.

Hi,

On Thu, 2 Aug 2007, Matthieu Moy wrote:

_You_ can afford compiling them extra-files.

That is _exactly_ what Junio meant by "corner case".

Ciao, Dscho

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

The first part of your sentence makes sense. You are trading "make" time with the reduction in effort to write a throw-away script. That makes sense --- and you pay with a more expensive make, but that is a valid tradeoff you choose to make. You can just choose to make the same tradeoff by running an extra refresh.

You could do something like the trivial patch attached below to squelch diff-files "cache-dirty" output, if you wanted to.

After trying to work with this modified git for some time, however, it has become very clear that doing this and nothing else is a stupid thing to do (I'll suggest potential improvements at the end). For one thing, it obviously loses the "quick git-diff to see what I touched" benefit. And that issue is real, as I first touched diff-lib.c to come up with this until I realized that doing it in here is much cleaner and simpler --- this patch is discarding that information, and makes writing a changelog for this patch, if it were to be included, more expensive for the user -- that is me.

Another thing is that it loses the coal-mine canary value the "cache-dirty" output has. After running a loosely written bulk regexp script, like this:

	perl -p -i -e 's/foo/bar/' *.c

the cached stat data are destroyed for all paths, and it makes all the subsequent git worktree operations needlessly more expensive; existing output makes me realize this situation.

It is not a stupid thing to run such a script [*1*]; in this case, I am choosing the convenience of using such a suboptimal (as you said) script. But after running such a script, I DO WANT git to tell me that I made the index suboptimal, so that I can and should refresh it to gain the lost performance back.

Personally, I almost never run "git status". The command is there primarily because other systems had a command called "status", and migrant wondered why we didn't. We do not need it, and we do not have to use it.

 * For getting the status so-far, I use "git diff" (and "git
   diff ." when in a subdirectory).  It is "how have I changed
   things?", and that is when it is very useful to know "ah, I
   originally went in that direction but decided against it and
   did it differently" by the cache-dirtiness output.  The
   coal-mine canary value is also felt here;

 * For a quick final status, "git diff --stat" is much simpler
   to read than "git status", and that is what I use.  It is
   "what have I changed overall?".  As this actually counts the
   real changes, you would not see those null changes that come
   from the cache dirtiness you are complaining about;

 * When I am ready to commit, "git status" output comes in the
   commit log editor.  At that point, I already have been
   reminded by the previous "git diff" (which is usually run
   often unless the patch is very small like this), and I do not
   need cache-dirtiness output anymore after making my commit.

You do not have to say, to the above paragraph, that it is different from your workflow. I am showing what the opmimum workflow would be, and it is up to you not to listen to me.

Even if we were willing to lose the "quick git-diff to see what I touched" benefit and want to go the route of the attached patch suggests (which I personally do not think we are), there should be a way to either (1) tell the user that many paths are found to be cache-dirty and it is a good idea to refresh the stat information, after squelching diff-files output this way, or (2) update the stat information automatically when diff-files finds too many paths are cache-dirty (perhaps without even telling the user). The latter requires you to declare that git-diff is not a read-only operation anymore, though, so you would need some thought before going in that direction.

[Footnote]

*1* Some people might argue that "perl -i" should have a mode to leave the original if the script does not modify the contents. Maybe they are right, but that is an orthogonal issue.

---

 diff.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

On Thu, Aug 02, 2007 at 12:56:19PM -0700, Junio C Hamano wrote:

So what is the recommended command to summarize which files have been modified, which files have been marked for commit, and which remain untracked?

I find "git-diff --stat" totally insufficient for seeing the progress of my work. How will it remind me that I have also previously git-added some changes? They won't be mentioned at all, unless you are really advocating "git-diff --stat HEAD". How will I be reminded that some files from my work need to be git-added? git-diff won't mention untracked files at all.

You are throwing the word "optimum" out here, but I have no idea what you mean in this context. Optimum with respect to what criteria?

I know you are just trying to show your workflow, and that you understand that others might have a different workflow. But you seem to be implying that workflows using "git-status" are lesser for some reason, and I really think it is a matter of taste.

-Peff

Jeff King <peff@peff.net> writes:

Ok, you got me. If I need such a summary, git-status would obviously be the choice. Although I do admit that I added the interactive commit to support people who want to keep 30 hunks across 10 different files in the working tree, and make a commit using only 3 of them, I do not make partial commits myself, so distinction between staged and unstaged are not something I am usually interested in. If your workflow care about that distinction, and that is a very valid and natural workflow in git, you would find git-status and git-diff --cached more useful than they are to me. I should not used words such as optimum. It is just "different".

When you think about it, in such a workflow whose work tree that does not match commits created from it, it is not very useful to know the "touched but ended up unmodified", because (1) the worktree changes are full of not-yet-ready changes (to the immediate commit you are going to create) anyway, and (2) the "touched but not modified" files may further be modified and become modified before their changes hit a (later) commit. The side effect that "git-status" loses that information suddenly becomes a useful feature for such a workflow.

On the other hand, if your workflow is "work on one thing at a time, and never make partial commits", then your diff tends to be small and more focused to begin with, and you can afford to care about "touched but ended up unmodified". Interestingly, it happens to be a useful correlation that "git status", which clears such information, is less useful command for such a workflow.

On Fri, Aug 03, 2007 at 12:59:43AM -0700, Junio C Hamano wrote:

In an ideal world, I would work that way. But often you uncover a bug in existing code while writing new code, and you want to make that bugfix a separate commit. I generally make a partial commit to stash the bugfix and test it individually. Without making a partial commit, how would you split the bugfix changes from the working changes? Or do you manually pull the bugfix into another branch or working tree?

There is one point you didn't address from my original mail which I would be curious to hear your take on. In your workflow, how do you remind yourself that there are untracked files that need to be added? Do you just wait until you see the commit template at the end?

-Peff

Jeff King <peff@peff.net> writes:

I typically stash the WIP away with "git diff HEAD >P.diff && git reset --hard" (I should learn to use "git stash" these days), and switch to an appropriate branch for bugfix (if it is generally applicable) or stay on the branch (if it is a fix-up for an earlier patch for the topic) to work on the fix. Then unstash to continue where I left off.

I do not leave files that need to be added untracked for a long time. Also, I tend to be picky about making sure that (1) things build from scratch, and that (2) "make clean" removes all crufts. Because of this, I run "make clean" followed by "git clean -n" more often than other people. The latter picks them up if I forget to add them when I created them.

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

git-gui?  ;-)

I also use the following two aliases:

  [alias]
    dw = diff --stat --summary
    di = diff --stat --summary --cached

...

I never used to either. Then git-gui got really useful at showing the distinction and I started using the index for a staging ground. I almost never make partial commits, unless it is completely trivial, e.g. a comment fixup that isn't related to what I'm really doing but that was too darn obvious to not fix _right now_.

But I always toss things into the index when I've read through the diff a few times and am very happy with it. I may not be done with the overall commit, but I park the hunks into the index so I don't have to look at them again. I use a trackball so "tossing into the index" is really just a flick of the wrist to select the menu item from the pop-up menu on that hunk. Quite like a toss. ;-)

I tend to test only once I have everything staged into the index and my working directory is clean (nothing changed that isn't staged). Its at that point that I think my change is done and I'm happy with how the diff looks. Usually the code is correct at this point too; but if its not I'll fix it, then commit.

So where does that leave me regarding the touched but not changed files? Usually they just get in my way in the end. I don't much care that I've undone the file back to what I had in the index. It just doesn't provide any value to my workflow. It is actually incredible rare that I cause it to happen too. Usually I won't write the file back to disk if I'm just going to undo it.

If I do write it to disk I'm likely to stage it or at least some hunks of it. If I later change my mind and undo those changes I'm going to effectively stage the reverse difference. This is a very nice hint showing me that yes in fact the older way was better.

Personally? The index is a killer feature for me. Totally. I can't work without it anymore, it has become a total crutch to me. You would have to pry the index from my cold dead fingers to get me to stop using it.

Yea, that is a total about-face for me. I used to think the index was only useful for merges. Boy was I wrong!

"Shawn O. Pearce" <spearce@spearce.org> writes:

I guess we are saying the same thing. I do use index for staging large-ish changes. I simply do not care about the staged/not-staged distinction in the sense that "I still haven't staged these, that's good because these do not belong to the commit I am going to make next".

Output from "git diff" being truly empty is the cue that such a large-ish worktree change is ready to be committed, and the final "git diff --cached" would give me the full picture. A small-ish change won't make "git diff" in the middle empty, but checking the final "git diff --cached" before committing is the same.

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

I fully agree that git should be optimized for the common case. But even for the common case, I also find the feature strange. You didn't answer that part of my message, but I still fail to see a rationale for making "git-diff; git-status" different from "git-status; git-diff".

IOW, why should people running git-status before git-diff not get a reminder if you think people running git-diff without or before git-status should get it?

Hi,

On Thu, 2 Aug 2007, Matthieu Moy wrote:

For performance reasons, git always compares the files' stat information with that stored in the index.

By updating the file, you make that check fail always.

Without updating the index (which is not a read-only operation, and therefore must not be done when doing a read-only operation like diff), you will therefore _destroy_ the main reason of git's kick-ass performance.

So when you do "git diff" and it tells you all those diff lines, while no file was really changed, it tells you "get your act together! You just _willfully_ slowed down git's performance".

Okay?

Ciao, Dscho

P.S.: I wish we had something similar for the cases that you did not "git gc", so that people, posting to their blogs all over the world that they became benchmark experts overnight and tested git and it sucks, would know that it is not git that sucks.

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I know that, but how does it answer the part of my message that you are citing?

The question remains: why should someone running git-diff get this, and someone running git-status not get this?

(I mean, from the user point of view, not the implementation point of view)

Hi,

On Thu, 2 Aug 2007, Matthieu Moy wrote:

You _acknowledge_ that git is optimized for performance! And therefore you should also acknowledge that you _throw that away_ if you let your index go out of sync.

Because git-status is an index-updating operation.  That's why.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

That sounds like "it is this way because it is not the other way around".

So, yes, git-status updates the index because it's an index-updating operation, while git-diff does not update the index because it's a non-index-updating operation.

Then, I'll rephrase my sentence as "*why* is git-status an index-updating operation while git-diff is not". But you'll probably find another way to avoid answering.

Hi,

On Thu, 2 Aug 2007, Matthieu Moy wrote:

Yes. I do. The issue whether or not git status should be a read-only operation has been discussed -- in _length_ -- here:

http://thread.gmane.org/gmane.comp.version-control.git/40205/focus=40339

Ciao, Dscho

On Thu, Aug 02, 2007 at 01:19:55PM +0100, Johannes Schindelin wrote:

<snip>

The idea that read-only operation like diff shouldn't update the index makes a lot of sense.

But, as a user of git and not a git developer, I certainly _thought_ that git-status was a read-only operation as well. Now I know it isn't, but this doesn't seem very consistent.

jr

Hi,

On Thu, 2 Aug 2007, Joel Reed wrote:

I'll not go into details again. See http://thread.gmane.org/gmane.comp.version-control.git/40205/focus=40339

Ciao, Dscho