Re: [PATCH v3] Enable minimal stat checking

[Junio C Hamano]

Jeff King  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.
--
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: [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
--
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: [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
--
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: [PATCH v3] Enable minimal stat checking

[Junio C Hamano]

Robin Rosenberg  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 

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) {
--
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: [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;


--
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: [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
--
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

[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 
---
 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