Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Torsten Bögershausen wrote: On 30.06.13 19:28, Ramsay Jones wrote: [ ... ] You have just described my second patch! :D Unfortunately, I have not had any time to work on the patch this weekend. However, despite the patch being a bit rough around the edges, I decided to send it out (see below) to get some early feedback. Note that it passes the t3210, t3211, t5500, t3200, t3301, t7606 and t1301 tests, but I have not run the full test suite. Comments welcome. ATB, Ramsay Jones -- 8 -- Subject: [PATCH] cygwin: Add fast_[l]stat() functions Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- builtin/apply.c| 6 +++--- builtin/commit.c | 2 +- builtin/ls-files.c | 2 +- builtin/rm.c | 2 +- builtin/update-index.c | 2 +- check-racy.c | 2 +- compat/cygwin.c| 12 compat/cygwin.h| 10 +++--- diff-lib.c | 2 +- diff.c | 2 +- entry.c| 4 ++-- git-compat-util.h | 13 +++-- help.c | 5 + path.c | 9 + preload-index.c| 2 +- read-cache.c | 6 +++--- unpack-trees.c | 8 17 files changed, 36 insertions(+), 53 deletions(-) [ ... ] I managed to apply your patch on next, and run the test suite before and after your patch. The performance running git status on a linux kernel is the same as in v1.8.3. Running test suite did not show new failures. The following test cases had failed, and are fixed now: t1400-update-ref t3210-pack-refs t3211-peel-ref t3306-notes-prune t5304-prune t5404-tracking-branches t5500-fetch-pack t5505-remote t5514-fetch-multiple t5515-fetch-merge-logic t6030-bisect-porcelain t9300-fast-import t9903-bash-prompt In short: Thanks, This looks good to me. Thank you for testing this! It is much appreciated. I sent a v2 patch to the list last night. It passes the full test suite for me on cygwin 1.5, although there appears to be a slight performance problem on MinGW (perhaps!). :( ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On 30.06.13 19:28, Ramsay Jones wrote:
Ramsay Jones wrote:
Michael Haggerty wrote:
On 06/27/2013 12:35 AM, Jeff King wrote:
[ ... ]
I think Michael's assessment above is missing one thing.
Peff is absolutely right; for some unknown reason I was thinking of the
consistency check as having been already fixed.
Well, the cygwin: Remove the Win32 l/stat() functions patch *does* fix
the problem. :-D It's just a pity we can't use it on performance grounds. :(
[...#ifdef out consistency check on cygwin when lock is held...]
Yes, this would work.
But, taking a step back, I think it is a bad idea to have an unreliable
stat() masquerading as a real stat(). If we want to allow the use of an
unreliable stat for certain purposes, let's have two stat() interfaces:
* the true stat() (in this case I guess cygwin's slow-but-correct
implementation)
* some fast_but_maybe_unreliable_stat(), which would map to stat() on
most platforms but might map to the Windows stat() on cygwin when so
configured.
By default the true stat() would always be used. It should have to be a
conscious decision, taken only in specific, vetted scenarios, to use the
unreliable stat.
You have just described my second patch! :D
Unfortunately, I have not had any time to work on the patch this weekend.
However, despite the patch being a bit rough around the edges, I decided
to send it out (see below) to get some early feedback.
Note that it passes the t3210, t3211, t5500, t3200, t3301, t7606 and t1301
tests, but I have not run the full test suite.
Comments welcome.
ATB,
Ramsay Jones
-- 8 --
Subject: [PATCH] cygwin: Add fast_[l]stat() functions
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
---
builtin/apply.c| 6 +++---
builtin/commit.c | 2 +-
builtin/ls-files.c | 2 +-
builtin/rm.c | 2 +-
builtin/update-index.c | 2 +-
check-racy.c | 2 +-
compat/cygwin.c| 12
compat/cygwin.h| 10 +++---
diff-lib.c | 2 +-
diff.c | 2 +-
entry.c| 4 ++--
git-compat-util.h | 13 +++--
help.c | 5 +
path.c | 9 +
preload-index.c| 2 +-
read-cache.c | 6 +++---
unpack-trees.c | 8
17 files changed, 36 insertions(+), 53 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 0e9b631..ca26caa 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3253,7 +3253,7 @@ static int load_current(struct image *image, struct
patch *patch)
if (pos 0)
return error(_(%s: does not exist in index), name);
ce = active_cache[pos];
- if (lstat(name, st)) {
+ if (fast_lstat(name, st)) {
if (errno != ENOENT)
return error(_(%s: %s), name, strerror(errno));
if (checkout_target(ce, st))
@@ -3396,7 +3396,7 @@ static int check_preimage(struct patch *patch, struct
cache_entry **ce, struct s
if (previous) {
st_mode = previous-new_mode;
} else if (!cached) {
- stat_ret = lstat(old_name, st);
+ stat_ret = fast_lstat(old_name, st);
if (stat_ret errno != ENOENT)
return error(_(%s: %s), old_name, strerror(errno));
}
@@ -3850,7 +3850,7 @@ static void add_index_file(const char *path, unsigned
mode, void *buf, unsigned
die(_(corrupt patch for subproject %s), path);
} else {
if (!cached) {
- if (lstat(path, st) 0)
+ if (fast_lstat(path, st) 0)
die_errno(_(unable to stat newly created file
'%s'),
path);
fill_stat_cache_info(ce, st);
diff --git a/builtin/commit.c b/builtin/commit.c
index 6b693c1..1d208c6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -231,7 +231,7 @@ static void add_remove_files(struct string_list *list)
if (p-util)
continue;
- if (!lstat(p-string, st)) {
+ if (!fast_lstat(p-string, st)) {
if (add_to_cache(p-string, st, 0))
die(_(updating files failed));
} else
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 08d9786..db66a0e 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -251,7 +251,7 @@ static void show_files(struct dir_struct *dir)
continue;
if (ce_skip_worktree(ce))
continue;
- err = lstat(ce-name, st);
+ err = fast_lstat(ce-name, st);
if (show_deleted err)
show_ce_entry(tag_removed, ce);
if (show_modified ce_modified(ce, st,
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Junio C Hamano wrote:
I like the part that gets rid of that get-mode-bits but at the
same time, I find this part wanting a reasonable in-code comment.
Indeed. (As I said, a bit rough around the edges ;-)
At least, with the earlier get-mode-bits, it was clear why we are
doing something special in that codepath, both from the name of the
helper function/macro and the comment attached to it describing how
the regular one is cheating.
We must say why this fast is not used everywhere and what criteria
you should apply when deciding to use it (or not use it). And the
fast name is much less descriptive.
I suspect (without thinking it through) that the rule would be
something like:
The fast variant is to be used to read from the filesystem the
stat bits that are stuffed into the index for quick touch
detection (aka cached stat info) and/or that are compared
with the cached stat info, and should not be used anywhere else.
Sounds good to me.
But then we always use lstat(2) and not stat(2) for that, so...
Indeed. Although there may be need of an fast_fstat (see below). :(
+#ifndef GIT_FAST_STAT
+static inline int fast_stat(const char *path, struct stat *st)
+{
+return stat(path, st);
+}
+static inline int fast_lstat(const char *path, struct stat *st)
+{
+return lstat(path, st);
+}
+#endif
Yes, I'm not very good at naming things, so suggestions welcome!
Note that I missed at least one lstat() call which needed to be renamed
to fast_lstat() (builtin/apply.c, line 3094 in checkout_target()).
This is my main concern with this patch (i.e. that I have missed some
more lstat calls that need to be renamed). I was a little surprised
at the size of the patch; direct index manipulation is more widespread
than I had expected.
Also, since cygwin has UNRELIABLE_FSTAT defined, on the first pass of
the patch, I ignored the use of fstat() in write_entry(). However, *if*
we allow for some other platform, which has a reliable fstat, but wants
to provide fast stat variants, then these fstat calls should logically
be fast. Alternatively, we could drop the use of fstat(), like so:
diff --git a/entry.c b/entry.c
index 4d2ac73..d04d7a1 100644
--- a/entry.c
+++ b/entry.c
@@ -104,21 +104,9 @@ static int open_output_fd(char *path, struct cache_entry
*ce, int to_tempfile)
}
}
-static int fstat_output(int fd, const struct checkout *state, struct stat
*st)
-{
- /* use fstat() only when path == ce-name */
- if (fstat_is_reliable()
- state-refresh_cache !state-base_dir_len) {
- fstat(fd, st);
- return 1;
- }
- return 0;
-}
-
static int streaming_write_entry(struct cache_entry *ce, char *path,
struct stream_filter *filter,
- const struct checkout *state, int to_tempfile,
- int *fstat_done, struct stat *statbuf)
+ const struct checkout *state, int to_tempfile)
{
int result = 0;
int fd;
@@ -128,7 +116,6 @@ static int streaming_write_entry(struct cache_entry *ce,
char *path,
return -1;
result |= stream_blob_to_fd(fd, ce-sha1, filter, 1);
- *fstat_done = fstat_output(fd, state, statbuf);
result |= close(fd);
if (result)
@@ -139,7 +126,7 @@ static int streaming_write_entry(struct cache_entry *ce,
char *path,
static int write_entry(struct cache_entry *ce, char *path, const struct
checkout *state, int to_tempfile)
{
unsigned int ce_mode_s_ifmt = ce-ce_mode S_IFMT;
- int fd, ret, fstat_done = 0;
+ int fd, ret;
char *new;
struct strbuf buf = STRBUF_INIT;
unsigned long size;
@@ -150,8 +137,7 @@ static int write_entry(struct cache_entry *ce, char
*path, const struct checkout
struct stream_filter *filter = get_stream_filter(ce-name,
ce-sha1);
if (filter
!streaming_write_entry(ce, path, filter,
-state, to_tempfile,
-fstat_done, st))
+state, to_tempfile))
goto finish;
}
@@ -190,8 +176,6 @@ static int write_entry(struct cache_entry *ce, char
*path, const struct checkout
}
wrote = write_in_full(fd, new, size);
- if (!to_tempfile)
- fstat_done = fstat_output(fd, state, st);
close(fd);
free(new);
if (wrote != size)
@@ -209,8 +193,7 @@ static int write_entry(struct cache_entry *ce, char
*path, const struct checkout
finish:
if (state-refresh_cache) {
- if (!fstat_done)
- fast_lstat(ce-name, st);
+ fast_lstat(ce-name, st);
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Ramsay Jones wrote:
Michael Haggerty wrote:
On 06/27/2013 12:35 AM, Jeff King wrote:
[ ... ]
I think Michael's assessment above is missing one thing.
Peff is absolutely right; for some unknown reason I was thinking of the
consistency check as having been already fixed.
Well, the cygwin: Remove the Win32 l/stat() functions patch *does* fix
the problem. :-D It's just a pity we can't use it on performance grounds. :(
[...#ifdef out consistency check on cygwin when lock is held...]
Yes, this would work.
But, taking a step back, I think it is a bad idea to have an unreliable
stat() masquerading as a real stat(). If we want to allow the use of an
unreliable stat for certain purposes, let's have two stat() interfaces:
* the true stat() (in this case I guess cygwin's slow-but-correct
implementation)
* some fast_but_maybe_unreliable_stat(), which would map to stat() on
most platforms but might map to the Windows stat() on cygwin when so
configured.
By default the true stat() would always be used. It should have to be a
conscious decision, taken only in specific, vetted scenarios, to use the
unreliable stat.
You have just described my second patch! :D
Unfortunately, I have not had any time to work on the patch this weekend.
However, despite the patch being a bit rough around the edges, I decided
to send it out (see below) to get some early feedback.
Note that it passes the t3210, t3211, t5500, t3200, t3301, t7606 and t1301
tests, but I have not run the full test suite.
Comments welcome.
ATB,
Ramsay Jones
-- 8 --
Subject: [PATCH] cygwin: Add fast_[l]stat() functions
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
---
builtin/apply.c| 6 +++---
builtin/commit.c | 2 +-
builtin/ls-files.c | 2 +-
builtin/rm.c | 2 +-
builtin/update-index.c | 2 +-
check-racy.c | 2 +-
compat/cygwin.c| 12
compat/cygwin.h| 10 +++---
diff-lib.c | 2 +-
diff.c | 2 +-
entry.c| 4 ++--
git-compat-util.h | 13 +++--
help.c | 5 +
path.c | 9 +
preload-index.c| 2 +-
read-cache.c | 6 +++---
unpack-trees.c | 8
17 files changed, 36 insertions(+), 53 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 0e9b631..ca26caa 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3253,7 +3253,7 @@ static int load_current(struct image *image, struct patch
*patch)
if (pos 0)
return error(_(%s: does not exist in index), name);
ce = active_cache[pos];
- if (lstat(name, st)) {
+ if (fast_lstat(name, st)) {
if (errno != ENOENT)
return error(_(%s: %s), name, strerror(errno));
if (checkout_target(ce, st))
@@ -3396,7 +3396,7 @@ static int check_preimage(struct patch *patch, struct
cache_entry **ce, struct s
if (previous) {
st_mode = previous-new_mode;
} else if (!cached) {
- stat_ret = lstat(old_name, st);
+ stat_ret = fast_lstat(old_name, st);
if (stat_ret errno != ENOENT)
return error(_(%s: %s), old_name, strerror(errno));
}
@@ -3850,7 +3850,7 @@ static void add_index_file(const char *path, unsigned
mode, void *buf, unsigned
die(_(corrupt patch for subproject %s), path);
} else {
if (!cached) {
- if (lstat(path, st) 0)
+ if (fast_lstat(path, st) 0)
die_errno(_(unable to stat newly created file
'%s'),
path);
fill_stat_cache_info(ce, st);
diff --git a/builtin/commit.c b/builtin/commit.c
index 6b693c1..1d208c6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -231,7 +231,7 @@ static void add_remove_files(struct string_list *list)
if (p-util)
continue;
- if (!lstat(p-string, st)) {
+ if (!fast_lstat(p-string, st)) {
if (add_to_cache(p-string, st, 0))
die(_(updating files failed));
} else
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 08d9786..db66a0e 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -251,7 +251,7 @@ static void show_files(struct dir_struct *dir)
continue;
if (ce_skip_worktree(ce))
continue;
- err = lstat(ce-name, st);
+ err = fast_lstat(ce-name, st);
if (show_deleted err)
show_ce_entry(tag_removed, ce);
if (show_modified ce_modified(ce, st, 0))
diff --git a/builtin/rm.c b/builtin/rm.c
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Ramsay Jones ram...@ramsay1.demon.co.uk writes:
Ramsay Jones wrote:
Michael Haggerty wrote:
On 06/27/2013 12:35 AM, Jeff King wrote:
[ ... ]
I think Michael's assessment above is missing one thing.
Peff is absolutely right; for some unknown reason I was thinking of the
consistency check as having been already fixed.
Well, the cygwin: Remove the Win32 l/stat() functions patch *does* fix
the problem. :-D It's just a pity we can't use it on performance grounds. :(
[...#ifdef out consistency check on cygwin when lock is held...]
Yes, this would work.
But, taking a step back, I think it is a bad idea to have an unreliable
stat() masquerading as a real stat(). If we want to allow the use of an
unreliable stat for certain purposes, let's have two stat() interfaces:
* the true stat() (in this case I guess cygwin's slow-but-correct
implementation)
* some fast_but_maybe_unreliable_stat(), which would map to stat() on
most platforms but might map to the Windows stat() on cygwin when so
configured.
By default the true stat() would always be used. It should have to be a
conscious decision, taken only in specific, vetted scenarios, to use the
unreliable stat.
...
I like the part that gets rid of that get-mode-bits but at the
same time, I find this part wanting a reasonable in-code comment.
At least, with the earlier get-mode-bits, it was clear why we are
doing something special in that codepath, both from the name of the
helper function/macro and the comment attached to it describing how
the regular one is cheating.
We must say why this fast is not used everywhere and what criteria
you should apply when deciding to use it (or not use it). And the
fast name is much less descriptive.
I suspect (without thinking it through) that the rule would be
something like:
The fast variant is to be used to read from the filesystem the
stat bits that are stuffed into the index for quick touch
detection (aka cached stat info) and/or that are compared
with the cached stat info, and should not be used anywhere else.
But then we always use lstat(2) and not stat(2) for that, so...
+#ifndef GIT_FAST_STAT
+static inline int fast_stat(const char *path, struct stat *st)
+{
+ return stat(path, st);
+}
+static inline int fast_lstat(const char *path, struct stat *st)
+{
+ return lstat(path, st);
+}
+#endif
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On 2013-06-26 23.54, Ramsay Jones wrote: Torsten Bögershausen wrote: On 2013-06-25 23.18, Junio C Hamano wrote: Johannes Sixt j...@kdbg.org writes: Some context: This is about a patch by Ramsay that removes the schizophrenic lstat hack for Cygwin. Junio, can you please queue that patch in pu? Sure. Thanks. First of all, thanks for the work. Here some benchmark results, (The test run of the test suite did the same amout of time). The test suite runs noticeably faster for me. But: git status -uno in real life takes double the time, git 1.8.3 compared against pu with the vanilla l/stat 1 second - 2 seconds on linux kernel 0.2 seconds - 0.4 seconds on git.git Hmm, OK, I guess I will have to try something else. Sigh :( Do we have any known problems with the current implementation ? Yes. The next branch is currently broken. (see reply to Junio) Does speed matter ? One vote to keep the special cygwin functions. (And have a look how to improve the core.filemode) I don't understand this (parenthetical) comment; could you elaborate on this. ATB, Ramsay Jones This is probably wrong information: I had in mind that cygwin sets core.filemode=false, which is quite annoying when exchanging .sh files with linux. But that seems to be wrong, a quick test shows that core.filemode=true. Sorry for confusion. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On Thu, Jun 27, 2013 at 07:51:57AM +0200, Michael Haggerty wrote: In theory we can drop the safety valve; it should never actually happen. But I'd like to keep it there for working systems. Perhaps it is worth doing something like this: [...#ifdef out consistency check on cygwin when lock is held...] Yes, this would work. But, taking a step back, I think it is a bad idea to have an unreliable stat() masquerading as a real stat(). If we want to allow the use of an unreliable stat for certain purposes, let's have two stat() interfaces: * the true stat() (in this case I guess cygwin's slow-but-correct implementation) * some fast_but_maybe_unreliable_stat(), which would map to stat() on most platforms but might map to the Windows stat() on cygwin when so configured. Yeah, that makes sense to me. I don't have a particular opinion on which way to go, as I do not use cygwin at all (and on most platforms, the two stat interfaces would just both call stat()). I will leave it up to Cygwin folks whether they want to do something like my patch as a band-aid while working up the two-stat() solution. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Jeff King p...@peff.net writes: But, taking a step back, I think it is a bad idea to have an unreliable stat() masquerading as a real stat(). If we want to allow the use of an unreliable stat for certain purposes, let's have two stat() interfaces: * the true stat() (in this case I guess cygwin's slow-but-correct implementation) * some fast_but_maybe_unreliable_stat(), which would map to stat() on most platforms but might map to the Windows stat() on cygwin when so configured. Yeah, that makes sense to me. I don't have a particular opinion on which way to go, as I do not use cygwin at all (and on most platforms, the two stat interfaces would just both call stat()). I will leave it up to Cygwin folks whether they want to do something like my patch as a band-aid while working up the two-stat() solution. I think in the longer term two-stat solution is a good thing. 0117c2f0 (Make core.sharedRepository work under cygwin 1.7, 2013-03-23) introduced get_st_mode_bits() to work around another glitch in the fast-but-cheating-and-unreliable replacement, which we may be able to revert once it is done. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Jeff King wrote: On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote: [ ... ] I think Michael's assessment above is missing one thing. It is true that a false positive is just a performance problem in most cases, as we unnecessarily reload the file, thinking it has changed. However, when we have taken a lock on the file, there is an additional safety measure: if we find the file is changed, we abort, as that should never happen (it means somebody else modified the file while we had it locked). But of course Cygwin's false positive here triggers the safety valve, and we die without even realizing that nothing changed. Indeed, this is exactly the situation. ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Michael Haggerty wrote: On 06/27/2013 12:35 AM, Jeff King wrote: [ ... ] I think Michael's assessment above is missing one thing. Peff is absolutely right; for some unknown reason I was thinking of the consistency check as having been already fixed. Well, the cygwin: Remove the Win32 l/stat() functions patch *does* fix the problem. :-D It's just a pity we can't use it on performance grounds. :( [...#ifdef out consistency check on cygwin when lock is held...] Yes, this would work. But, taking a step back, I think it is a bad idea to have an unreliable stat() masquerading as a real stat(). If we want to allow the use of an unreliable stat for certain purposes, let's have two stat() interfaces: * the true stat() (in this case I guess cygwin's slow-but-correct implementation) * some fast_but_maybe_unreliable_stat(), which would map to stat() on most platforms but might map to the Windows stat() on cygwin when so configured. By default the true stat() would always be used. It should have to be a conscious decision, taken only in specific, vetted scenarios, to use the unreliable stat. You have just described my second patch! :D For example, I can't imagine that checking the freshness of the index or of the packed-refs file is ever going to be a bottleneck, so there is no reason at all to use an unreliable stat() here. On the other hand, stat() seems definitely to be a bottleneck when testing for changes in a 100,000 file working tree, and here occasional mistakes might be considered acceptable. So for this purpose the unreliable stat() might be used. I have already written the first pass at this patch, but I'm having difficulty with naming (get_cached_stat_data, get_index_stat_data, get_stat_data, ... ;-) ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Torsten Bögershausen wrote: [ ... ] (And have a look how to improve the core.filemode) I don't understand this (parenthetical) comment; could you elaborate on this. This is probably wrong information: I had in mind that cygwin sets core.filemode=false, It does, see commit c869753e (Force core.filemode to false on Cygwin, 30-12-2006). which is quite annoying when exchanging .sh files with linux. Indeed, I used to build git with NO_TRUSTABLE_FILEMODE reset so that I wouldn't have to edit the config file by hand after a git-clone or git-init. But that seems to be wrong, a quick test shows that core.filemode=true. Hmm, it shouldn't - confused! Sorry for confusion. ATB Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Jeff King wrote:
On Wed, Jun 26, 2013 at 06:35:52PM -0400, Jeff King wrote:
I am curious how often Cygwin gives us the false positive. If it is
every time, then the check is not doing much good at all. Is it possible
for you to instrument stat_validity_check to report how often it does or
does not do anything useful?
Maybe like this:
diff --git a/read-cache.c b/read-cache.c
index d5201f9..19dcb69 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1958,6 +1958,14 @@ void stat_validity_clear(struct stat_validity *sv)
sv-sd = NULL;
}
+static int unchanged;
+static int attempts;
+static void print_stats(void)
+{
+ fprintf(stderr, stat data was unchanged %d/%d\n,
+ unchanged, attempts);
+}
+
int stat_validity_check(struct stat_validity *sv, const char *path)
{
struct stat st;
@@ -1966,7 +1974,16 @@ int stat_validity_check(struct stat_validity *sv,
const char *path)
return sv-sd == NULL;
if (!sv-sd)
return 0;
- return S_ISREG(st.st_mode) !match_stat_data(sv-sd, st);
+ if (!S_ISREG(st.st_mode))
+ return 0;
+ if (!attempts++)
+ atexit(print_stats);
+ if (!match_stat_data(sv-sd, st)) {
+ unchanged++;
+ return 1;
+ }
+ else
+ return 0;
}
void stat_validity_update(struct stat_validity *sv, int fd)
Running t3211 -v, I see things like:
stat data was unchanged 3/3
stat data was unchanged 20/20
stat data was unchanged 2/2
Deleted branch yadda (was d1ff1c9).
stat data was unchanged 8/8
ok 8 - peeled refs survive deletion of packed ref
I am curious if you will see 0/20 or 19/20 there on Cygwin.
Ah, no ... the problem is not as subtle as this! :-D
I'm sorry if I failed to mention that I already had a patch
to solve the problem, but that I *don't* want to submit it,
since it is ugly and just serves to spread the madness! ;-)
This is why I tried the cygwin: Remove the Win32 l/stat()
functions patch first; I think this is the correct approach
to fixing this problem (and similar *future* problems).
However, since that is no longer an option, on performance
grounds, I have other ideas I want to try. (see later email)
The schizophrenic stat functions have caused many subtle
problems, but this is not one of them; a brick to the head
would be more subtle. The problem is caused by the stat
validity code using both fstat() and stat() on the same
file. Note that there is no Win32 implementation of the
fstat() function.
So, a solution to the problem would be to provide just such
an implementation. The patch to do so is given below, just
for illustration purposes.
This fixes the failures to t3210, t3211 and t5500. However, I
have not run the full test suite. This may cause some further
*subtle* problems (just grep for fstat and consider what may
go wrong; I have not done that), especially when you consider
that cygwin has the UNRELIABLE_FSTAT build variable set.
HTH
ATB,
Ramsay Jones
-- 8 --
Subject: [PATCH] cygwin: Add an Win32 fstat implementation
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
---
compat/cygwin.c | 41 +
compat/cygwin.h | 3 +++
2 files changed, 44 insertions(+)
diff --git a/compat/cygwin.c b/compat/cygwin.c
index 91ce5d4..d59c9fb 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -4,6 +4,7 @@
#include sys/errno.h
#include win32.h
#include ../git-compat-util.h
+#include io.h
#include ../cache.h /* to read configuration */
/*
@@ -100,6 +101,38 @@ static int cygwin_stat(const char *path, struct stat *buf)
return do_stat(path, buf, stat);
}
+static int cygwin_fstat(int fd, struct stat *buf)
+{
+ HANDLE fh = (HANDLE)get_osfhandle(fd);
+ BY_HANDLE_FILE_INFORMATION fdata;
+
+ if (fh == INVALID_HANDLE_VALUE) {
+ errno = EBADF;
+ return -1;
+ }
+ if (GetFileType(fh) != FILE_TYPE_DISK)
+ return (fstat)(fd, buf);
+ if (GetFileInformationByHandle(fh, fdata)) {
+ buf-st_dev = buf-st_rdev = 0; /* not used by Git */
+ buf-st_ino = 0;
+ buf-st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
+ buf-st_nlink = 1;
+ buf-st_uid = buf-st_gid = 0;
+#ifdef __CYGWIN_USE_BIG_TYPES__
+ buf-st_size = ((_off64_t)fdata.nFileSizeHigh 32) +
+ fdata.nFileSizeLow;
+#else
+ buf-st_size = (off_t)fdata.nFileSizeLow;
+#endif
+ buf-st_blocks = size_to_blocks(buf-st_size);
+ filetime_to_timespec(fdata.ftLastAccessTime, buf-st_atim);
+ filetime_to_timespec(fdata.ftLastWriteTime, buf-st_mtim);
+ filetime_to_timespec(fdata.ftCreationTime, buf-st_ctim);
+ return 0;
+ }
+ errno = EBADF;
+ return -1;
+}
/*
* At start up, we are trying to determine whether Win32
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On 06/27/2013 06:58 PM, Ramsay Jones wrote: This is why I tried the cygwin: Remove the Win32 l/stat() functions patch first; I think this is the correct approach to fixing this problem (and similar *future* problems). I adamantly agree. However, since that is no longer an option, on performance grounds, I have other ideas I want to try. (see later email) Correctness first, speed later. 1) Keep the patch to remove the buggy and unreliable stat / lstat. 2) We fix the remaining test failures. 3) With the test suite passing, stat optimization(s) that cause no failures / regressions can be accepted. With the msys/mingw git available for years now, there really is not a reason to make Cygwin's git violate the Posix expectations of that platform. msys makes no such promises, so is the right tool for those on Windows who just want git as fast as possible on Windows (still slow) and don't care about file modes, softlinks, etc. I'm keeping your patch in my tree. Mark -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On 2013-06-25 23.18, Junio C Hamano wrote: Johannes Sixt j...@kdbg.org writes: Some context: This is about a patch by Ramsay that removes the schizophrenic lstat hack for Cygwin. Junio, can you please queue that patch in pu? Sure. Thanks. First of all, thanks for the work. Here some benchmark results, (The test run of the test suite did the same amout of time). But: git status -uno in real life takes double the time, git 1.8.3 compared against pu with the vanilla l/stat 1 second - 2 seconds on linux kernel 0.2 seconds - 0.4 seconds on git.git Do we have any known problems with the current implementation ? Does speed matter ? One vote to keep the special cygwin functions. (And have a look how to improve the core.filemode) /Torsten -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Torsten Bögershausen wrote: On 2013-06-25 23.18, Junio C Hamano wrote: Johannes Sixt j...@kdbg.org writes: Some context: This is about a patch by Ramsay that removes the schizophrenic lstat hack for Cygwin. Junio, can you please queue that patch in pu? Sure. Thanks. First of all, thanks for the work. Here some benchmark results, (The test run of the test suite did the same amout of time). The test suite runs noticeably faster for me. But: git status -uno in real life takes double the time, git 1.8.3 compared against pu with the vanilla l/stat 1 second - 2 seconds on linux kernel 0.2 seconds - 0.4 seconds on git.git Hmm, OK, I guess I will have to try something else. Sigh :( Do we have any known problems with the current implementation ? Yes. The next branch is currently broken. (see reply to Junio) Does speed matter ? One vote to keep the special cygwin functions. (And have a look how to improve the core.filemode) I don't understand this (parenthetical) comment; could you elaborate on this. ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Junio C Hamano wrote:
Ramsay Jones ram...@ramsay1.demon.co.uk writes:
Michael Haggerty and Jeff King have been re-vamping the reference
handling code. The failures noted above were provoked by patches
in the 'mh/ref-races' branch. At the time I wrote this patch, that
branch was only included in 'pu', but I notice that this topic has
now progressed to 'next' (see commit 71f1a182).
I had an impression that up to 98eeb09e (for_each_ref: load all
loose refs before packed refs, 2013-06-20) that is now in 'next'
does not agressively use the lstat timestamp of the packed-refs
file, and the optional bit 5d478f5c (refs: do not invalidate the
packed-refs cache unnecessarily, 2013-06-20), and the one in 'next'
should be safe with the cheating-lstat. Isn't it the case?
The next branch (from a couple of days ago, namely commit 4f488db) is
currently broken on cygwin, like so:
$ cd t
$ ./t3211-peel-ref.sh -i -v
[ ... ]
ok 7 - refs are peeled outside of refs/tags (old packed)
expecting success:
git pack-refs --all
cp .git/packed-refs fully-peeled
git branch yadda
git pack-refs --all
git branch -d yadda
test_cmp fully-peeled .git/packed-refs
fatal: internal error: packed-ref cache cleared while locked
not ok 8 - peeled refs survive deletion of packed ref
#
# git pack-refs --all
# cp .git/packed-refs fully-peeled
# git branch yadda
# git pack-refs --all
# git branch -d yadda
# test_cmp fully-peeled .git/packed-refs
#
$ cd trash\ directory.t3211-peel-ref/
$ ls
actual base.t expect
$ ../../bin-wrappers/git pack-refs --all
fatal: internal error: packed-ref cache cleared while locked
$ gdb ../../git.exe
GNU gdb 6.5.50.20060706-cvs (cygwin-special)
Copyright (C) 2006 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type show copying to see the conditions.
There is absolutely no warranty for GDB. Type show warranty for details.
This GDB was configured as i686-pc-cygwin...
(gdb) b stat_validity_check
Breakpoint 1 at 0x48a33f: file read-cache.c, line 1965.
(gdb) run pack-refs --all
Starting program: /home/ramsay/git/git.exe pack-refs --all
Loaded symbols for /cygdrive/c/WINDOWS/system32/ntdll.dll
Loaded symbols for /cygdrive/c/WINDOWS/system32/kernel32.dll
Loaded symbols for /usr/bin/cygcrypto-0.9.8.dll
Loaded symbols for /usr/bin/cygwin1.dll
Loaded symbols for /cygdrive/c/WINDOWS/system32/advapi32.dll
Loaded symbols for /cygdrive/c/WINDOWS/system32/rpcrt4.dll
Loaded symbols for /cygdrive/c/WINDOWS/system32/secur32.dll
Loaded symbols for /usr/bin/cygiconv-2.dll
Loaded symbols for /usr/bin/cygz.dll
Breakpoint 1, stat_validity_check (sv=0xa611a4,
path=0x57d98c .git/packed-refs) at read-cache.c:1965
1965if (stat(path, st) 0)
(gdb) n
1967if (!sv-sd)
(gdb) p sv
$1 = (struct stat_validity *) 0xa611a4
(gdb) p *sv
$2 = {sd = 0xa62478}
(gdb) p sv-sd
$3 = (struct stat_data *) 0xa62478
(gdb) p *sv-sd
$4 = {sd_ctime = {sec = 1372194879, nsec = 671875000}, sd_mtime = {
sec = 1372194879, nsec = 65625}, sd_dev = 2899104371,
sd_ino = 180184, sd_uid = 1005, sd_gid = 513, sd_size = 296}
(gdb) p st
$5 = {st_dev = 0, st_ino = 0, st_mode = 33152, st_nlink = 1, st_uid = 0,
st_gid = 0, st_rdev = 0, st_size = 296, st_atim = {tv_sec = 1372195048,
tv_nsec = 890625000}, st_mtim = {tv_sec = 1372194879,
tv_nsec = 65625}, st_ctim = {tv_sec = 1372194879,
tv_nsec = 15625000}, st_blksize = 5636381, st_blocks = 1, st_spare4 = {
10883480, 5757324}}
(gdb) c
Continuing.
fatal: internal error: packed-ref cache cleared while locked
Program exited with code 0200.
(gdb) quit
$ ../../test-stat .git/packed-refs
stat for '.git/packed-refs':
*dev: -1395862925, 0
*ino: 180184, 0
*mode: 100644 -rw-, 100600 -rw-
nlink: 1, 1
*uid: 1005, 0
*gid: 513, 0
*rdev: -1395862925, 0
size: 296, 296
atime: 1372195048, 1372195048 Tue Jun 25 22:17:28 2013
mtime: 1372194879, 1372194879 Tue Jun 25 22:14:39 2013
ctime: 1372194879, 1372194879 Tue Jun 25 22:14:39 2013
$
Note that, as noted before, (at least) the following tests fail
due to the 'mh/ref-races' branch:
t3210-pack-refs.sh(Wstat: 256 Tests: 16 Failed: 12)
Failed tests: 4-9, 11-16
Non-zero exit status: 1
t3211-peel-ref.sh (Wstat: 256 Tests: 8 Failed: 1)
Failed test: 8
Non-zero exit status: 1
t5500-fetch-pack.sh (Wstat: 256 Tests: 54 Failed: 1)
Failed test: 43
Non-zero exit status: 1
In any case, if removing the cheating-lstat will improve the
robustness without
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Michael Haggerty wrote: On 06/25/2013 07:07 AM, Junio C Hamano wrote: Ramsay Jones ram...@ramsay1.demon.co.uk writes: Michael Haggerty and Jeff King have been re-vamping the reference handling code. The failures noted above were provoked by patches in the 'mh/ref-races' branch. At the time I wrote this patch, that branch was only included in 'pu', but I notice that this topic has now progressed to 'next' (see commit 71f1a182). I had an impression that up to 98eeb09e (for_each_ref: load all loose refs before packed refs, 2013-06-20) that is now in 'next' does not agressively use the lstat timestamp of the packed-refs file, and the optional bit 5d478f5c (refs: do not invalidate the packed-refs cache unnecessarily, 2013-06-20), and the one in 'next' should be safe with the cheating-lstat. Isn't it the case? In any case, if removing the cheating-lstat will improve the robustness without hurting performance, I am all for it. The less platform specific hacks, the better ;-). The following patch in the non-optional commits (which has already been merged to next) uses stat() via the new stat_validity API: ca9199300e get_packed_ref_cache: reload packed-refs file when it changes This patch adds some *extra* cache invalidation that was heretofore missing. If stat() is broken it could (a) cause a false positive, resulting in some unnecessary cache invalidation and re-reading of packed-refs, which will hurt performance but not correctness; or (b) cause a false negative, in which case the stale cache might be used for reading (but not writing), just as was *always* the case before this patch. As far as I understand, the concern for cygwin is (a). I will leave it to others to measure and/or decide whether the performance loss is too grave to endure until the cygwin stat() situation is fixed. Hmm, I'm not sure I understand ... However, I can confirm that the 'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not just a speed issue; it provokes fatal errors). See reply to Junio. ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote: This patch adds some *extra* cache invalidation that was heretofore missing. If stat() is broken it could (a) cause a false positive, resulting in some unnecessary cache invalidation and re-reading of packed-refs, which will hurt performance but not correctness; or (b) cause a false negative, in which case the stale cache might be used for reading (but not writing), just as was *always* the case before this patch. As far as I understand, the concern for cygwin is (a). I will leave it to others to measure and/or decide whether the performance loss is too grave to endure until the cygwin stat() situation is fixed. Hmm, I'm not sure I understand ... However, I can confirm that the 'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not just a speed issue; it provokes fatal errors). I think Michael's assessment above is missing one thing. It is true that a false positive is just a performance problem in most cases, as we unnecessarily reload the file, thinking it has changed. However, when we have taken a lock on the file, there is an additional safety measure: if we find the file is changed, we abort, as that should never happen (it means somebody else modified the file while we had it locked). But of course Cygwin's false positive here triggers the safety valve, and we die without even realizing that nothing changed. In theory we can drop the safety valve; it should never actually happen. But I'd like to keep it there for working systems. Perhaps it is worth doing something like this: diff --git a/refs.c b/refs.c index 4302206..7cac42d 100644 --- a/refs.c +++ b/refs.c @@ -1080,6 +1080,16 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs) packed_refs_file = git_path(packed-refs); if (refs-packed +#ifdef STAT_VALIDITY_GIVES_FALSE_POSITIVES + /* +* If we get false positives from our stat calls on this platform, +* then we must assume that once we have locked the packed-refs +* file it does not change. Otherwise it looks like somebody else +* has changed it out from under us (while we have it locked!), and +* we die(). +*/ + !refs-packed-lock +#endif !stat_validity_check(refs-packed-validity, packed_refs_file)) clear_packed_ref_cache(refs); and then we can add that define to Cygwin in the Makefile. I verified the issue on Linux by breaking stat_validity_check to always return 0 (i.e., to always give a false positive that the file has changed, which is what Cygwin is doing). I am curious how often Cygwin gives us the false positive. If it is every time, then the check is not doing much good at all. Is it possible for you to instrument stat_validity_check to report how often it does or does not do anything useful? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On Wed, Jun 26, 2013 at 06:35:52PM -0400, Jeff King wrote:
I am curious how often Cygwin gives us the false positive. If it is
every time, then the check is not doing much good at all. Is it possible
for you to instrument stat_validity_check to report how often it does or
does not do anything useful?
Maybe like this:
diff --git a/read-cache.c b/read-cache.c
index d5201f9..19dcb69 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1958,6 +1958,14 @@ void stat_validity_clear(struct stat_validity *sv)
sv-sd = NULL;
}
+static int unchanged;
+static int attempts;
+static void print_stats(void)
+{
+ fprintf(stderr, stat data was unchanged %d/%d\n,
+ unchanged, attempts);
+}
+
int stat_validity_check(struct stat_validity *sv, const char *path)
{
struct stat st;
@@ -1966,7 +1974,16 @@ int stat_validity_check(struct stat_validity *sv, const
char *path)
return sv-sd == NULL;
if (!sv-sd)
return 0;
- return S_ISREG(st.st_mode) !match_stat_data(sv-sd, st);
+ if (!S_ISREG(st.st_mode))
+ return 0;
+ if (!attempts++)
+ atexit(print_stats);
+ if (!match_stat_data(sv-sd, st)) {
+ unchanged++;
+ return 1;
+ }
+ else
+ return 0;
}
void stat_validity_update(struct stat_validity *sv, int fd)
Running t3211 -v, I see things like:
stat data was unchanged 3/3
stat data was unchanged 20/20
stat data was unchanged 2/2
Deleted branch yadda (was d1ff1c9).
stat data was unchanged 8/8
ok 8 - peeled refs survive deletion of packed ref
I am curious if you will see 0/20 or 19/20 there on Cygwin.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On 06/26/2013 10:19 AM, Torsten Bögershausen wrote: On 2013-06-25 23.18, Junio C Hamano wrote: Johannes Sixt j...@kdbg.org writes: Some context: This is about a patch by Ramsay that removes the schizophrenic lstat hack for Cygwin. Junio, can you please queue that patch in pu? Sure. Thanks. First of all, thanks for the work. Here some benchmark results, (The test run of the test suite did the same amout of time). But: git status -uno in real life takes double the time, git 1.8.3 compared against pu with the vanilla l/stat 1 second - 2 seconds on linux kernel 0.2 seconds - 0.4 seconds on git.git Do we have any known problems with the current implementation ? Does speed matter ? One vote to keep the special cygwin functions. (And have a look how to improve the core.filemode) /Torsten There have been threads on the cygwin mailing lists for at least a decade looking to speed up cygwin's posix stat / lstat (and fork). If improvement were merely difficult, it would have been done long ago. As git cares about things like execute bits, file / repository permissions, and soft links, whatever stat / lstat git uses needs to fully support those under cygwin, either by using what cygwin provides or providing a complete replacement. Note my other reply - with Ramsay's patch I can complete the test suite (except for t0008.sh that has a known hang) while without it I find the test suite randomly (unrepeatable) hangs in many tests. So, this stat/lstat replacement is at least implicated in current troubles. Mark -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On 06/27/2013 12:35 AM, Jeff King wrote: On Wed, Jun 26, 2013 at 10:45:48PM +0100, Ramsay Jones wrote: This patch adds some *extra* cache invalidation that was heretofore missing. If stat() is broken it could (a) cause a false positive, resulting in some unnecessary cache invalidation and re-reading of packed-refs, which will hurt performance but not correctness; or (b) cause a false negative, in which case the stale cache might be used for reading (but not writing), just as was *always* the case before this patch. As far as I understand, the concern for cygwin is (a). I will leave it to others to measure and/or decide whether the performance loss is too grave to endure until the cygwin stat() situation is fixed. Hmm, I'm not sure I understand ... However, I can confirm that the 'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not just a speed issue; it provokes fatal errors). I think Michael's assessment above is missing one thing. Peff is absolutely right; for some unknown reason I was thinking of the consistency check as having been already fixed. However, when we have taken a lock on the file, there is an additional safety measure: if we find the file is changed, we abort, as that should never happen (it means somebody else modified the file while we had it locked). But of course Cygwin's false positive here triggers the safety valve, and we die without even realizing that nothing changed. In theory we can drop the safety valve; it should never actually happen. But I'd like to keep it there for working systems. Perhaps it is worth doing something like this: [...#ifdef out consistency check on cygwin when lock is held...] Yes, this would work. But, taking a step back, I think it is a bad idea to have an unreliable stat() masquerading as a real stat(). If we want to allow the use of an unreliable stat for certain purposes, let's have two stat() interfaces: * the true stat() (in this case I guess cygwin's slow-but-correct implementation) * some fast_but_maybe_unreliable_stat(), which would map to stat() on most platforms but might map to the Windows stat() on cygwin when so configured. By default the true stat() would always be used. It should have to be a conscious decision, taken only in specific, vetted scenarios, to use the unreliable stat. For example, I can't imagine that checking the freshness of the index or of the packed-refs file is ever going to be a bottleneck, so there is no reason at all to use an unreliable stat() here. On the other hand, stat() seems definitely to be a bottleneck when testing for changes in a 100,000 file working tree, and here occasional mistakes might be considered acceptable. So for this purpose the unreliable stat() might be used. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Am 25.06.2013 00:10, schrieb Junio C Hamano: Mark Levedahl mleved...@gmail.com writes: On 06/22/2013 03:38 PM, Ramsay Jones wrote: Also, apart from running the git test-suite, I have the Win32 l/stat functions disabled on all of my repos. In particular, I have core.filemode set to true. (At one point, I used to build git with NO_TRUSTABLE_FILEMODE reset so that I wouldn't have to remember to reset core.filemode by hand after a git-clone or git-init). I should also note that I run MinGW git on the same laptop and, using git.git as an example, it does not seem that much faster (if at all) than cygwin git. After applying your patch to master, I've had the test-suite running in a VM using WinXP + current cygwin (v1.7.x) for about 8 hours, no failures so far but it could take another day to complete. I never found any real speed up using the Win32 stat/lstat functions, and the lack of Posix compatibility breaking cross-platform projects, links, etc., made this function a mis-feature in my opinion for years. As I found no positive benefit from the Win32 lstat, I modified git for local use years ago to set core.filemode=true when cloning / initing to avoid as many issues as possible. So that's two votes to use the vanilla Cygwin stat/lstat, essentially reverting adbc0b6b (cygwin: Use native Win32 API for stat, 2008-09-30), which was added by Dmitry and Shawn while I was away. Let's wait and see if people give us more data points and decide. That'll be more productive if we let the list know ;-) Some context: This is about a patch by Ramsay that removes the schizophrenic lstat hack for Cygwin. Junio, can you please queue that patch in pu? -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
On 25.06.13 21:23, Johannes Sixt wrote: Am 25.06.2013 00:10, schrieb Junio C Hamano: Mark Levedahl mleved...@gmail.com writes: On 06/22/2013 03:38 PM, Ramsay Jones wrote: Also, apart from running the git test-suite, I have the Win32 l/stat functions disabled on all of my repos. In particular, I have core.filemode set to true. (At one point, I used to build git with NO_TRUSTABLE_FILEMODE reset so that I wouldn't have to remember to reset core.filemode by hand after a git-clone or git-init). I should also note that I run MinGW git on the same laptop and, using git.git as an example, it does not seem that much faster (if at all) than cygwin git. After applying your patch to master, I've had the test-suite running in a VM using WinXP + current cygwin (v1.7.x) for about 8 hours, no failures so far but it could take another day to complete. I never found any real speed up using the Win32 stat/lstat functions, and the lack of Posix compatibility breaking cross-platform projects, links, etc., made this function a mis-feature in my opinion for years. As I found no positive benefit from the Win32 lstat, I modified git for local use years ago to set core.filemode=true when cloning / initing to avoid as many issues as possible. So that's two votes to use the vanilla Cygwin stat/lstat, essentially reverting adbc0b6b (cygwin: Use native Win32 API for stat, 2008-09-30), which was added by Dmitry and Shawn while I was away. Let's wait and see if people give us more data points and decide. That'll be more productive if we let the list know ;-) Some context: This is about a patch by Ramsay that removes the schizophrenic lstat hack for Cygwin. Junio, can you please queue that patch in pu? -- Hannes Here some benchmarks: Clone the linux kernel from August 2012 (which was in house) to the windows box, 2.3 GhZ Core duo. Run with and without core.filemode, which triggers cygwinfstricks (and is shorter to type) Numbers are typical hot cache, cold cache is 6..8 seconds real $ git --version git version 1.8.3.1.g6f17ca7 __ $ time git -c core.filemode=true status -uno # On branch master # Changes not staged for commit: # (use git add file... to update what will be committed) # (use git checkout -- file... to discard changes in working directory) # # modified: include/linux/netfilter/xt_CONNMARK.h # modified: include/linux/netfilter/xt_DSCP.h # modified: include/linux/netfilter/xt_MARK.h # modified: include/linux/netfilter/xt_RATEEST.h # modified: include/linux/netfilter/xt_TCPMSS.h # modified: include/linux/netfilter_ipv4/ipt_ECN.h # modified: include/linux/netfilter_ipv4/ipt_TTL.h # modified: include/linux/netfilter_ipv6/ip6t_HL.h # modified: net/netfilter/xt_DSCP.c # modified: net/netfilter/xt_HL.c # modified: net/netfilter/xt_RATEEST.c # modified: net/netfilter/xt_TCPMSS.c # no changes added to commit (use git add and/or git commit -a) real0m2.313s user0m0.577s sys 0m1.765s -- tb@snoopy ~/projects/linux-2.6 $ time git -c core.filemode=false status -uno # On branch master # Changes not staged for commit: # (use git add file... to update what will be committed) # (use git checkout -- file... to discard changes in working directory) # # modified: include/linux/netfilter/xt_CONNMARK.h # modified: include/linux/netfilter/xt_DSCP.h # modified: include/linux/netfilter/xt_MARK.h # modified: include/linux/netfilter/xt_RATEEST.h # modified: include/linux/netfilter/xt_TCPMSS.h # modified: include/linux/netfilter_ipv4/ipt_ECN.h # modified: include/linux/netfilter_ipv4/ipt_TTL.h # modified: include/linux/netfilter_ipv6/ip6t_HL.h # modified: net/netfilter/xt_DSCP.c # modified: net/netfilter/xt_HL.c # modified: net/netfilter/xt_RATEEST.c # modified: net/netfilter/xt_TCPMSS.c # no changes added to commit (use git add and/or git commit -a) real0m1.047s user0m0.311s sys 0m0.765s /Torsten -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Johannes Sixt j...@kdbg.org writes: Some context: This is about a patch by Ramsay that removes the schizophrenic lstat hack for Cygwin. Junio, can you please queue that patch in pu? Sure. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
