From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ramsay Jones Subject: Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions Date: Sun, 30 Jun 2013 18:28:40 +0100 Message-ID: <51D06AC8.70206@ramsay1.demon.co.uk> References: <51C5FD28.1070004@ramsay1.demon.co.uk> <51C6BC4B.9030905@web.de> <51C8BF2C.2050203@ramsay1.demon.co.uk> <7vy59y4w3r.fsf@alter.siamese.dyndns.org> <51C94425.7050006@alum.mit.edu> <51CB610C.7050501@ramsay1.demon.co.uk> <20130626223552.GA12785@sigill.intra.peff.net> <51CBD2FD.5070905@alum.mit.edu> <51CCC643.1050702@ramsay1.demon.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Jeff King , Junio C Hamano , =?UTF-8?B?VG9yc3RlbiBCw7ZnZXJzaGF1c2Vu?= , Johannes Sixt , "Shawn O. Pearce" , mlevedahl@gmail.com, dpotapov@gmail.com, GIT Mailing-list To: Michael Haggerty X-From: git-owner@vger.kernel.org Sun Jun 30 19:30:55 2013 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1UtLSf-0004Qx-7X for gcvg-git-2@plane.gmane.org; Sun, 30 Jun 2013 19:30:53 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751261Ab3F3Rag (ORCPT ); Sun, 30 Jun 2013 13:30:36 -0400 Received: from mdfmta010.mxout.tbr.inty.net ([91.221.168.51]:57163 "EHLO smtp.demon.co.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750827Ab3F3Raf (ORCPT ); Sun, 30 Jun 2013 13:30:35 -0400 Received: from mdfmta010.tbr.inty.net (unknown [127.0.0.1]) by mdfmta010.tbr.inty.net (Postfix) with ESMTP id 64FBC6F99E6; Sun, 30 Jun 2013 18:30:32 +0100 (BST) Received: from mdfmta010.tbr.inty.net (unknown [127.0.0.1]) by mdfmta010.tbr.inty.net (Postfix) with ESMTP id A7BCD6F968B; Sun, 30 Jun 2013 18:30:29 +0100 (BST) Received: from [193.237.126.196] (unknown [193.237.126.196]) by mdfmta010.tbr.inty.net (Postfix) with ESMTP; Sun, 30 Jun 2013 18:30:26 +0100 (BST) User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130509 Thunderbird/17.0.6 In-Reply-To: <51CCC643.1050702@ramsay1.demon.co.uk> X-MDF-HostID: 3 Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: 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 --- 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 index 06025a2..4b783e7 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -143,7 +143,7 @@ static int check_local_mod(unsigned char *head, int index_only) } ce = active_cache[pos]; - if (lstat(ce->name, &st) < 0) { + if (fast_lstat(ce->name, &st) < 0) { if (errno != ENOENT && errno != ENOTDIR) warning("'%s': %s", ce->name, strerror(errno)); /* It already vanished from the working tree */ diff --git a/builtin/update-index.c b/builtin/update-index.c index 5c7762e..4790e4c 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -206,7 +206,7 @@ static int process_path(const char *path) * First things first: get the stat information, to decide * what to do about the pathname! */ - if (lstat(path, &st) < 0) + if (fast_lstat(path, &st) < 0) return process_lstat_error(path, errno); if (S_ISDIR(st.st_mode)) diff --git a/check-racy.c b/check-racy.c index 00d92a1..6124355 100644 --- a/check-racy.c +++ b/check-racy.c @@ -11,7 +11,7 @@ int main(int ac, char **av) struct cache_entry *ce = active_cache[i]; struct stat st; - if (lstat(ce->name, &st)) { + if (fast_lstat(ce->name, &st)) { error("lstat(%s): %s", ce->name, strerror(errno)); continue; } diff --git a/compat/cygwin.c b/compat/cygwin.c index 91ce5d4..f07cbd3 100644 --- a/compat/cygwin.c +++ b/compat/cygwin.c @@ -6,18 +6,6 @@ #include "../git-compat-util.h" #include "../cache.h" /* to read configuration */ -/* - * Return POSIX permission bits, regardless of core.ignorecygwinfstricks - */ -int cygwin_get_st_mode_bits(const char *path, int *mode) -{ - struct stat st; - if (lstat(path, &st) < 0) - return -1; - *mode = st.st_mode; - return 0; -} - static inline void filetime_to_timespec(const FILETIME *ft, struct timespec *ts) { long long winTime = ((long long)ft->dwHighDateTime << 32) + diff --git a/compat/cygwin.h b/compat/cygwin.h index c04965a..8299f58 100644 --- a/compat/cygwin.h +++ b/compat/cygwin.h @@ -4,11 +4,7 @@ typedef int (*stat_fn_t)(const char*, struct stat*); extern stat_fn_t cygwin_stat_fn; extern stat_fn_t cygwin_lstat_fn; -int cygwin_get_st_mode_bits(const char *path, int *mode); -#define get_st_mode_bits(p,m) cygwin_get_st_mode_bits((p),(m)) -#ifndef CYGWIN_C -/* cygwin.c needs the original lstat() */ -#define stat(path, buf) (*cygwin_stat_fn)(path, buf) -#define lstat(path, buf) (*cygwin_lstat_fn)(path, buf) -#endif +#define fast_stat(path, buf) (*cygwin_stat_fn)(path, buf) +#define fast_lstat(path, buf) (*cygwin_lstat_fn)(path, buf) +#define GIT_FAST_STAT diff --git a/diff-lib.c b/diff-lib.c index b6f4b21..401dab6 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -27,7 +27,7 @@ */ static int check_removed(const struct cache_entry *ce, struct stat *st) { - if (lstat(ce->name, st) < 0) { + if (fast_lstat(ce->name, st) < 0) { if (errno != ENOENT && errno != ENOTDIR) return -1; return 1; diff --git a/diff.c b/diff.c index 208094f..212d3ff 100644 --- a/diff.c +++ b/diff.c @@ -2642,7 +2642,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int * If ce matches the file in the work tree, we can reuse it. */ if (ce_uptodate(ce) || - (!lstat(name, &st) && !ce_match_stat(ce, &st, 0))) + (!fast_lstat(name, &st) && !ce_match_stat(ce, &st, 0))) return 1; return 0; diff --git a/entry.c b/entry.c index d7c131d..4d2ac73 100644 --- a/entry.c +++ b/entry.c @@ -210,7 +210,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout finish: if (state->refresh_cache) { if (!fstat_done) - lstat(ce->name, &st); + fast_lstat(ce->name, &st); fill_stat_cache_info(ce, &st); } return 0; @@ -230,7 +230,7 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen) errno = ENOENT; return -1; } - return lstat(path, st); + return fast_lstat(path, st); } int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath) diff --git a/git-compat-util.h b/git-compat-util.h index ff193f4..7997bdb 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -129,8 +129,6 @@ #include #endif -extern int get_st_mode_bits(const char *path, int *mode); - #if defined(__MINGW32__) /* pull in Windows compatibility stuff */ #include "compat/mingw.h" @@ -179,6 +177,17 @@ typedef unsigned long uintptr_t; #endif #endif +#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 + /* used on Mac OS X */ #ifdef PRECOMPOSE_UNICODE #include "compat/precompose_utf8.h" diff --git a/help.c b/help.c index 08c54ef..f068925 100644 --- a/help.c +++ b/help.c @@ -107,10 +107,7 @@ static int is_executable(const char *name) !S_ISREG(st.st_mode)) return 0; -#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__) -#if defined(__CYGWIN__) -if ((st.st_mode & S_IXUSR) == 0) -#endif +#if defined(GIT_WINDOWS_NATIVE) { /* cannot trust the executable bit, peek into the file instead */ char buf[3] = { 0 }; int n; diff --git a/path.c b/path.c index 7f3324a..3d244d3 100644 --- a/path.c +++ b/path.c @@ -5,13 +5,7 @@ #include "strbuf.h" #include "string-list.h" -#ifndef get_st_mode_bits -/* - * The replacement lstat(2) we use on Cygwin is incomplete and - * may return wrong permission bits. Most of the time we do not care, - * but the callsites of this wrapper do care. - */ -int get_st_mode_bits(const char *path, int *mode) +static int get_st_mode_bits(const char *path, int *mode) { struct stat st; if (lstat(path, &st) < 0) @@ -19,7 +13,6 @@ int get_st_mode_bits(const char *path, int *mode) *mode = st.st_mode; return 0; } -#endif static char bad_path[] = "/bad-path/"; diff --git a/preload-index.c b/preload-index.c index 49cb08d..1bece91 100644 --- a/preload-index.c +++ b/preload-index.c @@ -57,7 +57,7 @@ static void *preload_thread(void *_data) continue; if (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce))) continue; - if (lstat(ce->name, &st)) + if (fast_lstat(ce->name, &st)) continue; if (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY)) continue; diff --git a/read-cache.c b/read-cache.c index d5201f9..ed33d9e 100644 --- a/read-cache.c +++ b/read-cache.c @@ -689,7 +689,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, int add_file_to_index(struct index_state *istate, const char *path, int flags) { struct stat st; - if (lstat(path, &st)) + if (fast_lstat(path, &st)) die_errno("unable to stat '%s'", path); return add_to_index(istate, path, &st, flags); } @@ -1049,7 +1049,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate, return ce; } - if (lstat(ce->name, &st) < 0) { + if (fast_lstat(ce->name, &st) < 0) { if (err) *err = errno; return NULL; @@ -1635,7 +1635,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce) */ struct stat st; - if (lstat(ce->name, &st) < 0) + if (fast_lstat(ce->name, &st) < 0) return; if (ce_match_stat_basic(ce, &st)) return; diff --git a/unpack-trees.c b/unpack-trees.c index b27f2a6..1fe9b63 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1215,7 +1215,7 @@ static int verify_uptodate_1(const struct cache_entry *ce, else if (o->reset || ce_uptodate(ce)) return 0; - if (!lstat(ce->name, &st)) { + if (!fast_lstat(ce->name, &st)) { int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE; unsigned changed = ie_match_stat(o->src_index, ce, &st, flags); if (!changed) @@ -1432,13 +1432,13 @@ static int verify_absent_1(const struct cache_entry *ce, char path[PATH_MAX + 1]; memcpy(path, ce->name, len); path[len] = 0; - if (lstat(path, &st)) + if (fast_lstat(path, &st)) return error("cannot stat '%s': %s", path, strerror(errno)); return check_ok_to_remove(path, len, DT_UNKNOWN, NULL, &st, error_type, o); - } else if (lstat(ce->name, &st)) { + } else if (fast_lstat(ce->name, &st)) { if (errno != ENOENT) return error("cannot stat '%s': %s", ce->name, strerror(errno)); @@ -1852,7 +1852,7 @@ int oneway_merge(const struct cache_entry * const *src, int update = 0; if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) { struct stat st; - if (lstat(old->name, &st) || + if (fast_lstat(old->name, &st) || ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE)) update |= CE_UPDATE; } -- 1.8.3