* Stats in Git @ 2007-09-02 14:49 Marius Storm-Olsen 2007-09-02 14:51 ` [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too Marius Storm-Olsen ` (2 more replies) 0 siblings, 3 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-02 14:49 UTC (permalink / raw) To: Git Mailing List 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 ^ permalink raw reply related [flat|nested] 86+ messages in thread
* [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 14:49 Stats in Git Marius Storm-Olsen @ 2007-09-02 14:51 ` Marius Storm-Olsen 2007-09-02 14:57 ` Marius Storm-Olsen ` (3 more replies) 2007-09-02 20:02 ` Stats in Git Alex Riesen 2007-09-03 8:19 ` Matthieu Moy 2 siblings, 4 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-02 14:51 UTC (permalink / raw) To: Git Mailing List; +Cc: Johannes Schindelin, Johannes Sixt 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 ^ permalink raw reply related [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 14:51 ` [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too Marius Storm-Olsen @ 2007-09-02 14:57 ` Marius Storm-Olsen 2007-09-02 15:32 ` Reece Dunn ` (2 subsequent siblings) 3 siblings, 0 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-02 14:57 UTC (permalink / raw) Cc: Git Mailing List, Johannes Schindelin, Johannes Sixt Sorry, should have added that this is a MinGW port patch only. And I forgot to include the msysgit mailinglist, nice.. -- .marius ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 14:51 ` [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too Marius Storm-Olsen 2007-09-02 14:57 ` Marius Storm-Olsen @ 2007-09-02 15:32 ` Reece Dunn 2007-09-02 16:09 ` Marius Storm-Olsen 2007-09-02 18:16 ` Johannes Sixt 2007-09-03 7:47 ` Johannes Sixt 3 siblings, 1 reply; 86+ messages in thread From: Reece Dunn @ 2007-09-02 15:32 UTC (permalink / raw) To: Marius Storm-Olsen, Git Mailing List, Johannes Schindelin, Johannes Sixt On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote: > This gives us a significant speedup when adding, committing and stat'ing files. > (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat) > > + if (ext && (!_stricmp(ext, ".exe") || > + !_stricmp(ext, ".com") || > + !_stricmp(ext, ".bat") || > + !_stricmp(ext, ".cmd"))) > + fMode |= S_IEXEC; > + } This breaks executable mode reporting for things like configure scripts and other shell scripts that may, or may not, be executable. Also, you may want to turn off the executable state for some of these extensions (for example if com or cmd were not actually executable files). This makes it impossible to manipulate git repositories properly on the MinGW platform. Would it be possible to use the git tree to manage the executable state? That way, all files would not have their executable state set by default on Windows. The problem with this is how then to set the executable state? Having a git version of chmod may not be a good idea, but then how else are you going to reliably and efficiently modify the files permissions on Windows? The rest of the patch looks good on a brief initial scan. - Reece ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 15:32 ` Reece Dunn @ 2007-09-02 16:09 ` Marius Storm-Olsen 2007-09-02 16:33 ` Reece Dunn 0 siblings, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-02 16:09 UTC (permalink / raw) To: Reece Dunn; +Cc: Git Mailing List, Johannes Schindelin, Johannes Sixt [-- Attachment #1: Type: text/plain, Size: 2936 bytes --] Reece Dunn wrote: > On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote: >> This gives us a significant speedup when adding, committing and stat'ing files. >> (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat) >> >> + if (ext && (!_stricmp(ext, ".exe") || >> + !_stricmp(ext, ".com") || >> + !_stricmp(ext, ".bat") || >> + !_stricmp(ext, ".cmd"))) >> + fMode |= S_IEXEC; >> + } > > This breaks executable mode reporting for things like configure > scripts and other shell scripts that may, or may not, be executable. > Also, you may want to turn off the executable state for some of these > extensions (for example if com or cmd were not actually executable > files). This makes it impossible to manipulate git repositories > properly on the MinGW platform. Actually, you don't really need the EXEC bit for Git to work. I just added it for completeness. (We _could_ remove that too, since it's slowing us down slightly ;-) Remember that Git isn't using MSys for its builtins, so MinGW Git doesn't understand the MSys notion of executable files anyways. The MinGW port actually peeks at the beginning of a file (ignoring exe files), and sees if there's an interpreter there. If there is, it will expand git-foo args... into sh git-foo args... and execute the command. So, it's not really affected by this change. I haven't had any problems with this patch on my system, so could you explain what you mean with 'this makes it impossible to manipulate git repositories'? > Would it be possible to use the git tree to manage the executable > state? That way, all files would not have their executable state set > by default on Windows. The problem with this is how then to set the > executable state? Having a git version of chmod may not be a good > idea, but then how else are you going to reliably and efficiently > modify the files permissions on Windows? The file-state-in-git-tree belongs in a different discussion. Have a look at the '.gitignore, .gitattributes, .gitmodules, .gitprecious?, .gitacls? etc.' and 'tracking perms/ownership [was: empty directories]' threads. Permissions are not a trivial topic, since systems represent them differently. This patch just tries to reflect the read, write and execute permissions as normal Windows would; and it only cares about file extensions (and the PE header, if it exists). Also note that my patch totally ignores the Group & Others part of the permission bits. Again, we're on Windows so we don't really care. We _could_ make it reflect the ACLs in Windows, but then we'd have to make it optional, since that's _really_ slow to 'stat'. > The rest of the patch looks good on a brief initial scan. Thanks -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 16:09 ` Marius Storm-Olsen @ 2007-09-02 16:33 ` Reece Dunn 2007-09-02 16:47 ` Brian Gernhardt 0 siblings, 1 reply; 86+ messages in thread From: Reece Dunn @ 2007-09-02 16:33 UTC (permalink / raw) To: Marius Storm-Olsen, Git Mailing List, Johannes Schindelin, Johannes Sixt On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote: > Reece Dunn wrote: > > On 02/09/07, Marius Storm-Olsen <marius@trolltech.com> wrote: > >> This gives us a significant speedup when adding, committing and stat'ing files. > >> (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat) > >> > >> + if (ext && (!_stricmp(ext, ".exe") || > >> + !_stricmp(ext, ".com") || > >> + !_stricmp(ext, ".bat") || > >> + !_stricmp(ext, ".cmd"))) > >> + fMode |= S_IEXEC; > >> + } > > > > This breaks executable mode reporting for things like configure > > scripts and other shell scripts that may, or may not, be executable. > > Also, you may want to turn off the executable state for some of these > > extensions (for example if com or cmd were not actually executable > > files). This makes it impossible to manipulate git repositories > > properly on the MinGW platform. > > Actually, you don't really need the EXEC bit for Git to work. I just > added it for completeness. (We _could_ remove that too, since it's > slowing us down slightly ;-) > > Remember that Git isn't using MSys for its builtins, so MinGW Git > doesn't understand the MSys notion of executable files anyways. > The MinGW port actually peeks at the beginning of a file (ignoring exe > files), and sees if there's an interpreter there. If there is, it will > expand > git-foo args... > into > sh git-foo args... > and execute the command. So, it's not really affected by this change. > > I haven't had any problems with this patch on my system, so could you > explain what you mean with 'this makes it impossible to manipulate git > repositories'? You pull a repository that contains executable scripts that are required to work in order to build the system. You then make some modifications to the local repository and run the 'git add .' command. Since this patch is reporting executable bits differently, the mode change is stored as well as the local modifications. Now the changes are pushed upstream (along with the file mode changes). Someone running a Linux machine, pulls your changes. When those files are checked out, the executable state of those scripts has now changed, preventing the Linux user from running those scripts. _That_ is what I meant. Or am I misunderstanding how git works in this case? > > Would it be possible to use the git tree to manage the executable > > state? That way, all files would not have their executable state set > > by default on Windows. The problem with this is how then to set the > > executable state? Having a git version of chmod may not be a good > > idea, but then how else are you going to reliably and efficiently > > modify the files permissions on Windows? > > The file-state-in-git-tree belongs in a different discussion. Have a > look at the '.gitignore, .gitattributes, .gitmodules, .gitprecious?, > .gitacls? etc.' and 'tracking perms/ownership [was: empty directories]' > threads. Permissions are not a trivial topic, since systems represent > them differently. This patch just tries to reflect the read, write and > execute permissions as normal Windows would; and it only cares about > file extensions (and the PE header, if it exists). I understand that this is not a trivial topic. I was thinking that this different behaviour w.r.t. the executable permission will break things when you have developers on both Linux and Windows, such as the cairo developers, for current git usage. I have not really been tracking those threads, but I will take a look at them. > Also note that my patch totally ignores the Group & Others part of the > permission bits. Again, we're on Windows so we don't really care. We > _could_ make it reflect the ACLs in Windows, but then we'd have to make > it optional, since that's _really_ slow to 'stat'. Sure. Cygwin does use ACLs to implement stat which is why it is slow. So anything that can speed git up here, without any breakage in functionality, is a good thing. - Reece ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 16:33 ` Reece Dunn @ 2007-09-02 16:47 ` Brian Gernhardt 2007-09-02 16:53 ` Reece Dunn 0 siblings, 1 reply; 86+ messages in thread From: Brian Gernhardt @ 2007-09-02 16:47 UTC (permalink / raw) To: Reece Dunn Cc: Marius Storm-Olsen, Git Mailing List, Johannes Schindelin, Johannes Sixt On Sep 2, 2007, at 12:33 PM, Reece Dunn wrote: > You pull a repository that contains executable scripts that are > required to work in order to build the system. You then make some > modifications to the local repository and run the 'git add .' command. > Since this patch is reporting executable bits differently, the mode > change is stored as well as the local modifications. Now the changes > are pushed upstream (along with the file mode changes). > > Someone running a Linux machine, pulls your changes. When those files > are checked out, the executable state of those scripts has now > changed, preventing the Linux user from running those scripts. _That_ > is what I meant. Or am I misunderstanding how git works in this case? This is what "git config core.fileMode false" is for. See git- config's man page for information (or Documentation/config.txt). We already have a way to tell git that the "executable bit" is worthless, and any Windows port should use it. ~~ B ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 16:47 ` Brian Gernhardt @ 2007-09-02 16:53 ` Reece Dunn 2007-09-02 17:05 ` Marius Storm-Olsen 0 siblings, 1 reply; 86+ messages in thread From: Reece Dunn @ 2007-09-02 16:53 UTC (permalink / raw) To: Brian Gernhardt, Marius Storm-Olsen, Git Mailing List, Johannes Schindelin On 02/09/07, Brian Gernhardt <benji@silverinsanity.com> wrote: > > On Sep 2, 2007, at 12:33 PM, Reece Dunn wrote: > > > You pull a repository that contains executable scripts that are > > required to work in order to build the system. You then make some > > modifications to the local repository and run the 'git add .' command. > > Since this patch is reporting executable bits differently, the mode > > change is stored as well as the local modifications. Now the changes > > are pushed upstream (along with the file mode changes). > > > > Someone running a Linux machine, pulls your changes. When those files > > are checked out, the executable state of those scripts has now > > changed, preventing the Linux user from running those scripts. _That_ > > is what I meant. Or am I misunderstanding how git works in this case? > > This is what "git config core.fileMode false" is for. See git- > config's man page for information (or Documentation/config.txt). > > We already have a way to tell git that the "executable bit" is > worthless, and any Windows port should use it. Ok, so as the executable bit is worthless, there doesn't need to be any special casing in this patch to deal with it. - Reece ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 16:53 ` Reece Dunn @ 2007-09-02 17:05 ` Marius Storm-Olsen 2007-09-02 17:44 ` Johannes Schindelin 0 siblings, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-02 17:05 UTC (permalink / raw) To: Reece Dunn Cc: Brian Gernhardt, Git Mailing List, Johannes Schindelin, Johannes Sixt [-- Attachment #1: Type: text/plain, Size: 1507 bytes --] Reece Dunn wrote: > On 02/09/07, Brian Gernhardt <benji@silverinsanity.com> wrote: >> On Sep 2, 2007, at 12:33 PM, Reece Dunn wrote: >>> You pull a repository that contains executable scripts that are >>> required to work in order to build the system. You then make some >>> modifications to the local repository and run the 'git add .' >>> command. Since this patch is reporting executable bits >>> differently, the mode change is stored as well as the local >>> modifications. Now the changes are pushed upstream (along with >>> the file mode changes). >> >> We already have a way to tell git that the "executable bit" is >> worthless, and any Windows port should use it. > > Ok, so as the executable bit is worthless, there doesn't need to be > any special casing in this patch to deal with it. Right, this is true. And I was debating it with myself, and just added it for completion; at least for the first revision of the patch. It doesn't really affect the performance all that much anyways. The conversion of the FileTime to unix time_t is far more heavy. (Which is why I'm debating to just ignore the access time) If we could somehow rather use the FileTimes directly in the index, instead of having to convert them, we could have even better performance when stat'ing on Windows. (However, it would result in an incompatible index, so everyone would have to 'git update-index --refresh' on all repositories before they can use the new version.) -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 17:05 ` Marius Storm-Olsen @ 2007-09-02 17:44 ` Johannes Schindelin 2007-09-02 17:58 ` David Kastrup 2007-09-02 18:18 ` Marius Storm-Olsen 0 siblings, 2 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-02 17:44 UTC (permalink / raw) To: Marius Storm-Olsen Cc: Reece Dunn, Brian Gernhardt, Git Mailing List, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 17:44 ` Johannes Schindelin @ 2007-09-02 17:58 ` David Kastrup 2007-09-02 18:18 ` Marius Storm-Olsen 1 sibling, 0 replies; 86+ messages in thread From: David Kastrup @ 2007-09-02 17:58 UTC (permalink / raw) To: git Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi, > > On Sun, 2 Sep 2007, Marius Storm-Olsen wrote: > >> The conversion of the FileTime to unix time_t is far more heavy. > > Really? If so, we might consider storing FILETIME->dwHightDateTime and > ->dwLowDateTime in the index. > > But I doubt it. AFAICT _getting_ at the stat data is the expensive thing > in Windows, not a 64-bit addition, subtraction and division. 64-bit division conceivably could be somewhat expensive, but it sounds like it should not be much compared to the cost of a system call. What is the code doing that division? -- David Kastrup, Kriemhildstr. 15, 44793 Bochum ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 17:44 ` Johannes Schindelin 2007-09-02 17:58 ` David Kastrup @ 2007-09-02 18:18 ` Marius Storm-Olsen 1 sibling, 0 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-02 18:18 UTC (permalink / raw) To: Johannes Schindelin Cc: Reece Dunn, Brian Gernhardt, Git Mailing List, Johannes Sixt [-- Attachment #1: Type: text/plain, Size: 997 bytes --] Johannes Schindelin wrote: > On Sun, 2 Sep 2007, Marius Storm-Olsen wrote: >> The conversion of the FileTime to unix time_t is far more heavy. > > Really? If so, we might consider storing FILETIME->dwHightDateTime > and ->dwLowDateTime in the index. > > But I doubt it. AFAICT _getting_ at the stat data is the expensive > thing in Windows, not a 64-bit addition, subtraction and division. Haha, sure sure, _getting_ that stat data in the first place is the expensive part on Windows. However, that's something you _have_ to do no matter what, so there's no way around that. Turns out that it wasn't as bad as i thought. If you have filetime_to_time_t() just return, say 116444736, I see git add . improve with ~0.5 sec for 100K files and git status improve with 0.05 sec Surely, avoiding the tripple stat'ing in 'git status' would help a lot more ;-) So, I guess we'll just leave the timestamp conversion as is, and avoid complicating the index. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 14:51 ` [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too Marius Storm-Olsen 2007-09-02 14:57 ` Marius Storm-Olsen 2007-09-02 15:32 ` Reece Dunn @ 2007-09-02 18:16 ` Johannes Sixt 2007-09-02 18:44 ` Marius Storm-Olsen 2007-09-03 7:47 ` Johannes Sixt 3 siblings, 1 reply; 86+ messages in thread From: Johannes Sixt @ 2007-09-02 18:16 UTC (permalink / raw) To: git; +Cc: Marius Storm-Olsen, Johannes Schindelin On Sunday 02 September 2007 16:51, Marius Storm-Olsen wrote: > This gives us a significant speedup when adding, committing and stat'ing > files. (Also, since Windows doesn't really handle symlinks, it's fine that > stat just uses lstat) > > Signed-off-by: Marius Storm-Olsen <mstormo_git@storm-olsen.com> Your numbers show an improvement of 50% and more. That is terrific! I'll test it out an put the patch into mingw.git. I hope you don't mind if I also include your analysis and statistics in the commit message. It's worth keeping around! BTW, which of your email addresses would you like registered as author? > + ext = strrchr(file_name, '.'); > + if (ext && (!_stricmp(ext, ".exe") || > + !_stricmp(ext, ".com") || > + !_stricmp(ext, ".bat") || > + !_stricmp(ext, ".cmd"))) > + fMode |= S_IEXEC; > + } I'm slightly negative about this. For a native Windows project the executable bit does not matter, and for a cross-platform project this check is not sufficient, but can even become annoying (think of a file named 'www.google.com'). So we can just as well spare the few cycles. > + buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since > it's not a stat64 */ Here's an idea for the future: With this self-made stat() implementation it should also be possible to get rid of Windows's native struct stat: Make a private definition of it, too, and use all 64 bits. > return 0; > + } > + errno = ENOENT; Of course we need a bit more detailed error conditions, most importantly EACCES should be distinguished. > +/* Make git on Windows use git_lstat and git_stat instead of lstat and > stat */ +int git_lstat(const char *file_name, struct stat *buf); > +int git_stat(const char *file_name, struct stat *buf); > +#define lstat(x,y) git_lstat(x,y) > +#define stat(x,y) git_stat(x,y) I'd go the short route without git_stat() and #define stat(x,y) git_lstat(x,y) -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 18:16 ` Johannes Sixt @ 2007-09-02 18:44 ` Marius Storm-Olsen 2007-09-02 19:07 ` Johannes Sixt 2007-09-02 19:31 ` Marius Storm-Olsen 0 siblings, 2 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-02 18:44 UTC (permalink / raw) To: Johannes Sixt; +Cc: git, Johannes Schindelin [-- Attachment #1: Type: text/plain, Size: 4943 bytes --] Johannes Sixt wrote: > On Sunday 02 September 2007 16:51, Marius Storm-Olsen wrote: >> This gives us a significant speedup when adding, committing and stat'ing >> files. (Also, since Windows doesn't really handle symlinks, it's fine that >> stat just uses lstat) >> >> Signed-off-by: Marius Storm-Olsen <mstormo_git@storm-olsen.com> > > Your numbers show an improvement of 50% and more. That is terrific! Yes, I was surprised myself about the impact. Didn't think it would make _such_ a difference. And if you compare it to the results we had before Linus' performance fix too, just checkout the performance improvements we've had (based on Moe's test script): Before Now ------------------------- ------------------------- Command: git init Command: git init ------------------------- ------------------------- real 0m0.031s real 0m0.078s user 0m0.031s user 0m0.031s sys 0m0.000s sys 0m0.000s ------------------------- ------------------------- Command: git add . Command: git add . ------------------------- ------------------------- real 0m19.328s real 0m12.187s user 0m0.015s user 0m0.015s sys 0m0.015s sys 0m0.015s ------------------------- ------------------------- Command: git commit -a... Command: git commit -a... ------------------------- ------------------------- real 0m30.937s real 0m17.297s user 0m0.015s user 0m0.015s sys 0m0.015s sys 0m0.015s ------------------------- ------------------------- 3x Command: git-status 3x Command: git-status ------------------------- ------------------------- real 0m19.531s real 0m5.344s user 0m0.211s user 0m0.015s sys 0m0.136s sys 0m0.031s real 0m19.532s real 0m5.390s user 0m0.259s user 0m0.031s sys 0m0.091s sys 0m0.000s real 0m19.593s real 0m5.344s user 0m0.211s user 0m0.015s sys 0m0.152s sys 0m0.016s ------------------------- ------------------------- Command: git commit... Command: git commit... (single file) (single file) ------------------------- ------------------------- real 0m36.688s real 0m7.875s user 0m0.031s user 0m0.015s sys 0m0.000s sys 0m0.000s > I'll test it out an put the patch into mingw.git. I hope you don't mind if I > also include your analysis and statistics in the commit message. It's worth > keeping around! BTW, which of your email addresses would you like registered > as author? Sure, include the stats if you'd like. You can use mstormo_git@storm-olsen.com for email address. >> + ext = strrchr(file_name, '.'); >> + if (ext && (!_stricmp(ext, ".exe") || >> + !_stricmp(ext, ".com") || >> + !_stricmp(ext, ".bat") || >> + !_stricmp(ext, ".cmd"))) >> + fMode |= S_IEXEC; >> + } > > I'm slightly negative about this. For a native Windows project the executable > bit does not matter, and for a cross-platform project this check is not > sufficient, but can even become annoying (think of a file > named 'www.google.com'). So we can just as well spare the few cycles. Ok, that's fine by me. It was only added for completeness, and with no benefits I'd say we drop it too. >> + buf->st_size = fdata.nFileSizeLow; /* Can't use nFileSizeHigh, since >> it's not a stat64 */ > > Here's an idea for the future: With this self-made stat() implementation it > should also be possible to get rid of Windows's native struct stat: Make a > private definition of it, too, and use all 64 bits. Yep, that will shave off one assignment, bit-shift and addition. Quick operations, but still worth while IMO. No point wasting cycles where we don't have to. >> return 0; >> + } >> + errno = ENOENT; > > Of course we need a bit more detailed error conditions, most importantly > EACCES should be distinguished. Right, you want to do that in a second commit? >> +/* Make git on Windows use git_lstat and git_stat instead of lstat and >> stat */ +int git_lstat(const char *file_name, struct stat *buf); >> +int git_stat(const char *file_name, struct stat *buf); >> +#define lstat(x,y) git_lstat(x,y) >> +#define stat(x,y) git_stat(x,y) > > I'd go the short route without git_stat() and > > #define stat(x,y) git_lstat(x,y) Please do, thanks. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 18:44 ` Marius Storm-Olsen @ 2007-09-02 19:07 ` Johannes Sixt 2007-09-02 19:31 ` Marius Storm-Olsen 1 sibling, 0 replies; 86+ messages in thread From: Johannes Sixt @ 2007-09-02 19:07 UTC (permalink / raw) To: git; +Cc: Marius Storm-Olsen, Johannes Schindelin On Sunday 02 September 2007 20:44, Marius Storm-Olsen wrote: > Johannes Sixt wrote: > > I'm slightly negative about this. For a native Windows project the > > executable bit does not matter, and for a cross-platform project this > > check is not sufficient, but can even become annoying (think of a file > > named 'www.google.com'). So we can just as well spare the few cycles. > > Ok, that's fine by me. It was only added for completeness, and with no > benefits I'd say we drop it too. I'll amend the patch accordingly. > >> return 0; > >> + } > >> + errno = ENOENT; > > > > Of course we need a bit more detailed error conditions, most importantly > > EACCES should be distinguished. > > Right, you want to do that in a second commit? Yes, please. Please don't forget to take care of the trailing-slash annoyance. -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 18:44 ` Marius Storm-Olsen 2007-09-02 19:07 ` Johannes Sixt @ 2007-09-02 19:31 ` Marius Storm-Olsen 2007-09-02 20:27 ` Robin Rosenberg 2007-09-02 21:41 ` Alex Riesen 1 sibling, 2 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-02 19:31 UTC (permalink / raw) To: Johannes Sixt; +Cc: git, Johannes Schindelin 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 ^ permalink raw reply related [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 19:31 ` Marius Storm-Olsen @ 2007-09-02 20:27 ` Robin Rosenberg 2007-09-02 21:26 ` Johannes Schindelin ` (2 more replies) 2007-09-02 21:41 ` Alex Riesen 1 sibling, 3 replies; 86+ messages in thread From: Robin Rosenberg @ 2007-09-02 20:27 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, git, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 20:27 ` Robin Rosenberg @ 2007-09-02 21:26 ` Johannes Schindelin 2007-09-02 21:42 ` Robin Rosenberg 2007-09-02 21:38 ` Alex Riesen 2007-09-03 6:15 ` Marius Storm-Olsen 2 siblings, 1 reply; 86+ messages in thread From: Johannes Schindelin @ 2007-09-02 21:26 UTC (permalink / raw) To: Robin Rosenberg; +Cc: Marius Storm-Olsen, Johannes Sixt, git 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 ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 21:26 ` Johannes Schindelin @ 2007-09-02 21:42 ` Robin Rosenberg 2007-09-02 23:02 ` Johannes Schindelin 2007-09-03 7:07 ` Johannes Sixt 0 siblings, 2 replies; 86+ messages in thread From: Robin Rosenberg @ 2007-09-02 21:42 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Marius Storm-Olsen, Johannes Sixt, git söndag 02 september 2007 skrev Johannes Schindelin: > Hi, > > On Sun, 2 Sep 2007, Robin Rosenberg wrote: > > > s?ndag 02 september 2007 skrev Marius Storm-Olsen: > > > (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat) > > > > It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx > > Oh? *goes and tries to create one on a USB stick* No. Besides, IIRC you Set core.symlinks = false for working on a broken file system > cannot even create symlinks to another partition. Copying a symlink will That is not the normal use for symlinks. It is a case where it breaks, but symbolic links in a git repo that points outside the repo is probably not a good idea, especially if it is a cross platform project. It is far less broken than today anyway. > copy the _linked_ file. So to call this "symlink" is a little... uhm... > preposterous. $ ln -s Makefile x $ cp x y $ ls -ld x y lrwxrwxrwx 1 me me 8 sep 2 23:36 x -> Makefile -rw-r--r-- 1 me me 32164 sep 2 23:36 y Same behaviour as on linux. > Plus, on a page linked from the link you posted, it says that it is > only supported from Vista onwards. So you must be kidding me. core.symlinks = false if ithey aren't supported. You actually need admin privileges too, but I don't know any windows developer who hasn't got that. -- robin ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 21:42 ` Robin Rosenberg @ 2007-09-02 23:02 ` Johannes Schindelin 2007-09-03 7:07 ` Johannes Sixt 1 sibling, 0 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-02 23:02 UTC (permalink / raw) To: Robin Rosenberg; +Cc: Marius Storm-Olsen, Johannes Sixt, git 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 ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 21:42 ` Robin Rosenberg 2007-09-02 23:02 ` Johannes Schindelin @ 2007-09-03 7:07 ` Johannes Sixt 2007-09-03 11:21 ` Miklos Vajna 1 sibling, 1 reply; 86+ messages in thread From: Johannes Sixt @ 2007-09-03 7:07 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Marius Storm-Olsen, Johannes Sixt, git Robin Rosenberg schrieb: > $ ln -s Makefile x > $ cp x y > $ ls -ld x y > lrwxrwxrwx 1 me me 8 sep 2 23:36 x -> Makefile > -rw-r--r-- 1 me me 32164 sep 2 23:36 y And if I understand the documentation correctly, then $ mkdir foo && cd foo $ cat ../x x: No such file or directory Right? The docs say that symlinks without backslash are relative to the current directory (!!!). -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-03 7:07 ` Johannes Sixt @ 2007-09-03 11:21 ` Miklos Vajna 2007-09-03 11:32 ` David Kastrup 0 siblings, 1 reply; 86+ messages in thread From: Miklos Vajna @ 2007-09-03 11:21 UTC (permalink / raw) To: Johannes Sixt; +Cc: git, Johannes Schindelin, Marius Storm-Olsen, Johannes Sixt [-- Attachment #1: Type: text/plain, Size: 357 bytes --] On Mon, Sep 03, 2007 at 09:07:42AM +0200, Johannes Sixt <j.sixt@eudaptics.com> wrote: > >$ ls -ld x y > >lrwxrwxrwx 1 me me 8 sep 2 23:36 x -> Makefile > >-rw-r--r-- 1 me me 32164 sep 2 23:36 y > And if I understand the documentation correctly, then > $ mkdir foo && cd foo > $ cat ../x > x: No such file or directory > Right? correct. - VMiklos [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-03 11:21 ` Miklos Vajna @ 2007-09-03 11:32 ` David Kastrup 2007-09-05 16:02 ` Miklos Vajna 0 siblings, 1 reply; 86+ messages in thread From: David Kastrup @ 2007-09-03 11:32 UTC (permalink / raw) To: git Miklos Vajna <vmiklos@frugalware.org> writes: > On Mon, Sep 03, 2007 at 09:07:42AM +0200, Johannes Sixt <j.sixt@eudaptics.com> wrote: >> >$ ls -ld x y >> >lrwxrwxrwx 1 me me 8 sep 2 23:36 x -> Makefile >> >-rw-r--r-- 1 me me 32164 sep 2 23:36 y > >> And if I understand the documentation correctly, then > >> $ mkdir foo && cd foo >> $ cat ../x >> x: No such file or directory > >> Right? > > correct. Have you tested this, or is this from reading the documentation? In either case: brilliant, but the former would be funnier (depending on one's sense of humor, of course). -- David Kastrup ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-03 11:32 ` David Kastrup @ 2007-09-05 16:02 ` Miklos Vajna 2007-09-05 19:01 ` David Kastrup 0 siblings, 1 reply; 86+ messages in thread From: Miklos Vajna @ 2007-09-05 16:02 UTC (permalink / raw) To: David Kastrup; +Cc: git [-- Attachment #1: Type: text/plain, Size: 752 bytes --] On Mon, Sep 03, 2007 at 01:32:07PM +0200, David Kastrup <dak@gnu.org> wrote: > >> And if I understand the documentation correctly, then > > > >> $ mkdir foo && cd foo > >> $ cat ../x > >> x: No such file or directory > > > >> Right? > > > > correct. > > Have you tested this, or is this from reading the documentation? In > either case: brilliant, but the former would be funnier (depending on > one's sense of humor, of course). umm, thanks for the notice, i was wrong: ---- $ cat ../x this is makefile ---- the situation what triggers the 'no such file' problem is: ---- $ touch foo/Makefile $ mkdir bar $ ln -s foo/Makefile bar $ cd bar $ cat Makefile cat: Makefile: No such file or directory ---- - VMiklos [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-05 16:02 ` Miklos Vajna @ 2007-09-05 19:01 ` David Kastrup 2007-09-06 16:26 ` Miklos Vajna 0 siblings, 1 reply; 86+ messages in thread From: David Kastrup @ 2007-09-05 19:01 UTC (permalink / raw) To: Miklos Vajna; +Cc: git [Vista and symbolic links] Miklos Vajna <vmiklos@frugalware.org> writes: > On Mon, Sep 03, 2007 at 01:32:07PM +0200, David Kastrup <dak@gnu.org> wrote: >> >> And if I understand the documentation correctly, then >> > >> >> $ mkdir foo && cd foo >> >> $ cat ../x >> >> x: No such file or directory >> > >> > correct. >> >> Have you tested this, or is this from reading the documentation? > > umm, thanks for the notice, i was wrong: > > ---- > $ cat ../x > this is makefile > ---- > > the situation what triggers the 'no such file' problem is: > > ---- > $ touch foo/Makefile > $ mkdir bar > $ ln -s foo/Makefile bar > $ cd bar > $ cat Makefile > cat: Makefile: No such file or directory > ---- This is under Vista? It would be the same under Unix. A good rule of thumb is to create relative symbolic links _only_ when the current work directory is identical with the target directory (and make it so if it isn't). Alternatively, when one is completely awake and in full possession of all one's mental facilities. The state that every geek is in 90% of the time according to his own perception, and 9% of the time according to the computer. If you tested this on Vista, chances are that they only bungled the documentation in this case (and of course, needing sysadmin privileges for this would be just silly). Not without precedence. -- David Kastrup, Kriemhildstr. 15, 44793 Bochum ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-05 19:01 ` David Kastrup @ 2007-09-06 16:26 ` Miklos Vajna 2007-09-06 16:33 ` David Kastrup 0 siblings, 1 reply; 86+ messages in thread From: Miklos Vajna @ 2007-09-06 16:26 UTC (permalink / raw) To: David Kastrup; +Cc: git [-- Attachment #1: Type: text/plain, Size: 455 bytes --] On Wed, Sep 05, 2007 at 09:01:46PM +0200, David Kastrup <dak@gnu.org> wrote: > > the situation what triggers the 'no such file' problem is: > > > > ---- > > $ touch foo/Makefile > > $ mkdir bar > > $ ln -s foo/Makefile bar > > $ cd bar > > $ cat Makefile > > cat: Makefile: No such file or directory > > ---- > > This is under Vista? It would be the same under Unix. no, i never stated that this is Vista ;-) this is Linux. - VMiklos [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-06 16:26 ` Miklos Vajna @ 2007-09-06 16:33 ` David Kastrup 2007-09-06 23:31 ` Douglas Stockwell 0 siblings, 1 reply; 86+ messages in thread From: David Kastrup @ 2007-09-06 16:33 UTC (permalink / raw) To: git Miklos Vajna <vmiklos@frugalware.org> writes: > On Wed, Sep 05, 2007 at 09:01:46PM +0200, David Kastrup <dak@gnu.org> wrote: >> > the situation what triggers the 'no such file' problem is: >> > >> > ---- >> > $ touch foo/Makefile >> > $ mkdir bar >> > $ ln -s foo/Makefile bar >> > $ cd bar >> > $ cat Makefile >> > cat: Makefile: No such file or directory >> > ---- >> >> This is under Vista? It would be the same under Unix. > > no, i never stated that this is Vista ;-) this is Linux. Ok, so now we have established that we have not actually established anything with regard to relative links under Vista short of what Microsoft claims in its developer information (which has its fair share of misleading and wrong information). If anybody is as fortunate as to actually have Vista available, it would be nice if he corroborated that relative links under Vista are indeed (as Microsoft appears to claim) relative with regard to the current work directory rather than the directory containing the link. -- David Kastrup, Kriemhildstr. 15, 44793 Bochum ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-06 16:33 ` David Kastrup @ 2007-09-06 23:31 ` Douglas Stockwell 2007-09-07 6:36 ` David Kastrup 0 siblings, 1 reply; 86+ messages in thread From: Douglas Stockwell @ 2007-09-06 23:31 UTC (permalink / raw) To: David Kastrup; +Cc: git 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. ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-06 23:31 ` Douglas Stockwell @ 2007-09-07 6:36 ` David Kastrup 0 siblings, 0 replies; 86+ messages in thread From: David Kastrup @ 2007-09-07 6:36 UTC (permalink / raw) To: Douglas Stockwell; +Cc: git Douglas Stockwell <doug@11011.net> writes: > David Kastrup wrote: >> If anybody is as fortunate as to actually have Vista available, it >> would be nice if he corroborated that relative links under Vista are >> indeed (as Microsoft appears to claim) relative with regard to the >> current work directory rather than the directory containing the link. > > I believe the wording "resolves the path relative to the current > directory" actually refers to the creation of links, not to their use. > > C:\stest>ver > > Microsoft Windows [Version 6.0.6000] [Examples] Good. So we will ultimately be able to support symlinks on some Windows versions. -- David Kastrup, Kriemhildstr. 15, 44793 Bochum ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 20:27 ` Robin Rosenberg 2007-09-02 21:26 ` Johannes Schindelin @ 2007-09-02 21:38 ` Alex Riesen 2007-09-02 22:04 ` Robin Rosenberg 2007-09-03 6:15 ` Marius Storm-Olsen 2 siblings, 1 reply; 86+ messages in thread From: Alex Riesen @ 2007-09-02 21:38 UTC (permalink / raw) To: Robin Rosenberg Cc: Marius Storm-Olsen, Johannes Sixt, git, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 21:38 ` Alex Riesen @ 2007-09-02 22:04 ` Robin Rosenberg 0 siblings, 0 replies; 86+ messages in thread From: Robin Rosenberg @ 2007-09-02 22:04 UTC (permalink / raw) To: Alex Riesen; +Cc: Marius Storm-Olsen, Johannes Sixt, git, Johannes Schindelin söndag 02 september 2007 skrev Alex Riesen: > Robin Rosenberg, Sun, Sep 02, 2007 22:27:59 +0200: > > söndag 02 september 2007 skrev Marius Storm-Olsen: > > > (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat) > > > > It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx > > > > Except they fscked it up, as usual for microsoft: it 's got a > mandatory argument specifying what the target should be, file or > directory. And they don't tell what happens when the argument is wrong > or the target does not exists. Typical, too. Why would this API be an exception? -- robin ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 20:27 ` Robin Rosenberg 2007-09-02 21:26 ` Johannes Schindelin 2007-09-02 21:38 ` Alex Riesen @ 2007-09-03 6:15 ` Marius Storm-Olsen 2007-09-03 11:39 ` Johannes Schindelin 2 siblings, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-03 6:15 UTC (permalink / raw) To: Robin Rosenberg; +Cc: Johannes Sixt, git, Johannes Schindelin [-- Attachment #1: Type: text/plain, Size: 621 bytes --] Robin Rosenberg said the following on 02.09.2007 22:27: > söndag 02 september 2007 skrev Marius Storm-Olsen: >> (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat) > > It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx Yeah, I know about Vista's improved support for symbolic links. However, I think we can let that lay for a while, until we decide to make Git generate proper symlinks on Vista. I don't see it as a 1st priority at the moment, and we can always add the needed functionality in a separate stat() function later. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-03 6:15 ` Marius Storm-Olsen @ 2007-09-03 11:39 ` Johannes Schindelin 2007-09-03 11:51 ` David Kastrup 2007-09-03 11:53 ` Marius Storm-Olsen 0 siblings, 2 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-03 11:39 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Robin Rosenberg, Johannes Sixt, git Hi, On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: > Robin Rosenberg said the following on 02.09.2007 22:27: > > s?ndag 02 september 2007 skrev Marius Storm-Olsen: > > > (Also, since Windows doesn't really handle symlinks, it's fine that > > > stat just uses lstat) > > > > It does now: See > > http://msdn2.microsoft.com/en-us/library/aa363866.aspx > > Yeah, I know about Vista's improved support for symbolic links. However, > I think we can let that lay for a while, until we decide to make Git > generate proper symlinks on Vista. I don't see it as a 1st priority at > the moment, and we can always add the needed functionality in a separate > stat() function later. ... and force everybody to upgrade to Vista, thereby working for Microsoft for free? You _know_ that I will oppose that change. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-03 11:39 ` Johannes Schindelin @ 2007-09-03 11:51 ` David Kastrup 2007-09-03 11:53 ` Marius Storm-Olsen 1 sibling, 0 replies; 86+ messages in thread From: David Kastrup @ 2007-09-03 11:51 UTC (permalink / raw) To: git Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: > >> Robin Rosenberg said the following on 02.09.2007 22:27: >> > s?ndag 02 september 2007 skrev Marius Storm-Olsen: >> > > (Also, since Windows doesn't really handle symlinks, it's fine that >> > > stat just uses lstat) >> > >> > It does now: See >> > http://msdn2.microsoft.com/en-us/library/aa363866.aspx >> >> Yeah, I know about Vista's improved support for symbolic >> links. However, I think we can let that lay for a while, until we >> decide to make Git generate proper symlinks on Vista. I don't see >> it as a 1st priority at the moment, and we can always add the >> needed functionality in a separate stat() function later. > > ... and force everybody to upgrade to Vista, Nonsense. Supporting a feature is different from requiring a feature. > thereby working for Microsoft for free? You _know_ that I will > oppose that change. If Microsoft decides to shoot their users less in the foot than previously, I don't think that we should take over the gun. However, if the symbolic link semantics hinted at elsewhere indeed are as broken as claimed and/or documented, the actual usefulness of symbolic links seems so limited that we would not be doing their users a favor by supporting relative symlinks. And absolute links frankly have very little place in a _work_ directory (and git does not currently keep track of enough things in order to make it useful as a filesystem snapshot system). I would like to see actual test results to get a confirmation of whether indeed relative symlinks are as broken under Vista as rumored. If they are, it seems quite pointless supporting any symlinks under Windows at the moment. Until I see actual test results, I would give Microsoft the benefit of doubt. -- David Kastrup ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-03 11:39 ` Johannes Schindelin 2007-09-03 11:51 ` David Kastrup @ 2007-09-03 11:53 ` Marius Storm-Olsen 2007-09-03 12:33 ` Johannes Schindelin 1 sibling, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-03 11:53 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Robin Rosenberg, Johannes Sixt, git [-- Attachment #1: Type: text/plain, Size: 1079 bytes --] Johannes Schindelin said the following on 03.09.2007 13:39: > On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: >> Robin Rosenberg said the following on 02.09.2007 22:27: >>> It does now: See >>> http://msdn2.microsoft.com/en-us/library/aa363866.aspx >> Yeah, I know about Vista's improved support for symbolic links. >> However, I think we can let that lay for a while, until we decide >> to make Git generate proper symlinks on Vista. I don't see it as >> a 1st priority at the moment, and we can always add the needed >> functionality in a separate stat() function later. > > ... and force everybody to upgrade to Vista, thereby working for > Microsoft for free? You _know_ that I will oppose that change. ;-) I wouldn't dream of it! Nah, my comment was more 'allow usage of proper Symlinks on Vista' at a later point. I would still argue that the default would be what we have today. So, it would have to be an option. But seeing what they've done to the symlinks there, it might be far fetched. We'll worry about that (much) later.. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-03 11:53 ` Marius Storm-Olsen @ 2007-09-03 12:33 ` Johannes Schindelin 0 siblings, 0 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-03 12:33 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Robin Rosenberg, Johannes Sixt, git Hi, On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: > Johannes Schindelin said the following on 03.09.2007 13:39: > > On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: > > > Robin Rosenberg said the following on 02.09.2007 22:27: > > > > It does now: See http://msdn2.microsoft.com/en-us/library/aa363866.aspx > > > Yeah, I know about Vista's improved support for symbolic links. > > > However, I think we can let that lay for a while, until we decide > > > to make Git generate proper symlinks on Vista. I don't see it as > > > a 1st priority at the moment, and we can always add the needed > > > functionality in a separate stat() function later. > > > > ... and force everybody to upgrade to Vista, thereby working for > > Microsoft for free? You _know_ that I will oppose that change. > > ;-) I wouldn't dream of it! Hehe. > Nah, my comment was more 'allow usage of proper Symlinks on Vista' at a > later point. I would still argue that the default would be what we have > today. So, it would have to be an option. Okay, I could live with that. > But seeing what they've done to the symlinks there, it might be far > fetched. We'll worry about that (much) later.. Yes, it is funny how they do it over and over and over again. Embrace, "Extend", Extinguish. And I thought that eventually people would be clever enough to realise... Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 19:31 ` Marius Storm-Olsen 2007-09-02 20:27 ` Robin Rosenberg @ 2007-09-02 21:41 ` Alex Riesen 2007-09-03 6:12 ` Marius Storm-Olsen 1 sibling, 1 reply; 86+ messages in thread From: Alex Riesen @ 2007-09-02 21:41 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, git, Johannes Schindelin 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? ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 21:41 ` Alex Riesen @ 2007-09-03 6:12 ` Marius Storm-Olsen 0 siblings, 0 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-03 6:12 UTC (permalink / raw) To: Alex Riesen; +Cc: Johannes Sixt, git, Johannes Schindelin [-- Attachment #1: Type: text/plain, Size: 1241 bytes --] Alex Riesen said the following on 02.09.2007 23:41: > Marius Storm-Olsen, Sun, Sep 02, 2007 21:31:40 +0200: >> + buf->st_ino = 0; > > You sure about that? Ever wondered why it is not so on everywhere else? Pretty sure. If you look at Windows' native version of stat, it will return you st_ino = 0. Or maybe you where referring to something else, and I just missed your point? AFAIK, the ino in the index is only to be _really_ sure that nothing has changed with the file, and we can just skip it on Windows. If in doubt, try running this on your Windows box: #include <windows.h> #include <stdio.h> #include <sys/types.h> #include <sys/stat.h> int main(int, char **) { wchar_t DirSpec[] = L".\\*"; WIN32_FIND_DATA FindFileData; HANDLE hFind = FindFirstFile(DirSpec, &FindFileData); if (hFind == INVALID_HANDLE_VALUE) { printf ("Crap happened: %u\n", GetLastError()); return -1; } struct _stat buf; while (FindNextFile(hFind, &FindFileData) != 0) { if (!_wstat(FindFileData.cFileName, &buf)) printf("file: %S, ino: %u\n", FindFileData.cFileName, buf.st_ino); } FindClose(hFind); return 0; } -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-02 14:51 ` [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too Marius Storm-Olsen ` (2 preceding siblings ...) 2007-09-02 18:16 ` Johannes Sixt @ 2007-09-03 7:47 ` Johannes Sixt 2007-09-03 7:55 ` Marius Storm-Olsen [not found] ` <46DBFA2A.7050003@trolltech.com> 3 siblings, 2 replies; 86+ messages in thread From: Johannes Sixt @ 2007-09-03 7:47 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Git Mailing List, Johannes Schindelin, Johannes Sixt Marius Storm-Olsen schrieb: > This gives us a significant speedup when adding, committing and stat'ing files. > (Also, since Windows doesn't really handle symlinks, it's fine that stat just uses lstat) Unfortunately, the patch fails t0010-racy-git.sh. I suspect the filetime conversion: > -int lstat(const char *file_name, struct stat *buf) > +static inline time_t filetime_to_time_t(const FILETIME *ft) > +{ > + long long winTime = ((long long)ft->dwHighDateTime << 32) + ft->dwLowDateTime; > + winTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */ > + winTime /= 10000000; /* Nano to seconds resolution */ Shouldn't this be 1000000000 according to your comment? However, even if I make that change, the test still fails. Could you please look into this? > + return (time_t)winTime; > +} -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too. 2007-09-03 7:47 ` Johannes Sixt @ 2007-09-03 7:55 ` Marius Storm-Olsen [not found] ` <46DBFA2A.7050003@trolltech.com> 1 sibling, 0 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-03 7:55 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List, Johannes Schindelin, Johannes Sixt [-- Attachment #1: Type: text/plain, Size: 423 bytes --] Johannes Sixt said the following on 03.09.2007 09:47: > Marius Storm-Olsen schrieb: >> This gives us a significant speedup when adding, committing and >> stat'ing files. (Also, since Windows doesn't really handle >> symlinks, it's fine that stat just uses lstat) > > Unfortunately, the patch fails t0010-racy-git.sh. I suspect the > filetime conversion: Ok, I'll try to get to it later today. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
[parent not found: <46DBFA2A.7050003@trolltech.com>]
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API [not found] ` <46DBFA2A.7050003@trolltech.com> @ 2007-09-03 12:38 ` Marius Storm-Olsen 2007-09-03 13:33 ` Johannes Schindelin 2007-09-03 13:49 ` Johannes Sixt 2 siblings, 0 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-03 12:38 UTC (permalink / raw) To: Johannes Sixt; +Cc: Git Mailing List, Johannes Schindelin, Johannes Sixt [-- Attachment #1: Type: text/plain, Size: 811 bytes --] Marius Storm-Olsen said the following on 03.09.2007 14:12: > With the our own implementations of lstat & fstat, the following test cases > are now fixed: > t4116-apply-reverte.sh > ok 3: apply in reverse > t4200-rerere.sh > ok 17: young records still live > However, the following test cases seems to fail now: > t6024-recursive-merge.sh > FAIL 1: setup tests > FAIL 3: result contains a conflict > FAIL 4: virtual trees were processed > FAIL 5: refuse to merge binaries > > See attached test case logs. How about that, reading the diff backwards :-) Oh well, you see the issues based on the logs. Would be nice if you guys could also give the testcases a run to see if you get the same result. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API [not found] ` <46DBFA2A.7050003@trolltech.com> 2007-09-03 12:38 ` [PATCH] Add a new lstat and fstat implementation based on Win32 API Marius Storm-Olsen @ 2007-09-03 13:33 ` Johannes Schindelin 2007-09-03 13:52 ` Marius Storm-Olsen ` (2 more replies) 2007-09-03 13:49 ` Johannes Sixt 2 siblings, 3 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-03 13:33 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, Git Mailing List, Johannes Sixt Hi, On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: > There was a problem with racy conditions, which this revision fixes. > The problem was that fstat was using the builtin implementation, which for > for some reason is off by some amount of seconds. (This is probably due to > some leap-year issue in one of the implementations. However, Microsoft tells > us to use 116444736000000000 in http://support.microsoft.com/kb/167296, so > I'll stick with that.) > Also, since both stat and lstat proved to be rather slow, having our own > version of fstat is probably also wise. At least we now control all the > stat'ing, so we _know_ they are compatible. > Also note that this revision makes git_lstat call itself after modifying > the filename, instead of the builtin stat, for the same reasons. At least some of these informations should go into the commit message, too. > With the our own implementations of lstat & fstat, the following test cases > are now fixed: > t4116-apply-reverte.sh > ok 3: apply in reverse > t4200-rerere.sh > ok 17: young records still live > However, the following test cases seems to fail now: > t6024-recursive-merge.sh > FAIL 1: setup tests > FAIL 3: result contains a conflict > FAIL 4: virtual trees were processed > FAIL 5: refuse to merge binaries > > See attached test case logs. > Are some of these test cases unstable, so the result will fluctuate on > Windows? I saw some funny stuff on Windows, like test cases succeeding when run interactively, but failing when run from "make test". BTW it would have been way easier to apply your patch, had you followed SubmittingPatches... To make it easier on others, I just uploaded it into the "teststat" branch on 4msysgit.git (subject to removal in a few days). First comment: it seems git_fstat() is not declared properly, so there are quite a few compiler warnings. Running the tests now. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 13:33 ` Johannes Schindelin @ 2007-09-03 13:52 ` Marius Storm-Olsen 2007-09-03 14:39 ` Johannes Schindelin 2007-09-03 13:53 ` Johannes Sixt 2007-09-03 19:21 ` Marius Storm-Olsen 2 siblings, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-03 13:52 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Johannes Sixt, Git Mailing List, Johannes Sixt [-- Attachment #1: Type: text/plain, Size: 2522 bytes --] Johannes Schindelin said the following on 03.09.2007 15:33: > On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: >> There was a problem with racy conditions, which this revision fixes. >> The problem was that fstat was using the builtin implementation, which for >> for some reason is off by some amount of seconds. (This is probably due to >> some leap-year issue in one of the implementations. However, Microsoft tells >> us to use 116444736000000000 in http://support.microsoft.com/kb/167296, so >> I'll stick with that.) >> Also, since both stat and lstat proved to be rather slow, having our own >> version of fstat is probably also wise. At least we now control all the >> stat'ing, so we _know_ they are compatible. >> Also note that this revision makes git_lstat call itself after modifying >> the filename, instead of the builtin stat, for the same reasons. > > At least some of these informations should go into the commit message, > too. Sure >> With the our own implementations of lstat & fstat, the following test cases >> are now fixed: >> t4116-apply-reverte.sh >> ok 3: apply in reverse >> t4200-rerere.sh >> ok 17: young records still live >> However, the following test cases seems to fail now: >> t6024-recursive-merge.sh >> FAIL 1: setup tests >> FAIL 3: result contains a conflict >> FAIL 4: virtual trees were processed >> FAIL 5: refuse to merge binaries >> >> See attached test case logs. >> Are some of these test cases unstable, so the result will fluctuate on >> Windows? > > I saw some funny stuff on Windows, like test cases succeeding when run > interactively, but failing when run from "make test". Ok, I ran 'make test', so maybe that's it? I'll rerun them later. > BTW it would have been way easier to apply your patch, had you followed > SubmittingPatches... Heh, I actually tried, using the Thunderbird way. Of course the attachments are non-conforming :-) What was the problem? Whitespace issues, Windows EOL, attachments, or all of the above? :-) > To make it easier on others, I just uploaded it into the "teststat" branch > on 4msysgit.git (subject to removal in a few days). Cool, thanks > First comment: it seems git_fstat() is not declared properly, so there are > quite a few compiler warnings. /me slaps self. Right, sorry 'bout that. I'll amend the declaration in git-compat-util.h. > Running the tests now. Great, thanks! -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 13:52 ` Marius Storm-Olsen @ 2007-09-03 14:39 ` Johannes Schindelin 2007-09-03 16:22 ` Marius Storm-Olsen 0 siblings, 1 reply; 86+ messages in thread From: Johannes Schindelin @ 2007-09-03 14:39 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, Git Mailing List, Johannes Sixt Hi, On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: > Johannes Schindelin said the following on 03.09.2007 15:33: > > > BTW it would have been way easier to apply your patch, had you > > followed SubmittingPatches... > > Heh, I actually tried, using the Thunderbird way. Of course the > attachments are non-conforming :-) What was the problem? Whitespace > issues, Windows EOL, attachments, or all of the above? :-) git am said that the patch was empty. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 14:39 ` Johannes Schindelin @ 2007-09-03 16:22 ` Marius Storm-Olsen 2007-09-03 16:56 ` Johannes Schindelin 0 siblings, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-03 16:22 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Johannes Sixt, Git Mailing List, Johannes Sixt [-- Attachment #1: Type: text/plain, Size: 651 bytes --] Johannes Schindelin wrote: > On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: >> Johannes Schindelin said the following on 03.09.2007 15:33: >>> BTW it would have been way easier to apply your patch, had you >>> followed SubmittingPatches... >> Heh, I actually tried, using the Thunderbird way. Of course the >> attachments are non-conforming :-) What was the problem? Whitespace >> issues, Windows EOL, attachments, or all of the above? :-) > > git am said that the patch was empty. Hmm, must be the attachments then. I'll use the 4msysgit.git repo from now on. I assume it'll be ok if I +push to the teststat branch? -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 16:22 ` Marius Storm-Olsen @ 2007-09-03 16:56 ` Johannes Schindelin 0 siblings, 0 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-03 16:56 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, Git Mailing List, Johannes Sixt 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 ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 13:33 ` Johannes Schindelin 2007-09-03 13:52 ` Marius Storm-Olsen @ 2007-09-03 13:53 ` Johannes Sixt 2007-09-03 14:35 ` Johannes Schindelin 2007-09-03 19:21 ` Marius Storm-Olsen 2 siblings, 1 reply; 86+ messages in thread From: Johannes Sixt @ 2007-09-03 13:53 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Marius Storm-Olsen, Git Mailing List, Johannes Sixt Johannes Schindelin schrieb: > I saw some funny stuff on Windows, like test cases succeeding when run > interactively, but failing when run from "make test". That's very likely the issue that we work around by inserting "sleep 1" at strategic points, which is a timing (race condition) issue and does not depend on interactive vs. "make test". -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 13:53 ` Johannes Sixt @ 2007-09-03 14:35 ` Johannes Schindelin 0 siblings, 0 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-03 14:35 UTC (permalink / raw) To: Johannes Sixt; +Cc: Marius Storm-Olsen, Git Mailing List, Johannes Sixt Hi, On Mon, 3 Sep 2007, Johannes Sixt wrote: > Johannes Schindelin schrieb: > > I saw some funny stuff on Windows, like test cases succeeding when run > > interactively, but failing when run from "make test". > > That's very likely the issue that we work around by inserting "sleep 1" at > strategic points, which is a timing (race condition) issue and does not > depend on interactive vs. "make test". Makes sense to me now. Especially around t5510 -- t5701 I see those (and I do not run "make test" often, since it takes _ages_. Thanks, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 13:33 ` Johannes Schindelin 2007-09-03 13:52 ` Marius Storm-Olsen 2007-09-03 13:53 ` Johannes Sixt @ 2007-09-03 19:21 ` Marius Storm-Olsen 2007-09-04 2:21 ` Johannes Schindelin 2007-09-04 7:41 ` Johannes Sixt 2 siblings, 2 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-03 19:21 UTC (permalink / raw) To: Johannes Schindelin, Johannes Sixt, Johannes Sixt; +Cc: Git Mailing List [-- Attachment #1: Type: text/plain, Size: 983 bytes --] Johannes Schindelin wrote: > To make it easier on others, I just uploaded it into the "teststat" > branch on 4msysgit.git (subject to removal in a few days). Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. RFC, and please test. The patch also incorporates some of Hannes local changes, with some modifications. Hannes, does it look ok for you? You can add a tag to the commit message if you'd like, and just +push it. On Hannes' request (and to which I fully agree), I've gone back to the old implementation of filetime_to_time_t(), since it was a bit 'nasty'. (If we want to target CE in the future, it will quite possibly break) http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f4f3fbddf6e0f16f66f94cedf66614e0e3643496 > First comment: it seems git_fstat() is not declared properly, so > there are quite a few compiler warnings. This is also fixed, of course. Hope this is the final 'cut' :-) Later! -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 19:21 ` Marius Storm-Olsen @ 2007-09-04 2:21 ` Johannes Schindelin 2007-09-04 7:41 ` Johannes Sixt 1 sibling, 0 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-04 2:21 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, Johannes Sixt, Git Mailing List Hi, On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: > Johannes Schindelin wrote: > > To make it easier on others, I just uploaded it into the "teststat" > > branch on 4msysgit.git (subject to removal in a few days). > > Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. > RFC, and please test. I'm too tired for more, but the first test which fails (consistently) is t4200 here: * FAIL 17: young records still live test -f .git/rr-cache/08f6c39f296af7e0dd1b3b7d8bba18d0365f605f/preimage && test -f .git/rr-cache/4000000000000000000000000000000000000000/preimage Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 19:21 ` Marius Storm-Olsen 2007-09-04 2:21 ` Johannes Schindelin @ 2007-09-04 7:41 ` Johannes Sixt 2007-09-04 8:53 ` David Kastrup ` (3 more replies) 1 sibling, 4 replies; 86+ messages in thread From: Johannes Sixt @ 2007-09-04 7:41 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Schindelin, Johannes Sixt, Git Mailing List Marius Storm-Olsen schrieb: > Johannes Schindelin wrote: >> To make it easier on others, I just uploaded it into the "teststat" >> branch on 4msysgit.git (subject to removal in a few days). > > Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. > RFC, and please test. Thanks a lot! I've pushed it out in mingw.git's master. The reason that t4200-rerere.sh fails is that we now store UTC in st_mtime. However, for the garbage-collection we compare this entry to a local time stamp. Therefore, I've pushed out a fixup patch at the top of mingw.git's devel branch that converts mtime to local time (http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=1b62ecb31068af06c2fa7664f06c6c36316aac2c). Would you kindly conduct the performance test with this patch? I'm afraid that this makes us substantially slower. -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 7:41 ` Johannes Sixt @ 2007-09-04 8:53 ` David Kastrup 2007-09-04 10:20 ` Marius Storm-Olsen ` (2 subsequent siblings) 3 siblings, 0 replies; 86+ messages in thread From: David Kastrup @ 2007-09-04 8:53 UTC (permalink / raw) To: git Johannes Sixt <j.sixt@eudaptics.com> writes: > Marius Storm-Olsen schrieb: >> Johannes Schindelin wrote: >>> To make it easier on others, I just uploaded it into the "teststat" >>> branch on 4msysgit.git (subject to removal in a few days). >> >> Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. >> RFC, and please test. > > Thanks a lot! I've pushed it out in mingw.git's master. > > The reason that t4200-rerere.sh fails is that we now store UTC in > st_mtime. However, for the garbage-collection we compare this entry > to a local time stamp. Therefore, I've pushed out a fixup patch at > the top of mingw.git's devel branch that converts mtime to local > time > (http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=1b62ecb31068af06c2fa7664f06c6c36316aac2c). Would > you kindly conduct the performance test with this patch? I'm afraid > that this makes us substantially slower. Wouldn't it make more sense to convert local time to mtime? That's one conversion per second at most rather than one conversion per file. -- David Kastrup ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 7:41 ` Johannes Sixt 2007-09-04 8:53 ` David Kastrup @ 2007-09-04 10:20 ` Marius Storm-Olsen 2007-09-04 10:53 ` Johannes Sixt 2007-09-04 10:48 ` Johannes Schindelin 2007-09-06 16:18 ` Johannes Sixt 3 siblings, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-04 10:20 UTC (permalink / raw) To: Johannes Sixt, Johannes Schindelin; +Cc: Johannes Sixt, Git Mailing List [-- Attachment #1: Type: text/plain, Size: 1880 bytes --] Johannes Sixt said the following on 04.09.2007 09:41: > Marius Storm-Olsen schrieb: >> Johannes Schindelin wrote: >>> To make it easier on others, I just uploaded it into the >>> "teststat" branch on 4msysgit.git (subject to removal in a few >>> days). >> Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' >> branch. RFC, and please test. > > Thanks a lot! I've pushed it out in mingw.git's master. Ops, already in master branch? Ok, I found out that the custom fstat function was incomplete, so I completed it. However, since you've already pushed it to your main branch, I've added it as a separate commit to the 4msysgit.git 'teststat' branch. It might also explain some of the testfailures we were seeing, but I haven't finished the test run yet. (So, consider the patch something to play with, and don't commit it to your 'master' branch yet! ;-) http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127 > The reason that t4200-rerere.sh fails is that we now store UTC in > st_mtime. However, for the garbage-collection we compare this entry > to a local time stamp. Therefore, I've pushed out a fixup patch at > the top of mingw.git's devel branch that converts mtime to local > time > (http://repo.or.cz/w/git/mingw.git?a=commitdiff;h=1b62ecb31068af06c2fa7664f06c6c36316aac2c). > Would you kindly conduct the performance test with this patch? I'm > afraid that this makes us substantially slower. Ok, I can give it a performance test, but I tend to agree with David Kastrup there. It would be better if we rather fix the places where we check with the local timestamp instead; depending of course on how many places we actually do this. We'll see how much the timezone conversion in the custom stat functions actually hurt us performance wise. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 10:20 ` Marius Storm-Olsen @ 2007-09-04 10:53 ` Johannes Sixt 2007-09-04 11:21 ` Marius Storm-Olsen 2007-09-04 21:31 ` David Kastrup 0 siblings, 2 replies; 86+ messages in thread From: Johannes Sixt @ 2007-09-04 10:53 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Schindelin, Johannes Sixt, Git Mailing List Marius Storm-Olsen schrieb: > Johannes Sixt said the following on 04.09.2007 09:41: >> Thanks a lot! I've pushed it out in mingw.git's master. > > Ops, already in master branch? Yes, it looked so polished ;) > http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127 Looks good, although you should now handle INVALID_HANDLE_VALUE at the beginning of git_fstat() like this: HANDLE fh = (HANDLE)_get_osfhandle(fd); if (fh == INVALID_HANDLE_VALUE) return -1; /* errno has been set */ if (GetFileInformationByHandle(... > Ok, I can give it a performance test, but I tend to agree with David > Kastrup there. It would be better if we rather fix the places where we > check with the local timestamp instead; depending of course on how many > places we actually do this. > We'll see how much the timezone conversion in the custom stat functions > actually hurt us performance wise. I'd make the decision on the grounds of a perfomance test. If it turns out that the penalty is bearable, we should keep this stuff private to the MinGW build. Otherwise, we would need MinGW specific code at the call sites (unless we can hide the opposite conversion in some other wrapper function). ... time passes ... Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I can run it 100,000,000 times per second. So I'm confident that there won't be any noticable degradation with my proposed change. -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 10:53 ` Johannes Sixt @ 2007-09-04 11:21 ` Marius Storm-Olsen 2007-09-04 11:28 ` Johannes Sixt 2007-09-04 21:31 ` David Kastrup 1 sibling, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-04 11:21 UTC (permalink / raw) To: Johannes Sixt; +Cc: Johannes Schindelin, Johannes Sixt, Git Mailing List [-- Attachment #1: Type: text/plain, Size: 2435 bytes --] Johannes Sixt said the following on 04.09.2007 12:53: > Marius Storm-Olsen schrieb: >> Johannes Sixt said the following on 04.09.2007 09:41: >>> Thanks a lot! I've pushed it out in mingw.git's master. >> Ops, already in master branch? > > Yes, it looked so polished ;) > >> http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127 > > Looks good, although you should now handle INVALID_HANDLE_VALUE at the > beginning of git_fstat() like this: > > HANDLE fh = (HANDLE)_get_osfhandle(fd); > if (fh == INVALID_HANDLE_VALUE) > return -1; /* errno has been set */ > > if (GetFileInformationByHandle(... Actually, that's already handled. GetFileType will report FILE_TYPE_UNKNOWN (0), and GetLastError() will return a result != NO_ERROR (<-- so you can follow the codepath, but it actually returns ERROR_INVALID_HANDLE (6)) So, it will fall all the way through the function, and end up returning -1, with errno = EBADF. (I use the case FILE_TYPE_UNKNOWN: if (GetLastError() != NO_ERROR) break; construct, since the documentation says that an FILE_TYPE_UNKNOWN is still a _valid_ handle iff GetLastError() returns NO_ERROR. So, then we pass it on to the normal fstat function to let that figure out what we're dealing with) >> Ok, I can give it a performance test, but I tend to agree with David >> Kastrup there. It would be better if we rather fix the places where we >> check with the local timestamp instead; depending of course on how many >> places we actually do this. >> We'll see how much the timezone conversion in the custom stat functions >> actually hurt us performance wise. > > I'd make the decision on the grounds of a perfomance test. If it turns out > that the penalty is bearable, we should keep this stuff private to the MinGW > build. Otherwise, we would need MinGW specific code at the call sites > (unless we can hide the opposite conversion in some other wrapper function). > > ... time passes ... > > Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I can run > it 100,000,000 times per second. So I'm confident that there won't be any > noticable degradation with my proposed change. Ok. I haven't done the performance test with Git yet, but we'll see. If it's not noticeable, I'll add it to all the timestamps we have. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 11:21 ` Marius Storm-Olsen @ 2007-09-04 11:28 ` Johannes Sixt 0 siblings, 0 replies; 86+ messages in thread From: Johannes Sixt @ 2007-09-04 11:28 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Schindelin, Johannes Sixt, Git Mailing List Marius Storm-Olsen schrieb: > Johannes Sixt said the following on 04.09.2007 12:53: >> Marius Storm-Olsen schrieb: >>> Johannes Sixt said the following on 04.09.2007 09:41: >>> http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127 >> >> >> Looks good, although you should now handle INVALID_HANDLE_VALUE at the >> beginning of git_fstat() like this: > > Actually, that's already handled. It's not a big deal: It's an unlikely code path, actually an indication of a coding error, so you can leave your version. >> Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I can >> run it 100,000,000 times per second. So I'm confident that there won't >> be any noticable degradation with my proposed change. > > Ok. I haven't done the performance test with Git yet, but we'll see. If > it's not noticeable, I'll add it to all the timestamps we have. Ack. -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 10:53 ` Johannes Sixt 2007-09-04 11:21 ` Marius Storm-Olsen @ 2007-09-04 21:31 ` David Kastrup 1 sibling, 0 replies; 86+ messages in thread From: David Kastrup @ 2007-09-04 21:31 UTC (permalink / raw) To: Johannes Sixt Cc: Marius Storm-Olsen, Johannes Schindelin, Johannes Sixt, Git Mailing List Johannes Sixt <j.sixt@eudaptics.com> writes: > Marius Storm-Olsen schrieb: > >> Ok, I can give it a performance test, but I tend to agree with >> David Kastrup there. It would be better if we rather fix the places >> where we check with the local timestamp instead; depending of >> course on how many places we actually do this. We'll see how much >> the timezone conversion in the custom stat functions actually hurt >> us performance wise. > > I'd make the decision on the grounds of a perfomance test. If it > turns out that the penalty is bearable, we should keep this stuff > private to the MinGW build. Otherwise, we would need MinGW specific > code at the call sites (unless we can hide the opposite conversion > in some other wrapper function). > > ... time passes ... > > Ok, I just tested FileTimeToLocalFileTime() in a tight loop, and I > can run it 100,000,000 times per second. So I'm confident that there > won't be any noticable degradation with my proposed change. The problem is that it will break in the hour when day light saving time ends. Or when you travel between timezones. Local time is not continuous or unique. That is why it is a bad idea to make decisions based on it. -- David Kastrup, Kriemhildstr. 15, 44793 Bochum ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 7:41 ` Johannes Sixt 2007-09-04 8:53 ` David Kastrup 2007-09-04 10:20 ` Marius Storm-Olsen @ 2007-09-04 10:48 ` Johannes Schindelin 2007-09-04 11:36 ` Johannes Sixt 2007-09-06 16:18 ` Johannes Sixt 3 siblings, 1 reply; 86+ messages in thread From: Johannes Schindelin @ 2007-09-04 10:48 UTC (permalink / raw) To: Johannes Sixt; +Cc: Marius Storm-Olsen, Johannes Sixt, Git Mailing List Hi, On Tue, 4 Sep 2007, Johannes Sixt wrote: > Marius Storm-Olsen schrieb: > > Johannes Schindelin wrote: > > > To make it easier on others, I just uploaded it into the "teststat" > > > branch on 4msysgit.git (subject to removal in a few days). > > > > Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. > > RFC, and please test. > > Thanks a lot! I've pushed it out in mingw.git's master. > > The reason that t4200-rerere.sh fails is that we now store UTC in st_mtime. > However, for the garbage-collection we compare this entry to a local time > stamp. Thanks for the explanation. > Therefore, I've pushed out a fixup patch at the top of mingw.git's devel > branch that converts mtime to local time On Linux, we compare to UTC to begin with, right? We should do that here, too... So if time(NULL) does not return UTC on MinGW, we have to wrap that function, too. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 10:48 ` Johannes Schindelin @ 2007-09-04 11:36 ` Johannes Sixt 2007-09-04 11:53 ` Marius Storm-Olsen 2007-09-04 12:46 ` Johannes Schindelin 0 siblings, 2 replies; 86+ messages in thread From: Johannes Sixt @ 2007-09-04 11:36 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Marius Storm-Olsen, Johannes Sixt, Git Mailing List Johannes Schindelin schrieb: > On Tue, 4 Sep 2007, Johannes Sixt wrote: >> Therefore, I've pushed out a fixup patch at the top of mingw.git's devel >> branch that converts mtime to local time > > On Linux, we compare to UTC to begin with, right? We should do that here, > too... So if time(NULL) does not return UTC on MinGW, we have to wrap > that function, too. According to MSDN, time(NULL) returns "the number of seconds elapsed since [epoch] according to the system clock". Please don't ask me what "the system clock" is. Reading the implementation of time(), it starts with GetLocalTime(), determines whether daylight saving is in effect, and continues with another round of timezone adjustment - mind you: _not_ a timezone reversal (!!). Doesn't this look extremely bogus? It seems we really need a wrapper for time(). -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 11:36 ` Johannes Sixt @ 2007-09-04 11:53 ` Marius Storm-Olsen 2007-09-04 13:56 ` Marius Storm-Olsen 2007-09-04 12:46 ` Johannes Schindelin 1 sibling, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-04 11:53 UTC (permalink / raw) To: Johannes Sixt, Johannes Schindelin; +Cc: Johannes Sixt, Git Mailing List [-- Attachment #1: Type: text/plain, Size: 1183 bytes --] Johannes Sixt said the following on 04.09.2007 13:36: > Johannes Schindelin schrieb: >> On Tue, 4 Sep 2007, Johannes Sixt wrote: >>> Therefore, I've pushed out a fixup patch at the top of >>> mingw.git's devel branch that converts mtime to local time >> On Linux, we compare to UTC to begin with, right? We should do >> that here, too... So if time(NULL) does not return UTC on MinGW, >> we have to wrap that function, too. > > According to MSDN, time(NULL) returns "the number of seconds > elapsed since [epoch] according to the system clock". Please don't > ask me what "the system clock" is. > > Reading the implementation of time(), it starts with > GetLocalTime(), determines whether daylight saving is in effect, > and continues with another round of timezone adjustment - mind you: > _not_ a timezone reversal (!!). Doesn't this look extremely bogus? > > It seems we really need a wrapper for time(). Hmm, could be. In the meantime, I've pushed out a new patch http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59 /me starts another test run, to see how our tests are doing now.. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 11:53 ` Marius Storm-Olsen @ 2007-09-04 13:56 ` Marius Storm-Olsen 2007-09-04 14:07 ` Johannes Sixt ` (2 more replies) 0 siblings, 3 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-04 13:56 UTC (permalink / raw) To: Johannes Sixt, Johannes Schindelin; +Cc: Johannes Sixt, Git Mailing List [-- Attachment #1: Type: text/plain, Size: 548 bytes --] Marius Storm-Olsen said the following on 04.09.2007 13:53: > In the meantime, I've pushed out a new patch > http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59 > > /me starts another test run, to see how our tests are doing now.. Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git 'devel' branch, I only have one failing testcase t6024-recursive-merge.sh when running $ NO_SYMLINKS=1 make -k The rest are passing with flying colors! -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 13:56 ` Marius Storm-Olsen @ 2007-09-04 14:07 ` Johannes Sixt 2007-09-04 14:32 ` Johannes Schindelin 2007-09-04 14:16 ` Johannes Schindelin 2007-09-04 14:30 ` Johannes Schindelin 2 siblings, 1 reply; 86+ messages in thread From: Johannes Sixt @ 2007-09-04 14:07 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Schindelin, Johannes Sixt, Git Mailing List Marius Storm-Olsen schrieb: > Marius Storm-Olsen said the following on 04.09.2007 13:53: >> In the meantime, I've pushed out a new patch >> http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59 >> >> >> /me starts another test run, to see how our tests are doing now.. > > Neat, with the custom stat() changes cherry-picked on top of > 4msysgit.git 'devel' branch, I only have one failing testcase > t6024-recursive-merge.sh > when running > $ NO_SYMLINKS=1 make -k > > The rest are passing with flying colors! And that one will eventually pass if only you try it often enough. See 71ee4210c in mingw.git. -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 14:07 ` Johannes Sixt @ 2007-09-04 14:32 ` Johannes Schindelin 2007-09-04 14:52 ` Johannes Sixt 0 siblings, 1 reply; 86+ messages in thread From: Johannes Schindelin @ 2007-09-04 14:32 UTC (permalink / raw) To: Johannes Sixt; +Cc: Marius Storm-Olsen, Johannes Sixt, Git Mailing List Hi, On Tue, 4 Sep 2007, Johannes Sixt wrote: > Marius Storm-Olsen schrieb: > > Marius Storm-Olsen said the following on 04.09.2007 13:53: > > > In the meantime, I've pushed out a new patch > > > http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59 > > > > > > /me starts another test run, to see how our tests are doing now.. > > > > Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git > > 'devel' branch, I only have one failing testcase > > t6024-recursive-merge.sh > > when running > > $ NO_SYMLINKS=1 make -k > > > > The rest are passing with flying colors! > > And that one will eventually pass if only you try it often enough. See > 71ee4210c in mingw.git. Do you have Linus' patch applied? The one where the config is read at the start of write-tree? The problem only occurred since we have core.crlf = input unilaterally now. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 14:32 ` Johannes Schindelin @ 2007-09-04 14:52 ` Johannes Sixt 0 siblings, 0 replies; 86+ messages in thread From: Johannes Sixt @ 2007-09-04 14:52 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Marius Storm-Olsen, Johannes Sixt, Git Mailing List Johannes Schindelin schrieb: > Hi, > > On Tue, 4 Sep 2007, Johannes Sixt wrote: > >> Marius Storm-Olsen schrieb: >>> Marius Storm-Olsen said the following on 04.09.2007 13:53: >>>> In the meantime, I've pushed out a new patch >>>> http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59 >>>> >>>> /me starts another test run, to see how our tests are doing now.. >>> Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git >>> 'devel' branch, I only have one failing testcase >>> t6024-recursive-merge.sh >>> when running >>> $ NO_SYMLINKS=1 make -k >>> >>> The rest are passing with flying colors! >> And that one will eventually pass if only you try it often enough. See >> 71ee4210c in mingw.git. > > Do you have Linus' patch applied? The one where the config is read at the > start of write-tree? > > The problem only occurred since we have core.crlf = input unilaterally > now. Yes, I have that patch, but I don't have core.crlf set. There are a lot of tests that do a series of commits in a row, with file changes such that the file size does _not_ change, aka "racy git" problem. This problem is obviously not 100% fixed on MSys: We do not correctly detect that there are files that might be modified ("racily clean" files). Some tests are prone to trigger the bug, others not. -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 13:56 ` Marius Storm-Olsen 2007-09-04 14:07 ` Johannes Sixt @ 2007-09-04 14:16 ` Johannes Schindelin 2007-09-04 14:30 ` Johannes Schindelin 2 siblings, 0 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-04 14:16 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, Johannes Sixt, Git Mailing List Hi, On Tue, 4 Sep 2007, Marius Storm-Olsen wrote: > Marius Storm-Olsen said the following on 04.09.2007 13:53: > > In the meantime, I've pushed out a new patch > > http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59 > > > > /me starts another test run, to see how our tests are doing now.. > > Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git > 'devel' branch, I only have one failing testcase > t6024-recursive-merge.sh > when running > $ NO_SYMLINKS=1 make -k > > The rest are passing with flying colors! Really? It is failing again? IIRC it was Linus' patch (which I cherry-picked into 59f8c189a5) that fixed it for me. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 13:56 ` Marius Storm-Olsen 2007-09-04 14:07 ` Johannes Sixt 2007-09-04 14:16 ` Johannes Schindelin @ 2007-09-04 14:30 ` Johannes Schindelin 2007-09-04 14:43 ` Marius Storm-Olsen 2 siblings, 1 reply; 86+ messages in thread From: Johannes Schindelin @ 2007-09-04 14:30 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, Johannes Sixt, Git Mailing List Hi, On Tue, 4 Sep 2007, Marius Storm-Olsen wrote: > Marius Storm-Olsen said the following on 04.09.2007 13:53: > > In the meantime, I've pushed out a new patch > > http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59 > > > > /me starts another test run, to see how our tests are doing now.. > > Neat, with the custom stat() changes cherry-picked on top of 4msysgit.git > 'devel' branch, I only have one failing testcase > t6024-recursive-merge.sh > when running > $ NO_SYMLINKS=1 make -k > > The rest are passing with flying colors! Bad news. I do not know if it was the newest version I tried, but I could no longer fetch... said something about some bad file. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 14:30 ` Johannes Schindelin @ 2007-09-04 14:43 ` Marius Storm-Olsen 2007-09-04 14:48 ` Johannes Schindelin 0 siblings, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-04 14:43 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Johannes Sixt, Johannes Sixt, Git Mailing List [-- Attachment #1: Type: text/plain, Size: 906 bytes --] Johannes Schindelin wrote: > On Tue, 4 Sep 2007, Marius Storm-Olsen wrote: >> Neat, with the custom stat() changes cherry-picked on top of >> 4msysgit.git 'devel' branch, I only have one failing testcase >> t6024-recursive-merge.sh when running $ NO_SYMLINKS=1 make -k >> >> The rest are passing with flying colors! > > Bad news. I do not know if it was the newest version I tried, but I > could no longer fetch... said something about some bad file. Then you're missing this patch: http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127 I guess the quickest way is to manually apply this patch and recompile. (or add the '#undef fstat', and have git_fstat just 'return fstat(fd, buf)') The problem is that without this patch fstat(0, buf) would fail with bad filedescriptor instead of returning the st_mode = S_IFIFO. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 14:43 ` Marius Storm-Olsen @ 2007-09-04 14:48 ` Johannes Schindelin 2007-09-04 15:05 ` David Kastrup 2007-09-04 16:32 ` Marius Storm-Olsen 0 siblings, 2 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-04 14:48 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, Johannes Sixt, Git Mailing List Hi, On Tue, 4 Sep 2007, Marius Storm-Olsen wrote: > Johannes Schindelin wrote: > > On Tue, 4 Sep 2007, Marius Storm-Olsen wrote: > >> Neat, with the custom stat() changes cherry-picked on top of > >> 4msysgit.git 'devel' branch, I only have one failing testcase > >> t6024-recursive-merge.sh when running $ NO_SYMLINKS=1 make -k > >> > >> The rest are passing with flying colors! > > > > Bad news. I do not know if it was the newest version I tried, but I > > could no longer fetch... said something about some bad file. > > Then you're missing this patch: > http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127 > > I guess the quickest way is to manually apply this patch and recompile. > (or add the '#undef fstat', and have git_fstat just 'return fstat(fd, buf)') > > The problem is that without this patch fstat(0, buf) would fail with bad > filedescriptor instead of returning the st_mode = S_IFIFO. I guessed as much, but could not fetch the patch, since fetch was broken ;-) For some utterly strange reason, "git fetch" accessed "git-fetch" in the cwd, not in /bin/... Funny. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 14:48 ` Johannes Schindelin @ 2007-09-04 15:05 ` David Kastrup 2007-09-04 16:32 ` Marius Storm-Olsen 1 sibling, 0 replies; 86+ messages in thread From: David Kastrup @ 2007-09-04 15:05 UTC (permalink / raw) To: git Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > On Tue, 4 Sep 2007, Marius Storm-Olsen wrote: > >> The problem is that without this patch fstat(0, buf) would fail >> with bad filedescriptor instead of returning the st_mode = S_IFIFO. > > I guessed as much, but could not fetch the patch, since fetch was broken > ;-) > > For some utterly strange reason, "git fetch" accessed "git-fetch" in > the cwd, not in /bin/... Funny. Regardless of what you write in your PATH variable, Windows will _always_ search for binaries first in your current work directory. It is not really funny. -- David Kastrup ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 14:48 ` Johannes Schindelin 2007-09-04 15:05 ` David Kastrup @ 2007-09-04 16:32 ` Marius Storm-Olsen 1 sibling, 0 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-04 16:32 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Johannes Sixt, Johannes Sixt, Git Mailing List [-- Attachment #1: Type: text/plain, Size: 1079 bytes --] Johannes Schindelin wrote: > On Tue, 4 Sep 2007, Marius Storm-Olsen wrote: >> Johannes Schindelin wrote: >>> Bad news. I do not know if it was the newest version I tried, >>> but I could no longer fetch... said something about some bad >>> file. >> Then you're missing this patch: >> http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=f15974add93bdfa92775c77c00e7c65aefd42127 >> >> I guess the quickest way is to manually apply this patch and >> recompile. (or add the '#undef fstat', and have git_fstat just >> 'return fstat(fd, buf)') >> >> The problem is that without this patch fstat(0, buf) would fail >> with bad filedescriptor instead of returning the st_mode = S_IFIFO. > > I guessed as much, but could not fetch the patch, since fetch was > broken ;-) Heh, yeah, that's happened to me too before. One neat thing with this gitweb thingy though, is that you can actually grab the patch, like this http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff_plain;h=f15974add93bdfa92775c77c00e7c65aefd42127 No excuse! ;-) -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 11:36 ` Johannes Sixt 2007-09-04 11:53 ` Marius Storm-Olsen @ 2007-09-04 12:46 ` Johannes Schindelin 2007-09-04 12:57 ` Johannes Schindelin 2007-09-04 13:03 ` Johannes Sixt 1 sibling, 2 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-04 12:46 UTC (permalink / raw) To: Johannes Sixt; +Cc: Marius Storm-Olsen, Johannes Sixt, Git Mailing List Hi, On Tue, 4 Sep 2007, Johannes Sixt wrote: > Johannes Schindelin schrieb: > > On Tue, 4 Sep 2007, Johannes Sixt wrote: > > > Therefore, I've pushed out a fixup patch at the top of mingw.git's devel > > > branch that converts mtime to local time > > > > On Linux, we compare to UTC to begin with, right? We should do that here, > > too... So if time(NULL) does not return UTC on MinGW, we have to wrap that > > function, too. > > According to MSDN, time(NULL) returns "the number of seconds elapsed since > [epoch] according to the system clock". Please don't ask me what "the system > clock" is. I think I know. From my QEmu adventures I know that DOS/Windows expects the system clock to be set to local time, in contrast to _all_ other operating systems. > Reading the implementation of time(), it starts with GetLocalTime(), > determines whether daylight saving is in effect, and continues with > another round of timezone adjustment - mind you: _not_ a timezone > reversal (!!). Doesn't this look extremely bogus? > > It seems we really need a wrapper for time(). I absolutely concur. Something like this (most of it is blatantly copied from Marius' patch)? -- snip -- diff --git a/git-compat-util.h b/git-compat-util.h index 172e828..2984319 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -470,6 +470,17 @@ static inline int git_unlink(const char *pathname) { #include <time.h> struct tm *gmtime_r(const time_t *timep, struct tm *result); struct tm *localtime_r(const time_t *timep, struct tm *result); +static inline time_t mingw_time(void *dummy) +{ + FILETIME ft; + GetSystemTimeAsFileTime(&ft); + long long winTime = ((long long)ft.dwHighDateTime << 32) + ft.dwLowDateTime; + winTime -= 116444736000000000LL; /* Windows to Unix Epoch conversion */ + winTime /= 10000000; /* Nano to seconds resolution */ + return (time_t)winTime; + +} +#define time mingw_time #define hstrerror strerror char *mingw_getcwd(char *pointer, int len); -- snap -- Ciao, Dscho ^ permalink raw reply related [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 12:46 ` Johannes Schindelin @ 2007-09-04 12:57 ` Johannes Schindelin 2007-09-04 21:02 ` Rutger Nijlunsing 2007-09-04 13:03 ` Johannes Sixt 1 sibling, 1 reply; 86+ messages in thread From: Johannes Schindelin @ 2007-09-04 12:57 UTC (permalink / raw) To: Johannes Sixt; +Cc: Marius Storm-Olsen, Johannes Sixt, Git Mailing List Hi, On Tue, 4 Sep 2007, Johannes Schindelin wrote: > On Tue, 4 Sep 2007, Johannes Sixt wrote: > > > Johannes Schindelin schrieb: > > > On Tue, 4 Sep 2007, Johannes Sixt wrote: > > > > Therefore, I've pushed out a fixup patch at the top of mingw.git's > > > > devel branch that converts mtime to local time > > > > > > On Linux, we compare to UTC to begin with, right? We should do that > > > here, too... So if time(NULL) does not return UTC on MinGW, we have > > > to wrap that function, too. > > > > According to MSDN, time(NULL) returns "the number of seconds elapsed > > since [epoch] according to the system clock". Please don't ask me what > > "the system clock" is. > > I think I know. From my QEmu adventures I know that DOS/Windows expects > the system clock to be set to local time, in contrast to _all_ other > operating systems. Now I am utterly confused. MSDN says FILETIME Contains a 64-bit value representing the number of 100-nanosecond intervals since January 1, 1601 (UTC). Hmm. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 12:57 ` Johannes Schindelin @ 2007-09-04 21:02 ` Rutger Nijlunsing 2007-09-04 21:54 ` Reece Dunn 2007-09-05 6:22 ` Marius Storm-Olsen 0 siblings, 2 replies; 86+ messages in thread From: Rutger Nijlunsing @ 2007-09-04 21:02 UTC (permalink / raw) To: Johannes Schindelin Cc: Johannes Sixt, Marius Storm-Olsen, Johannes Sixt, Git Mailing List On Tue, Sep 04, 2007 at 01:57:38PM +0100, Johannes Schindelin wrote: > Hi, > > On Tue, 4 Sep 2007, Johannes Schindelin wrote: > > > On Tue, 4 Sep 2007, Johannes Sixt wrote: > > > > > Johannes Schindelin schrieb: > > > > On Tue, 4 Sep 2007, Johannes Sixt wrote: > > > > > Therefore, I've pushed out a fixup patch at the top of mingw.git's > > > > > devel branch that converts mtime to local time > > > > > > > > On Linux, we compare to UTC to begin with, right? We should do that > > > > here, too... So if time(NULL) does not return UTC on MinGW, we have > > > > to wrap that function, too. > > > > > > According to MSDN, time(NULL) returns "the number of seconds elapsed > > > since [epoch] according to the system clock". Please don't ask me what > > > "the system clock" is. > > > > I think I know. From my QEmu adventures I know that DOS/Windows expects > > the system clock to be set to local time, in contrast to _all_ other > > operating systems. > > Now I am utterly confused. MSDN says > > FILETIME > > Contains a 64-bit value representing the number of 100-nanosecond > intervals since January 1, 1601 (UTC). > > Hmm. [Warning: war stories ahead...] If you really, really want to know more: http://search.cpan.org/~shay/Win32-UTCFileTime-1.45/lib/Win32/UTCFileTime.pm -- Rutger Nijlunsing ---------------------------------- eludias ed dse.nl never attribute to a conspiracy which can be explained by incompetence ---------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 21:02 ` Rutger Nijlunsing @ 2007-09-04 21:54 ` Reece Dunn 2007-09-05 6:22 ` Marius Storm-Olsen 1 sibling, 0 replies; 86+ messages in thread From: Reece Dunn @ 2007-09-04 21:54 UTC (permalink / raw) To: git, Johannes Schindelin, Johannes Sixt, Marius Storm-Olsen, "Johannes Sixt" <johan On 04/09/07, Rutger Nijlunsing <rutger@nospam.com> wrote: > On Tue, Sep 04, 2007 at 01:57:38PM +0100, Johannes Schindelin wrote: > > Hi, > > > > On Tue, 4 Sep 2007, Johannes Schindelin wrote: > > > > > On Tue, 4 Sep 2007, Johannes Sixt wrote: > > > > > > > Johannes Schindelin schrieb: > > > > > On Tue, 4 Sep 2007, Johannes Sixt wrote: > > > > > > Therefore, I've pushed out a fixup patch at the top of mingw.git's > > > > > > devel branch that converts mtime to local time > > > > > > > > > > On Linux, we compare to UTC to begin with, right? We should do that > > > > > here, too... So if time(NULL) does not return UTC on MinGW, we have > > > > > to wrap that function, too. > > > > > > > > According to MSDN, time(NULL) returns "the number of seconds elapsed > > > > since [epoch] according to the system clock". Please don't ask me what > > > > "the system clock" is. > > > > > > I think I know. From my QEmu adventures I know that DOS/Windows expects > > > the system clock to be set to local time, in contrast to _all_ other > > > operating systems. > > > > Now I am utterly confused. MSDN says > > > > FILETIME > > > > Contains a 64-bit value representing the number of 100-nanosecond > > intervals since January 1, 1601 (UTC). > > > > Hmm. > > > [Warning: war stories ahead...] > > If you really, really want to know more: > > http://search.cpan.org/~shay/Win32-UTCFileTime-1.45/lib/Win32/UTCFileTime.pm Hmm, this may explain something that I have been observing on Windows+cygwin. When I run `git diff`, I sometimes get it reporting that all (from what I can tell) files have changed, like a `find . -type f -exec touch {} \;` command was run. I was going to report this on a new thread, but this looks like a more relevant place to do so. My Windows machine is currently in Daylight Savings Time mode, and from my observations, I have only seen this repeat first thing the next day. I am not sure why, but every 24hrs, it looks as if the file time reported by and checked from git is different to that reported by stat. I have not had time yet to play around with the mingw port and this new stat implementation to see if it addresses this issue. - Reece ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 21:02 ` Rutger Nijlunsing 2007-09-04 21:54 ` Reece Dunn @ 2007-09-05 6:22 ` Marius Storm-Olsen 2007-09-05 10:15 ` Johannes Schindelin 1 sibling, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-05 6:22 UTC (permalink / raw) To: git, Johannes Schindelin, Johannes Sixt; +Cc: Johannes Sixt, Git Mailing List [-- Attachment #1: Type: text/plain, Size: 904 bytes --] Rutger Nijlunsing said the following on 04.09.2007 23:02: > On Tue, Sep 04, 2007 at 01:57:38PM +0100, Johannes Schindelin wrote: >> Now I am utterly confused. MSDN says >> FILETIME >> Contains a 64-bit value representing the number of 100-nanosecond >> intervals since January 1, 1601 (UTC). >> Hmm. > > [Warning: war stories ahead...] > If you really, really want to know more: > http://search.cpan.org/~shay/Win32-UTCFileTime-1.45/lib/Win32/UTCFileTime.pm Thanks, seems like it's the right decision then to ensure that we use UTC throughout Git on Windows Hannes & Dscho, looks like we should revert http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59 then, and rather apply Dscho's patch for a custom time() implementation. Dscho, was the custom implementation of time() enough to fix the issues for you? -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-05 6:22 ` Marius Storm-Olsen @ 2007-09-05 10:15 ` Johannes Schindelin 0 siblings, 0 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-05 10:15 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: git, Johannes Sixt, Johannes Sixt, Git Mailing List Hi, On Wed, 5 Sep 2007, Marius Storm-Olsen wrote: > Rutger Nijlunsing said the following on 04.09.2007 23:02: > > On Tue, Sep 04, 2007 at 01:57:38PM +0100, Johannes Schindelin wrote: > > > Now I am utterly confused. MSDN says > > > FILETIME > > > Contains a 64-bit value representing the number of 100-nanosecond > > > intervals since January 1, 1601 (UTC). > > > Hmm. > > > > [Warning: war stories ahead...] > > If you really, really want to know more: > > http://search.cpan.org/~shay/Win32-UTCFileTime-1.45/lib/Win32/UTCFileTime.pm > > Thanks, seems like it's the right decision then to ensure that we use UTC > throughout Git on Windows > > Hannes & Dscho, looks like we should revert > http://repo.or.cz/w/git/mingw/4msysgit.git?a=commitdiff;h=683775c00d9fb95bcbe4632f95b67a96b902fa59 > then, and rather apply Dscho's patch for a custom time() implementation. > Dscho, was the custom implementation of time() enough to fix the issues > for you? Umm. I am helplessly overloaded with non-Git work, and just made sure that it compiles ;-) Besides, I did not really understand what I was doing; just copying your code to convert from FILETIME to time_t. Sorry, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 12:46 ` Johannes Schindelin 2007-09-04 12:57 ` Johannes Schindelin @ 2007-09-04 13:03 ` Johannes Sixt 1 sibling, 0 replies; 86+ messages in thread From: Johannes Sixt @ 2007-09-04 13:03 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Marius Storm-Olsen, Johannes Sixt, Git Mailing List Johannes Schindelin schrieb: > On Tue, 4 Sep 2007, Johannes Sixt wrote: >> Reading the implementation of time(), it starts with GetLocalTime(), >> determines whether daylight saving is in effect, and continues with >> another round of timezone adjustment - mind you: _not_ a timezone >> reversal (!!). Doesn't this look extremely bogus? >> >> It seems we really need a wrapper for time(). > > I absolutely concur. Something like this (most of it is blatantly copied > from Marius' patch)? Well, I don't think it'll make a difference. The tiny test program below prints twice the same number. My analysis of the time() implementation is obviously flawed. -- Hannes #include <windows.h> #include <stdio.h> #include <time.h> int main() { time_t t = time(NULL); FILETIME ft; GetSystemTimeAsFileTime(&ft); long long winTime = ((long long)ft.dwHighDateTime << 32) + ft.dwLowDateTime; winTime -= 116444736000000000LL; winTime /= 10000000; printf("%d %d\n", t, (int) winTime); return 0; } ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-04 7:41 ` Johannes Sixt ` (2 preceding siblings ...) 2007-09-04 10:48 ` Johannes Schindelin @ 2007-09-06 16:18 ` Johannes Sixt 2007-09-06 16:34 ` Marius Storm-Olsen 3 siblings, 1 reply; 86+ messages in thread From: Johannes Sixt @ 2007-09-06 16:18 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Schindelin, Johannes Sixt, Git Mailing List Johannes Sixt schrieb: > Marius Storm-Olsen schrieb: >> Johannes Schindelin wrote: >>> To make it easier on others, I just uploaded it into the "teststat" >>> branch on 4msysgit.git (subject to removal in a few days). >> >> Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' branch. >> RFC, and please test. > > Thanks a lot! I've pushed it out in mingw.git's master. > > The reason that t4200-rerere.sh fails is that we now store UTC in > st_mtime. However, for the garbage-collection we compare this entry to a > local time stamp. This analysis is incorrect, I think. The reason we fail seems to be that t4200 uses test-chmtime, which uses utime(). Likely, we need a wrapper for that one? -- Hannes ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-06 16:18 ` Johannes Sixt @ 2007-09-06 16:34 ` Marius Storm-Olsen 0 siblings, 0 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-06 16:34 UTC (permalink / raw) To: Johannes Sixt; +Cc: Johannes Schindelin, Johannes Sixt, Git Mailing List [-- Attachment #1: Type: text/plain, Size: 942 bytes --] Johannes Sixt wrote: > Johannes Sixt schrieb: >> Marius Storm-Olsen schrieb: >>> Johannes Schindelin wrote: >>>> To make it easier on others, I just uploaded it into the >>>> "teststat" branch on 4msysgit.git (subject to removal in a few >>>> days). >>> >>> Ok, I've updated the patch in the 4msysgit.git repo, 'teststat' >>> branch. RFC, and please test. >> >> Thanks a lot! I've pushed it out in mingw.git's master. >> >> The reason that t4200-rerere.sh fails is that we now store UTC in >> st_mtime. However, for the garbage-collection we compare this entry >> to a local time stamp. > > This analysis is incorrect, I think. The reason we fail seems to be > that t4200 uses test-chmtime, which uses utime(). Likely, we need a > wrapper for that one? Ok, could you make a quick patch to add a git_utime() (and probably git_time()) and see if the tests pass without the UTC to Local time patch? -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API [not found] ` <46DBFA2A.7050003@trolltech.com> 2007-09-03 12:38 ` [PATCH] Add a new lstat and fstat implementation based on Win32 API Marius Storm-Olsen 2007-09-03 13:33 ` Johannes Schindelin @ 2007-09-03 13:49 ` Johannes Sixt 2007-09-03 14:38 ` Johannes Schindelin 2 siblings, 1 reply; 86+ messages in thread From: Johannes Sixt @ 2007-09-03 13:49 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Git Mailing List, Johannes Schindelin, Johannes Sixt Marius Storm-Olsen schrieb: > There was a problem with racy conditions, which this revision fixes. > The problem was that fstat was using the builtin implementation, which for > for some reason is off by some amount of seconds. (This is probably due to > some leap-year issue in one of the implementations. However, Microsoft > tells > us to use 116444736000000000 in http://support.microsoft.com/kb/167296, so > I'll stick with that.) Thanks for the analysis and new patch. Indeed, FILETIME is UTC, which is good; the native implementation returns local time, if I read the code (of msvcrt) correctly. > With the our own implementations of lstat & fstat, the following test cases > are now fixed: > t4116-apply-reverte.sh > ok 3: apply in reverse > t4200-rerere.sh > ok 17: young records still live > However, the following test cases seems to fail now: > t6024-recursive-merge.sh > FAIL 1: setup tests > FAIL 3: result contains a conflict > FAIL 4: virtual trees were processed > FAIL 5: refuse to merge binaries > > See attached test case logs. > Are some of these test cases unstable, so the result will fluctuate on > Windows? I see many more failures. in: t4001-diff-rename.sh: 5 t4101-apply-nonl.sh: all t4102-apply-rename.sh: 2,3,4 t4116-apply-reverse.sh: 3 t4200-rerere.sh: 12,13,17 t5515-fetch-merge-logic.sh: 54 etc... There is something fishy going on. But at least the failure in t4116 is easy to work around: diff --git a/t/t4116-apply-reverse.sh b/t/t4116-apply-reverse.sh index fc2f622..f3e0c4a 100755 --- a/t/t4116-apply-reverse.sh +++ b/t/t4116-apply-reverse.sh @@ -43,7 +43,8 @@ test_expect_success 'apply in reverse' ' git reset --hard second && git apply --reverse --binary --index patch && git diff >diff && - git diff /dev/null diff + : > empty && + git diff empty diff ' -- Hannes ^ permalink raw reply related [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 13:49 ` Johannes Sixt @ 2007-09-03 14:38 ` Johannes Schindelin 2007-09-03 16:15 ` Marius Storm-Olsen 0 siblings, 1 reply; 86+ messages in thread From: Johannes Schindelin @ 2007-09-03 14:38 UTC (permalink / raw) To: Johannes Sixt; +Cc: Marius Storm-Olsen, Git Mailing List, Johannes Sixt Hi, On Mon, 3 Sep 2007, Johannes Sixt wrote: > Marius Storm-Olsen schrieb: > > There was a problem with racy conditions, which this revision fixes. > > The problem was that fstat was using the builtin implementation, which for > > for some reason is off by some amount of seconds. (This is probably due to > > some leap-year issue in one of the implementations. However, Microsoft > > tells > > us to use 116444736000000000 in http://support.microsoft.com/kb/167296, so > > I'll stick with that.) > > Thanks for the analysis and new patch. Indeed, FILETIME is UTC, which is > good; the native implementation returns local time, if I read the code (of > msvcrt) correctly. > > > With the our own implementations of lstat & fstat, the following test cases > > are now fixed: > > t4116-apply-reverte.sh > > ok 3: apply in reverse > > t4200-rerere.sh > > ok 17: young records still live > > However, the following test cases seems to fail now: > > t6024-recursive-merge.sh > > FAIL 1: setup tests > > FAIL 3: result contains a conflict > > FAIL 4: virtual trees were processed > > FAIL 5: refuse to merge binaries > > > > See attached test case logs. > > Are some of these test cases unstable, so the result will fluctuate on > > Windows? > > I see many more failures. in: > > t4001-diff-rename.sh: 5 > t4101-apply-nonl.sh: all > t4102-apply-rename.sh: 2,3,4 > t4116-apply-reverse.sh: 3 > t4200-rerere.sh: 12,13,17 > t5515-fetch-merge-logic.sh: 54 > etc... Funny. I do not get most of these: t4200-rerere, t5510-fetch, t5515-fetch-merge-logic, t5700-clone-reference, t5701-clone-local, t7004 This is all on top of 4msysgit's "devel" branch, and the t5* and t7004 failed there already. Oh, and I actually run from sh, not from cmd. (Wouldn't you have guessed?) Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 14:38 ` Johannes Schindelin @ 2007-09-03 16:15 ` Marius Storm-Olsen 2007-09-03 16:55 ` Johannes Schindelin 0 siblings, 1 reply; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-03 16:15 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Johannes Sixt, Git Mailing List, Johannes Sixt [-- Attachment #1: Type: text/plain, Size: 772 bytes --] Johannes Schindelin wrote: > On Mon, 3 Sep 2007, Johannes Sixt wrote: >> I see many more failures. in: >> >> t4001-diff-rename.sh: 5 >> t4101-apply-nonl.sh: all >> t4102-apply-rename.sh: 2,3,4 >> t4116-apply-reverse.sh: 3 >> t4200-rerere.sh: 12,13,17 >> t5515-fetch-merge-logic.sh: 54 >> etc... > > Funny. I do not get most of these: > > t4200-rerere, > t5510-fetch, > t5515-fetch-merge-logic, > t5700-clone-reference, > t5701-clone-local, > t7004 > > This is all on top of 4msysgit's "devel" branch, and the t5* and > t7004 failed there already. > > Oh, and I actually run from sh, not from cmd. (Wouldn't you have > guessed?) I was also running the tests under sh, however on top of mingw.git, and not 4msysgit.git. -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: [PATCH] Add a new lstat and fstat implementation based on Win32 API 2007-09-03 16:15 ` Marius Storm-Olsen @ 2007-09-03 16:55 ` Johannes Schindelin 0 siblings, 0 replies; 86+ messages in thread From: Johannes Schindelin @ 2007-09-03 16:55 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Johannes Sixt, Git Mailing List, Johannes Sixt Hi, On Mon, 3 Sep 2007, Marius Storm-Olsen wrote: > Johannes Schindelin wrote: > > > t4200-rerere, > > t5510-fetch, > > t5515-fetch-merge-logic, > > t5700-clone-reference, > > t5701-clone-local, > > t7004 > > > > This is all on top of 4msysgit's "devel" branch, and the t5* and t7004 > > failed there already. > > > > Oh, and I actually run from sh, not from cmd. (Wouldn't you have > > guessed?) > > I was also running the tests under sh, however on top of mingw.git, and > not 4msysgit.git. I switched back recently, as more tests are succeeding in 4msysgit.git's "devel" ATM. Ciao, Dscho ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: Stats in Git 2007-09-02 14:49 Stats in Git Marius Storm-Olsen 2007-09-02 14:51 ` [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too Marius Storm-Olsen @ 2007-09-02 20:02 ` Alex Riesen 2007-09-02 20:09 ` Marius Storm-Olsen 2007-09-03 8:19 ` Matthieu Moy 2 siblings, 1 reply; 86+ messages in thread From: Alex Riesen @ 2007-09-02 20:02 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Git Mailing List 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. ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: Stats in Git 2007-09-02 20:02 ` Stats in Git Alex Riesen @ 2007-09-02 20:09 ` Marius Storm-Olsen 0 siblings, 0 replies; 86+ messages in thread From: Marius Storm-Olsen @ 2007-09-02 20:09 UTC (permalink / raw) To: Alex Riesen; +Cc: Git Mailing List [-- Attachment #1: Type: text/plain, Size: 623 bytes --] Alex Riesen wrote: > Marius Storm-Olsen, Sun, Sep 02, 2007 16:49:55 +0200: >> By applying the diff below, you can see for yourself what happens when > > just use "strace -e fstat,stat,lstat,stat64,lstat64 -f git-status" > on sane platform. Right, I was doing this on Windows, where strace is rather.. limited, so I thought I'd just share the code for all to play with :-) But, on my linux box: $ strace -e fstat,stat,lstat,stat64,lstat64 -f git-status 2>&1 | wc -l 300195 (Slightly more stats there, as expected, due to the listings of the shells stats too, and not just the builtins.) -- .marius [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 187 bytes --] ^ permalink raw reply [flat|nested] 86+ messages in thread
* Re: Stats in Git 2007-09-02 14:49 Stats in Git Marius Storm-Olsen 2007-09-02 14:51 ` [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too Marius Storm-Olsen 2007-09-02 20:02 ` Stats in Git Alex Riesen @ 2007-09-03 8:19 ` Matthieu Moy 2 siblings, 0 replies; 86+ messages in thread From: Matthieu Moy @ 2007-09-03 8:19 UTC (permalink / raw) To: Marius Storm-Olsen; +Cc: Git Mailing List 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 ^ permalink raw reply [flat|nested] 86+ messages in thread
end of thread, other threads:[~2007-09-07 6:36 UTC | newest] Thread overview: 86+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-09-02 14:49 Stats in Git Marius Storm-Olsen 2007-09-02 14:51 ` [PATCH] Add a new lstat implementation based on Win32 API, and make stat use that implementation too Marius Storm-Olsen 2007-09-02 14:57 ` Marius Storm-Olsen 2007-09-02 15:32 ` Reece Dunn 2007-09-02 16:09 ` Marius Storm-Olsen 2007-09-02 16:33 ` Reece Dunn 2007-09-02 16:47 ` Brian Gernhardt 2007-09-02 16:53 ` Reece Dunn 2007-09-02 17:05 ` Marius Storm-Olsen 2007-09-02 17:44 ` Johannes Schindelin 2007-09-02 17:58 ` David Kastrup 2007-09-02 18:18 ` Marius Storm-Olsen 2007-09-02 18:16 ` Johannes Sixt 2007-09-02 18:44 ` Marius Storm-Olsen 2007-09-02 19:07 ` Johannes Sixt 2007-09-02 19:31 ` Marius Storm-Olsen 2007-09-02 20:27 ` Robin Rosenberg 2007-09-02 21:26 ` Johannes Schindelin 2007-09-02 21:42 ` Robin Rosenberg 2007-09-02 23:02 ` Johannes Schindelin 2007-09-03 7:07 ` Johannes Sixt 2007-09-03 11:21 ` Miklos Vajna 2007-09-03 11:32 ` David Kastrup 2007-09-05 16:02 ` Miklos Vajna 2007-09-05 19:01 ` David Kastrup 2007-09-06 16:26 ` Miklos Vajna 2007-09-06 16:33 ` David Kastrup 2007-09-06 23:31 ` Douglas Stockwell 2007-09-07 6:36 ` David Kastrup 2007-09-02 21:38 ` Alex Riesen 2007-09-02 22:04 ` Robin Rosenberg 2007-09-03 6:15 ` Marius Storm-Olsen 2007-09-03 11:39 ` Johannes Schindelin 2007-09-03 11:51 ` David Kastrup 2007-09-03 11:53 ` Marius Storm-Olsen 2007-09-03 12:33 ` Johannes Schindelin 2007-09-02 21:41 ` Alex Riesen 2007-09-03 6:12 ` Marius Storm-Olsen 2007-09-03 7:47 ` Johannes Sixt 2007-09-03 7:55 ` Marius Storm-Olsen [not found] ` <46DBFA2A.7050003@trolltech.com> 2007-09-03 12:38 ` [PATCH] Add a new lstat and fstat implementation based on Win32 API Marius Storm-Olsen 2007-09-03 13:33 ` Johannes Schindelin 2007-09-03 13:52 ` Marius Storm-Olsen 2007-09-03 14:39 ` Johannes Schindelin 2007-09-03 16:22 ` Marius Storm-Olsen 2007-09-03 16:56 ` Johannes Schindelin 2007-09-03 13:53 ` Johannes Sixt 2007-09-03 14:35 ` Johannes Schindelin 2007-09-03 19:21 ` Marius Storm-Olsen 2007-09-04 2:21 ` Johannes Schindelin 2007-09-04 7:41 ` Johannes Sixt 2007-09-04 8:53 ` David Kastrup 2007-09-04 10:20 ` Marius Storm-Olsen 2007-09-04 10:53 ` Johannes Sixt 2007-09-04 11:21 ` Marius Storm-Olsen 2007-09-04 11:28 ` Johannes Sixt 2007-09-04 21:31 ` David Kastrup 2007-09-04 10:48 ` Johannes Schindelin 2007-09-04 11:36 ` Johannes Sixt 2007-09-04 11:53 ` Marius Storm-Olsen 2007-09-04 13:56 ` Marius Storm-Olsen 2007-09-04 14:07 ` Johannes Sixt 2007-09-04 14:32 ` Johannes Schindelin 2007-09-04 14:52 ` Johannes Sixt 2007-09-04 14:16 ` Johannes Schindelin 2007-09-04 14:30 ` Johannes Schindelin 2007-09-04 14:43 ` Marius Storm-Olsen 2007-09-04 14:48 ` Johannes Schindelin 2007-09-04 15:05 ` David Kastrup 2007-09-04 16:32 ` Marius Storm-Olsen 2007-09-04 12:46 ` Johannes Schindelin 2007-09-04 12:57 ` Johannes Schindelin 2007-09-04 21:02 ` Rutger Nijlunsing 2007-09-04 21:54 ` Reece Dunn 2007-09-05 6:22 ` Marius Storm-Olsen 2007-09-05 10:15 ` Johannes Schindelin 2007-09-04 13:03 ` Johannes Sixt 2007-09-06 16:18 ` Johannes Sixt 2007-09-06 16:34 ` Marius Storm-Olsen 2007-09-03 13:49 ` Johannes Sixt 2007-09-03 14:38 ` Johannes Schindelin 2007-09-03 16:15 ` Marius Storm-Olsen 2007-09-03 16:55 ` Johannes Schindelin 2007-09-02 20:02 ` Stats in Git Alex Riesen 2007-09-02 20:09 ` Marius Storm-Olsen 2007-09-03 8:19 ` Matthieu Moy
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).