Re: [PATCH] Make use of stat.ctime configurable

Alex Riesen [off-list ref] writes:

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.

This justification is good and I am very inclined to advocate for its
inclusion in 1.6.0, but any new configuration needs to be in the
documentation.

It appears there is "gui.trustmtime"; shouldn't this be called
"core.trustctime" or something?

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.

The default is to allow use of ctime.

Signed-off-by: Alex Riesen <redacted>
---
Junio C Hamano, Sun, Jul 27, 2008 21:46:42 +0200:

Alex Riesen [off-list ref] writes:

quoted

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.

This justification is good and I am very inclined to advocate for its
inclusion in 1.6.0, but any new configuration needs to be in the
documentation.

Done.

It appears there is "gui.trustmtime"; shouldn't this be called
"core.trustctime" or something?

Getting old... I even called the global flag trust_file_ctime!
Corrected. Changed trust_file_ctime to trust_ctime.

 Documentation/config.txt           |    7 +++++++
 Documentation/git-update-index.txt |    5 +++++
 cache.h                            |    1 +
 config.c                           |    4 ++++
 environment.c                      |    1 +
 read-cache.c                       |    2 +-
 6 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1a13abc..552c134 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -149,6 +149,13 @@ core.safecrlf::
 	`core.autocrlf`, git will reject the file.  The variable can
 	be set to "warn", in which case git will only warn about an
 	irreversible conversion but continue the operation.
+
+core.trustctime::
+	If false, the ctime differences between the index and the
+	working copy are ignored; useful when the inode change time
+	is regularly modified by something outside Git (file system
+	crawlers and some backup systems).
+	See linkgit:git-update-index[1]. True by default.
 +
 CRLF conversion bears a slight chance of corrupting data.
 autocrlf=true will convert CRLF to LF during commit and LF to

diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index 6b930bc..1d9d81a 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt

@@ -323,6 +323,11 @@ from symbolic link to regular file.
 The command looks at `core.ignorestat` configuration variable.  See
 'Using "assume unchanged" bit' section above.
 
+The command also looks at `core.trustctime` configuration variable.
+It can be useful when the inode change time is regularly modified by
+something outside Git (file system crawlers and backup systems use
+ctime for marking files processed) (see linkgit:git-config[1]).
+
 
 SEE ALSO
 --------

diff --git a/cache.h b/cache.h
index 4b6c0a6..2475de9 100644
--- a/cache.h
+++ b/cache.h

@@ -423,6 +423,7 @@ extern int delete_ref(const char *, const unsigned char *sha1);
 
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
+extern int trust_ctime;
 extern int quote_path_fully;
 extern int has_symlinks;
 extern int ignore_case;

diff --git a/config.c b/config.c
index 1e066c7..53f04a0 100644
--- a/config.c
+++ b/config.c

@@ -341,6 +341,10 @@ static int git_default_core_config(const char *var, const char *value)
 		trust_executable_bit = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "core.trustctime")) {
+		trust_ctime = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (!strcmp(var, "core.quotepath")) {
 		quote_path_fully = git_config_bool(var, value);

diff --git a/environment.c b/environment.c
index 4a88a17..0c6d11f 100644
--- a/environment.c
+++ b/environment.c

@@ -13,6 +13,7 @@ char git_default_email[MAX_GITNAME];
 char git_default_name[MAX_GITNAME];
 int user_ident_explicitly_given;
 int trust_executable_bit = 1;
+int trust_ctime = 1;
 int has_symlinks = 1;
 int ignore_case;
 int assume_unchanged;

diff --git a/read-cache.c b/read-cache.c
index 6c08803..1cae361 100644
--- a/read-cache.c
+++ b/read-cache.c

@@ -197,7 +197,7 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	}
 	if (ce->ce_mtime != (unsigned int) st->st_mtime)
 		changed |= MTIME_CHANGED;
-	if (ce->ce_ctime != (unsigned int) st->st_ctime)
+	if (trust_ctime && ce->ce_ctime != (unsigned int) st->st_ctime)
 		changed |= CTIME_CHANGED;
 
 	if (ce->ce_uid != (unsigned int) st->st_uid ||

-- 
1.6.0.rc0.76.g581e

On Mon, Jul 28, 2008 at 08:31:28AM +0200, Alex Riesen wrote:

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.

That's interesting, since most backup software uses the ctime to determine
file changes.

David

On Mon, 28 Jul 2008, David Brown wrote:

On Mon, Jul 28, 2008 at 08:31:28AM +0200, Alex Riesen wrote:

quoted

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.

That's interesting, since most backup software uses the ctime to determine
file changes.

It really is just Beagle that is (was? I can dream) a piece of 
unbelievable crap.

Anybody who uses extended attributes as part of a indexing scheme is just 
insane. Modifying the file you are indexing is not just fundamentally 
wrong to begin with, but it will then also be incredibly inefficient to 
read those entries one at a time.

And no other sane model would ever touch 'ctime'.

Oh, well. Making ctime configurable is not wrong per se. But if it's 
Beagle that triggers this, the fix is sadly in the wrong place.

		Linus

On Mon, Jul 28, 2008 at 08:31:28AM +0200, Alex Riesen wrote:

Somehow, this particular position of the new hunk does not feel like the
best choice. ;-)

				Petr "Pasky" Baudis

Accidentally, it split a _chapter_ about a file data corrup...
conversion for a weird, but common operating system.

Signed-off-by: Alex Riesen <redacted>
---

Petr Baudis, Mon, Jul 28, 2008 18:20:43 +0200:

On Mon, Jul 28, 2008 at 08:31:28AM +0200, Alex Riesen wrote:

quoted

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1a13abc..552c134 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -149,6 +149,13 @@ core.safecrlf::
 	`core.autocrlf`, git will reject the file.  The variable can
 	be set to "warn", in which case git will only warn about an
 	irreversible conversion but continue the operation.
+
+core.trustctime::
+	If false, the ctime differences between the index and the
+	working copy are ignored; useful when the inode change time
+	is regularly modified by something outside Git (file system
+	crawlers and some backup systems).
+	See linkgit:git-update-index[1]. True by default.
 +
 CRLF conversion bears a slight chance of corrupting data.
 autocrlf=true will convert CRLF to LF during commit and LF to

Somehow, this particular position of the new hunk does not feel like the
best choice. ;-)

It's alphabetical. Why? Oh, shit... Screw alphabetical

 Documentation/config.txt |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 552c134..61c3760 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -117,6 +117,13 @@ core.fileMode::
 	the working copy are ignored; useful on broken filesystems like FAT.
 	See linkgit:git-update-index[1]. True by default.
 
+core.trustctime::
+	If false, the ctime differences between the index and the
+	working copy are ignored; useful when the inode change time
+	is regularly modified by something outside Git (file system
+	crawlers and some backup systems).
+	See linkgit:git-update-index[1]. True by default.
+
 core.quotepath::
 	The commands that output paths (e.g. 'ls-files',
 	'diff'), when not given the `-z` option, will quote

@@ -149,13 +156,6 @@ core.safecrlf::
 	`core.autocrlf`, git will reject the file.  The variable can
 	be set to "warn", in which case git will only warn about an
 	irreversible conversion but continue the operation.
-
-core.trustctime::
-	If false, the ctime differences between the index and the
-	working copy are ignored; useful when the inode change time
-	is regularly modified by something outside Git (file system
-	crawlers and some backup systems).
-	See linkgit:git-update-index[1]. True by default.
 +
 CRLF conversion bears a slight chance of corrupting data.
 autocrlf=true will convert CRLF to LF during commit and LF to

-- 
1.6.0.rc0.76.g581e

Linus Torvalds, Mon, Jul 28, 2008 18:09:32 +0200:

It really is just Beagle that is (was? I can dream) a piece of 
unbelievable crap.

Anybody who uses extended attributes as part of a indexing scheme is just 
insane. Modifying the file you are indexing is not just fundamentally 
wrong to begin with, but it will then also be incredibly inefficient to 
read those entries one at a time.

And no other sane model would ever touch 'ctime'.

Beagle is not alone. Google Desktop Search was mentioned before.

Oh, well. Making ctime configurable is not wrong per se. But if it's 
Beagle that triggers this, the fix is sadly in the wrong place.

Never said it was a fix. Same as CRLF conversion is not a feature.