Stats in Git

Stats in Git

[Marius Storm-Olsen]

I was checking out the performance situation with Git on Windows, and
found out that the Posix stat functions on Windows are just obscenely
slow. We really can't use them, at least on in Git. So, I made a patch
for the MinGW version, which I'll post right after this mail.

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!
I know that git-status is a script, so I think this clearly indicates
that git-status is a prime candidate for a built-in ;-)

I haven't looked into details as to why it stats the files so many
times. I guess someone more experienced in Git core could give an
opinion, if by writing git-status as a builtin it would be possible to
only stat the files once. It would have a huge impact on Windows where
stats are inheritly much slower than on Linux.

By applying the diff below, you can see for yourself what happens when
you stat the repo created with Moe's script:
    mkdir bummer
    cd bummer
    for ((i=0;i<100;i++)); do
    mkdir $i && pushd $i;
    for ((j=0;j<1000;j++)); do
    echo "$j" >$j; done; popd;
    done

$ git status 2>&1 | wc -l
300137

Fast on Linux now, but still quite slow on Windows..

--
.marius


diff --git a/git-compat-util.h b/git-compat-util.h
index ca0a597..6b6405c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -369,4 +369,23 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)
 	return 0;
 }

+static inline int git_lstat(const char *file_name, struct stat *buf)
+{
+	fprintf(stderr, "lstat: %s\n", file_name);
+	return lstat(file_name, buf);
+}
+static inline int git_fstat(int fd, struct stat *buf)
+{
+	fprintf(stderr, "fstat: %d\n", fd);
+	return fstat(fd, buf);
+}
+static inline int git_stat(const char *file_name, struct stat *buf)
+{
+	fprintf(stderr, "stat: %s\n", file_name);
+	return stat(file_name, buf);
+}
+#define lstat(x,y) git_lstat(x,y)
+#define fstat(x,y) git_fstat(x,y)
+#define stat(x,y) git_stat(x,y)
+
 #endif

[PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[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 <mstormo_git@storm-olsen.com>
---
The following patch will override the normal Posix implementation of stat and
lstat on Windows, and use normal Windows API to ensure we're stat'ing as fast
as possible. With this patch I get the performance increase to the far right.

Initially, I only replaced lstat, since that's what the MinGW port of Git had
implemented from before. But, since we don't really care about symlinks on
Windows, I decided to simply use the same implementation for stat as well. The
performance benefit is clearly indicated in the report below.


 With normal lstat & stat   lstat, based on Win32      lstat & stat, as Win32
 -------------------------  -------------------------  -------------------------
 Command: git init          Command: git init          Command: git init
 -------------------------  -------------------------  -------------------------

 real    0m0.047s           real      0m0.047s         real       0m0.078s
 user    0m0.031s           user      0m0.031s         user       0m0.031s
 sys     0m0.000s           sys       0m0.000s         sys        0m0.000s

 -------------------------  -------------------------  -------------------------
 Command: git add .         Command: git add .         Command: git add .
 -------------------------  -------------------------  -------------------------

 real    0m19.390s          real      0m19.390s        real       0m12.187s
 user    0m0.015s           user      0m0.015s         user       0m0.015s
 sys     0m0.030s           sys       0m0.030s         sys        0m0.015s

 -------------------------  -------------------------  -------------------------
 Command: git commit -a...  Command: git commit -a...  Command: git commit -a...
 -------------------------  -------------------------  -------------------------

 real    0m30.812s          real      0m22.547s        real       0m17.297s
 user    0m0.015s           user      0m0.031s         user       0m0.015s
 sys     0m0.000s           sys       0m0.000s         sys        0m0.015s

 -------------------------  -------------------------  -------------------------
 3x Command: git-status     3x Command: git-status     3x Command: git-status
 -------------------------  -------------------------  -------------------------

 real    0m11.860s          real      0m5.360s         real       0m5.344s
 user    0m0.015s           user      0m0.015s         user       0m0.015s
 sys     0m0.015s           sys       0m0.015s         sys        0m0.031s

 real    0m11.703s          real      0m5.312s         real       0m5.390s
 user    0m0.015s           user      0m0.015s         user       0m0.031s
 sys     0m0.000s           sys       0m0.000s         sys        0m0.000s

 real    0m11.672s          real      0m5.359s         real       0m5.344s
 user    0m0.031s           user      0m0.015s         user       0m0.015s
 sys     0m0.000s           sys       0m0.015s         sys        0m0.016s

 -------------------------  -------------------------  -------------------------
 Command: git commit...     Command: git commit...     Command: git commit...
 (single file)              (single file)              (single file)
 -------------------------  -------------------------  -------------------------

 real    0m14.234s          real      0m7.969s         real       0m7.875s
 user    0m0.015s           user      0m0.015s         user       0m0.015s
 sys     0m0.000s           sys       0m0.016s         sys        0m0.000s

 compat/mingw.c    |   52 ++++++++++++++++++++++++++++++++++++++++++++--------
 git-compat-util.h |    5 +++++
 2 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 7711a3f..207378c 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -23,19 +23,52 @@ int fchmod(int fildes, mode_t mode)
 	return -1;
 }

-int lstat(const char *file_name, struct stat *buf)
+static inline time_t filetime_to_time_t(const FILETIME *ft)
+{
+	long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
+	winTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */
+	winTime /= 10000000;		 /* Nano to seconds resolution */
+	return (time_t)winTime;
+}
+
+extern int _getdrive( void );
+int git_lstat(const char *file_name, struct stat *buf)
 {
 	int namelen;
 	static char alt_name[PATH_MAX];
-
-	if (!stat(file_name, buf))
+	char* ext;
+	WIN32_FILE_ATTRIBUTE_DATA fdata;
+
+	if (GetFileAttributesExA(file_name, GetFileExInfoStandard, &fdata)) {
+		int fMode = S_IREAD;
+		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+			fMode |= S_IFDIR;
+		else {
+			fMode |= S_IFREG;
+		ext = strrchr(file_name, '.');
+		if (ext && (!_stricmp(ext, ".exe") ||
+			    !_stricmp(ext, ".com") ||
+			    !_stricmp(ext, ".bat") ||
+			    !_stricmp(ext, ".cmd")))
+			fMode |= S_IEXEC;
+		}
+		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
+			fMode |= S_IWRITE;
+
+		buf->st_ino = 0;
+		buf->st_gid = 0;
+		buf->st_uid = 0;
+		buf->st_nlink = 1;
+		buf->st_mode = fMode;
+		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since it's not a stat64 */
+		buf->st_dev = buf->st_rdev = (_getdrive() - 1);
+		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
+		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
+		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
 		return 0;
+	}
+	errno = ENOENT;

-	/* if file_name ended in a '/', Windows returned ENOENT;
-	 * try again without trailing slashes
-	 */
-	if (errno != ENOENT)
-		return -1;
 	namelen = strlen(file_name);
 	if (namelen && file_name[namelen-1] != '/')
 		return -1;
@@ -47,6 +80,9 @@ int lstat(const char *file_name, struct stat *buf)
 	alt_name[namelen] = 0;
 	return stat(alt_name, buf);
 }
+int git_stat(const char *file_name, struct stat *buf) {
+    return git_lstat(file_name, buf);
+}

 /* missing: link, mkstemp, fchmod, getuid (?), gettimeofday */
 int socketpair(int d, int type, int protocol, int sv[2])
diff --git a/git-compat-util.h b/git-compat-util.h
index 1ba499f..de1f062 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -488,6 +488,11 @@ int mingw_rename(const char*, const char*);
 extern void quote_argv(const char **dst, const char **src);
 extern const char *parse_interpreter(const char *cmd);

+/* 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);
+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)
 #endif /* __MINGW32__ */

 #endif
--
mingw.v1.5.2.4.1311.g376df-dirty

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen]

Sorry, should have added that this is a MinGW port patch only. And I
forgot to include the msysgit mailinglist, nice..

--
.marius

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Reece Dunn]

On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote:
> 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)
>
> +               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.

- Reece

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]

Reece Dunn wrote:
> On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote:
>> 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)
>>
>> +               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.

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

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

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

Thanks

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Reece Dunn]

On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote:
> Reece Dunn wrote:
> > On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote:
> >> 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)
> >>
> >> +               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.
>
> 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'?

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?

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

I understand that this is not a trivial topic. I was thinking that
this different behaviour w.r.t. the executable permission will break
things when you have developers on both Linux and Windows, such as the
cairo developers, for current git usage.

I have not really been tracking those threads, but I will take a look at them.

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

Sure. Cygwin does use ACLs to implement stat which is why it is slow.
So anything that can speed git up here, without any breakage in
functionality, is a good thing.

- Reece

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Brian Gernhardt]

On Sep 2, 2007, at 12:33 PM, Reece Dunn wrote:

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

~~ B

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Reece Dunn]

On 02/09/07, Brian Gernhardt <benji@silverinsanity.com> wrote:
>
> On Sep 2, 2007, at 12:33 PM, Reece Dunn wrote:
>
> > 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.

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

- Reece

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

Reece Dunn wrote:
> On 02/09/07, Brian Gernhardt <benji@silverinsanity.com> wrote:
>> On Sep 2, 2007, at 12:33 PM, Reece Dunn wrote:
>>> 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).
>> 
>> 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.

Right, this is true. And I was debating it with myself, and just added
it for completion; at least for the first revision of the patch. It
doesn't really affect the performance all that much anyways. The
conversion of the FileTime to unix time_t is far more heavy. (Which is
why I'm debating to just ignore the access time)

If we could somehow rather use the FileTimes directly in the index,
instead of having to convert them, we could have even better performance
when stat'ing on Windows. (However, it would result in an incompatible
index, so everyone would have to 'git update-index --refresh' on all
repositories before they can use the new version.)

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Johannes Schindelin]

Hi,

On Sun, 2 Sep 2007, Marius Storm-Olsen wrote:

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

Ciao,
Dscho

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[David Kastrup]

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

> Hi,
>
> On Sun, 2 Sep 2007, Marius Storm-Olsen wrote:
>
>> 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.

64-bit division conceivably could be somewhat expensive, but it sounds
like it should not be much compared to the cost of a system call.
What is the code doing that division?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Johannes Sixt]

On Sunday 02 September 2007 16:51, Marius Storm-Olsen wrote:
> 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 <mstormo_git@storm-olsen.com>

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?

> +		ext = strrchr(file_name, '.');
> +		if (ext && (!_stricmp(ext, ".exe") ||
> +			    !_stricmp(ext, ".com") ||
> +			    !_stricmp(ext, ".bat") ||
> +			    !_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.

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

>  		return 0;
> +	}
> +	errno = ENOENT;

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

> +/* 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);
> +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)

-- Hannes

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

Johannes Schindelin wrote:
> On Sun, 2 Sep 2007, Marius Storm-Olsen wrote:
>> 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.

Haha, sure sure, _getting_ that stat data in the first place is the
expensive part on Windows. However, that's something you _have_ to do no
matter what, so there's no way around that.

Turns out that it wasn't as bad as i thought. If you have
filetime_to_time_t() just return, say 116444736, I see
    git add . improve with ~0.5 sec for 100K files
and git status improve with 0.05 sec

Surely, avoiding the tripple stat'ing in 'git status' would help a lot
more ;-) So, I guess we'll just leave the timestamp conversion as is,
and avoid complicating the index.

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 4943 bytes --]

Johannes Sixt wrote:
> On Sunday 02 September 2007 16:51, Marius Storm-Olsen wrote:
>> 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 <mstormo_git@storm-olsen.com>
> 
> Your numbers show an improvement of 50% and more. That is terrific!

Yes, I was surprised myself about the impact. Didn't think it would make
_such_ a difference. And if you compare it to the results we had before
Linus' performance fix too, just checkout the performance improvements
we've had (based on Moe's test script):

    Before                        Now
    -------------------------     -------------------------
    Command: git init             Command: git init
    -------------------------     -------------------------
    real    0m0.031s              real       0m0.078s
    user    0m0.031s              user       0m0.031s
    sys     0m0.000s              sys        0m0.000s
    -------------------------     -------------------------
    Command: git add .            Command: git add .
    -------------------------     -------------------------
    real    0m19.328s             real       0m12.187s
    user    0m0.015s              user       0m0.015s
    sys     0m0.015s              sys        0m0.015s
    -------------------------     -------------------------
    Command: git commit -a...     Command: git commit -a...
    -------------------------     -------------------------
    real    0m30.937s             real       0m17.297s
    user    0m0.015s              user       0m0.015s
    sys     0m0.015s              sys        0m0.015s
    -------------------------     -------------------------
    3x Command: git-status        3x Command: git-status
    -------------------------     -------------------------
    real    0m19.531s             real       0m5.344s
    user    0m0.211s              user       0m0.015s
    sys     0m0.136s              sys        0m0.031s

    real    0m19.532s             real       0m5.390s
    user    0m0.259s              user       0m0.031s
    sys     0m0.091s              sys        0m0.000s

    real    0m19.593s             real       0m5.344s
    user    0m0.211s              user       0m0.015s
    sys     0m0.152s              sys        0m0.016s
    -------------------------     -------------------------
    Command: git commit...        Command: git commit...
             (single file)                 (single file)
    -------------------------     -------------------------
    real    0m36.688s             real       0m7.875s
    user    0m0.031s              user       0m0.015s
    sys     0m0.000s              sys        0m0.000s

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

Sure, include the stats if you'd like. You can use
mstormo_git@storm-olsen.com for email address.

>> +		ext = strrchr(file_name, '.');
>> +		if (ext && (!_stricmp(ext, ".exe") ||
>> +			    !_stricmp(ext, ".com") ||
>> +			    !_stricmp(ext, ".bat") ||
>> +			    !_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.

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

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

Yep, that will shave off one assignment, bit-shift and addition. Quick
operations, but still worth while IMO. No point wasting cycles where we
don't have to.

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

>> +/* 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);
>> +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)

Please do, thanks.

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Johannes Sixt]

On Sunday 02 September 2007 20:44, Marius Storm-Olsen wrote:
> Johannes Sixt wrote:
> > 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.

I'll amend the patch accordingly.

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

Yes, please. Please don't forget to take care of the trailing-slash annoyance.

-- Hannes

[PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[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 <mstormo_git@storm-olsen.com>
---
 Revision #2 of the patch.
 For this one I change the filetime_to_time_t function to do the
 timestamp conversion inline in the FILETIME struct. That way we
 also avoid one assignment, bitshifting and addition.
 Sneaky, huh? ;-)
 New stats:
    -------------------------
    Command: git init
    -------------------------

    real    0m0.047s
    user    0m0.031s
    sys     0m0.000s

    -------------------------
    Command: git add .
    -------------------------

    real    0m12.016s
    user    0m0.015s
    sys     0m0.000s

    -------------------------
    Command: git commit -a...
    -------------------------

    real    0m17.031s
    user    0m0.015s
    sys     0m0.030s

    -------------------------
    3x Command: git-status
    -------------------------

    real    0m5.265s
    user    0m0.015s
    sys     0m0.015s

    real    0m5.297s
    user    0m0.015s
    sys     0m0.000s

    real    0m5.250s
    user    0m0.015s
    sys     0m0.016s

    -------------------------
    Command: git commit...
    (single file)
    -------------------------

    real    0m7.859s
    user    0m0.015s
    sys     0m0.015s


 compat/mingw.c    |   41 +++++++++++++++++++++++++++++++++--------
 git-compat-util.h |    4 ++++
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 7711a3f..86a1419 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -23,19 +23,44 @@ int fchmod(int fildes, mode_t mode)
 	return -1;
 }

-int lstat(const char *file_name, struct stat *buf)
+static inline time_t filetime_to_time_t(const FILETIME *ft)
+{
+	long long *winTime = (long long*)ft;
+	*winTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */
+	*winTime /= 10000000;		  /* Nano to seconds resolution */
+	return (time_t)ft->dwLowDateTime;
+}
+
+extern int _getdrive( void );
+int git_lstat(const char *file_name, struct stat *buf)
 {
 	int namelen;
 	static char alt_name[PATH_MAX];
-
-	if (!stat(file_name, buf))
+	WIN32_FILE_ATTRIBUTE_DATA fdata;
+
+	if (GetFileAttributesExA(file_name, GetFileExInfoStandard, &fdata)) {
+		int fMode = S_IREAD;
+		if (fdata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+			fMode |= S_IFDIR;
+		else
+			fMode |= S_IFREG;
+		if (!(fdata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
+			fMode |= S_IWRITE;
+
+		buf->st_ino = 0;
+		buf->st_gid = 0;
+		buf->st_uid = 0;
+		buf->st_nlink = 1;
+		buf->st_mode = fMode;
+		buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since it's not a stat64 */
+		buf->st_dev = buf->st_rdev = (_getdrive() - 1);
+		buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
+		buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
+		buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
 		return 0;
+	}
+	errno = ENOENT;

-	/* if file_name ended in a '/', Windows returned ENOENT;
-	 * try again without trailing slashes
-	 */
-	if (errno != ENOENT)
-		return -1;
 	namelen = strlen(file_name);
 	if (namelen && file_name[namelen-1] != '/')
 		return -1;
diff --git a/git-compat-util.h b/git-compat-util.h
index 1ba499f..4122465 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -488,6 +488,10 @@ int mingw_rename(const char*, const char*);
 extern void quote_argv(const char **dst, const char **src);
 extern const char *parse_interpreter(const char *cmd);

+/* Make git on Windows use git_lstat instead of lstat and stat */
+int git_lstat(const char *file_name, struct stat *buf);
+#define lstat(x,y) git_lstat(x,y)
+#define stat(x,y) git_lstat(x,y)
 #endif /* __MINGW32__ */

 #endif
--
1.5.3.GIT-dirty

Re: Stats in Git

[Alex Riesen]

Marius Storm-Olsen, Sun, Sep 02, 2007 16:49:55 +0200:
> 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.

Re: Stats in Git

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

Alex Riesen wrote:
> Marius Storm-Olsen, Sun, Sep 02, 2007 16:49:55 +0200:
>> 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.

Right, I was doing this on Windows, where strace is rather.. limited, so
I thought I'd just share the code for all to play with :-)

But, on my linux box:
$ strace -e fstat,stat,lstat,stat64,lstat64 -f git-status 2>&1 | wc -l
300195

(Slightly more stats there, as expected, due to the listings of the
shells stats too, and not just the builtins.)

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Robin Rosenberg]

söndag 02 september 2007 skrev Marius Storm-Olsen:
> (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

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Johannes Schindelin]

Hi,

On Sun, 2 Sep 2007, Robin Rosenberg wrote:

> s?ndag 02 september 2007 skrev Marius Storm-Olsen:
> > (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.

Ciao,
Dscho

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Alex Riesen]

Robin Rosenberg, Sun, Sep 02, 2007 22:27:59 +0200:
> söndag 02 september 2007 skrev Marius Storm-Olsen:
> > (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.

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Alex Riesen]

Marius Storm-Olsen, Sun, Sep 02, 2007 21:31:40 +0200:
> +		buf->st_ino = 0;

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

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Robin Rosenberg]

söndag 02 september 2007 skrev Johannes Schindelin:
> Hi,
> 
> On Sun, 2 Sep 2007, Robin Rosenberg wrote:
> 
> > s?ndag 02 september 2007 skrev Marius Storm-Olsen:
> > > (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 
Set core.symlinks = false for working on a broken file system

> cannot even create symlinks to another partition.  Copying a symlink will 
That is not the normal use for symlinks. It is a case where it breaks, but symbolic
links in a git repo that points outside the repo is probably not a good idea, especially
if it is a cross platform project. It is far less broken than today anyway.

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

$ 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

Same behaviour as on linux.

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

core.symlinks = false if ithey aren't supported. You actually need
admin privileges too, but I don't know any windows developer who
hasn't got that.

-- robin

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Robin Rosenberg]

söndag 02 september 2007 skrev Alex Riesen:
> Robin Rosenberg, Sun, Sep 02, 2007 22:27:59 +0200:
> > söndag 02 september 2007 skrev Marius Storm-Olsen:
> > > (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.

Why would this API be an exception?

-- robin

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Johannes Schindelin]

Hi,

On Sun, 2 Sep 2007, Robin Rosenberg wrote:

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

Like almost every developer in the corporate world?

Fact is: this support of symlinks is ridiculous.  Why not just admit it?

Ciao,
Dscho

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

Alex Riesen said the following on 02.09.2007 23:41:
> Marius Storm-Olsen, Sun, Sep 02, 2007 21:31:40 +0200:
>> +		buf->st_ino = 0;
> 
> You sure about that? Ever wondered why it is not so on everywhere else?

Pretty sure. If you look at Windows' native version of stat, it will
return you st_ino = 0. Or maybe you where referring to something else,
and I just missed your point?

AFAIK, the ino in the index is only to be _really_ sure that nothing
has changed with the file, and we can just skip it on Windows. If in
doubt, try running this on your Windows box:

#include <windows.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>

int main(int, char **)
{
    wchar_t DirSpec[] = L".\\*";
    WIN32_FIND_DATA FindFileData;
    HANDLE hFind = FindFirstFile(DirSpec, &FindFileData);
    if (hFind == INVALID_HANDLE_VALUE) {
        printf ("Crap happened: %u\n", GetLastError());
        return -1;
    } 

    struct _stat buf;
    while (FindNextFile(hFind, &FindFileData) != 0) 
    {
        if (!_wstat(FindFileData.cFileName, &buf))
            printf("file: %S, ino: %u\n", FindFileData.cFileName, buf.st_ino);
    }

    FindClose(hFind);
    return 0;
}



-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

Robin Rosenberg said the following on 02.09.2007 22:27:
> söndag 02 september 2007 skrev Marius Storm-Olsen:
>> (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

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.

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Johannes Sixt]

Robin Rosenberg schrieb:
> $ 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

And if I understand the documentation correctly, then

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

Right?

The docs say that symlinks without backslash are relative to the current 
directory (!!!).

-- Hannes

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Johannes Sixt]

Marius Storm-Olsen schrieb:
> 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:

> -int lstat(const char *file_name, struct stat *buf)
> +static inline time_t filetime_to_time_t(const FILETIME *ft)
> +{
> +	long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime;
> +	winTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */
> +	winTime /= 10000000;		 /* Nano to seconds resolution */

Shouldn't this be 1000000000 according to your comment? However, even if 
I make that change, the test still fails. Could you please look into this?

> +	return (time_t)winTime;
> +}

-- Hannes

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

Johannes Sixt said the following on 03.09.2007 09:47:
> Marius Storm-Olsen schrieb:
>> 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:

Ok, I'll try to get to it later today.

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: Stats in Git

[Matthieu Moy]

Marius Storm-Olsen <marius@trolltech.com> writes:

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

My experiments show 2 stats only:

$ strace -f git status |& grep -e execve -e  foo                  
[...]
[pid 22492] execve("/home/moy/local/usr//bin/git", ["git", "update-index", "-q", "--unmerged", "--refresh"], [/* 50 vars */]) = 0
[pid 22492] lstat64("foo", {st_mode=S_IFREG|0755, st_size=0, ...}) = 0
[pid 22493] execve("/home/moy/local/usr//bin/git", ["git", "runstatus"], [/* 49 vars */]) = 0
[pid 22493] lstat64("foo", {st_mode=S_IFREG|0755, st_size=0, ...}) = 0
[...]

Once for "git update-index --refresh" and once more for "git
runstatus". Obviously, a builtin with one tree traversal only would
provide a good speedup.

-- 
Matthieu

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On Mon, Sep 03, 2007 at 09:07:42AM +0200, Johannes Sixt <j.sixt@eudaptics.com> wrote:
> >$ 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?

correct.

- VMiklos

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[David Kastrup]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Mon, Sep 03, 2007 at 09:07:42AM +0200, Johannes Sixt <j.sixt@eudaptics.com> wrote:
>> >$ 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?
>
> 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).

-- 
David Kastrup

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:

> Robin Rosenberg said the following on 02.09.2007 22:27:
> > s?ndag 02 september 2007 skrev Marius Storm-Olsen:
> > > (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
> 
> 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.

Ciao,
Dscho

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[David Kastrup]

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

> On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
>
>> Robin Rosenberg said the following on 02.09.2007 22:27:
>> > s?ndag 02 september 2007 skrev Marius Storm-Olsen:
>> > > (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
>> 
>> 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,

Nonsense.  Supporting a feature is different from requiring a feature.

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

If Microsoft decides to shoot their users less in the foot than
previously, I don't think that we should take over the gun.

However, if the symbolic link semantics hinted at elsewhere indeed are
as broken as claimed and/or documented, the actual usefulness of
symbolic links seems so limited that we would not be doing their users
a favor by supporting relative symlinks.  And absolute links frankly
have very little place in a _work_ directory (and git does not
currently keep track of enough things in order to make it useful as a
filesystem snapshot system).

I would like to see actual test results to get a confirmation of
whether indeed relative symlinks are as broken under Vista as rumored.
If they are, it seems quite pointless supporting any symlinks under
Windows at the moment.  Until I see actual test results, I would give
Microsoft the benefit of doubt.

-- 
David Kastrup

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

Johannes Schindelin said the following on 03.09.2007 13:39:
> On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
>> Robin Rosenberg said the following on 02.09.2007 22:27:
>>> 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.

;-) 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..

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:

> Johannes Schindelin said the following on 03.09.2007 13:39:
> > On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
> > > Robin Rosenberg said the following on 02.09.2007 22:27:
> > > > 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.
> 
> ;-) I wouldn't dream of it!

Hehe.

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

Okay, I could live with that.

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

Yes, it is funny how they do it over and over and over again.  Embrace, 
"Extend", Extinguish.  And I thought that eventually people would be 
clever enough to realise...

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

Marius Storm-Olsen said the following on 03.09.2007 14:12:
>  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.

How about that, reading the diff backwards :-)
Oh well, you see the issues based on the logs. Would be nice if you 
guys could also give the testcases a run to see if you get the same 
result.

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:

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

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

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.

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Marius Storm-Olsen schrieb:
> 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.

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

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

There is something fishy going on.

But at least the failure in t4116 is easy to work around:

diff --git a/t/t4116-apply-reverse.sh b/t/t4116-apply-reverse.sh
index fc2f622..f3e0c4a 100755
--- a/t/t4116-apply-reverse.sh
+++ b/t/t4116-apply-reverse.sh
@@ -43,7 +43,8 @@ test_expect_success 'apply in reverse' '
         git reset --hard second &&
         git apply --reverse --binary --index patch &&
         git diff >diff &&
-       git diff /dev/null diff
+       : > empty &&
+       git diff empty diff

  '

-- Hannes

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 2522 bytes --]

Johannes Schindelin said the following on 03.09.2007 15:33:
> On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
>> 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.

Sure

>> 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?
> 
> I saw some funny stuff on Windows, like test cases succeeding when run 
> interactively, but failing when run from "make test".

Ok, I ran 'make test', so maybe that's it? I'll rerun them later.

> 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? :-)

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

Cool, thanks

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

/me slaps self. Right, sorry 'bout that. I'll amend the declaration in 
git-compat-util.h.

> Running the tests now.

Great, thanks!
-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Johannes Schindelin schrieb:
> 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".

-- Hannes

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > 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".

Makes sense to me now.  Especially around t5510 -- t5701 I see those (and 
I do not run "make test" often, since it takes _ages_.

Thanks,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Johannes Sixt wrote:

> Marius Storm-Olsen schrieb:
> > 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.
> 
> > 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?
> 
> 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?)

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:

> Johannes Schindelin said the following on 03.09.2007 15:33:
>
> > 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.

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

Johannes Schindelin wrote:
> On Mon, 3 Sep 2007, Johannes Sixt wrote:
>> 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?)

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

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

Johannes Schindelin wrote:
> On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
>> Johannes Schindelin said the following on 03.09.2007 15:33:
>>> 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.

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

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:

> Johannes Schindelin wrote:
> 
> > 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?)
> 
> I was also running the tests under sh, however on top of mingw.git, and 
> not 4msysgit.git.

I switched back recently, as more tests are succeeding in 
4msysgit.git's "devel" ATM.

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:

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

I should think so.

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

Johannes Schindelin wrote:
> 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.

The patch also incorporates some of Hannes local changes, with some
modifications. Hannes, does it look ok for you? You can add a tag to the
commit message if you'd like, and just +push it.

On Hannes' request (and to which I fully agree), I've gone back to the
old implementation of filetime_to_time_t(), since it was a bit 'nasty'.
(If we want to target CE in the future, it will quite possibly break)

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

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

This is also fixed, of course.

Hope this is the final 'cut' :-)

Later!
--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:

> Johannes Schindelin wrote:
> > 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.

I'm too tired for more, but the first test which fails (consistently) is 
t4200 here:

* FAIL 17: young records still live
	test -f .git/rr-cache/08f6c39f296af7e0dd1b3b7d8bba18d0365f605f/preimage
&& test -f .git/rr-cache/4000000000000000000000000000000000000000/preimage

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Marius Storm-Olsen schrieb:
> Johannes Schindelin wrote:
>> 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.

-- Hannes

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[David Kastrup]

Johannes Sixt <j.sixt@eudaptics.com> writes:

> Marius Storm-Olsen schrieb:
>> Johannes Schindelin wrote:
>>> 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.

Wouldn't it make more sense to convert local time to mtime?  That's
one conversion per second at most rather than one conversion per file.

-- 
David Kastrup

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]

Johannes Sixt said the following on 04.09.2007 09:41:
> Marius Storm-Olsen schrieb:
>> Johannes Schindelin wrote:
>>> 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.

Ops, already in master branch?
Ok, I found out that the custom fstat function was incomplete, so I 
completed it. However, since you've already pushed it to your main 
branch, I've added it as a separate commit to the 4msysgit.git 
'teststat' branch. It might also explain some of the testfailures we 
were seeing, but I haven't finished the test run yet. (So, consider 
the patch something to play with, and don't commit it to your 'master' 
branch yet! ;-)

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


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

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.

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Tue, 4 Sep 2007, Johannes Sixt wrote:

> Marius Storm-Olsen schrieb:
> > Johannes Schindelin wrote:
> > > 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.

Thanks for the explanation.

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

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Marius Storm-Olsen schrieb:
> Johannes Sixt said the following on 04.09.2007 09:41:
>> Thanks a lot! I've pushed it out in mingw.git's master.
> 
> Ops, already in master branch?

Yes, it looked so polished ;)

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

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

-- Hannes

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 2435 bytes --]

Johannes Sixt said the following on 04.09.2007 12:53:
> Marius Storm-Olsen schrieb:
>> Johannes Sixt said the following on 04.09.2007 09:41:
>>> Thanks a lot! I've pushed it out in mingw.git's master.
>> Ops, already in master branch?
> 
> Yes, it looked so polished ;)
> 
>> 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(...

Actually, that's already handled.
GetFileType will report FILE_TYPE_UNKNOWN (0), and GetLastError() will 
return a result != NO_ERROR (<-- so you can follow the codepath, but 
it actually returns ERROR_INVALID_HANDLE (6))
So, it will fall all the way through the function, and end up 
returning -1, with errno = EBADF.

(I use the
     case FILE_TYPE_UNKNOWN:
         if (GetLastError() != NO_ERROR)
             break;
construct, since the documentation says that an FILE_TYPE_UNKNOWN is 
still a _valid_ handle iff GetLastError() returns NO_ERROR. So, then 
we pass it on to the normal fstat function to let that figure out what 
we're dealing with)


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

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.

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Marius Storm-Olsen schrieb:
> Johannes Sixt said the following on 04.09.2007 12:53:
>> Marius Storm-Olsen schrieb:
>>> 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.

It's not a big deal: It's an unlikely code path, actually an indication of a 
coding error, so you can leave your version.

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

Ack.

-- Hannes

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Johannes Schindelin schrieb:
> On Tue, 4 Sep 2007, Johannes Sixt wrote:
>> 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().

-- Hannes

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

Johannes Sixt said the following on 04.09.2007 13:36:
> Johannes Schindelin schrieb:
>> On Tue, 4 Sep 2007, Johannes Sixt wrote:
>>> 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().

Hmm, could be.
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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Tue, 4 Sep 2007, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > On Tue, 4 Sep 2007, Johannes Sixt wrote:
> > > 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.

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.

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

-- snip --
diff --git a/git-compat-util.h b/git-compat-util.h
index 172e828..2984319 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -470,6 +470,17 @@ static inline int git_unlink(const char *pathname) 
{
 #include <time.h>
 struct tm *gmtime_r(const time_t *timep, struct tm *result);
 struct tm *localtime_r(const time_t *timep, struct tm *result);
+static inline time_t mingw_time(void *dummy)
+{
+	FILETIME ft;
+	GetSystemTimeAsFileTime(&ft);
+	long long winTime = ((long long)ft.dwHighDateTime << 32) + 
ft.dwLowDateTime;
+	winTime -= 116444736000000000LL; /* Windows to Unix Epoch 
conversion */
+	winTime /= 10000000;		 /* Nano to seconds resolution 
*/
+	return (time_t)winTime;
+
+}
+#define time mingw_time
 #define hstrerror strerror
 
 char *mingw_getcwd(char *pointer, int len);
-- snap --

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Tue, 4 Sep 2007, Johannes Schindelin wrote:

> On Tue, 4 Sep 2007, Johannes Sixt wrote:
> 
> > Johannes Schindelin schrieb:
> > > On Tue, 4 Sep 2007, Johannes Sixt wrote:
> > > > 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.
> 
> 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.

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Johannes Schindelin schrieb:
> On Tue, 4 Sep 2007, Johannes Sixt wrote:
>> 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)?

Well, I don't think it'll make a difference. The tiny test program below 
prints twice the same number. My analysis of the time() implementation is 
obviously flawed.

-- Hannes

#include <windows.h>
#include <stdio.h>
#include <time.h>

int main()
{
	time_t t = time(NULL);
	FILETIME ft;

	GetSystemTimeAsFileTime(&ft);
	long long winTime = ((long long)ft.dwHighDateTime << 32)
		+ ft.dwLowDateTime;
	winTime -= 116444736000000000LL;
	winTime /= 10000000;

	printf("%d %d\n", t, (int) winTime);
	return 0;
}

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

Marius Storm-Olsen said the following on 04.09.2007 13:53:
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Marius Storm-Olsen schrieb:
> Marius Storm-Olsen said the following on 04.09.2007 13:53:
>> 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!

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

-- Hannes

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:

> Marius Storm-Olsen said the following on 04.09.2007 13:53:
> > 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!

Really?  It is failing again?  IIRC it was Linus' patch (which I 
cherry-picked into 59f8c189a5) that fixed it for me.

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:

> Marius Storm-Olsen said the following on 04.09.2007 13:53:
> > 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!

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.

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Tue, 4 Sep 2007, Johannes Sixt wrote:

> Marius Storm-Olsen schrieb:
> > Marius Storm-Olsen said the following on 04.09.2007 13:53:
> > > 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!
> 
> 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.

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

Johannes Schindelin wrote:
> On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:
>> 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.

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:

> Johannes Schindelin wrote:
> > On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:
> >> 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.

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.

Ciao,
Dscho

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Johannes Schindelin schrieb:
> Hi,
> 
> On Tue, 4 Sep 2007, Johannes Sixt wrote:
> 
>> Marius Storm-Olsen schrieb:
>>> Marius Storm-Olsen said the following on 04.09.2007 13:53:
>>>> 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!
>> 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.

Yes, I have that patch, but I don't have core.crlf set.

There are a lot of tests that do a series of commits in a row, with file 
changes such that the file size does _not_ change, aka "racy git" problem. 
This problem is obviously not 100% fixed on MSys: We do not correctly detect 
that there are files that might be modified ("racily clean" files). Some 
tests are prone to trigger the bug, others not.

-- Hannes

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[David Kastrup]

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

> On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:
>
>> 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.

Regardless of what you write in your PATH variable, Windows will
_always_ search for binaries first in your current work directory.  It
is not really funny.

-- 
David Kastrup

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

Johannes Schindelin wrote:
> On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:
>> Johannes Schindelin wrote:
>>> 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.
> 
> I guessed as much, but could not fetch the patch, since fetch was
> broken ;-)

Heh, yeah, that's happened to me too before. One neat thing with this
gitweb thingy though, is that you can actually grab the patch, like this
http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff_plain;h=f15974add93bdfa92775c77c00e7c65aefd42127

No excuse! ;-)

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Rutger Nijlunsing]

On Tue, Sep 04, 2007 at 01:57:38PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 4 Sep 2007, Johannes Schindelin wrote:
> 
> > On Tue, 4 Sep 2007, Johannes Sixt wrote:
> > 
> > > Johannes Schindelin schrieb:
> > > > On Tue, 4 Sep 2007, Johannes Sixt wrote:
> > > > > 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.
> > 
> > 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.


[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 ---------------------------------- eludias ed dse.nl
never attribute to a conspiracy which can be explained by incompetence
----------------------------------------------------------------------

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[David Kastrup]

Johannes Sixt <j.sixt@eudaptics.com> writes:

> Marius Storm-Olsen schrieb:
>
>> 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.

The problem is that it will break in the hour when day light saving
time ends.  Or when you travel between timezones.

Local time is not continuous or unique.  That is why it is a bad idea
to make decisions based on it.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Reece Dunn]

On 04/09/07, Rutger Nijlunsing <rutger@nospam.com> wrote:
> On Tue, Sep 04, 2007 at 01:57:38PM +0100, Johannes Schindelin wrote:
> > Hi,
> >
> > On Tue, 4 Sep 2007, Johannes Schindelin wrote:
> >
> > > On Tue, 4 Sep 2007, Johannes Sixt wrote:
> > >
> > > > Johannes Schindelin schrieb:
> > > > > On Tue, 4 Sep 2007, Johannes Sixt wrote:
> > > > > > 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.
> > >
> > > 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.
>
>
> [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

Hmm, this may explain something that I have been observing on
Windows+cygwin. When I run `git diff`, I sometimes get it reporting
that all (from what I can tell) files have changed, like a `find .
-type f -exec touch {} \;` command was run.

I was going to report this on a new thread, but this looks like a more
relevant place to do so.

My Windows machine is currently in Daylight Savings Time mode, and
from my observations, I have only seen this repeat first thing the
next day. I am not sure why, but every 24hrs, it looks as if the file
time reported by and checked from git is different to that reported by
stat.

I have not had time yet to play around with the mingw port and this
new stat implementation to see if it addresses this issue.

- Reece

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

Rutger Nijlunsing said the following on 04.09.2007 23:02:
> On Tue, Sep 04, 2007 at 01:57:38PM +0100, Johannes Schindelin wrote:
>> 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?

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Schindelin]

Hi,

On Wed, 5 Sep 2007, Marius Storm-Olsen wrote:

> Rutger Nijlunsing said the following on 04.09.2007 23:02:
> > On Tue, Sep 04, 2007 at 01:57:38PM +0100, Johannes Schindelin wrote:
> > > 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?

Umm.  I am helplessly overloaded with non-Git work, and just made sure 
that it compiles ;-)  Besides, I did not really understand what I was 
doing; just copying your code to convert from FILETIME to time_t.

Sorry,
Dscho

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Mon, Sep 03, 2007 at 01:32:07PM +0200, David Kastrup <dak@gnu.org> wrote:
> >> And if I understand the documentation correctly, then
> >
> >> $ mkdir foo && cd foo
> >> $ cat ../x
> >> x: No such file or directory
> >
> >> 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).

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

- VMiklos

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[David Kastrup]

[Vista and symbolic links]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Mon, Sep 03, 2007 at 01:32:07PM +0200, David Kastrup <dak@gnu.org> wrote:
>> >> 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
> ----

This is under Vista?  It would be the same under Unix.  A good rule of
thumb is to create relative symbolic links _only_ when the current
work directory is identical with the target directory (and make it so
if it isn't).  Alternatively, when one is completely awake and in full
possession of all one's mental facilities.  The state that every geek
is in 90% of the time according to his own perception, and 9% of the
time according to the computer.

If you tested this on Vista, chances are that they only bungled the
documentation in this case (and of course, needing sysadmin privileges
for this would be just silly).  Not without precedence.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Johannes Sixt]

Johannes Sixt schrieb:
> Marius Storm-Olsen schrieb:
>> Johannes Schindelin wrote:
>>> 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.

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?

-- Hannes

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

On Wed, Sep 05, 2007 at 09:01:46PM +0200, David Kastrup <dak@gnu.org> wrote:
> > 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.

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

- VMiklos

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[David Kastrup]

Miklos Vajna <vmiklos@frugalware.org> writes:

> On Wed, Sep 05, 2007 at 09:01:46PM +0200, David Kastrup <dak@gnu.org> wrote:
>> > 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.
>
> no, i never stated that this is Vista ;-) this is Linux.

Ok, so now we have established that we have not actually established
anything with regard to relative links under Vista short of what
Microsoft claims in its developer information (which has its fair
share of misleading and wrong information).

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.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

Johannes Sixt wrote:
> Johannes Sixt schrieb:
>> Marius Storm-Olsen schrieb:
>>> Johannes Schindelin wrote:
>>>> 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.
> 
> 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?

Ok, could you make a quick patch to add a git_utime() (and probably
git_time()) and see if the tests pass without the UTC to Local time patch?

--
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[Douglas Stockwell]

David Kastrup wrote:
> 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]

C:\stest>echo test > file

C:\stest>mkdir links

C:\stest>cd links

C:\stest\links>echo links > file

C:\stest\links>mklink relative file
symbolic link created for relative <<===>> file

C:\stest\links>mklink dotted .\file
symbolic link created for dotted <<===>> .\file

C:\stest\links>mklink parent ..\file
symbolic link created for parent <<===>> ..\file

C:\stest\links>mklink rooted \stest\links\file
symbolic link created for rooted <<===>> \stest\links\file

C:\stest\links>mklink absolute c:file
symbolic link created for absolute <<===>> c:file

C:\stest\links>dir
07/09/2007  08:09 AM    <DIR>          .
07/09/2007  08:09 AM    <DIR>          ..
07/09/2007  08:09 AM    <SYMLINK>      absolute [C:\stest\links\file]
07/09/2007  08:09 AM    <SYMLINK>      dotted [.\file]
07/09/2007  08:09 AM                 8 file
07/09/2007  08:09 AM    <SYMLINK>      parent [..\file]
07/09/2007  08:09 AM    <SYMLINK>      relative [file]
07/09/2007  08:09 AM    <SYMLINK>      rooted [\stest\links\file]

C:\stest\links>cd ..

C:\stest>type links\relative
links

C:\stest>type links\dotted
links

C:\stest>type links\parent
test

C:\stest>type links\rooted
links

C:\stest>type links\absolute
links

C:\stest>mkdir a

C:\stest>echo a > a\file

C:\stest>move links a
         1 dir(s) moved.

C:\stest>type a\links\relative
links

C:\stest>type a\links\dotted
links

C:\stest>type a\links\parent
a

C:\stest>type a\links\rooted
The system cannot find the path specified.

C:\stest>type a\links\absolute
The system cannot find the path specified.

Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too.

[David Kastrup]

Douglas Stockwell <doug@11011.net> writes:

> David Kastrup wrote:
>> 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]

[Examples]

Good.  So we will ultimately be able to support symlinks on some
Windows versions.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum