threads / discuss / 9746

Stats in Git

Subject: Stats in Git

## tl;dr

86 messages between Sep 2, 2007 and Sep 7, 2007.

replies: 85people: 12as markdown or json

Marius Storm-Olsen· Sep 2, 2007, 14:49 UTC · lore

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
Marius Storm-Olsen· Sep 2, 2007, 14:51 UTC · re: Marius Storm-Olsen · lore

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

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
Marius Storm-Olsen· Sep 2, 2007, 14:57 UTC · re: Marius Storm-Olsen · lore

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

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

-- .marius

Reece Dunn· Sep 2, 2007, 15:32 UTC · re: Marius Storm-Olsen · lore

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

On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote:
Show 9 quoted lines
> 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
Marius Storm-Olsen· Sep 2, 2007, 16:09 UTC · re: Reece Dunn · lore

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

Reece Dunn wrote:
Show 17 quoted lines
> 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'?

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

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

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

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

-- .marius

Reece Dunn· Sep 2, 2007, 16:33 UTC · re: Marius Storm-Olsen · lore

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

On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote:
Show 36 quoted lines
> 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?

Show 14 quoted lines
> > Would it be possible to use the git tree to manage the executable
> > state? That way, all files would not have their executable state set
> > by default on Windows. The problem with this is how then to set the
> > executable state? Having a git version of chmod may not be a good
> > idea, but then how else are you going to reliably and efficiently
> > modify the files permissions on Windows?
>
> The file-state-in-git-tree belongs in a different discussion. Have a
> look at the '.gitignore, .gitattributes, .gitmodules, .gitprecious?,
> .gitacls? etc.' and 'tracking perms/ownership [was: empty directories]'
> threads. Permissions are not a trivial topic, since systems represent
> them differently. This patch just tries to reflect the read, write and
> execute permissions as normal Windows would; and it only cares about
> file extensions (and the PE header, if it exists).

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
Brian Gernhardt· Sep 2, 2007, 16:47 UTC · re: Reece Dunn · lore

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

On Sep 2, 2007, at 12:33 PM, Reece Dunn wrote:
Show 11 quoted lines
> 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
Reece Dunn· Sep 2, 2007, 16:53 UTC · re: Brian Gernhardt · lore

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

On 02/09/07, Brian Gernhardt <benji@silverinsanity.com> wrote:
Show 20 quoted lines
>
> 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
Marius Storm-Olsen· Sep 2, 2007, 17:05 UTC · re: Reece Dunn · lore

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

Reece Dunn wrote:
Show 15 quoted lines
> 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

Johannes Schindelin· Sep 2, 2007, 17:44 UTC · re: Marius Storm-Olsen · lore

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

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

David Kastrup· Sep 2, 2007, 17:58 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 11 quoted lines
> 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
Marius Storm-Olsen· Sep 2, 2007, 18:18 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:
Show 8 quoted lines
> 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

Johannes Sixt· Sep 2, 2007, 18:16 UTC · re: Marius Storm-Olsen · lore

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

On Sunday 02 September 2007 16:51, Marius Storm-Olsen wrote:
Show 5 quoted lines
> 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?

Show 7 quoted lines
> +		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.

Show 5 quoted lines
> +/* Make git on Windows use git_lstat and git_stat instead of lstat and
> stat */ +int git_lstat(const char *file_name, struct stat *buf);
> +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
Marius Storm-Olsen· Sep 2, 2007, 18:44 UTC · re: Johannes Sixt · lore

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

Johannes Sixt wrote:
Show 8 quoted lines
> 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.

Show 12 quoted lines
>> +		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.

Show 6 quoted lines
>> +		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.

Show 6 quoted lines
>>  		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?
Show 9 quoted lines
>> +/* Make git on Windows use git_lstat and git_stat instead of lstat and
>> stat */ +int git_lstat(const char *file_name, struct stat *buf);
>> +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

Johannes Sixt· Sep 2, 2007, 19:07 UTC · re: Marius Storm-Olsen · lore

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

On Sunday 02 September 2007 20:44, Marius Storm-Olsen wrote:
Show 8 quoted lines
> 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.
Show 8 quoted lines
> >>  		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
Marius Storm-Olsen· Sep 2, 2007, 19:31 UTC · re: Marius Storm-Olsen · lore

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

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
Robin Rosenberg· Sep 2, 2007, 20:27 UTC · re: Marius Storm-Olsen · lore

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

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
Johannes Schindelin· Sep 2, 2007, 21:26 UTC · re: Robin Rosenberg · lore

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

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

Robin Rosenberg· Sep 2, 2007, 21:42 UTC · re: Johannes Schindelin · lore

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

söndag 02 september 2007 skrev Johannes Schindelin:
Show 10 quoted lines
> 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
Johannes Schindelin· Sep 2, 2007, 23:02 UTC · re: Robin Rosenberg · lore

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

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

Johannes Sixt· Sep 3, 2007, 07:07 UTC · re: Robin Rosenberg · lore

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

Robin Rosenberg schrieb:
Show 5 quoted lines
> $ 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
Miklos Vajna· Sep 3, 2007, 11:21 UTC · re: Johannes Sixt · lore

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

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
David Kastrup· Sep 3, 2007, 11:32 UTC · re: Miklos Vajna · lore

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

Miklos Vajna <vmiklos@frugalware.org> writes:
Show 14 quoted lines
> 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
Miklos Vajna· Sep 5, 2007, 16:02 UTC · re: David Kastrup · lore

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

On Mon, Sep 03, 2007 at 01:32:07PM +0200, David Kastrup <dak@gnu.org> wrote:
Show 13 quoted lines
> >> 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
David Kastrup· Sep 5, 2007, 19:01 UTC · re: Miklos Vajna · lore

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

[Vista and symbolic links]
Miklos Vajna <vmiklos@frugalware.org> writes:
Show 28 quoted lines
> 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
Miklos Vajna· Sep 6, 2007, 16:26 UTC · re: David Kastrup · lore

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

On Wed, Sep 05, 2007 at 09:01:46PM +0200, David Kastrup <dak@gnu.org> wrote:
Show 12 quoted lines
> > the situation what triggers the 'no such file' problem is:
> >
> > ----
> > $ touch foo/Makefile
> > $ mkdir bar
> > $ ln -s foo/Makefile bar
> > $ cd bar
> > $ cat Makefile
> > cat: Makefile: No such file or directory
> > ----
> 
> This is under Vista?  It would be the same under Unix.
no, i never stated that this is Vista ;-) this is Linux.
- VMiklos
David Kastrup· Sep 6, 2007, 16:33 UTC · re: Miklos Vajna · lore

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

Miklos Vajna <vmiklos@frugalware.org> writes:
Show 15 quoted lines
> 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
Douglas Stockwell· Sep 6, 2007, 23:31 UTC · re: David Kastrup · lore

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

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.

David Kastrup· Sep 7, 2007, 06:36 UTC · re: Douglas Stockwell · lore

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

Douglas Stockwell <doug@11011.net> writes:
Show 12 quoted lines
> 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
Alex Riesen· Sep 2, 2007, 21:38 UTC · re: Robin Rosenberg · lore

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

Robin Rosenberg, Sun, Sep 02, 2007 22:27:59 +0200:
Show 5 quoted lines
> 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.

Robin Rosenberg· Sep 2, 2007, 22:04 UTC · re: Alex Riesen · lore

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

söndag 02 september 2007 skrev Alex Riesen:
Show 11 quoted lines
> 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
Marius Storm-Olsen· Sep 3, 2007, 06:15 UTC · re: Robin Rosenberg · lore

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

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
Johannes Schindelin· Sep 3, 2007, 11:39 UTC · re: Marius Storm-Olsen · lore

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

Hi,
On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
Show 13 quoted lines
> 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

David Kastrup· Sep 3, 2007, 11:51 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 17 quoted lines
> 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
Marius Storm-Olsen· Sep 3, 2007, 11:53 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin said the following on 03.09.2007 13:39:
Show 12 quoted lines
> 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
Johannes Schindelin· Sep 3, 2007, 12:33 UTC · re: Marius Storm-Olsen · lore

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

Hi,
On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
Show 14 quoted lines
> 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

Alex Riesen· Sep 2, 2007, 21:41 UTC · re: Marius Storm-Olsen · lore

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

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?
Marius Storm-Olsen· Sep 3, 2007, 06:12 UTC · re: Alex Riesen · lore

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

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
Johannes Sixt· Sep 3, 2007, 07:47 UTC · re: Marius Storm-Olsen · lore

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

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:

Show 6 quoted lines
> -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
Marius Storm-Olsen· Sep 3, 2007, 07:55 UTC · re: Johannes Sixt · lore

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

Johannes Sixt said the following on 03.09.2007 09:47:
Show 7 quoted lines
> 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
Alex Riesen· Sep 2, 2007, 20:02 UTC · re: Marius Storm-Olsen · lore

Re: Stats in Git

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.

Marius Storm-Olsen· Sep 2, 2007, 20:09 UTC · re: Alex Riesen · lore

Re: Stats in Git

Alex Riesen wrote:
Show 5 quoted lines
> 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

Matthieu Moy· Sep 3, 2007, 08:19 UTC · re: Marius Storm-Olsen · lore

Re: Stats in Git

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
Marius Storm-Olsen· Sep 3, 2007, 12:38 UTC · lore

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

Marius Storm-Olsen said the following on 03.09.2007 14:12:
Show 14 quoted lines
>  With the our own implementations of lstat & fstat, the following test cases
>  are now fixed:
>      t4116-apply-reverte.sh
>          ok 3: apply in reverse
>      t4200-rerere.sh
>          ok 17: young records still live
>  However, the following test cases seems to fail now:
>      t6024-recursive-merge.sh
>          FAIL 1: setup tests
>          FAIL 3: result contains a conflict
>          FAIL 4: virtual trees were processed
>          FAIL 5: refuse to merge binaries
>  
>  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
Johannes Schindelin· Sep 3, 2007, 13:33 UTC · lore

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

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

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

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

Marius Storm-Olsen· Sep 3, 2007, 13:52 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin said the following on 03.09.2007 15:33:
Show 15 quoted lines
> 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
Show 19 quoted lines
>> With the our own implementations of lstat & fstat, the following test cases
>> are now fixed:
>>     t4116-apply-reverte.sh
>>         ok 3: apply in reverse
>>     t4200-rerere.sh
>>         ok 17: young records still live
>> However, the following test cases seems to fail now:
>>     t6024-recursive-merge.sh
>>         FAIL 1: setup tests
>>         FAIL 3: result contains a conflict
>>         FAIL 4: virtual trees were processed
>>         FAIL 5: refuse to merge binaries
>>
>> 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
Johannes Schindelin· Sep 3, 2007, 14:39 UTC · re: Marius Storm-Olsen · lore

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

Hi,
On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
Show 8 quoted lines
> 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

Marius Storm-Olsen· Sep 3, 2007, 16:22 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:
Show 9 quoted lines
> 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

Johannes Schindelin· Sep 3, 2007, 16:56 UTC · re: Marius Storm-Olsen · lore

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

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

Johannes Sixt· Sep 3, 2007, 13:53 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Sep 3, 2007, 14:35 UTC · re: Johannes Sixt · lore

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

Hi,
On Mon, 3 Sep 2007, Johannes Sixt wrote:
Show 7 quoted lines
> 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

Marius Storm-Olsen· Sep 3, 2007, 19:21 UTC · re: Johannes Schindelin · lore

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

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

Johannes Schindelin· Sep 4, 2007, 02:21 UTC · re: Marius Storm-Olsen · lore

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

Hi,
On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
Show 6 quoted lines
> 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

Johannes Sixt· Sep 4, 2007, 07:41 UTC · re: Marius Storm-Olsen · lore

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

Marius Storm-Olsen schrieb:
Show 6 quoted lines
> 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
David Kastrup· Sep 4, 2007, 08:53 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@eudaptics.com> writes:
Show 18 quoted lines
> 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
Marius Storm-Olsen· Sep 4, 2007, 10:20 UTC · re: Johannes Sixt · lore

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

Johannes Sixt said the following on 04.09.2007 09:41:
Show 9 quoted lines
> 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
Show 8 quoted lines
> The reason that t4200-rerere.sh fails is that we now store UTC in
> st_mtime. However, for the garbage-collection we compare this entry
> to a local time stamp. 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
Johannes Sixt· Sep 4, 2007, 10:53 UTC · re: Marius Storm-Olsen · lore

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

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

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

... time passes ...

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

-- Hannes
Marius Storm-Olsen· Sep 4, 2007, 11:21 UTC · re: Johannes Sixt · lore

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

Johannes Sixt said the following on 04.09.2007 12:53:
Show 17 quoted lines
> 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)
Show 17 quoted lines
>> Ok, I can give it a performance test, but I tend to agree with David 
>> Kastrup there. It would be better if we rather fix the places where we 
>> check with the local timestamp instead; depending of course on how many 
>> places we actually do this.
>> We'll see how much the timezone conversion in the custom stat functions 
>> actually hurt us performance wise.
> 
> I'd make the decision on the grounds of a perfomance test. If it turns out 
> that the penalty is bearable, we should keep this stuff private to the MinGW 
> build. Otherwise, we would need MinGW specific code at the call sites 
> (unless we can hide the opposite conversion in some other wrapper function).
> 
> ... time passes ...
> 
> Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I can run 
> it 100,000,000 times per second. So I'm confident that there won't be any 
> noticable degradation with my proposed change.

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
Johannes Sixt· Sep 4, 2007, 11:28 UTC · re: Marius Storm-Olsen · lore

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

Marius Storm-Olsen schrieb:
Show 10 quoted lines
> 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.

Show 6 quoted lines
>> 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
David Kastrup· Sep 4, 2007, 21:31 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j.sixt@eudaptics.com> writes:
Show 20 quoted lines
> 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
Johannes Schindelin· Sep 4, 2007, 10:48 UTC · re: Johannes Sixt · lore

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

Hi,
On Tue, 4 Sep 2007, Johannes Sixt wrote:
Show 13 quoted lines
> 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

Johannes Sixt· Sep 4, 2007, 11:36 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin schrieb:
Show 7 quoted lines
> 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
Marius Storm-Olsen· Sep 4, 2007, 11:53 UTC · re: Johannes Sixt · lore

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

Johannes Sixt said the following on 04.09.2007 13:36:
Show 18 quoted lines
> 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
Marius Storm-Olsen· Sep 4, 2007, 13:56 UTC · re: Marius Storm-Olsen · lore

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

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
Johannes Sixt· Sep 4, 2007, 14:07 UTC · re: Marius Storm-Olsen · lore

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

Marius Storm-Olsen schrieb:
Show 14 quoted lines
> 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
Johannes Schindelin· Sep 4, 2007, 14:32 UTC · re: Johannes Sixt · lore

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

Hi,
On Tue, 4 Sep 2007, Johannes Sixt wrote:
Show 17 quoted lines
> 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

Johannes Sixt· Sep 4, 2007, 14:52 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin schrieb:
Show 25 quoted lines
> 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
Johannes Schindelin· Sep 4, 2007, 14:16 UTC · re: Marius Storm-Olsen · lore

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

Hi,
On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:
Show 13 quoted lines
> 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

Johannes Schindelin· Sep 4, 2007, 14:30 UTC · re: Marius Storm-Olsen · lore

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

Hi,
On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:
Show 13 quoted lines
> 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

Marius Storm-Olsen· Sep 4, 2007, 14:43 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:
Show 9 quoted lines
> 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

Johannes Schindelin· Sep 4, 2007, 14:48 UTC · re: Marius Storm-Olsen · lore

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

Hi,
On Tue, 4 Sep 2007, Marius Storm-Olsen wrote:
Show 19 quoted lines
> 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

David Kastrup· Sep 4, 2007, 15:05 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
> 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
Marius Storm-Olsen· Sep 4, 2007, 16:32 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:
Show 17 quoted lines
> 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

Johannes Schindelin· Sep 4, 2007, 12:46 UTC · re: Johannes Sixt · lore

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

Hi,
On Tue, 4 Sep 2007, Johannes Sixt wrote:
Show 12 quoted lines
> 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.

Show 6 quoted lines
> 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
Johannes Schindelin· Sep 4, 2007, 12:57 UTC · re: Johannes Schindelin · lore

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

Hi,
On Tue, 4 Sep 2007, Johannes Schindelin wrote:
Show 18 quoted lines
> 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

Rutger Nijlunsing· Sep 4, 2007, 21:02 UTC · re: Johannes Schindelin · lore

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

On Tue, Sep 04, 2007 at 01:57:38PM +0100, Johannes Schindelin wrote:
Show 31 quoted lines
> 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
----------------------------------------------------------------------
Reece Dunn· Sep 4, 2007, 21:54 UTC · re: Rutger Nijlunsing · lore

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

On 04/09/07, Rutger Nijlunsing <rutger@nospam.com> wrote:
Show 39 quoted lines
> 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
Marius Storm-Olsen· Sep 5, 2007, 06:22 UTC · re: Rutger Nijlunsing · lore

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

Rutger Nijlunsing said the following on 04.09.2007 23:02:
Show 10 quoted lines
> 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
Johannes Schindelin· Sep 5, 2007, 10:15 UTC · re: Marius Storm-Olsen · lore

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

Hi,
On Wed, 5 Sep 2007, Marius Storm-Olsen wrote:
Show 20 quoted lines
> 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

Johannes Sixt· Sep 4, 2007, 13:03 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin schrieb:
Show 10 quoted lines
> 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;
}
Johannes Sixt· Sep 6, 2007, 16:18 UTC · re: Johannes Sixt · lore

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

Johannes Sixt schrieb:
Show 13 quoted lines
> 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
Marius Storm-Olsen· Sep 6, 2007, 16:34 UTC · re: Johannes Sixt · lore

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

Johannes Sixt wrote:
Show 19 quoted lines
> 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

Johannes Sixt· Sep 3, 2007, 13:49 UTC · lore

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

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

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.

Show 16 quoted lines
> With the our own implementations of lstat & fstat, the following test cases
> are now fixed:
>     t4116-apply-reverte.sh
>         ok 3: apply in reverse
>     t4200-rerere.sh
>         ok 17: young records still live
> However, the following test cases seems to fail now:
>     t6024-recursive-merge.sh
>         FAIL 1: setup tests
>         FAIL 3: result contains a conflict
>         FAIL 4: virtual trees were processed
>         FAIL 5: refuse to merge binaries
> 
> 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
Johannes Schindelin· Sep 3, 2007, 14:38 UTC · re: Johannes Sixt · lore

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

Hi,
On Mon, 3 Sep 2007, Johannes Sixt wrote:
Show 39 quoted lines
> 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

Marius Storm-Olsen· Sep 3, 2007, 16:15 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:
Show 25 quoted lines
> 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

Johannes Schindelin· Sep 3, 2007, 16:55 UTC · re: Marius Storm-Olsen · lore

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

Hi,
On Mon, 3 Sep 2007, Marius Storm-Olsen wrote:
Show 17 quoted lines
> 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

← back to recent threads