[PATCH] Perform minimal stat comparison when some stat fields are not set

[PATCH] Perform minimal stat comparison when some stat fields are not set

[Robin Rosenberg]

At least JGit does sets uid, gid, ctime, ino and dev fields to zero
on update. To Git this looks like the stat data does not match and
a full file compare will be forced even it size and mtime match. This
is in practice unnecessary. Sense JGit's presence by checking if ino
and dev is zero.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 read-cache.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index fda78bc..6f13a22 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -197,21 +197,26 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	}
 	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
 		changed |= MTIME_CHANGED;
-	if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
+
+	int minimal_stat = (ce->ce_ino == 0 && ce->ce_dev == 0);
+
+	if (trust_ctime && !minimal_stat && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
 		changed |= CTIME_CHANGED;
 
 #ifdef USE_NSEC
 	if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
 		changed |= MTIME_CHANGED;
-	if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
+	if (trust_ctime && !minimal_stat && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
 		changed |= CTIME_CHANGED;
 #endif
 
-	if (ce->ce_uid != (unsigned int) st->st_uid ||
-	    ce->ce_gid != (unsigned int) st->st_gid)
-		changed |= OWNER_CHANGED;
-	if (ce->ce_ino != (unsigned int) st->st_ino)
-		changed |= INODE_CHANGED;
+	if (!minimal_stat) {
+		if (ce->ce_uid != (unsigned int) st->st_uid ||
+			ce->ce_gid != (unsigned int) st->st_gid)
+			changed |= OWNER_CHANGED;
+		if (ce->ce_ino != 0 && ce->ce_ino != (unsigned int) st->st_ino)
+			changed |= INODE_CHANGED;
+	}
 
 #ifdef USE_STDEV
 	/*
@@ -219,7 +224,7 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	 * clients will have different views of what "device"
 	 * the filesystem is on
 	 */
-	if (ce->ce_dev != (unsigned int) st->st_dev)
+	if (!minimal_stat && ce->ce_dev != (unsigned int) st->st_dev)
 		changed |= INODE_CHANGED;
 #endif
 
-- 
1.8.0.msysgit.0.dirty

Re: [PATCH] Perform minimal stat comparison when some stat fields are not set

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> At least JGit does sets uid, gid, ctime, ino and dev fields to zero
> on update. To Git this looks like the stat data does not match and
> a full file compare will be forced even it size and mtime match. This
> is in practice unnecessary. Sense JGit's presence by checking if ino
> and dev is zero.
>
> Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
> ---
>  read-cache.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index fda78bc..6f13a22 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -197,21 +197,26 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
>  	}
>  	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
>  		changed |= MTIME_CHANGED;
> -	if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> +
> +	int minimal_stat = (ce->ce_ino == 0 && ce->ce_dev == 0);

decl-after-stmt.

Besides, is it sane to do this unconditionally to affect people who
do not use JGit?

Re: [PATCH] Perform minimal stat comparison when some stat fields are not set

[Robin Rosenberg]

----- Ursprungligt meddelande -----
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> 
> > At least JGit does sets uid, gid, ctime, ino and dev fields to zero
> > on update. To Git this looks like the stat data does not match and
> > a full file compare will be forced even it size and mtime match.
> > This
> > is in practice unnecessary. Sense JGit's presence by checking if
> > ino
> > and dev is zero.
> >
> > Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
> > ---
> >  read-cache.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/read-cache.c b/read-cache.c
> > index fda78bc..6f13a22 100644
> > --- a/read-cache.c
> > +++ b/read-cache.c
> > @@ -197,21 +197,26 @@ static int ce_match_stat_basic(struct
> > cache_entry *ce, struct stat *st)
> >  	}
> >  	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
> >  		changed |= MTIME_CHANGED;
> > -	if (trust_ctime && ce->ce_ctime.sec != (unsigned
> > int)st->st_ctime)
> > +
> > +	int minimal_stat = (ce->ce_ino == 0 && ce->ce_dev == 0);
> 
> decl-after-stmt.

Ok, btw. Which C version do we adhere to? C99 is quite old by now.

> Besides, is it sane to do this unconditionally to affect people who
> do not use JGit?
> 

Would a config option like core.minstat be better? The name would imply no dynamic detection.

- robin

Re: [PATCH] Perform minimal stat comparison when some stat fields are not set

[Johannes Sixt]

Am 12/6/2012 2:09, schrieb Robin Rosenberg:
>> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
>>> At least JGit does sets uid, gid, ctime, ino and dev fields to zero
>>> on update. To Git this looks like the stat data does not match and
>>> a full file compare will be forced even it size and mtime match.
>>> This
>>> is in practice unnecessary. Sense JGit's presence by checking if
>>> ino
>>> and dev is zero.

Is this meant to better support C git and JGit working on the same repository?

MinGW git sets these two stat fields to zero as well. But we have less of
an interoparability problem between different git implementations in
practice on Windows, I think.

>> Besides, is it sane to do this unconditionally to affect people who
>> do not use JGit?
> 
> Would a config option like core.minstat be better? The name would imply no dynamic detection.

A configuration option is the way to go. We already have core.trustctime,
core.symlinks, core.filemode, core.ignoreCygwinFSTricks.

But your new mode is not "minimal". In some implementations or on some
filesystems, even more bits of stat information could be meaningless
(think of atime, rdev, nlink, uid, gid). Perhaps core.trustdevandino? Or
an enumeration core.ignoreCacheStat=ctime,dev,ino?

-- Hannes

Re: [PATCH] Perform minimal stat comparison when some stat fields are not set

[Robin Rosenberg]

----- Ursprungligt meddelande -----
> Am 12/6/2012 2:09, schrieb Robin Rosenberg:
> >> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> >>> At least JGit does sets uid, gid, ctime, ino and dev fields to
> >>> zero
> >>> on update. To Git this looks like the stat data does not match
> >>> and
> >>> a full file compare will be forced even it size and mtime match.
> >>> This
> >>> is in practice unnecessary. Sense JGit's presence by checking if
> >>> ino
> >>> and dev is zero.
> 
> Is this meant to better support C git and JGit working on the same
> repository?
> 
> MinGW git sets these two stat fields to zero as well. But we have
> less of
> an interoparability problem between different git implementations in
> practice on Windows, I think.

It is purely for performance in some situations.

> >> Besides, is it sane to do this unconditionally to affect people
> >> who
> >> do not use JGit?
> > 
> > Would a config option like core.minstat be better? The name would
> > imply no dynamic detection.
> 
> A configuration option is the way to go. We already have
> core.trustctime,
> core.symlinks, core.filemode, core.ignoreCygwinFSTricks.
> 
> But your new mode is not "minimal". In some implementations or on
> some
> filesystems, even more bits of stat information could be meaningless
> (think of atime, rdev, nlink, uid, gid). Perhaps core.trustdevandino?

I already excluded uid and gid so the only thing left is mtime and size.
I can't see any reason for anyone to look at atime (somebody read the file,
so what?), ok for rdev and nlink, but we don not look at them my patch
does not avoid looking at them.

> Or
> an enumeration core.ignoreCacheStat=ctime,dev,ino?

That would mean only one configuration option. Good.

-- robin

[PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Robin Rosenberg]

Specifically the fields uid, gid, ctime, ino and dev are set to zero
by JGit. Any stat checking by git will then need to check content,
which may be very slow, particularly on Windows. Since mtime and size
is typically enough we should allow the user to tell git to avoid
checking these fields if they are set to zero in the index.

This change introduces a core.ignorezerostat config option where the
user can list the fields to ignore using the names above.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 Documentation/config.txt |  9 +++++++++
 cache.h                  |  8 ++++++++
 config.c                 | 25 +++++++++++++++++++++++++
 environment.c            |  1 +
 read-cache.c             | 24 +++++++++++++++---------
 5 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5809e0..7f34c94 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -235,6 +235,15 @@ core.trustctime::
 	crawlers and some backup systems).
 	See linkgit:git-update-index[1]. True by default.
 
+core.ignorezerostat::
+	Affects the interpretation of some fields in the index. If
+	unset has no effect. When set to a comma separated list of fields,
+	each of the fields in the index will be excluded from comparison with
+	working tree if the index value is zero. The following fields
+	are recognzed: `uid', `gid', `ctime', `ino' and `dev'. When ctime is ignored
+	the setting of 'core.trustctime' is overridden by by this config
+	value.
+
 core.quotepath::
 	The commands that output paths (e.g. 'ls-files',
 	'diff'), when not given the `-z` option, will quote
diff --git a/cache.h b/cache.h
index c257953..524e49a 100644
--- a/cache.h
+++ b/cache.h
@@ -536,6 +536,14 @@ extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int trust_ctime;
+extern int check_nonzero_stat;
+#define CHECK_NONZERO_STAT_UID (1<<0)
+#define CHECK_NONZERO_STAT_GID (1<<1)
+#define CHECK_NONZERO_STAT_CTIME (1<<2)
+#define CHECK_NONZERO_STAT_INO (1<<3)
+#define CHECK_NONZERO_STAT_DEV (1<<4)
+#define CHECK_NONZERO_STAT_MASK ((1<<5)-1)
+
 extern int quote_path_fully;
 extern int has_symlinks;
 extern int minimum_abbrev, default_abbrev;
diff --git a/config.c b/config.c
index 7b444b6..79485cd 100644
--- a/config.c
+++ b/config.c
@@ -566,6 +566,31 @@ static int git_default_core_config(const char *var, const char *value)
 		trust_ctime = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "core.ignorezerostat")) {
+		char *copy, *tok;
+		git_config_string(&copy, "core.ignorezerostat", value);
+		check_nonzero_stat = CHECK_NONZERO_STAT_MASK;
+		tok = strtok(value, ",");
+		while (tok) {
+			if (strcasecmp(tok, "uid") == 0)
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_UID;
+			else if (strcasecmp(tok, "gid") == 0)
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_GID;
+			else if (strcasecmp(tok, "ctime") == 0) {
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_CTIME;
+				trust_ctime = 0;
+			} else if (strcasecmp(tok, "ino") == 0)
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_INO;
+			else if (strcasecmp(tok, "dev") == 0)
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_DEV;
+			else
+				die_bad_config(var);
+			tok = strtok(NULL, ",");
+		}
+		if (check_nonzero_stat >= CHECK_NONZERO_STAT_MASK)
+			die_bad_config(var);
+		free(copy);
+	}
 
 	if (!strcmp(var, "core.quotepath")) {
 		quote_path_fully = git_config_bool(var, value);
diff --git a/environment.c b/environment.c
index 85edd7f..e90b52f 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
+int check_nonzero_stat = CHECK_NONZERO_STAT_MASK;
 int has_symlinks = 1;
 int minimum_abbrev = 4, default_abbrev = 7;
 int ignore_case;
diff --git a/read-cache.c b/read-cache.c
index fda78bc..f7fe15d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -197,8 +197,9 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	}
 	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
 		changed |= MTIME_CHANGED;
-	if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
-		changed |= CTIME_CHANGED;
+	if ((trust_ctime || ((check_nonzero_stat&CHECK_NONZERO_STAT_CTIME) && ce->ce_ctime.sec)))
+		if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
+			changed |= CTIME_CHANGED;
 
 #ifdef USE_NSEC
 	if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
@@ -207,11 +208,15 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 		changed |= CTIME_CHANGED;
 #endif
 
-	if (ce->ce_uid != (unsigned int) st->st_uid ||
-	    ce->ce_gid != (unsigned int) st->st_gid)
-		changed |= OWNER_CHANGED;
-	if (ce->ce_ino != (unsigned int) st->st_ino)
-		changed |= INODE_CHANGED;
+	if ((check_nonzero_stat&CHECK_NONZERO_STAT_UID) || ce->ce_uid)
+		if (ce->ce_uid != (unsigned int) st->st_uid)
+			changed |= OWNER_CHANGED;
+	if ((check_nonzero_stat&CHECK_NONZERO_STAT_GID) || ce->ce_gid)
+		if (ce->ce_gid != (unsigned int) st->st_gid)
+			changed |= OWNER_CHANGED;
+	if ((check_nonzero_stat&CHECK_NONZERO_STAT_INO) || ce->ce_ino)
+		if (ce->ce_ino != (unsigned int) st->st_ino)
+			changed |= INODE_CHANGED;
 
 #ifdef USE_STDEV
 	/*
@@ -219,8 +224,9 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	 * clients will have different views of what "device"
 	 * the filesystem is on
 	 */
-	if (ce->ce_dev != (unsigned int) st->st_dev)
-		changed |= INODE_CHANGED;
+	if ((check_nonzero_stat&CHECK_NONZERO_STAT_DEV) || ce->ce_dev)
+		if (ce->ce_dev != (unsigned int) st->st_dev)
+			changed |= INODE_CHANGED;
 #endif
 
 	if (ce->ce_size != (unsigned int) st->st_size)
-- 
1.8.1.337.g63e8afb.dirty

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> diff --git a/read-cache.c b/read-cache.c
> index fda78bc..f7fe15d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -197,8 +197,9 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
>  	}
>  	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
>  		changed |= MTIME_CHANGED;
> -	if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> -		changed |= CTIME_CHANGED;
> +	if ((trust_ctime || ((check_nonzero_stat&CHECK_NONZERO_STAT_CTIME) && ce->ce_ctime.sec)))

One SP is required on each side of a binary operator; please have
one after check_nonzero_stat and after the & after it.

I wonder if we should lose the trust_ctime variable and use this
check_nonzero_stat bitset exclusively, provided that this were a
good direction to go?

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> @@ -566,6 +566,31 @@ static int git_default_core_config(const char *var, const char *value)
>  		trust_ctime = git_config_bool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp(var, "core.ignorezerostat")) {
> +		char *copy, *tok;
> +		git_config_string(&copy, "core.ignorezerostat", value);
> +		check_nonzero_stat = CHECK_NONZERO_STAT_MASK;
> +		tok = strtok(value, ",");
> +		while (tok) {
> +			if (strcasecmp(tok, "uid") == 0)
> +				check_nonzero_stat &= ~CHECK_NONZERO_STAT_UID;
> +			else if (strcasecmp(tok, "gid") == 0)
> +				check_nonzero_stat &= ~CHECK_NONZERO_STAT_GID;
> +			else if (strcasecmp(tok, "ctime") == 0) {
> +				check_nonzero_stat &= ~CHECK_NONZERO_STAT_CTIME;
> +				trust_ctime = 0;
> +			} else if (strcasecmp(tok, "ino") == 0)
> +				check_nonzero_stat &= ~CHECK_NONZERO_STAT_INO;
> +			else if (strcasecmp(tok, "dev") == 0)
> +				check_nonzero_stat &= ~CHECK_NONZERO_STAT_DEV;
> +			else
> +				die_bad_config(var);
> +			tok = strtok(NULL, ",");
> +		}
> +		if (check_nonzero_stat >= CHECK_NONZERO_STAT_MASK)
> +			die_bad_config(var);
> +		free(copy);
> +	}

Also I am getting these:

config.c: In function 'git_default_core_config':
config.c:571: error: passing argument 1 of 'git_config_string' from incompatible pointer type
config.c:540: note: expected 'const char **' but argument is of type 'char **'
config.c:573: error: passing argument 1 of 'strtok' discards qualifiers from pointer target type

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Robin Rosenberg]

----- Ursprungligt meddelande -----
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> 
> > diff --git a/read-cache.c b/read-cache.c
> > index fda78bc..f7fe15d 100644
> > --- a/read-cache.c
> > +++ b/read-cache.c
> > @@ -197,8 +197,9 @@ static int ce_match_stat_basic(struct
> > cache_entry *ce, struct stat *st)
> >  	}
> >  	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
> >  		changed |= MTIME_CHANGED;
> > -	if (trust_ctime && ce->ce_ctime.sec != (unsigned
> > int)st->st_ctime)
> > -		changed |= CTIME_CHANGED;
> > +	if ((trust_ctime ||
> > ((check_nonzero_stat&CHECK_NONZERO_STAT_CTIME) &&
> > ce->ce_ctime.sec)))
> 
> One SP is required on each side of a binary operator; please have
> one after check_nonzero_stat and after the & after it.
> 
> I wonder if we should lose the trust_ctime variable and use this
> check_nonzero_stat bitset exclusively, provided that this were a
> good direction to go?

Semantically they're somewhat different. My flags are for ignoring
a value when it's not used as indicated by the value zero, while
trustctime is for ignoring untrustworthy, non-zero, values.

>From 1ce4790bf5e:
    A new configuration variable 'core.trustctime' is introduced to
    allow ignoring st_ctime information when checking if paths
    in the working tree has changed, because there are situations where
    it produces too much false positives.  Like when file system crawlers
    keep changing it when scanning and using the ctime for marking scanned
    files.

(your second mail)
>Also I am getting these:
>
>config.c: In function 'git_default_core_config':
>config.c:571: error: passing argument 1 of 'git_config_string' from incompatible pointer type
>config.c:540: note: expected 'const char **' but argument is of type 'char **'
>config.c:573: error: passing argument 1 of 'strtok' discards qualifiers from pointer target type

Different compilers have different defaults. I'm on OS X (mountain lion), or am I missing
something? I do get a warning. Am I allowed to modify the value, like strtok does? Seems I
missed the opportunity to use the copy rather then the original value.

Another thing that I noticed, is that I probably wanto to be able to filter on the precision
of timestamps. Again, this i JGit-related. Current JGit has milliseconds precision (max), whereas
Git has down to nanosecond precision in timestamps. Newer JGits may get nanoseconds timestamps too,
but on current Linux versions JGit gets only integral seconds regardless of file system. 

Would the names, milli, micro, nano be good for ignoring the tail when zero, or n1..n9 (obviously
n2 would be ok too). nN = ignore all but first N nsec digits if they are zero)?

-- robin

[PATCH v3] Make git selectively and conditionally ignore certain stat fields

[Robin Rosenberg]

Specifically the fields uid, gid, ctime, ino and dev are set to zero
by JGit. Any stat checking by git will then need to check content,
which may be very slow, particularly on Windows. Since mtime and size
is typically enough we should allow the user to tell git to avoid
checking these fields if they are set to zero in the index.

This change introduces a core.ignorezerostat config option where the
user can list the fields to ignore using the names above.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 Documentation/config.txt |  9 +++++++++
 cache.h                  |  8 ++++++++
 config.c                 | 26 ++++++++++++++++++++++++++
 environment.c            |  1 +
 read-cache.c             | 29 ++++++++++++++++++-----------
 5 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5809e0..7f34c94 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -235,6 +235,15 @@ core.trustctime::
 	crawlers and some backup systems).
 	See linkgit:git-update-index[1]. True by default.
 
+core.ignorezerostat::
+	Affects the interpretation of some fields in the index. If
+	unset has no effect. When set to a comma separated list of fields,
+	each of the fields in the index will be excluded from comparison with
+	working tree if the index value is zero. The following fields
+	are recognzed: `uid', `gid', `ctime', `ino' and `dev'. When ctime is ignored
+	the setting of 'core.trustctime' is overridden by by this config
+	value.
+
 core.quotepath::
 	The commands that output paths (e.g. 'ls-files',
 	'diff'), when not given the `-z` option, will quote
diff --git a/cache.h b/cache.h
index c257953..524e49a 100644
--- a/cache.h
+++ b/cache.h
@@ -536,6 +536,14 @@ extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int trust_ctime;
+extern int check_nonzero_stat;
+#define CHECK_NONZERO_STAT_UID (1<<0)
+#define CHECK_NONZERO_STAT_GID (1<<1)
+#define CHECK_NONZERO_STAT_CTIME (1<<2)
+#define CHECK_NONZERO_STAT_INO (1<<3)
+#define CHECK_NONZERO_STAT_DEV (1<<4)
+#define CHECK_NONZERO_STAT_MASK ((1<<5)-1)
+
 extern int quote_path_fully;
 extern int has_symlinks;
 extern int minimum_abbrev, default_abbrev;
diff --git a/config.c b/config.c
index 7b444b6..6b617bc 100644
--- a/config.c
+++ b/config.c
@@ -566,6 +566,32 @@ static int git_default_core_config(const char *var, const char *value)
 		trust_ctime = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "core.ignorezerostat")) {
+		const char *copy;
+		const char *tok;
+		git_config_string(&copy, "core.ignorezerostat", value);
+		check_nonzero_stat = CHECK_NONZERO_STAT_MASK;
+		tok = strtok((char*)copy, ",");
+		while (tok) {
+			if (strcasecmp(tok, "uid") == 0)
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_UID;
+			else if (strcasecmp(tok, "gid") == 0)
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_GID;
+			else if (strcasecmp(tok, "ctime") == 0) {
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_CTIME;
+				trust_ctime = 0;
+			} else if (strcasecmp(tok, "ino") == 0)
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_INO;
+			else if (strcasecmp(tok, "dev") == 0)
+				check_nonzero_stat &= ~CHECK_NONZERO_STAT_DEV;
+			else
+				die_bad_config(var);
+			tok = strtok(NULL, ",");
+		}
+		if (check_nonzero_stat >= CHECK_NONZERO_STAT_MASK)
+			die_bad_config(var);
+		free((char*)copy);
+	}
 
 	if (!strcmp(var, "core.quotepath")) {
 		quote_path_fully = git_config_bool(var, value);
diff --git a/environment.c b/environment.c
index 85edd7f..e90b52f 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
+int check_nonzero_stat = CHECK_NONZERO_STAT_MASK;
 int has_symlinks = 1;
 int minimum_abbrev = 4, default_abbrev = 7;
 int ignore_case;
diff --git a/read-cache.c b/read-cache.c
index fda78bc..c4226ee 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -197,21 +197,27 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	}
 	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
 		changed |= MTIME_CHANGED;
-	if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
-		changed |= CTIME_CHANGED;
+	if ((trust_ctime || ((check_nonzero_stat & CHECK_NONZERO_STAT_CTIME) && ce->ce_ctime.sec)))
+		if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
+			changed |= CTIME_CHANGED;
 
 #ifdef USE_NSEC
 	if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
 		changed |= MTIME_CHANGED;
-	if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
-		changed |= CTIME_CHANGED;
+	if ((trust_ctime || ((check_nonzero_stat & CHECK_NONZERO_STAT_CTIME) && ce->ce_ctime.nsec))
+		if (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
+			changed |= CTIME_CHANGED;
 #endif
 
-	if (ce->ce_uid != (unsigned int) st->st_uid ||
-	    ce->ce_gid != (unsigned int) st->st_gid)
-		changed |= OWNER_CHANGED;
-	if (ce->ce_ino != (unsigned int) st->st_ino)
-		changed |= INODE_CHANGED;
+	if ((check_nonzero_stat & CHECK_NONZERO_STAT_UID) || ce->ce_uid)
+		if (ce->ce_uid != (unsigned int) st->st_uid)
+			changed |= OWNER_CHANGED;
+	if ((check_nonzero_stat & CHECK_NONZERO_STAT_GID) || ce->ce_gid)
+		if (ce->ce_gid != (unsigned int) st->st_gid)
+			changed |= OWNER_CHANGED;
+	if ((check_nonzero_stat & CHECK_NONZERO_STAT_INO) || ce->ce_ino)
+		if (ce->ce_ino != (unsigned int) st->st_ino)
+			changed |= INODE_CHANGED;
 
 #ifdef USE_STDEV
 	/*
@@ -219,8 +225,9 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	 * clients will have different views of what "device"
 	 * the filesystem is on
 	 */
-	if (ce->ce_dev != (unsigned int) st->st_dev)
-		changed |= INODE_CHANGED;
+	if ((check_nonzero_stat & CHECK_NONZERO_STAT_DEV) || ce->ce_dev)
+		if (ce->ce_dev != (unsigned int) st->st_dev)
+			changed |= INODE_CHANGED;
 #endif
 
 	if (ce->ce_size != (unsigned int) st->st_size)
-- 
1.8.1.337.gc903ef9.dirty

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> Semantically they're somewhat different. My flags are for ignoring
> a value when it's not used as indicated by the value zero, while
> trustctime is for ignoring untrustworthy, non-zero, values.

Yeah, I realized that after writing that message.

> Another thing that I noticed, is that I probably wanto to be able to filter on the precision
> of timestamps. Again, this i JGit-related. Current JGit has milliseconds precision (max), whereas
> Git has down to nanosecond precision in timestamps. Newer JGits may get nanoseconds timestamps too,
> but on current Linux versions JGit gets only integral seconds regardless of file system. 
>
> Would the names, milli, micro, nano be good for ignoring the tail when zero, or n1..n9 (obviously
> n2 would be ok too). nN = ignore all but first N nsec digits if they are zero)?

It somehow starts to sound like over-engineering to solve a wrong
problem.

I'd say a simplistic "ignore if zero is stored" or even "ignore this
as one of the systems that shares this file writes crap in it" may
be sufficient, and if this is a jGit specific issue, it might even
make sense to introduce a single configuration variable with string
"jgit" somewhere in its name and bypass the stat field comparison
for known-problematic fields, instead of having the user know and
list what stat fields need special attention.

Is this "the user edits in eclipse and then runs 'git status' from the
terminal" problem?

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Robin Rosenberg]

> Is this "the user edits in eclipse and then runs 'git status' from
> the
> terminal" problem?

Yes. Of course not just status, but any command that validates
the index. On Unix this is usually bearable, though slow, but on
Windows I often see git status take minutes (yes large files...).

-- robin

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Johannes Sixt]

Am 1/15/2013 1:11, schrieb Junio C Hamano:
> I'd say a simplistic "ignore if zero is stored" or even "ignore this
> as one of the systems that shares this file writes crap in it" may
> be sufficient, and if this is a jGit specific issue, it might even
> make sense to introduce a single configuration variable with string
> "jgit" somewhere in its name and bypass the stat field comparison
> for known-problematic fields, instead of having the user know and
> list what stat fields need special attention.

It was my suggestion to have a list of names to ignore because the answer
to this question

> Is this "the user edits in eclipse and then runs 'git status' from the
> terminal" problem?

was "It is purely for performance in some situations" back then. But
today, the answer is "Yes". With this new background, your suggestion to
have just a single option that contains the token "jgit" may make more
sense. (core.ignoreCygwinFSTricks may serve as a precedent.) The original
patch was along this way, and the name contained "minimal", which I
objected to.

-- Hannes

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Robin Rosenberg]

----- Ursprungligt meddelande -----
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> 
> > Semantically they're somewhat different. My flags are for ignoring
> > a value when it's not used as indicated by the value zero, while
> > trustctime is for ignoring untrustworthy, non-zero, values.
> 
> Yeah, I realized that after writing that message.
> 
> > Another thing that I noticed, is that I probably wanto to be able
> > to filter on the precision
> > of timestamps. Again, this i JGit-related. Current JGit has
> > milliseconds precision (max), whereas
> > Git has down to nanosecond precision in timestamps. Newer JGits may
> > get nanoseconds timestamps too,
> > but on current Linux versions JGit gets only integral seconds
> > regardless of file system.
> >
> > Would the names, milli, micro, nano be good for ignoring the tail
> > when zero, or n1..n9 (obviously
> > n2 would be ok too). nN = ignore all but first N nsec digits if
> > they are zero)?
> 
> It somehow starts to sound like over-engineering to solve a wrong
> problem.
> 
> I'd say a simplistic "ignore if zero is stored" or even "ignore this
> as one of the systems that shares this file writes crap in it" may
> be sufficient, and if this is a jGit specific issue, it might even
> make sense to introduce a single configuration variable with string
> "jgit" somewhere in its name and bypass the stat field comparison
> for known-problematic fields, instead of having the user know and
> list what stat fields need special attention.

My first patch was something like that, just not using the word jgit. As
for what fields to ignore, it's something that can be configured by EGit
and documented on the EGit/JGit wiki. 

-- robin

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

>> I'd say a simplistic "ignore if zero is stored" or even "ignore this
>> as one of the systems that shares this file writes crap in it" may
>> be sufficient, and if this is a jGit specific issue, it might even
>> make sense to introduce a single configuration variable with string
>> "jgit" somewhere in its name and bypass the stat field comparison
>> for known-problematic fields, instead of having the user know and
>> list what stat fields need special attention.
>
> My first patch was something like that, just not using the word jgit. As
> for what fields to ignore, it's something that can be configured by EGit
> and documented on the EGit/JGit wiki. 

That configurability is a slipperly slope to drag us into giving users
more complexity that does not help them very much, I suspect.

Earlier somebody mentioned "size and mtime is often enough", so I
think a single option core.looseStatInfo (substitute "loose" with
short, minimum or whatever adjective that is more appropriate---I am
not good at picking phrases, it sounds to me a way to more loosely
define stat info cleanliness than we usually do) that makes us
ignore all fields (regardless of their zero-ness) other than those
two fields might not be a bad way to go.

I do not offhand know if such a loose mode is too simple and make it
excessively risky, though.

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Ramsay Jones]

Junio C Hamano wrote:
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> That configurability is a slipperly slope to drag us into giving users
> more complexity that does not help them very much, I suspect.
> 
> Earlier somebody mentioned "size and mtime is often enough", so I
> think a single option core.looseStatInfo (substitute "loose" with
> short, minimum or whatever adjective that is more appropriate---I am
> not good at picking phrases, it sounds to me a way to more loosely
> define stat info cleanliness than we usually do) that makes us
> ignore all fields (regardless of their zero-ness) other than those
> two fields might not be a bad way to go.

At one point, I used to build (and test) the MSVC version of git on
cygwin, which leads to exactly the same problem. So, this is not just
an EGit/JGit vs c-git issue, although there can't be many people that
will have this problem. (Mixing the MinGW and cygwin versions on the
same repo will also have this problem).

I had a patch which, essentially, did what you suggest above; ie ignore
everything other than size and mtime, *including* ignoring the zero-ness
in the index. (I just don't understand why you would think of doing
otherwise!! ;-) ). As part of that patch, I also suppressed the "empty diff"
output that used to be shown for stat-dirty files (that's been fixed now
right?), otherwise using gitk was a pain.

[BTW, given the "schizophrenic stat" functions on cygwin, you can have
this problem with the cygwin version of git - all on it's lonesome!]

I can't help with naming, BTW, since I called the config variable
"core.ramsay-stat". :-P

> 
> I do not offhand know if such a loose mode is too simple and make it
> excessively risky, though.

I suspect it would be fine ... *however*, I never sent my patch because
I didn't think there would be many idiots^H^H^H^H^H^H pioneers like me! :-D

ATB,
Ramsay Jones

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Robin Rosenberg]

----- Ursprungligt meddelande -----

> That configurability is a slipperly slope to drag us into giving
> users
> more complexity that does not help them very much, I suspect.
> 
> Earlier somebody mentioned "size and mtime is often enough", so I
> think a single option core.looseStatInfo (substitute "loose" with
> short, minimum or whatever adjective that is more appropriate---I am
> not good at picking phrases, it sounds to me a way to more loosely
> define stat info cleanliness than we usually do) that makes us
> ignore all fields (regardless of their zero-ness) other than those
> two fields might not be a bad way to go.

Would something like this be good?

core.statinfo = 
default = all fields
minimal = whole seconds of mtime and size
medium = seconds, nanos of mtime and size
nonzero = all non-zero fields

-- robin

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> ----- Ursprungligt meddelande -----
>
>> That configurability is a slipperly slope to drag us into giving
>> users
>> more complexity that does not help them very much, I suspect.
>> 
>> Earlier somebody mentioned "size and mtime is often enough", so I
>> think a single option core.looseStatInfo (substitute "loose" with
>> short, minimum or whatever adjective that is more appropriate---I am
>> not good at picking phrases, it sounds to me a way to more loosely
>> define stat info cleanliness than we usually do) that makes us
>> ignore all fields (regardless of their zero-ness) other than those
>> two fields might not be a bad way to go.
>
> Would something like this be good?
>
> core.statinfo = 
> default = all fields
> minimal = whole seconds of mtime and size
> medium = seconds, nanos of mtime and size
> nonzero = all non-zero fields
>
> -- robin

If you mean to exclude ctime and other fields we already exclude as
useless from your "all", that may make sense, but do we really need
that much "flexibility", or do "more choices" just confuse users? I
have this suspicion that it may be the latter.

Wouldn't a single boolean that lets users choose between your
"minimal" and "default" be sufficient?

[PATCH v3] Enable minimal stat checking

[Robin Rosenberg]

Specifically the fields uid, gid, ctime, ino and dev are set to zero
by JGit. Other implementations, eg. Git in cygwin are allegedly also
somewhat incompatible with Git For Windows and on *nix platforms
the resolution of the timestamps may differ.

Any stat checking by git will then need to check content, which may
be very slow, particularly on Windows. Since mtime and size
is typically enough we should allow the user to tell git to avoid
checking these fields if they are set to zero in the index.

This change introduces a core.checkstat config option where the
the user can select to check all fields (default), or just size
and the whole second part of mtime (minimal).

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 Documentation/config.txt |  6 ++++++
 cache.h                  |  1 +
 config.c                 |  8 ++++++++
 environment.c            |  1 +
 read-cache.c             | 28 ++++++++++++++++------------
 5 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5809e0..47c213d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -235,6 +235,12 @@ core.trustctime::
 	crawlers and some backup systems).
 	See linkgit:git-update-index[1]. True by default.
 
+core.checkstat::
+	Determines which stat fields to match between the index
+	and work tree. The user can set this to 'default' or
+	'minimal'. Default (or explicitly 'default'), is to check
+	all fields, including the sub-second part of mtime and ctime.
+
 core.quotepath::
 	The commands that output paths (e.g. 'ls-files',
 	'diff'), when not given the `-z` option, will quote
diff --git a/cache.h b/cache.h
index c257953..ab20c4d 100644
--- a/cache.h
+++ b/cache.h
@@ -536,6 +536,7 @@ extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int trust_ctime;
+extern int check_stat;
 extern int quote_path_fully;
 extern int has_symlinks;
 extern int minimum_abbrev, default_abbrev;
diff --git a/config.c b/config.c
index 7b444b6..2b58c75 100644
--- a/config.c
+++ b/config.c
@@ -566,6 +566,14 @@ static int git_default_core_config(const char *var, const char *value)
 		trust_ctime = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "core.statinfo")) {
+		if (!strcasecmp(value, "default"))
+			check_stat = 1;
+		else if (!strcasecmp(value, "minimal"))
+			check_stat = 0;
+		else
+			die_bad_config(var);
+	}
 
 	if (!strcmp(var, "core.quotepath")) {
 		quote_path_fully = git_config_bool(var, value);
diff --git a/environment.c b/environment.c
index 85edd7f..e828b37 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@
 
 int trust_executable_bit = 1;
 int trust_ctime = 1;
+int check_stat = 1;
 int has_symlinks = 1;
 int minimum_abbrev = 4, default_abbrev = 7;
 int ignore_case;
diff --git a/read-cache.c b/read-cache.c
index fda78bc..23db681 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -197,21 +197,25 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	}
 	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
 		changed |= MTIME_CHANGED;
-	if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
-		changed |= CTIME_CHANGED;
+	if (trust_ctime ? check_stat : trust_ctime/*false*/)
+		if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
+			changed |= CTIME_CHANGED;
 
 #ifdef USE_NSEC
-	if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
+	if (check_stat && ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
 		changed |= MTIME_CHANGED;
-	if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
-		changed |= CTIME_CHANGED;
+	if (trust_ctime ? check_stat : trust_ctime/*false*/)
+		if (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
+			changed |= CTIME_CHANGED;
 #endif
 
-	if (ce->ce_uid != (unsigned int) st->st_uid ||
-	    ce->ce_gid != (unsigned int) st->st_gid)
-		changed |= OWNER_CHANGED;
-	if (ce->ce_ino != (unsigned int) st->st_ino)
-		changed |= INODE_CHANGED;
+	if (check_stat) {
+		if (ce->ce_uid != (unsigned int) st->st_uid ||
+			ce->ce_gid != (unsigned int) st->st_gid)
+			changed |= OWNER_CHANGED;
+		if (ce->ce_ino != (unsigned int) st->st_ino)
+			changed |= INODE_CHANGED;
+	}
 
 #ifdef USE_STDEV
 	/*
@@ -219,8 +223,8 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	 * clients will have different views of what "device"
 	 * the filesystem is on
 	 */
-	if (ce->ce_dev != (unsigned int) st->st_dev)
-		changed |= INODE_CHANGED;
+	if (check_stat && ce->ce_dev != (unsigned int) st->st_dev)
+			changed |= INODE_CHANGED;
 #endif
 
 	if (ce->ce_size != (unsigned int) st->st_size)
-- 
1.8.1.337.g6672977.dirty

Re: [PATCH v3] Enable minimal stat checking

[Johannes Sixt]

Am 1/22/2013 8:49, schrieb Robin Rosenberg:
> Specifically the fields uid, gid, ctime, ino and dev are set to zero
> by JGit. Other implementations, eg. Git in cygwin are allegedly also
> somewhat incompatible with Git For Windows and on *nix platforms
> the resolution of the timestamps may differ.
> 
> Any stat checking by git will then need to check content, which may
> be very slow, particularly on Windows. Since mtime and size
> is typically enough we should allow the user to tell git to avoid
> checking these fields if they are set to zero in the index.

Isn't this paragraph about slowness in the commit message misleading, as
what the patch does has no influence on the speed of stat checking? Am I
missing something?

> This change introduces a core.checkstat config option where the
> the user can select to check all fields (default), or just size
> and the whole second part of mtime (minimal).

> +core.checkstat::
> +	Determines which stat fields to match between the index
> +	and work tree. The user can set this to 'default' or
> +	'minimal'. Default (or explicitly 'default'), is to check
> +	all fields, including the sub-second part of mtime and ctime.

I think this needs some more clarification, less 1337 speak, as well as a
hint when to set the option.

	Determines which file attributes are checked to detect whether
	a file has been modified. Set this option to 'minimal', when...,
	which checks only the file size and whole-seconds of the last
	modification time. Otherwise, leave unset or set to the value
	'default'.

By starting with the hint when to set to 'minimal' in this way allows us
to omit a specification what the 'default' is.

> diff --git a/read-cache.c b/read-cache.c
> index fda78bc..23db681 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -197,21 +197,25 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
>  	}
>  	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
>  		changed |= MTIME_CHANGED;
> -	if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> -		changed |= CTIME_CHANGED;
> +	if (trust_ctime ? check_stat : trust_ctime/*false*/)
> +		if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> +			changed |= CTIME_CHANGED;

It took me a while to understand why you write /*false*/ there. Isn't the
the condition merely this:

	if (trust_ctime && check_stat &&
	    ce->ce_ctime.sec != (unsigned int)st->st_ctime)
		changed |= CTIME_CHANGED;

>  
>  #ifdef USE_NSEC
> -	if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
> +	if (check_stat && ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
>  		changed |= MTIME_CHANGED;
> -	if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
> -		changed |= CTIME_CHANGED;
> +	if (trust_ctime ? check_stat : trust_ctime/*false*/)
> +		if (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
> +			changed |= CTIME_CHANGED;

Same here.

>  #endif

-- Hannes

Re: [PATCH v3] Enable minimal stat checking

[Torsten Bögershausen]

+core.checkstat::
+	Determines which stat fields to match between the index
+	and work tree. The user can set this to 'default' or
+	'minimal'. Default (or explicitly 'default'), is to check
+	all fields, including the sub-second part of mtime and ctime.
+
Setting 'minimal' implies core.trustctime = false

[snip]

> -	if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> -		changed |= CTIME_CHANGED;
> +	if (trust_ctime ? check_stat : trust_ctime/*false*/)
> +		if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> +			changed |= CTIME_CHANGED;

Could that be written as:
+	if (trust_ctime && check_stat && (ce->ce_ctime.sec != (unsigned int)st->st_ctime))
+			changed |= CTIME_CHANGED;


>  
>  #ifdef USE_NSEC
> -	if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
> +	if (check_stat && ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
>  		changed |= MTIME_CHANGED;
> -	if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
> -		changed |= CTIME_CHANGED;
> +	if (trust_ctime ? check_stat : trust_ctime/*false*/)
> +		if (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
> +			changed |= CTIME_CHANGED;

And here:
+	if (trust_ctime && check_stat && (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
+		changed |= CTIME_CHANGED;

Re: [PATCH v3] Enable minimal stat checking

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> Specifically the fields uid, gid, ctime, ino and dev are set to zero
> by JGit. Other implementations, eg. Git in cygwin are allegedly also
> somewhat incompatible with Git For Windows and on *nix platforms
> the resolution of the timestamps may differ.
>
> Any stat checking by git will then need to check content, which may
> be very slow, particularly on Windows. Since mtime and size
> is typically enough we should allow the user to tell git to avoid
> checking these fields if they are set to zero in the index.
>
> This change introduces a core.checkstat config option where the
> the user can select to check all fields (default), or just size
> and the whole second part of mtime (minimal).
>
> Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>

The "trust_ctime ? check_stat : trust_ctime/*false*/" gave me the
same "Huh?" as it did to J6t, so I locally fixed them while
applying.

Also, even though we settled on "default/minimal", we may regret in
the future if old implementations died on an unrecognized value, as
that will forbid users from using an old Git and a new Git on the
same repository at the same time, so I'd suggest removing the "if
not default or minimal, die" and replacing it with "treat unknown
token as a do-no-harm no-op".

Interdiff would look like this.

Thanks.

 config.c     |  2 --
 read-cache.c | 12 ++++++------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/config.c b/config.c
index 2b58c75..3f638e3 100644
--- a/config.c
+++ b/config.c
@@ -571,8 +571,6 @@ static int git_default_core_config(const char *var, const char *value)
 			check_stat = 1;
 		else if (!strcasecmp(value, "minimal"))
 			check_stat = 0;
-		else
-			die_bad_config(var);
 	}
 
 	if (!strcmp(var, "core.quotepath")) {
diff --git a/read-cache.c b/read-cache.c
index 23db681..827ae55 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -197,16 +197,16 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	}
 	if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
 		changed |= MTIME_CHANGED;
-	if (trust_ctime ? check_stat : trust_ctime/*false*/)
-		if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
-			changed |= CTIME_CHANGED;
+	if (trust_ctime && check_stat &&
+	    ce->ce_ctime.sec != (unsigned int)st->st_ctime)
+		changed |= CTIME_CHANGED;
 
 #ifdef USE_NSEC
 	if (check_stat && ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
 		changed |= MTIME_CHANGED;
-	if (trust_ctime ? check_stat : trust_ctime/*false*/)
-		if (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
-			changed |= CTIME_CHANGED;
+	if (trust_ctime && check_stat &&
+	    ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
+		changed |= CTIME_CHANGED;
 #endif
 
 	if (check_stat) {

Re: [PATCH v3] Enable minimal stat checking

[Robin Rosenberg]

----- Ursprungligt meddelande -----

> Also, even though we settled on "default/minimal", we may regret in
> the future if old implementations died on an unrecognized value, as
> that will forbid users from using an old Git and a new Git on the
> same repository at the same time, so I'd suggest removing the "if
> not default or minimal, die" and replacing it with "treat unknown
> token as a do-no-harm no-op".

I decided on error after looking at how other configuration errors
are handled, but I can change, though I personally prefer to get
configuration mistakes thrown in my face so I know.

-- robin

Re: [PATCH v3] Enable minimal stat checking

[Jeff King]

On Tue, Jan 22, 2013 at 08:49:22AM +0100, Robin Rosenberg wrote:

> Specifically the fields uid, gid, ctime, ino and dev are set to zero
> by JGit. Other implementations, eg. Git in cygwin are allegedly also
> somewhat incompatible with Git For Windows and on *nix platforms
> the resolution of the timestamps may differ.

This is an old commit, but I noticed a bug today...

> This change introduces a core.checkstat config option where the
> [...]
> +core.checkstat::
> [...]
> +	if (!strcmp(var, "core.statinfo")) {

One of these is not like the others. I didn't prepare a patch, though,
because I wasn't sure which it was supposed to be. A documentation bug
or a code bug? :)

-Peff

Re: [PATCH v3] Enable minimal stat checking

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jan 22, 2013 at 08:49:22AM +0100, Robin Rosenberg wrote:
>
>> Specifically the fields uid, gid, ctime, ino and dev are set to zero
>> by JGit. Other implementations, eg. Git in cygwin are allegedly also
>> somewhat incompatible with Git For Windows and on *nix platforms
>> the resolution of the timestamps may differ.
>
> This is an old commit, but I noticed a bug today...
>
>> This change introduces a core.checkstat config option where the
>> [...]
>> +core.checkstat::
>> [...]
>> +	if (!strcmp(var, "core.statinfo")) {
>
> One of these is not like the others. I didn't prepare a patch, though,
> because I wasn't sure which it was supposed to be. A documentation bug
> or a code bug? :)

I'd say checkstat reads much better than statinfo.

[PATCH] deprecate core.statinfo at Git 2.0 boundary

[Junio C Hamano]

c08e4d5b5cfa (Enable minimal stat checking, 2013-01-22) advertised
the configuration variable core.checkstat in the documentation and
its log message, but the code expected core.statinfo instead.

For now, add core.checkstat, and warn people who have core.statinfo
in their configuration file that we will remove it in Git 2.0.

Noticed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index aefd80b..7c55d05 100644
--- a/config.c
+++ b/config.c
@@ -566,7 +566,20 @@ static int git_default_core_config(const char *var, const char *value)
 		trust_ctime = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp(var, "core.statinfo")) {
+	if (!strcmp(var, "core.statinfo") ||
+	    !strcmp(var, "core.checkstat")) {
+		/*
+		 * NEEDSWORK: statinfo was a typo in v1.8.2 that has
+		 * never been advertised.  we will remove it at Git
+		 * 2.0 boundary.
+		 */
+		if (!strcmp(var, "core.statinfo")) {
+			static int warned;
+			if (!warned++) {
+				warning("'core.statinfo' will be removed in Git 2.0; "
+					"use 'core.checkstat' instead.");
+			}
+		}
 		if (!strcasecmp(value, "default"))
 			check_stat = 1;
 		else if (!strcasecmp(value, "minimal"))

Re: [PATCH] deprecate core.statinfo at Git 2.0 boundary

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> For now, add core.checkstat, and warn people who have core.statinfo
> in their configuration file that we will remove it in Git 2.0.

And an obvious follow-up for the 2.0 looks like this.

-- >8 --
Subject: [PATCH] core.statinfo: remove as promised in Git 2.0

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/config.c b/config.c
index 7c55d05..1f2cc90 100644
--- a/config.c
+++ b/config.c
@@ -566,20 +566,7 @@ static int git_default_core_config(const char *var, const char *value)
 		trust_ctime = git_config_bool(var, value);
 		return 0;
 	}
-	if (!strcmp(var, "core.statinfo") ||
-	    !strcmp(var, "core.checkstat")) {
-		/*
-		 * NEEDSWORK: statinfo was a typo in v1.8.2 that has
-		 * never been advertised.  we will remove it at Git
-		 * 2.0 boundary.
-		 */
-		if (!strcmp(var, "core.statinfo")) {
-			static int warned;
-			if (!warned++) {
-				warning("'core.statinfo' will be removed in Git 2.0; "
-					"use 'core.checkstat' instead.");
-			}
-		}
+	if (!strcmp(var, "core.checkstat")) {
 		if (!strcasecmp(value, "default"))
 			check_stat = 1;
 		else if (!strcasecmp(value, "minimal"))
-- 
1.8.3-rc1-154-g10dfae1

Re: [PATCH] deprecate core.statinfo at Git 2.0 boundary

[Jeff King]

On Mon, May 06, 2013 at 10:31:10PM -0700, Junio C Hamano wrote:

> c08e4d5b5cfa (Enable minimal stat checking, 2013-01-22) advertised
> the configuration variable core.checkstat in the documentation and
> its log message, but the code expected core.statinfo instead.
> 
> For now, add core.checkstat, and warn people who have core.statinfo
> in their configuration file that we will remove it in Git 2.0.

Yeah, that looks like a fine solution to me.

To be honest, I doubt that it is even necessary to handle the backwards
compatibility. The checkstat option never actually worked, statinfo was
never advertised, and the broken state was available in only one
release. So I'd be very surprised if anyone more than the author was
actually using it.

Still, it is not that hard to handle both, so I suppose it is better to
be conservative.

-Peff

Re: [PATCH] deprecate core.statinfo at Git 2.0 boundary

[Robin Rosenberg]

This looks ok with me, though I can manage without backward compatibility.

-- robin