patchFix update-index --refresh for submodules if stat(2) returns st_size 0

For example - Cygwin.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Can MSys folks please try it? I noticed it when the test t2103-update-index-ignore-missing.sh (the 5th case) started failing.

 read-cache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Hi,

On Mon, 21 Jul 2008, Alex Riesen wrote:

Please enhance: your oneline is too long, and your commit message body too short.

Since M$' documentation says "This member does not have a meaning for directories." about the member nFileSizeLow of WIN32_FILE_ATTRIBUTE_DATA which we use to implement a sane "lstat()", I think this bug hits MinGW (not MSys) as well.

Ciao, Dscho

Johannes Schindelin, Mon, Jul 21, 2008 20:20:43 +0200:

Well, I'm really not sure. I just found this difference between linux and cygwin (st_stat is 0 for dirs on cygwin). Than I noticed that the routine where I made the change explicitely checks for st_size not being 0. I must admit I can't make much out of comment, and hope this discussion will help to clear the check up.

Could you, just for completeness, try? I don't have mingw at hand and SUSv3 (http://www.opengroup.org/onlinepubs/009695399/toc.htm) does not tells much too. No UNIX system I know about has it 0 for directories.

Hi,

On Mon, 21 Jul 2008, Alex Riesen wrote:

No, I cannot.  I have no Windows machine.

Ciao, Dscho

Alex Riesen <raa.lkml@gmail.com> writes:

The cached stat information in the index is used to speed up comparison between "the last staged data" and what is in the working tree. ie_match_stat() compares ce_xxx fields with the result from lstat(2) we just did, and if there are differences, we take it as a sign that what's in the working tree is different from what we saw when we updated the index entry.

But there is a twist.

Ordinarily, when an entry enteres the index, the hash of the blob contents goes along with the lstat(2) information taken from the file that supplied the contents. However there are some cases we populate the index without lstat(2). update-index --cacheinfo, update-index --index-info are two examples, and when they add index entries, they leave ce_size field to zero. ie_match_stat() will compare that zero ce_size with the size information obtained from the working tree, and declare (falsely) that "what's in the working tree is different -- it can never match, and there is no point trying to re-index to see if they actually match", even though the reason ce_size is zero is *not* because we observed the size of the working tree file *was* zero when we indexed it the last time (it is zero merely because we haven't looked at it yet). The ce_modified_check_fs() call is there to deal with this "we cannot trust the ce_xxx fields" case.

I however have to wonder if you also need to touch the end of ce_match_stat_basic() that checks for zero sized cache entry.

Junio C Hamano, Tue, Jul 22, 2008 10:07:49 +0200:

I frankly don't know.

Alex Riesen schrieb:

I tested it. mingw.git does suffer from the problem, and this fixes it.

But!

Does this mean that ce->ce_size is non-zero for gitlinks, at least on Unix? Is this value useful in anyway? I don't think so. Then it shouldn't be a random value that lstat() happens to return.

-- Hannes

Johannes Sixt, Tue, Jul 22, 2008 09:17:16 +0200:

Yes, I did too (at work).

It is non-zero for directories (which is what gitlinks are in working directories) on UNIX operating systems I met.

Sometimes it is (the size a directory takes on storage)

The problem is: it is not random. I even suspect that Windows is the ONLY system which has st_size 0 for directories.

Alex Riesen schrieb:

Sure; but is ce->ce_size of gitlinks useful?

-- Hannes

Johannes Sixt <j.sixt@viscovery.net> writes:

These ce_xxx fields are the values we read from lstat(2) when the user told us to stage that working tree entity, be it a regular file, a symlink, or a directory that is a submodule. The only thing required for them is that they are stable (i.e. if you haven't touched the working tree entity, the value stays the same), and changes across modification. The value itself does not have to "mean" anything.

When trying to see if the user has changes in the working tree entity since the last such staging of the path, we compare that value with what comes back from lstat(2), before actually comparing the contents. If the filesize changed, they cannot be the same and the code says you have modified it without having to look at the contents.

	Side note.  This is why you need to be careful after modifying
	autocrlf related configuration and attributes.  If you had CRLF
	contents in the working tree that was incorrectly staged as-is,
	then switch autocrlf-on, and "git add" to fix the staged copy to
	be LF-terminated, we say "it's unchanged and we do not bother
	rehashing" by comparing the ce_xxx fields without looking at the
	contents (this is an absolutely necessary optimization to make
	"git add ."  usable), because ce_size records the size of CRLF
	version you have in the working tree, and you haven't changed the
	working tree file in this sequence above.

        Removing the file and checking things out would be the most
	straightforward solution in such a case.

We used to include ce_dev (taken from struct stat.st_dev) in the list of fields to cache and compare to detect changes, but that is now excluded because it is not stable (see comments in read-cache.c). If the directory size is unstable, perhaps it would be better to force it to some fixed magic value so that it is not used by this "quick change detection" check.

If you network-mount the same directory from POSIX and windows, the former may give "storage size of the directory" while the latter may give 0. This would mean that you would need a "update-index --refresh" when you switch between such machines.

On Windows, the only file attribute we need (executable) cannot be used, so ctime can be ignored as well. Change time is updated when file attributes were changed (or it is written to, but in this case, mtime is updated as well).

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Junio C Hamano, Tue, Jul 22, 2008 19:28:46 +0200:

This reminds me... We can't use the only file attribute we care about on Windows, so we can as well skip check for ctime. Besides, Google Desktop Search keeps changing ctime when crawling files (ok, GDS is a major usability nuance anyway, but the point is - we don't use the file attribute).

 read-cache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Hi,

On Tue, 22 Jul 2008, Alex Riesen wrote:

Surely you meant trust_executable_bit instead, right?

Otherwise, if you really want to tell at compile time,I think for clarity you have to introduce another #define, since NO_TRUSTABLE_FILEMODE definitely says something different than CTIME_IS_USELESS.

Ciao, Dscho

Johannes Schindelin, Tue, Jul 22, 2008 22:17:21 +0200:

No. Just what I said: we don't have filemode (like "at all") - so no ctime as well. But maybe you're right, and trust_executable_bit is more flexible. Or maybe both (the #ifdef _and_ trust_executable_bit) and must be used...

I had that at first (NO_DEPENDABLE_CTIME, than IGNORE_CTIME), than deemed it excessive.

Alex Riesen <raa.lkml@gmail.com> writes:

Why is it excessive? My initial reaction was "what does trustable filemode nor trust_executable_bit has anything to do with ctime". Please explain.

Junio C Hamano, Wed, Jul 23, 2008 02:12:50 +0200:

Because exactly the file mode (the executable bit) is the reason for checking ctime. Otherwise there is no point: Git doesn't save any other data which when changed cause a ctime update. And exactly the file mode is completely broken on that cygwin thing. Which is precisely pointed by NO_TRUSTABLE_FILEMODE. Hence - just it.

Hi,

On Wed, 23 Jul 2008, Alex Riesen wrote:

But ctime is broken on Windows. Because ctime is supposed to change whenever the _inode_ changes.

You have to admit that saying "I ignore the ctime because the executable bit is broken" must leave the reader puzzled.

Ciao, Dscho

Johannes Schindelin, Wed, Jul 23, 2008 18:59:02 +0200:

It is not that it is broken. We just don't need it, because the st_mode is not used, and the rest of inode information is not used anyway.

This is conclusion. I said "file mode" and "file attributes", which is how reason for ctime update is defined in SUSv3. man 2 stat says:

       The field st_ctime is changed by writing or by setting  inode
       information (i.e., owner, group, link count, mode, etc.).

Not just "inode" but "inode information". Only mode is used, and even that is ignored on Windows.

On Wed, 23 Jul 2008, Alex Riesen wrote:

That is NOT why git checks the ctime.

Git checks the ctime not because it cares about the inode state being modified per se: since it can see that _directly_ - so why should it care about inode state like st_mode?

No, git checks ctime because it in general tries to make it VERY VERY hard for people to try to "fake out" git and replace files from underneath it without git noticing.

It's much easier and much more common for tools to restore 'mtime' when they do some operation on a file than it is for them to restore 'ctime'.

For example, if you rsync files between two hosts, the '-t' flag will make rsync try to keep the mtimes in sync (and it's part of '-a', which is the common form that you'd use for rsync). So if you only look at mtime and size, you often miss the fact that the file has actually been messed with!

Looking at ctime gets around a number of those things. Of course, it can cause git to be _too_ eager in thinking that a file is modified, and an example of that is the insane indexing that 'beagle' does, which actually modifies the files by adding inode extended attributes to them and thus changes ctime due to the indexing. But in general it's a lot better to be too careful than to miss the fact that somebody changed the file.

		Linus

Linus Torvalds, Fri, Jul 25, 2008 04:00:29 +0200:

But, given the fact, that somewhere sometimes its very-very annoying to have even one (un)changed file, something must be done about it. Maybe just direct

    # my .gitconfig for Windows machines with GDS
    [core]
	filemode = false
	trustctime = false
	logallrefupdates = false
    [pack]
	threads = 1

    etc...

Hi,

On Fri, 25 Jul 2008, Alex Riesen wrote:

... which is all Junio and I were asking all along: a separate way to ask for ignoring ctime; not just DWIM it on top of the executable bit.

Ciao, Dscho

because there are situations where it produces too much false positives. Like when file system crawlers keep changing it when scanning and using the ctime for marking scanned files.

The default is to allow use of ctime.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Johannes Schindelin, Sat, Jul 26, 2008 02:57:25 +0200:

Oh...

 cache.h       |    1 +
 config.c      |    4 ++++
 environment.c |    1 +
 read-cache.c  |    2 +-
 4 files changed, 7 insertions(+), 1 deletions(-)

Alex Riesen <raa.lkml@gmail.com> writes:

This justification is good and I am very inclined to advocate for its inclusion in 1.6.0, but any new configuration needs to be in the documentation.

It appears there is "gui.trustmtime"; shouldn't this be called "core.trustctime" or something?

Alex Riesen <raa.lkml@gmail.com> writes:

This justification is good and I am very inclined to advocate for its inclusion in 1.6.0, but any new configuration needs to be in the documentation.

It appears there is "gui.trustmtime"; shouldn't this be called "core.trustctime" or something?

because there are situations where it produces too much false positives. Like when file system crawlers keep changing it when scanning and using the ctime for marking scanned files.

The default is to allow use of ctime.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Junio C Hamano, Sun, Jul 27, 2008 21:46:42 +0200:

Done.

Getting old... I even called the global flag trust_file_ctime! Corrected. Changed trust_file_ctime to trust_ctime.

 Documentation/config.txt           |    7 +++++++
 Documentation/git-update-index.txt |    5 +++++
 cache.h                            |    1 +
 config.c                           |    4 ++++
 environment.c                      |    1 +
 read-cache.c                       |    2 +-
 6 files changed, 19 insertions(+), 1 deletions(-)

On Mon, Jul 28, 2008 at 08:31:28AM +0200, Alex Riesen wrote:

That's interesting, since most backup software uses the ctime to determine file changes.

David

On Mon, 28 Jul 2008, David Brown wrote:

It really is just Beagle that is (was? I can dream) a piece of unbelievable crap.

Anybody who uses extended attributes as part of a indexing scheme is just insane. Modifying the file you are indexing is not just fundamentally wrong to begin with, but it will then also be incredibly inefficient to read those entries one at a time.

And no other sane model would ever touch 'ctime'.

Oh, well. Making ctime configurable is not wrong per se. But if it's Beagle that triggers this, the fix is sadly in the wrong place.

		Linus

Linus Torvalds, Mon, Jul 28, 2008 18:09:32 +0200:

Beagle is not alone. Google Desktop Search was mentioned before.

Never said it was a fix. Same as CRLF conversion is not a feature.

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

It's a typo and you are saying it _is_ fundamentally wrong, aren't you?

If you are prepared to pick up new files, you need to crawl everywhere anyway, so if the xattr is used to leave a mark "The last time I looked at this file was this" in the file itself, it does not sound too bad to me. It would be irritating that it touches ctime, though, but I do not use it so it is not my problem ;-)

http://beagle-project.org/FAQ "Do I really need extended attributes?" talks about BEAGLE_DISABLE_XATTR environment variable and interestingly it says disabling use of xattr would slow you down.

On Mon, 28 Jul 2008, Junio C Hamano wrote:

Not a typo, and I'm sayin that "it's not _just_ fundamentally wrong"

So yes, it's fundamentally wrong, but it's worse than that. It's fundamentally wrong _and_ it's inefficient as hell.

It's absolutely horrible. 

It means that you have another extra indirection and accompanying disk seek to check the thing. It's a total performance nightmare. Trust me, anybody who uses extended attributes like this simply does not know what he is doing.

They don't have a clue. They say that, but it's simply not true. 

Of course, the fact that they think it is probably implies that they did something EVEN MORE STUPID for the non-xattr case. That wouldn't surprise me at all. If I had to guess, I'd guess that they used an SQL database and query language, and did all their tests with hot caches too.

The kernel does caching really well, and the kernel is fast as hell, so _of_course_ when you benchmark, using kernel data structures looks good, especially if you benchmark against code that isn't well written for the particular usage case.

			Linus

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

Ok. While I have your attention on st_ctime, let me ask you a stupid question. Why does "rename(old, new)" change st_ctime when you move a regular file?

On Mon, Jul 28, 2008 at 06:31:24PM -0700, Junio C Hamano wrote:

A simple answer might be that posix requires it. But, from the point of view of backup software, not updating the ctime on rename would be horrible, because you'd never know when files got renamed.

David

David Brown <git@davidb.org> writes:

I would understand that an obvious implementation would be to link to new and then unlink the old, and the link count of the moved entity needs to change (although in the end, the increment and decrement would cancel out) in each step, so it would be convenient for the implementation to update ctime in both steps; however my reading of POSIX does not seem to require it.

The only mention of ctime I find is about updating the parent directories of old and new, as contents of both change so do their mtime and ctime obviously need to change. But it does not talk about ctime of the entity being moved.

Additionally, the only way rename(2) is described to fail with EMLINK is when renaming an directory and the parent of the new location cannot have any more links; which implies that it does not have to fail if the first step of link+unlink overflows the link count of old.

Ah, I found in the informative Application Usage section that this is implementation dependent.

Sorry for the noise.

Hi,

On Mon, 28 Jul 2008, David Brown wrote:

Any backup software that does not discover that there is a new _filename_ is not worth the label "software". Rather "daftware" or somesuch.

Ciao, Dscho

On Mon, 28 Jul 2008, Junio C Hamano wrote:

Hmm. I think that's just a plain POSIX oddity. There's no real "reason" for it, except the historical one: in really old UNIX terms, rename used to be a "link+unlink".

And that "link+unlink" updated ctime because the 'nlink' part of the inode changed. Never mind that it got changed right back ;)

			Linus

On Mon, 28 Jul 2008, Linus Torvalds wrote:

Side note: a lot of the mtime/ctime/atime rules are really pretty arbitrary. They've grown over time, and have various historic reasons. 'ctime' in particular is more arbitrary than most, and I don't at all guarantee that all Unixes will work exactly the same wrt ctime and rename.

In fact, I -can- guarantee that some older versions of Linux haven't always updated ctime on renames, for example, and it's probably still per-filesystem.

			Linus

Hi,

On Mon, 28 Jul 2008, Linus Torvalds wrote:

I haven't looked at Beagle's source code either, but as a _user_ I can say that it really became horribly, horribly slow after half a year of normal usage.

And yes, uninstalling Beagle, backing up the files, reformatting and putting the files back (to really get rid of the extended attributes already in the file system) helped.

So the first thing I did, back when I still used openSUSE, was to uninstall Beagle after the system install.

Ciao, Dscho

On Mon, Jul 28, 2008 at 08:31:28AM +0200, Alex Riesen wrote:

Somehow, this particular position of the new hunk does not feel like the best choice. ;-)

				Petr "Pasky" Baudis

Accidentally, it split a _chapter_ about a file data corrup... conversion for a weird, but common operating system.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Petr Baudis, Mon, Jul 28, 2008 18:20:43 +0200:

It's alphabetical. Why? Oh, shit... Screw alphabetical

 Documentation/config.txt |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

Alex Riesen <raa.lkml@gmail.com> writes:

Yeah, I think it makes sense to put this after core.filemode.

On some file systems, the only part of inode information we need (executable) cannot be used, so ctime can be ignored as well. Change time is updated when file attributes were changed (or it is written to, but in this case, mtime is updated as well).

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Junio C Hamano, Wed, Jul 23, 2008 02:12:50 +0200:

You know, you have a good point... (and I'm sometimes really stupid) Of course it depends on the underlying filesystem!

The updated patch is untested yet, but should be obviously correct.

BTW, any idea how to check if all callers of ce_match_stat_basic have read the configuration? It is not that essential to have trust_executable_bit set correctly, though. In worst case an index entry will be marked not up-to-date.

 read-cache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Hi

I built git 1.5.6.3 manually (no package with 1.5.4+ on gutsy), and I am getting the error below when running 'git svn rebase'. svn works fine on its own though. The default locale is set to en_US.UTF-8

svn: error: cannot set LC_ALL locale svn: error: environment variable LANG is en_US.UTF-8 svn: error: please check that your locale name is correct

I found a workaround that works, but it would be nice to have it work properly. $> LANG= git svn rebase"

Thanks in advance. -Anton