Stats in Git

By applying the diff below, you can see for yourself what happens when

Marius Storm-Olsen, Sun, Sep 02, 2007 16:49:55 +0200:

Quoted 1 line

By applying the diff below, you can see for yourself what happens when

just use "strace -e fstat,stat,lstat,stat64,lstat64 -f git-status" on sane platform.

This gives us a significant speedup when adding, committing and stat'ing files. (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

Signed-off-by Marius Storm-Olsen <[email protected]>

(quoted diff) +1 -0

+		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since

it's not a stat64 */

Quoted 5 lines · Marius Storm-Olsen

This gives us a significant speedup when adding, committing and stat'ing files. (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

Signed-off-by Marius Storm-Olsen <[email protected]>

Your numbers show an improvement of 50% and more. That is terrific!

I'll test it out an put the patch into mingw.git. I hope you don't mind if I also include your analysis and statistics in the commit message. It's worth keeping around! BTW, which of your email addresses would you like registered as author?

Quoted 4 lines

compat/mingw.c +4 -0

+		ext = strrchr(file_name, '.');
+		if (ext && (!_stricmp(ext, ".exe") ||
+			    !_stricmp(ext, ".com") ||
+			    !_stricmp(ext, ".bat") ||

compat/mingw.c int fchmod(int fildes, mode_t mode) · line 54

Quoted 3 lines

compat/mingw.c +3 -0

+			    !_stricmp(ext, ".cmd")))
+			fMode |= S_IEXEC;
+		}

I'm slightly negative about this. For a native Windows project the executable bit does not matter, and for a cross-platform project this check is not sufficient, but can even become annoying (think of a file
named 'www.google.com'). So we can just as well spare the few cycles.

Quoted 2 lines

(quoted diff) +1 -0

+		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since

it's not a stat64 */

Here's an idea for the future: With this self-made stat() implementation it should also be possible to get rid of Windows's native struct stat: Make a private definition of it, too, and use all 64 bits.

compat/mingw.c int fchmod(int fildes, mode_t mode) · line 70

Quoted 3 lines

compat/mingw.c +2 -0

 		return 0;
+	}
+	errno = ENOENT;

Of course we need a bit more detailed error conditions, most importantly EACCES should be distinguished.

git-compat-util.h int mingw_rename(const char*, const char*); · line 495

Quoted 5 lines

+/* Make git on Windows use git_lstat and git_stat instead of lstat and stat */ +int git_lstat(const char *file_name, struct stat *buf);

git-compat-util.h +3 -0

+int git_stat(const char *file_name, struct stat *buf);
+#define lstat(x,y) git_lstat(x,y)
+#define stat(x,y) git_stat(x,y)

I'd go the short route without git_stat() and

#define stat(x,y) git_lstat(x,y)

Quoted 4 lines · Johannes Sixt

I'm slightly negative about this. For a native Windows project the executable bit does not matter, and for a cross-platform project this check is not sufficient, but can even become annoying (think of a file named 'www.google.com'). So we can just as well spare the few cycles.

Ok, that's fine by me. It was only added for completeness, and with no benefits I'd say we drop it too.

Quoted 6 lines

compat/mingw.c int fchmod(int fildes, mode_t mode) · line 70

Quoted 3 lines

compat/mingw.c +2 -0

 		return 0;
+	}
+	errno = ENOENT;

Of course we need a bit more detailed error conditions, most importantly EACCES should be distinguished.

Right, you want to do that in a second commit?

Marius Storm-Olsen, Sun, Sep 02, 2007 21:31:40 +0200:

compat/mingw.c int fchmod(int fildes, mode_t mode) · line 50

Quoted 1 line

compat/mingw.c +1 -0

+		buf->st_ino = 0;

You sure about that? Ever wondered why it is not so on everywhere else?

(Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

söndag 02 september 2007 skrev Marius Storm-Olsen:

Quoted 1 line

(Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx

Robin Rosenberg, Sun, Sep 02, 2007 22:27:59 +0200:

Quoted 5 lines

söndag 02 september 2007 skrev Marius Storm-Olsen:

Quoted 1 line

(Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx

Except they fscked it up, as usual for microsoft: it 's got a mandatory argument specifying what the target should be, file or directory. And they don't tell what happens when the argument is wrong or the target does not exists. Typical, too.

söndag 02 september 2007 skrev Marius Storm-Olsen:

Quoted 1 line

(Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx

Robin Rosenberg said the following on 02.09.2007 22:27:

Quoted 4 lines

s?ndag 02 september 2007 skrev Marius Storm-Olsen:

Quoted 2 lines

(Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

Quoted 2 lines

It does now: See
http://msdn2.microsoft.com/en-us/library/aa363866.aspx

Yeah, I know about Vista's improved support for symbolic links. However, I think we can let that lay for a while, until we decide to make Git generate proper symlinks on Vista. I don't see it as a 1st priority at the moment, and we can always add the needed functionality in a separate stat() function later.

Quoted 7 lines

Robin Rosenberg said the following on 02.09.2007 22:27:

Quoted 1 line

It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx

Yeah, I know about Vista's improved support for symbolic links. However, I think we can let that lay for a while, until we decide to make Git generate proper symlinks on Vista. I don't see it as a 1st priority at the moment, and we can always add the needed functionality in a separate stat() function later.

... and force everybody to upgrade to Vista, thereby working for Microsoft for free? You _know_ that I will oppose that change.

Johannes Schindelin said the following on 03.09.2007 13:39:

Quoted 9 lines · Marius Storm-Olsen

Quoted 7 lines

Robin Rosenberg said the following on 02.09.2007 22:27:

Quoted 1 line

It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx

Yeah, I know about Vista's improved support for symbolic links. However, I think we can let that lay for a while, until we decide to make Git generate proper symlinks on Vista. I don't see it as a 1st priority at the moment, and we can always add the needed functionality in a separate stat() function later.

Quoted 2 lines

... and force everybody to upgrade to Vista, thereby working for Microsoft for free? You _know_ that I will oppose that change.

;-) I wouldn't dream of it!

Nah, my comment was more 'allow usage of proper Symlinks on Vista' at a later point. I would still argue that the default would be what we have today. So, it would have to be an option.

But seeing what they've done to the symlinks there, it might be far fetched. We'll worry about that (much) later..

Quoted 8 lines · Marius Storm-Olsen

Robin Rosenberg said the following on 02.09.2007 22:27:

Quoted 6 lines

s?ndag 02 september 2007 skrev Marius Storm-Olsen:

Quoted 2 lines

(Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

It does now: See
http://msdn2.microsoft.com/en-us/library/aa363866.aspx

Quoted 5 lines

Yeah, I know about Vista's improved support for symbolic links. However, I think we can let that lay for a while, until we decide to make Git generate proper symlinks on Vista. I don't see it as a 1st priority at the moment, and we can always add the needed functionality in a separate stat() function later.

... and force everybody to upgrade to Vista,

thereby working for Microsoft for free? You _know_ that I will oppose that change.

s?ndag 02 september 2007 skrev Marius Storm-Olsen:

Quoted 1 line

(Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx

Hi,

Quoted 4 lines · Robin Rosenberg

s?ndag 02 september 2007 skrev Marius Storm-Olsen:

Quoted 1 line

(Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx

Oh? *goes and tries to create one on a USB stick* No. Besides, IIRC you

cannot even create symlinks to another partition. Copying a symlink will

copy the _linked_ file. So to call this "symlink" is a little... uhm... preposterous.

Plus, on a page linked from the link you posted, it says that it is only supported from Vista onwards. So you must be kidding me.

$ ln -s Makefile x
$ cp x y
$ ls -ld x y
lrwxrwxrwx 1 me me     8 sep  2 23:36 x -> Makefile
-rw-r--r-- 1 me me 32164 sep  2 23:36 y

Quoted 3 lines

$ ls -ld x y
lrwxrwxrwx 1 me me     8 sep  2 23:36 x -> Makefile
-rw-r--r-- 1 me me 32164 sep  2 23:36 y

And if I understand the documentation correctly, then

$ mkdir foo && cd foo
$ cat ../x
x: No such file or directory

Right?

Quoted 5 lines · Johannes Sixt

Quoted 3 lines

$ ls -ld x y
lrwxrwxrwx 1 me me     8 sep  2 23:36 x -> Makefile
-rw-r--r-- 1 me me 32164 sep  2 23:36 y

And if I understand the documentation correctly, then

Quoted 5 lines

$ mkdir foo && cd foo
$ cat ../x
x: No such file or directory

Right?

correct.

Quoted 6 lines

Quoted 5 lines

And if I understand the documentation correctly, then

$ mkdir foo && cd foo
$ cat ../x
x: No such file or directory

Quoted 3 lines

Quoted 1 line

Right?

correct.

Have you tested this, or is this from reading the documentation? In either case: brilliant, but the former would be funnier (depending on one's sense of humor, of course).

Quoted 9 lines · David Kastrup

Quoted 7 lines

Quoted 5 lines

And if I understand the documentation correctly, then

$ mkdir foo && cd foo
$ cat ../x
x: No such file or directory

correct.

Have you tested this, or is this from reading the documentation?

umm, thanks for the notice, i was wrong:

----

$ cat ../x
this is makefile
----

the situation what triggers the 'no such file' problem is:

----

$ touch foo/Makefile
$ mkdir bar
$ ln -s foo/Makefile bar
$ cd bar
$ cat Makefile
cat: Makefile: No such file or directory
----

Quoted 10 lines

the situation what triggers the 'no such file' problem is:

----

$ touch foo/Makefile
$ mkdir bar
$ ln -s foo/Makefile bar
$ cd bar
$ cat Makefile
cat: Makefile: No such file or directory
----

This is under Vista? It would be the same under Unix.

Quoted 11 lines · David Kastrup

Quoted 10 lines

the situation what triggers the 'no such file' problem is:

----

$ touch foo/Makefile
$ mkdir bar
$ ln -s foo/Makefile bar
$ cd bar
$ cat Makefile
cat: Makefile: No such file or directory
----

Quoted 1 line

This is under Vista? It would be the same under Unix.

no, i never stated that this is Vista ;-) this is Linux.

If anybody is as fortunate as to actually have Vista available, it would be nice if he corroborated that relative links under Vista are indeed (as Microsoft appears to claim) relative with regard to the current work directory rather than the directory containing the link.

Quoted 4 lines · David Kastrup

If anybody is as fortunate as to actually have Vista available, it would be nice if he corroborated that relative links under Vista are indeed (as Microsoft appears to claim) relative with regard to the current work directory rather than the directory containing the link.

I believe the wording "resolves the path relative to the current directory" actually refers to the creation of links, not to their use.

C:\stest>ver

Microsoft Windows [Version 6.0.6000]

You actually need admin privileges too, but I don't know any windows developer who hasn't got that.

compat/mingw.c int fchmod(int fildes, mode_t mode) · line 54

Quoted 9 lines · Marius Storm-Olsen

This gives us a significant speedup when adding, committing and stat'ing files. (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

compat/mingw.c +6 -0

+               if (ext && (!_stricmp(ext, ".exe") ||
+                           !_stricmp(ext, ".com") ||
+                           !_stricmp(ext, ".bat") ||
+                           !_stricmp(ext, ".cmd")))
+                       fMode |= S_IEXEC;
+               }

This breaks executable mode reporting for things like configure scripts and other shell scripts that may, or may not, be executable. Also, you may want to turn off the executable state for some of these extensions (for example if com or cmd were not actually executable files). This makes it impossible to manipulate git repositories properly on the MinGW platform.

Would it be possible to use the git tree to manage the executable state? That way, all files would not have their executable state set by default on Windows. The problem with this is how then to set the executable state? Having a git version of chmod may not be a good idea, but then how else are you going to reliably and efficiently modify the files permissions on Windows?

The rest of the patch looks good on a brief initial scan.

Quoted 11 lines · Reece Dunn

compat/mingw.c int fchmod(int fildes, mode_t mode) · line 54

Quoted 9 lines · Marius Storm-Olsen

This gives us a significant speedup when adding, committing and stat'ing files. (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

compat/mingw.c +6 -0

+               if (ext && (!_stricmp(ext, ".exe") ||
+                           !_stricmp(ext, ".com") ||
+                           !_stricmp(ext, ".bat") ||
+                           !_stricmp(ext, ".cmd")))
+                       fMode |= S_IEXEC;
+               }

Quoted 6 lines

This breaks executable mode reporting for things like configure scripts and other shell scripts that may, or may not, be executable. Also, you may want to turn off the executable state for some of these extensions (for example if com or cmd were not actually executable files). This makes it impossible to manipulate git repositories properly on the MinGW platform.

Actually, you don't really need the EXEC bit for Git to work. I just added it for completeness. (We _could_ remove that too, since it's slowing us down slightly ;-)

Remember that Git isn't using MSys for its builtins, so MinGW Git doesn't understand the MSys notion of executable files anyways. The MinGW port actually peeks at the beginning of a file (ignoring exe files), and sees if there's an interpreter there. If there is, it will expand

    git-foo args...

into
sh git-foo args...
and execute the command. So, it's not really affected by this change.

I haven't had any problems with this patch on my system, so could you explain what you mean with 'this makes it impossible to manipulate git repositories'?

Quoted 6 lines

Would it be possible to use the git tree to manage the executable state? That way, all files would not have their executable state set by default on Windows. The problem with this is how then to set the executable state? Having a git version of chmod may not be a good idea, but then how else are you going to reliably and efficiently modify the files permissions on Windows?

The file-state-in-git-tree belongs in a different discussion. Have a look at the '.gitignore, .gitattributes, .gitmodules, .gitprecious?, .gitacls? etc.' and 'tracking perms/ownership [was: empty directories]' threads. Permissions are not a trivial topic, since systems represent them differently. This patch just tries to reflect the read, write and execute permissions as normal Windows would; and it only cares about file extensions (and the PE header, if it exists).

Also note that my patch totally ignores the Group & Others part of the permission bits. Again, we're on Windows so we don't really care. We _could_ make it reflect the ACLs in Windows, but then we'd have to make it optional, since that's _really_ slow to 'stat'.

You pull a repository that contains executable scripts that are required to work in order to build the system. You then make some modifications to the local repository and run the 'git add .' command. Since this patch is reporting executable bits differently, the mode change is stored as well as the local modifications. Now the changes are pushed upstream (along with the file mode changes).

Someone running a Linux machine, pulls your changes. When those files are checked out, the executable state of those scripts has now changed, preventing the Linux user from running those scripts. _That_ is what I meant. Or am I misunderstanding how git works in this case?

Quoted 11 lines · Reece Dunn

You pull a repository that contains executable scripts that are required to work in order to build the system. You then make some modifications to the local repository and run the 'git add .' command. Since this patch is reporting executable bits differently, the mode change is stored as well as the local modifications. Now the changes are pushed upstream (along with the file mode changes).

Someone running a Linux machine, pulls your changes. When those files are checked out, the executable state of those scripts has now changed, preventing the Linux user from running those scripts. _That_ is what I meant. Or am I misunderstanding how git works in this case?

This is what "git config core.fileMode false" is for. See git- config's man page for information (or Documentation/config.txt).

We already have a way to tell git that the "executable bit" is worthless, and any Windows port should use it.

Quoted 9 lines · Brian Gernhardt

Quoted 7 lines · Reece Dunn

You pull a repository that contains executable scripts that are required to work in order to build the system. You then make some
modifications to the local repository and run the 'git add .'
command. Since this patch is reporting executable bits differently, the mode change is stored as well as the local modifications. Now the changes are pushed upstream (along with the file mode changes).

Quoted 2 lines

We already have a way to tell git that the "executable bit" is worthless, and any Windows port should use it.

Ok, so as the executable bit is worthless, there doesn't need to be any special casing in this patch to deal with it.

The conversion of the FileTime to unix time_t is far more heavy.

Quoted 1 line · Marius Storm-Olsen

The conversion of the FileTime to unix time_t is far more heavy.

Really? If so, we might consider storing FILETIME->dwHightDateTime and ->dwLowDateTime in the index.

But I doubt it. AFAICT _getting_ at the stat data is the expensive thing in Windows, not a 64-bit addition, subtraction and division.

Hi,

Quoted 1 line · Marius Storm-Olsen

The conversion of the FileTime to unix time_t is far more heavy.

Really? If so, we might consider storing FILETIME->dwHightDateTime and ->dwLowDateTime in the index.

But I doubt it. AFAICT _getting_ at the stat data is the expensive thing in Windows, not a 64-bit addition, subtraction and division.

This gives us a significant speedup when adding, committing and stat'ing files. (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

Quoted 3 lines · Marius Storm-Olsen

This gives us a significant speedup when adding, committing and stat'ing files. (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat)

Unfortunately, the patch fails t0010-racy-git.sh. I suspect the filetime conversion:

With the our own implementations of lstat & fstat, the following test cases are now fixed:
t4116-apply-reverte.sh
ok 3: apply in reverse
t4200-rerere.sh
ok 17: young records still live
However, the following test cases seems to fail now:
t6024-recursive-merge.sh
FAIL 1: setup tests
FAIL 3: result contains a conflict
FAIL 4: virtual trees were processed
FAIL 5: refuse to merge binaries

See attached test case logs.

There was a problem with racy conditions, which this revision fixes. The problem was that fstat was using the builtin implementation, which for for some reason is off by some amount of seconds. (This is probably due to some leap-year issue in one of the implementations. However, Microsoft tells us to use 116444736000000000 in http://support.microsoft.com/kb/167296, so I'll stick with that.)

With the our own implementations of lstat & fstat, the following test cases are now fixed:
t4116-apply-reverte.sh
ok 3: apply in reverse
t4200-rerere.sh
ok 17: young records still live
However, the following test cases seems to fail now:
t6024-recursive-merge.sh
FAIL 1: setup tests
FAIL 3: result contains a conflict
FAIL 4: virtual trees were processed
FAIL 5: refuse to merge binaries

See attached test case logs.
Are some of these test cases unstable, so the result will fluctuate on Windows?

Quoted 7 lines · Marius Storm-Olsen

There was a problem with racy conditions, which this revision fixes. The problem was that fstat was using the builtin implementation, which for for some reason is off by some amount of seconds. (This is probably due to some leap-year issue in one of the implementations. However, Microsoft tells
us to use 116444736000000000 in http://support.microsoft.com/kb/167296, so I'll stick with that.)

Thanks for the analysis and new patch. Indeed, FILETIME is UTC, which is good; the native implementation returns local time, if I read the code (of msvcrt) correctly.

Quoted 13 lines

With the our own implementations of lstat & fstat, the following test cases are now fixed:
t4116-apply-reverte.sh
ok 3: apply in reverse
t4200-rerere.sh
ok 17: young records still live
However, the following test cases seems to fail now:
t6024-recursive-merge.sh
FAIL 1: setup tests
FAIL 3: result contains a conflict
FAIL 4: virtual trees were processed
FAIL 5: refuse to merge binaries

Quoted 3 lines

See attached test case logs.
Are some of these test cases unstable, so the result will fluctuate on Windows?

I see many more failures. in:

t4001-diff-rename.sh: 5
t4101-apply-nonl.sh: all
t4102-apply-rename.sh: 2,3,4
t4116-apply-reverse.sh: 3
t4200-rerere.sh: 12,13,17
t5515-fetch-merge-logic.sh: 54
etc...

Quoted 9 lines · Johannes Sixt

I see many more failures. in:

t4001-diff-rename.sh: 5
t4101-apply-nonl.sh: all
t4102-apply-rename.sh: 2,3,4
t4116-apply-reverse.sh: 3
t4200-rerere.sh: 12,13,17
t5515-fetch-merge-logic.sh: 54
etc...

Funny. I do not get most of these:

t4200-rerere,
t5510-fetch,
t5515-fetch-merge-logic,
t5700-clone-reference,
t5701-clone-local,
t7004

This is all on top of 4msysgit's "devel" branch, and the t5* and t7004 failed there already.

Oh, and I actually run from sh, not from cmd. (Wouldn't you have guessed?)

Quoted 7 lines · Johannes Schindelin

t4200-rerere,
t5510-fetch,
t5515-fetch-merge-logic,
t5700-clone-reference,
t5701-clone-local,
t7004

Quoted 5 lines

This is all on top of 4msysgit's "devel" branch, and the t5* and t7004 failed there already.

Oh, and I actually run from sh, not from cmd. (Wouldn't you have guessed?)

I was also running the tests under sh, however on top of mingw.git, and not 4msysgit.git.

There was a problem with racy conditions, which this revision fixes. The problem was that fstat was using the builtin implementation, which for for some reason is off by some amount of seconds. (This is probably due to some leap-year issue in one of the implementations. However, Microsoft tells us to use 116444736000000000 in http://support.microsoft.com/kb/167296, so I'll stick with that.)
Also, since both stat and lstat proved to be rather slow, having our own version of fstat is probably also wise. At least we now control all the stat'ing, so we _know_ they are compatible.
Also note that this revision makes git_lstat call itself after modifying the filename, instead of the builtin stat, for the same reasons.

With the our own implementations of lstat & fstat, the following test cases are now fixed:
t4116-apply-reverte.sh
ok 3: apply in reverse
t4200-rerere.sh
ok 17: young records still live
However, the following test cases seems to fail now:
t6024-recursive-merge.sh
FAIL 1: setup tests
FAIL 3: result contains a conflict
FAIL 4: virtual trees were processed
FAIL 5: refuse to merge binaries

See attached test case logs.
Are some of these test cases unstable, so the result will fluctuate on Windows?

Quoted 11 lines

There was a problem with racy conditions, which this revision fixes. The problem was that fstat was using the builtin implementation, which for for some reason is off by some amount of seconds. (This is probably due to some leap-year issue in one of the implementations. However, Microsoft tells us to use 116444736000000000 in http://support.microsoft.com/kb/167296, so I'll stick with that.)
Also, since both stat and lstat proved to be rather slow, having our own version of fstat is probably also wise. At least we now control all the stat'ing, so we _know_ they are compatible.
Also note that this revision makes git_lstat call itself after modifying the filename, instead of the builtin stat, for the same reasons.

At least some of these informations should go into the commit message, too.

Quoted 13 lines

With the our own implementations of lstat & fstat, the following test cases are now fixed:
t4116-apply-reverte.sh
ok 3: apply in reverse
t4200-rerere.sh
ok 17: young records still live
However, the following test cases seems to fail now:
t6024-recursive-merge.sh
FAIL 1: setup tests
FAIL 3: result contains a conflict
FAIL 4: virtual trees were processed
FAIL 5: refuse to merge binaries

Quoted 3 lines

See attached test case logs.
Are some of these test cases unstable, so the result will fluctuate on Windows?

I saw some funny stuff on Windows, like test cases succeeding when run interactively, but failing when run from "make test".

BTW it would have been way easier to apply your patch, had you followed SubmittingPatches...

To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days).

First comment: it seems git_fstat() is not declared properly, so there are quite a few compiler warnings.

Running the tests now.

Johannes Schindelin said the following on 03.09.2007 15:33:

Quoted 2 lines

BTW it would have been way easier to apply your patch, had you followed SubmittingPatches...

Heh, I actually tried, using the Thunderbird way. Of course the attachments are non-conforming :-) What was the problem? Whitespace issues, Windows EOL, attachments, or all of the above? :-)

Quoted 6 lines · Marius Storm-Olsen

Johannes Schindelin said the following on 03.09.2007 15:33:

Quoted 2 lines

BTW it would have been way easier to apply your patch, had you followed SubmittingPatches...

Heh, I actually tried, using the Thunderbird way. Of course the attachments are non-conforming :-) What was the problem? Whitespace
issues, Windows EOL, attachments, or all of the above? :-)

git am said that the patch was empty.

I'll use the 4msysgit.git repo from now on. I assume it'll be ok if I +push to the teststat branch?

I saw some funny stuff on Windows, like test cases succeeding when run interactively, but failing when run from "make test".

Quoted 2 lines · Johannes Schindelin

I saw some funny stuff on Windows, like test cases succeeding when run interactively, but failing when run from "make test".

That's very likely the issue that we work around by inserting "sleep 1" at strategic points, which is a timing (race condition) issue and does not depend on interactive vs. "make test".

To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days).

First comment: it seems git_fstat() is not declared properly, so there are quite a few compiler warnings.

Quoted 2 lines · Johannes Schindelin

To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days).

Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. RFC, and please test.

Quoted 6 lines · Marius Storm-Olsen

Quoted 3 lines · Johannes Schindelin

To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days).

Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. RFC, and please test.

Thanks a lot! I've pushed it out in mingw.git's master.

The reason that t4200-rerere.sh fails is that we now store UTC in
st_mtime. However, for the garbage-collection we compare this entry
to a local time stamp. Therefore, I've pushed out a fixup patch at
the top of mingw.git's devel branch that converts mtime to local
time (http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=1b62ecb31068af06c2fa7664f06c6c36316aac2c).
Would you kindly conduct the performance test with this patch? I'm
afraid that this makes us substantially slower.

Johannes Sixt said the following on 04.09.2007 09:41:

Quoted 1 line

Thanks a lot! I've pushed it out in mingw.git's master.

Ops, already in master branch?

http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127

Ok, I can give it a performance test, but I tend to agree with David Kastrup there. It would be better if we rather fix the places where we check with the local timestamp instead; depending of course on how many places we actually do this. We'll see how much the timezone conversion in the custom stat functions actually hurt us performance wise.

Quoted 3 lines · Marius Storm-Olsen

Johannes Sixt said the following on 04.09.2007 09:41:

Quoted 1 line

Thanks a lot! I've pushed it out in mingw.git's master.

Ops, already in master branch?

Yes, it looked so polished ;)

Quoted 1 line

http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127

Looks good, although you should now handle INVALID_HANDLE_VALUE at the beginning of git_fstat() like this:

	HANDLE fh = (HANDLE)_get_osfhandle(fd);
	if (fh == INVALID_HANDLE_VALUE)
		return -1;	/* errno has been set */

	if (GetFileInformationByHandle(...

Quoted 2 lines

Ok, I can give it a performance test, but I tend to agree with David Kastrup there. It would be better if we rather fix the places where we check with the local timestamp instead; depending of course on how many places we actually do this. We'll see how much the timezone conversion in the custom stat functions actually hurt us performance wise.

I'd make the decision on the grounds of a perfomance test. If it turns out that the penalty is bearable, we should keep this stuff private to the MinGW build. Otherwise, we would need MinGW specific code at the call sites (unless we can hide the opposite conversion in some other wrapper function).

... time passes ...

Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I can run it 100,000,000 times per second. So I'm confident that there won't be any noticable degradation with my proposed change.

Johannes Sixt said the following on 04.09.2007 12:53:

Quoted 6 lines

Quoted 2 lines · Marius Storm-Olsen

Johannes Sixt said the following on 04.09.2007 09:41:
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127

Looks good, although you should now handle INVALID_HANDLE_VALUE at the beginning of git_fstat() like this:

Actually, that's already handled.

Quoted 1 line

Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I can run it 100,000,000 times per second. So I'm confident that there won't be any noticable degradation with my proposed change.

Ok. I haven't done the performance test with Git yet, but we'll see. If it's not noticeable, I'll add it to all the timestamps we have.

Quoted 6 lines · Marius Storm-Olsen

Ok, I can give it a performance test, but I tend to agree with David Kastrup there. It would be better if we rather fix the places where we check with the local timestamp instead; depending of course on how many places we actually do this. We'll see how much the timezone conversion in the custom stat functions actually hurt us performance wise.

I'd make the decision on the grounds of a perfomance test. If it turns out that the penalty is bearable, we should keep this stuff private to the MinGW build. Otherwise, we would need MinGW specific code at the call sites (unless we can hide the opposite conversion in some other wrapper function).

... time passes ...

Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I can run it 100,000,000 times per second. So I'm confident that there won't be any noticable degradation with my proposed change.

Quoted 6 lines · Marius Storm-Olsen

Quoted 2 lines · Johannes Schindelin

To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days).

Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. RFC, and please test.

Thanks a lot! I've pushed it out in mingw.git's master.

The reason that t4200-rerere.sh fails is that we now store UTC in st_mtime. However, for the garbage-collection we compare this entry to a local time stamp.

Quoted 11 lines · Johannes Sixt

Quoted 7 lines · Marius Storm-Olsen

Quoted 3 lines · Johannes Schindelin

To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days).

Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. RFC, and please test.

Thanks a lot! I've pushed it out in mingw.git's master.

Quoted 3 lines

The reason that t4200-rerere.sh fails is that we now store UTC in st_mtime. However, for the garbage-collection we compare this entry to a local time stamp.

This analysis is incorrect, I think. The reason we fail seems to be that t4200 uses test-chmtime, which uses utime(). Likely, we need a wrapper for that one?

Quoted 6 lines · Marius Storm-Olsen

Quoted 2 lines · Johannes Schindelin

To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days).

Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. RFC, and please test.

Thanks a lot! I've pushed it out in mingw.git's master.

The reason that t4200-rerere.sh fails is that we now store UTC in
st_mtime. However, for the garbage-collection we compare this entry
to a local time stamp. Therefore, I've pushed out a fixup patch at
the top of mingw.git's devel branch that converts mtime to local
time
(http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=1b62ecb31068af06c2fa7664f06c6c36316aac2c). Would you kindly conduct the performance test with this patch? I'm afraid
that this makes us substantially slower.

Quoted 4 lines · Marius Storm-Olsen

Quoted 2 lines · Johannes Schindelin

To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days).

Quoted 2 lines

Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. RFC, and please test.

Thanks a lot! I've pushed it out in mingw.git's master.

The reason that t4200-rerere.sh fails is that we now store UTC in st_mtime. However, for the garbage-collection we compare this entry to a local time stamp.

Therefore, I've pushed out a fixup patch at the top of mingw.git's devel branch that converts mtime to local time

Quoted 1 line · Johannes Sixt

Therefore, I've pushed out a fixup patch at the top of mingw.git's devel branch that converts mtime to local time

On Linux, we compare to UTC to begin with, right? We should do that here, too... So if time(NULL) does not return UTC on MinGW, we have to wrap that function, too.

Quoted 6 lines · Johannes Schindelin

Quoted 2 lines · Johannes Sixt

Therefore, I've pushed out a fixup patch at the top of mingw.git's devel branch that converts mtime to local time

On Linux, we compare to UTC to begin with, right? We should do that here, too... So if time(NULL) does not return UTC on MinGW, we have to wrap that function, too.

According to MSDN, time(NULL) returns "the number of seconds elapsed since [epoch] according to the system clock". Please don't ask me what "the system clock" is.

Reading the implementation of time(), it starts with GetLocalTime(), determines whether daylight saving is in effect, and continues with another round of timezone adjustment - mind you:
_not_ a timezone reversal (!!). Doesn't this look extremely bogus?

It seems we really need a wrapper for time().

In the meantime, I've pushed out a new patch
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59

/me starts another test run, to see how our tests are doing now..

Marius Storm-Olsen said the following on 04.09.2007 13:53:

Quoted 4 lines

In the meantime, I've pushed out a new patch
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59

Quoted 1 line

/me starts another test run, to see how our tests are doing now..

Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git 'devel' branch, I only have one failing testcase

    t6024-recursive-merge.sh
when running
    $ NO_SYMLINKS=1 make -k

The rest are passing with flying colors!

Quoted 12 lines · Marius Storm-Olsen

Marius Storm-Olsen said the following on 04.09.2007 13:53:

Quoted 4 lines

In the meantime, I've pushed out a new patch
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59

/me starts another test run, to see how our tests are doing now..

Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git 'devel' branch, I only have one failing testcase

    t6024-recursive-merge.sh
when running
    $ NO_SYMLINKS=1 make -k

Quoted 1 line

The rest are passing with flying colors!

And that one will eventually pass if only you try it often enough. See 71ee4210c in mingw.git.

Hi,

Quoted 12 lines · Johannes Sixt

Quoted 11 lines · Marius Storm-Olsen

Marius Storm-Olsen said the following on 04.09.2007 13:53:

Quoted 4 lines

In the meantime, I've pushed out a new patch
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59

/me starts another test run, to see how our tests are doing now..

Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git 'devel' branch, I only have one failing testcase

    t6024-recursive-merge.sh
when running
    $ NO_SYMLINKS=1 make -k

Quoted 3 lines

Quoted 1 line

The rest are passing with flying colors!

And that one will eventually pass if only you try it often enough. See 71ee4210c in mingw.git.

Do you have Linus' patch applied? The one where the config is read at the start of write-tree?

The problem only occurred since we have core.crlf = input unilaterally now.

Marius Storm-Olsen said the following on 04.09.2007 13:53:

Quoted 4 lines

In the meantime, I've pushed out a new patch
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59

/me starts another test run, to see how our tests are doing now..

Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git 'devel' branch, I only have one failing testcase

    t6024-recursive-merge.sh
when running
    $ NO_SYMLINKS=1 make -k

The rest are passing with flying colors!

Marius Storm-Olsen said the following on 04.09.2007 13:53:

Quoted 4 lines

In the meantime, I've pushed out a new patch
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59

/me starts another test run, to see how our tests are doing now..

Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git 'devel' branch, I only have one failing testcase

    t6024-recursive-merge.sh
when running
    $ NO_SYMLINKS=1 make -k

The rest are passing with flying colors!

Quoted 5 lines · Marius Storm-Olsen

Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git 'devel' branch, I only have one failing testcase t6024-recursive-merge.sh when running $ NO_SYMLINKS=1 make -k

The rest are passing with flying colors!

Bad news. I do not know if it was the newest version I tried, but I could no longer fetch... said something about some bad file.

Quoted 9 lines · Johannes Schindelin

Quoted 5 lines · Marius Storm-Olsen

Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git 'devel' branch, I only have one failing testcase t6024-recursive-merge.sh when running $ NO_SYMLINKS=1 make -k

The rest are passing with flying colors!

Bad news. I do not know if it was the newest version I tried, but I could no longer fetch... said something about some bad file.

Then you're missing this patch:
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127

I guess the quickest way is to manually apply this patch and recompile. (or add the '#undef fstat', and have git_fstat just 'return fstat(fd, buf)')

The problem is that without this patch fstat(0, buf) would fail with bad filedescriptor instead of returning the st_mode = S_IFIFO.

Quoted 11 lines · Marius Storm-Olsen

Quoted 3 lines · Johannes Schindelin

Bad news. I do not know if it was the newest version I tried, but I could no longer fetch... said something about some bad file.

Then you're missing this patch:
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127

I guess the quickest way is to manually apply this patch and recompile. (or add the '#undef fstat', and have git_fstat just 'return fstat(fd, buf)')

Quoted 2 lines

The problem is that without this patch fstat(0, buf) would fail with bad filedescriptor instead of returning the st_mode = S_IFIFO.

I guessed as much, but could not fetch the patch, since fetch was broken ;-)

Quoted 2 lines · Marius Storm-Olsen

The problem is that without this patch fstat(0, buf) would fail with bad filedescriptor instead of returning the st_mode = S_IFIFO.

I guessed as much, but could not fetch the patch, since fetch was broken ;-)

For some utterly strange reason, "git fetch" accessed "git-fetch" in the cwd, not in /bin/... Funny.

Quoted 4 lines · Johannes Schindelin

Quoted 2 lines · Johannes Sixt

Therefore, I've pushed out a fixup patch at the top of mingw.git's devel branch that converts mtime to local time

Quoted 3 lines

On Linux, we compare to UTC to begin with, right? We should do that here, too... So if time(NULL) does not return UTC on MinGW, we have to wrap that function, too.

According to MSDN, time(NULL) returns "the number of seconds elapsed since [epoch] according to the system clock". Please don't ask me what "the system clock" is.

Reading the implementation of time(), it starts with GetLocalTime(), determines whether daylight saving is in effect, and continues with another round of timezone adjustment - mind you: _not_ a timezone reversal (!!). Doesn't this look extremely bogus?

It seems we really need a wrapper for time().

Quoted 3 lines · Johannes Sixt

Reading the implementation of time(), it starts with GetLocalTime(), determines whether daylight saving is in effect, and continues with another round of timezone adjustment - mind you: _not_ a timezone reversal (!!). Doesn't this look extremely bogus?

It seems we really need a wrapper for time().

I absolutely concur. Something like this (most of it is blatantly copied from Marius' patch)?

Quoted 9 lines · Johannes Sixt

Quoted 7 lines · Johannes Schindelin

Quoted 2 lines · Johannes Sixt

Therefore, I've pushed out a fixup patch at the top of mingw.git's devel branch that converts mtime to local time

On Linux, we compare to UTC to begin with, right? We should do that here, too... So if time(NULL) does not return UTC on MinGW, we have to wrap that function, too.

Quoted 3 lines

According to MSDN, time(NULL) returns "the number of seconds elapsed since [epoch] according to the system clock". Please don't ask me what "the system clock" is.

I think I know. From my QEmu adventures I know that DOS/Windows expects the system clock to be set to local time, in contrast to _all_ other operating systems.

Hi,

Quoted 11 lines · Johannes Schindelin

Quoted 9 lines · Johannes Sixt

Quoted 7 lines · Johannes Schindelin

Quoted 2 lines · Johannes Sixt

Therefore, I've pushed out a fixup patch at the top of mingw.git's devel branch that converts mtime to local time

On Linux, we compare to UTC to begin with, right? We should do that here, too... So if time(NULL) does not return UTC on MinGW, we have to wrap that function, too.

Quoted 7 lines

Quoted 3 lines

According to MSDN, time(NULL) returns "the number of seconds elapsed since [epoch] according to the system clock". Please don't ask me what "the system clock" is.

I think I know. From my QEmu adventures I know that DOS/Windows expects the system clock to be set to local time, in contrast to _all_ other operating systems.

Now I am utterly confused. MSDN says

FILETIME

Contains a 64-bit value representing the number of 100-nanosecond intervals since January 1, 1601 (UTC).

Hmm.

Quoted 30 lines · Johannes Schindelin

Hi,

Quoted 11 lines · Johannes Schindelin

Quoted 9 lines · Johannes Sixt

Quoted 7 lines · Johannes Schindelin

Quoted 2 lines · Johannes Sixt

Therefore, I've pushed out a fixup patch at the top of mingw.git's devel branch that converts mtime to local time

On Linux, we compare to UTC to begin with, right? We should do that here, too... So if time(NULL) does not return UTC on MinGW, we have to wrap that function, too.

Quoted 7 lines

Quoted 3 lines

According to MSDN, time(NULL) returns "the number of seconds elapsed since [epoch] according to the system clock". Please don't ask me what "the system clock" is.

I think I know. From my QEmu adventures I know that DOS/Windows expects the system clock to be set to local time, in contrast to _all_ other operating systems.

Now I am utterly confused. MSDN says

FILETIME

Contains a 64-bit value representing the number of 100-nanosecond intervals since January 1, 1601 (UTC).

Quoted 1 line

Hmm.

[Warning: war stories ahead...]

If you really, really want to know more:

http://search.cpan.org/~shay/Win32-UTCFileTime-1.45/lib/Win32/UTCFileTime.pm

Quoted 5 lines

Now I am utterly confused. MSDN says
FILETIME
Contains a 64-bit value representing the number of 100-nanosecond intervals since January 1, 1601 (UTC).
Hmm.

[Warning: war stories ahead...]
If you really, really want to know more:
http://search.cpan.org/~shay/Win32-UTCFileTime-1.45/lib/Win32/UTCFileTime.pm

Rutger Nijlunsing said the following on 04.09.2007 23:02:

Quoted 10 lines · Johannes Schindelin

Quoted 5 lines

Now I am utterly confused. MSDN says
FILETIME
Contains a 64-bit value representing the number of 100-nanosecond
intervals since January 1, 1601 (UTC).
Hmm.

[Warning: war stories ahead...]
If you really, really want to know more:
http://search.cpan.org/~shay/Win32-UTCFileTime-1.45/lib/Win32/UTCFileTime.pm

Thanks, seems like it's the right decision then to ensure that we use UTC throughout Git on Windows

Hannes & Dscho, looks like we should revert
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59
then, and rather apply Dscho's patch for a custom time() implementation. Dscho, was the custom implementation of time() enough to fix the issues for you?

Quoted 2 lines · Johannes Schindelin

To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days).

Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. RFC, and please test.

However, while look at that whole stat'ing situation in git, I saw that doing 'git status' actually stats all the files _thrice_! Yup, that's not 1 time, or 2 times, but actually 3(!) times before 'git status' is content!