threads / patch / 13687

patchEnsure that commit/status don't stat all files when core.ignoreStat = true

Subject: [PATCH] Ensure that commit/status don't stat all files when core.ignoreStat = true

## tl;dr

11 messages between May 27, 2008 and May 31, 2008. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Marius Storm-Olsen· May 27, 2008, 09:29 UTC · lore

The core.ignoreStat option is used to assume that files in the index are unchanged, thus avoiding expensive lstat()s on slow systems. However, due to refresh_cache_ent still stating but ignoring the info, and the listing of untracked files in commit/status, we would still lstat() all the files.

This change shortcuts the refresh_cache_ent(), and makes commit/status not list untracked files, unless the -u option is specified.

Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
---
 read-cache.c |   10 ++++++++++
 wt-status.c  |   11 ++++++++++-
 2 files changed, 20 insertions(+), 1 deletions(-)
Show changes to 2 files +20 −1

read-cache.c, wt-status.c

diff --git a/read-cache.c b/read-cache.c
index 8b467f8..104e387 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -882,6 +882,16 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	if (ce_uptodate(ce))
 		return ce;
 
+	/*
+	 * assume_unchanged is used to avoid lstats to check if the
+	 * file has been modified. When true, the user need to
+	 * manually update the index.
+	 */
+	if (assume_unchanged) {
+		ce_mark_uptodate(ce);
+		return ce;
+	}
+
 	if (lstat(ce->name, &st) < 0) {
 		if (err)
 			*err = errno;
diff --git a/wt-status.c b/wt-status.c
index a44c543..72db466 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -342,7 +342,14 @@ void wt_status_print(struct wt_status *s)
 	wt_status_print_changed(s);
 	if (wt_status_submodule_summary)
 		wt_status_print_submodule_summary(s);
-	wt_status_print_untracked(s);
+
+	if (assume_unchanged && !s->untracked) {
+		if (s->commitable)
+			fprintf(s->fp, "# Untracked files not listed (use -u option to show untracked files)\n");
+		/* !s->commitable message displayed below */
+	}
+	else
+		wt_status_print_untracked(s);
 
 	if (s->verbose && !s->is_initial)
 		wt_status_print_verbose(s);
@@ -357,6 +364,8 @@ void wt_status_print(struct wt_status *s)
 			printf("nothing added to commit but untracked files present (use \"git add\" to track)\n");
 		else if (s->is_initial)
 			printf("nothing to commit (create/copy files and use \"git add\" to track)\n");
+		else if (assume_unchanged && !s->untracked)
+			printf("nothing to commit (use -u to show untracked files)\n");
 		else
 			printf("nothing to commit (working directory clean)\n");
 	}
-- 
1.5.5.1.501.gefb4
Junio C Hamano· May 27, 2008, 20:00 UTC · re: Marius Storm-Olsen · lore

Re: [PATCH] Ensure that commit/status don't stat all files when core.ignoreStat = true

Marius Storm-Olsen <marius@trolltech.com> writes:
Show 18 quoted lines
> diff --git a/read-cache.c b/read-cache.c
> index 8b467f8..104e387 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -882,6 +882,16 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
>  	if (ce_uptodate(ce))
>  		return ce;
>  
> +	/*
> +	 * assume_unchanged is used to avoid lstats to check if the
> +	 * file has been modified. When true, the user need to
> +	 * manually update the index.
> +	 */
> +	if (assume_unchanged) {
> +		ce_mark_uptodate(ce);
> +		return ce;
> +	}
> +

The description for core.ignorestat in Documentation/config.txt is quite bogus. That single bit does _not_ determine globally if we lstat(2) or not. The description in Documentation/git-update-index.txt about it (look for the section "Using assume unchanged bit") accurately describes what it is meant to do. The rules are:

 - (ce->ce_flags & CE_VALID) is the only thing that decides if we can omit
   lstat(2) for _that particular path_.  There is no global "we would
   never ever run lstat(2)" option, and core.ignorestat certainly isn't
   it.
 - you can use the assume-unchanged mechanism without setting
   core.ignorestat.  You flip the CE_VALID bit for selected paths manually
   and forget about them afterwards, when you would want all of your usual
   "active" changes noticed by git, while skipping lstat(2) overhead in
   areas you are not interested in.
 - when you say "git update-index" (or "git add") for a path, if you have
   core.ignorestat set, that path is automatically marked with CE_VALID,
   so that later lstat(2) will be omitted for that particular path.  IOW,
   by having core.ignorestat set, you are promising that you are not going
   to _further_ change the work tree contents _without_ telling git --- or
   at least you are promising that you _will_ tell git if you change it
   when it matters.  But you have to tell git at least once what the
   contents are.

Would it be sufficient for what you are trying to do if you changed that test to something like this?

        /*
         * CE_VALID means the user promised us that the change to
         * the work tree does not matter and told us not to worry.
         */
	if (!ignore_valid && (ce->ce_flags & CE_VALID)) {
        	ce_mark_uptodate(ce);
		return ce;
	}
Show 29 quoted lines
> diff --git a/wt-status.c b/wt-status.c
> index a44c543..72db466 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -342,7 +342,14 @@ void wt_status_print(struct wt_status *s)
>  	wt_status_print_changed(s);
>  	if (wt_status_submodule_summary)
>  		wt_status_print_submodule_summary(s);
> -	wt_status_print_untracked(s);
> +
> +	if (assume_unchanged && !s->untracked) {
> +		if (s->commitable)
> +			fprintf(s->fp, "# Untracked files not listed (use -u option to show untracked files)\n");
> +		/* !s->commitable message displayed below */
> +	}
> +	else
> +		wt_status_print_untracked(s);
>  
>  	if (s->verbose && !s->is_initial)
>  		wt_status_print_verbose(s);
> @@ -357,6 +364,8 @@ void wt_status_print(struct wt_status *s)
>  			printf("nothing added to commit but untracked files present (use \"git add\" to track)\n");
>  		else if (s->is_initial)
>  			printf("nothing to commit (create/copy files and use \"git add\" to track)\n");
> +		else if (assume_unchanged && !s->untracked)
> +			printf("nothing to commit (use -u to show untracked files)\n");
>  		else
>  			printf("nothing to commit (working directory clean)\n");
>  	}

The core.ignorestat variable does not have anything to do with showing untracked files. It is about "do we mark the added path as CE_VALID, meaning that we do not have to lstat(2) them?" IOW, it is about tracked files.

While it might be useful in certain workflows to ignore untracked files, I do not think it is a good idea to overload such an unrelated meaning to the variable.

Marius Storm-Olsen· May 27, 2008, 20:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] Ensure that commit/status don't stat all files when core.ignoreStat = true

Junio C Hamano said the following on 27.05.2008 22:00:
Show 6 quoted lines
> Marius Storm-Olsen <marius@trolltech.com> writes:
> The description for core.ignorestat in Documentation/config.txt is quite
> bogus.  That single bit does _not_ determine globally if we lstat(2) or
> not.  The description in Documentation/git-update-index.txt about it (look
> for the section "Using assume unchanged bit") accurately describes what it
> is meant to do.  The rules are:

Aha! Thanks for the detailed explanation of core.ignoreStat. Given your description, the patch is certainly bogus.

Show 11 quoted lines
> Would it be sufficient for what you are trying to do if you changed that
> test to something like this?
> 
>         /*
>          * CE_VALID means the user promised us that the change to
>          * the work tree does not matter and told us not to worry.
>          */
> 	if (!ignore_valid && (ce->ce_flags & CE_VALID)) {
>         	ce_mark_uptodate(ce);
> 		return ce;
> 	}

I'll give it a shot tomorrow, to see how it affects my use-cases. Thanks.

>> diff --git a/wt-status.c b/wt-status.c
>> index a44c543..72db466 100644
>> --- a/wt-status.c
>> +++ b/wt-status.c
...
Show 8 quoted lines
> The core.ignorestat variable does not have anything to do with showing
> untracked files.  It is about "do we mark the added path as CE_VALID,
> meaning that we do not have to lstat(2) them?"  IOW, it is about tracked
> files.
> 
> While it might be useful in certain workflows to ignore untracked files, I
> do not think it is a good idea to overload such an unrelated meaning to
> the variable.

Indeed. I'll resend a new patch tomorrow with a new variable which will only affect the stat'ing of untracked files, if you think that's reasonable. IMO, we certainly need a way of avoiding to stat the whole filetree on commits and status. I mean, that's what you have the -u option for, right? :-) In any case, an opt-in feature, of course.

Thanks for checking the patch!

-- .marius

Marius Storm-Olsen· May 30, 2008, 11:14 UTC · re: Marius Storm-Olsen · lore

[PATCH 1/3] Clearify the documentation for core.ignoreStat

The previous documentation didn't make it clear that the "assume unchanged" was on per file basis, and not a global flag.

Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
---
 Documentation/config.txt |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)
Show changes to Documentation/config.txt +7 −4
diff --git a/Documentation/config.txt b/Documentation/config.txt
index c298dc2..5331b45 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -205,10 +205,13 @@ Can be overridden by the 'GIT_PROXY_COMMAND' environment variable
 handling).
 
 core.ignoreStat::
-	The working copy files are assumed to stay unchanged until you
-	mark them otherwise manually - Git will not detect the file changes
-	by lstat() calls. This is useful on systems where those are very
-	slow, such as Microsoft Windows.  See linkgit:git-update-index[1].
+	If true, commands which modify both the working tree and the index
+	will mark the updated paths with the "assume unchanged" bit in the
+	index. These marked files are then assumed to stay unchanged in the
+	working copy, until you	mark them otherwise manually - Git will not
+	detect the file changes	by lstat() calls. This is useful on systems
+	where those are very slow, such as Microsoft Windows.
+	See linkgit:git-update-index[1].
 	False by default.
 
 core.preferSymlinkRefs::
-- 
1.5.5.GIT
Simon Hausmann· May 30, 2008, 08:54 UTC · re: Marius Storm-Olsen · lore

[PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files.

Determining untracked files can be a very slow operation on large trees. This commit introduces a configuration variable that makes it possible to disable showing of untracked files by default as well as a -U commandline option to override this.

Signed-off-by: Simon Hausmann <simon@lst.de>
Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
---
 Documentation/config.txt     |    5 +++++
 Documentation/git-commit.txt |   11 ++++++++---
 builtin-commit.c             |    1 +
 config.c                     |    7 +++++++
 environment.c                |    1 +
 wt-status.c                  |    7 ++++++-
 wt-status.h                  |    1 +
 7 files changed, 29 insertions(+), 4 deletions(-)
Show changes to 7 files +29 −4

Documentation/config.txt, Documentation/git-commit.txt, builtin-commit.c, config.c, environment.c, wt-status.c, wt-status.h

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5331b45..e42ead0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -214,6 +214,11 @@ core.ignoreStat::
 	See linkgit:git-update-index[1].
 	False by default.
 
+core.showUntrackedFiles::
+	A boolean to enable/disable displaying untracked files in the output
+	of linkgit:git-status[1] and linkgit:git-commit[1].
+	Defaults to true.
+
 core.preferSymlinkRefs::
 	Instead of the default "symref" format for HEAD
 	and other symbolic reference files, use symbolic links.
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index c3c9f5b..a3174e4 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -150,12 +150,17 @@ but can be used to amend a merge commit.
 	the last commit without committing changes that have
 	already been staged.
 
+-U|--untracked::
+	Show untracked files, in the "Untracked files:" section of commit
+	message template.
+	This option overrides the core.showUntrackedFiles
+	configuration option, and is normally not needed.
+
 -u|--untracked-files::
 	Show all untracked files, also those in uninteresting
-	directories, in the "Untracked files:" section of commit
-	message template.  Without this option only its name and
+	directories.  Without this option only its name and
 	a trailing slash are displayed for each untracked
-	directory.
+	directory. This option implies --untracked.
 
 -v|--verbose::
 	Show unified diff between the HEAD commit and what
diff --git a/builtin-commit.c b/builtin-commit.c
index b294c1f..28cc170 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -103,6 +103,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN('n', "no-verify", &no_verify, "bypass pre-commit hook"),
 	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 	OPT_BOOLEAN('u', "untracked-files", &untracked_files, "show all untracked files"),
+	OPT_BOOLEAN('U', "untracked", &show_untracked_files, "show untracked files"),
 	OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
 	OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
 
diff --git a/config.c b/config.c
index c2f2bbb..ba3efd1 100644
--- a/config.c
+++ b/config.c
@@ -7,6 +7,7 @@
  */
 #include "cache.h"
 #include "exec_cmd.h"
+#include "wt-status.h"
 
 #define MAXNAME (256)
 
@@ -511,6 +512,12 @@ int git_default_config(const char *var, const char *value, void *dummy)
 			return error("Malformed value for %s", var);
 		return 0;
 	}
+	if (!strcmp(var, "core.showuntrackedfiles")) {
+		if (!value)
+			return config_error_nonbool(var);
+		show_untracked_files = git_config_bool(var, value);
+		return 0;
+	}
 
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
diff --git a/environment.c b/environment.c
index 73feb2d..210ae17 100644
--- a/environment.c
+++ b/environment.c
@@ -41,6 +41,7 @@ enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
+int show_untracked_files = 1;
 
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
diff --git a/wt-status.c b/wt-status.c
index 5b4d74c..819fe2d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -347,7 +347,10 @@ void wt_status_print(struct wt_status *s)
 	wt_status_print_changed(s);
 	if (wt_status_submodule_summary)
 		wt_status_print_submodule_summary(s);
-	wt_status_print_untracked(s);
+	if (show_untracked_files)
+		wt_status_print_untracked(s);
+	else if (s->commitable)
+		fprintf(s->fp, "# Untracked files not listed (use -U option to show untracked files)\n");
 
 	if (s->verbose && !s->is_initial)
 		wt_status_print_verbose(s);
@@ -362,6 +365,8 @@ void wt_status_print(struct wt_status *s)
 			printf("nothing added to commit but untracked files present (use \"git add\" to track)\n");
 		else if (s->is_initial)
 			printf("nothing to commit (create/copy files and use \"git add\" to track)\n");
+		else if (!show_untracked_files)
+			printf("nothing to commit (use -U to show untracked files)\n");
 		else
 			printf("nothing to commit (working directory clean)\n");
 	}
diff --git a/wt-status.h b/wt-status.h
index 597c7ea..4b643b4 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -33,5 +33,6 @@ extern int wt_status_use_color;
 extern int wt_status_relative_paths;
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
+extern int show_untracked_files;
 
 #endif /* STATUS_H */
-- 
1.5.5.GIT
Marius Storm-Olsen· May 30, 2008, 12:38 UTC · re: Simon Hausmann · lore

[PATCH 3/3] Add shortcut in refresh_cache_ent() for marked entries.

When a cache entry has been marked as CE_VALID, the user has promised us that any change in the work tree does not matter. Just mark the entry as up-to-date, and continue.

Done-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
---
 read-cache.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
Show changes to read-cache.c +9 −0
diff --git a/read-cache.c b/read-cache.c
index ac9a8e7..8e5fbb6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -893,6 +893,15 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	if (ce_uptodate(ce))
 		return ce;
 
+	/*
+	 * CE_VALID means the user promised us that the change to
+	 * the work tree does not matter and told us not to worry.
+	 */
+	if (!ignore_valid && (ce->ce_flags & CE_VALID)) {
+		ce_mark_uptodate(ce);
+		return ce;
+	}
+
 	if (lstat(ce->name, &st) < 0) {
 		if (err)
 			*err = errno;
-- 
1.5.5.GIT
Marius Storm-Olsen· May 30, 2008, 13:14 UTC · re: Marius Storm-Olsen · lore

Re: [PATCH 3/3] Add shortcut in refresh_cache_ent() for marked entries.

Marius Storm-Olsen said the following on 30.05.2008 14:38:
Show 6 quoted lines
> When a cache entry has been marked as CE_VALID, the user has
> promised us that any change in the work tree does not matter.
> Just mark the entry as up-to-date, and continue.
> 
> Done-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>

This patch actually cuts the commit/status time in half for me, on my Windows machine, when the whole work tree is validated, and core.ignoreStat == true.

-- 
.marius [@trolltech.com]
'if you know what you're doing, it's not research'
Marius Storm-Olsen· May 30, 2008, 13:10 UTC · re: Simon Hausmann · lore

Re: [PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files.

(Err.. Sent from me, and should have had a:)
From: Simon Hausmann <simon@lst.de>
Show 6 quoted lines
> Determining untracked files can be a very slow operation on large trees. This commit introduces
> a configuration variable that makes it possible to disable showing of untracked files by default
> as well as a -U commandline option to override this.
> 
> Signed-off-by: Simon Hausmann <simon@lst.de>
> Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
Sorry for the confusion...
-- 
.marius [@trolltech.com]
'if you know what you're doing, it's not research'
Marius Storm-Olsen· May 30, 2008, 13:16 UTC · re: Simon Hausmann · lore

Re: [PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files.

Simon Hausmann said the following on 30.05.2008 10:54:
Show 7 quoted lines
> Determining untracked files can be a very slow operation on large trees. This commit introduces
> a configuration variable that makes it possible to disable showing of untracked files by default
> as well as a -U commandline option to override this.
> 
> Signed-off-by: Simon Hausmann <simon@lst.de>
> Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
> ---

This gives me ~80% improvement on commit/status. Reasonable, since my work tree nearly doubles in size on a full build.

-- 
.marius [@trolltech.com]
'if you know what you're doing, it's not research'
Junio C Hamano· May 30, 2008, 20:27 UTC · re: Simon Hausmann · lore

Re: [PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files.

Simon Hausmann <simon@lst.de> writes:
Show 13 quoted lines
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 5331b45..e42ead0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -214,6 +214,11 @@ core.ignoreStat::
>  	See linkgit:git-update-index[1].
>  	False by default.
>  
> +core.showUntrackedFiles::
> +	A boolean to enable/disable displaying untracked files in the output
> +	of linkgit:git-status[1] and linkgit:git-commit[1].
> +	Defaults to true.
> +

This does not belong to the 'core.*', which is about the low-level plumbing. It perhaps could live in 'status.*' section, but I think you can do better than introducing this as a boolean.

Show 22 quoted lines
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index c3c9f5b..a3174e4 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -150,12 +150,17 @@ but can be used to amend a merge commit.
>  	the last commit without committing changes that have
>  	already been staged.
>  
> +-U|--untracked::
> +	Show untracked files, in the "Untracked files:" section of commit
> +	message template.
> +	This option overrides the core.showUntrackedFiles
> +	configuration option, and is normally not needed.
> +
>  -u|--untracked-files::
>  	Show all untracked files, also those in uninteresting
> -	directories, in the "Untracked files:" section of commit
> -	message template.  Without this option only its name and
> +	directories.  Without this option only its name and
>  	a trailing slash are displayed for each untracked
> -	directory.
> +	directory. This option implies --untracked.

I wonder if we really need a new option that is half independent to an existing one.

Step back a bit and think.  You have three choice:
 (1) Do not show untracked files at all; or
 (2) Show untracked but summarize untracked directories; or
 (3) Show all untracked files.

We have had (2) and (3) so far, and you are adding (1) as a new feature. How about allowing -u on the command line to take an optional parameter to say what kind the user wants? I.e.

        -u=none		shows nothing (i.e. (1))
        -u=normal	shows summarized report (i.e. (2))
	-u=all		shows all untracked files (i.e. (3))

And (3) can also be spelled as "-u without parameter"; absense of -u anywhere defaults to (2). That would be the first patch.

Then, in the second patch, you can add support to 'status.showuntracked'; you pretend that it is set to 'normal' if it is not defined in the configuration file.

Hmm?
Marius Storm-Olsen· May 31, 2008, 06:41 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files.

Junio C Hamano wrote:
> I wonder if we really need a new option that is half independent to an
> existing one.
...
>         -u=none		shows nothing (i.e. (1))
>         -u=normal	shows summarized report (i.e. (2))
> 	-u=all		shows all untracked files (i.e. (3))
...
Show 5 quoted lines
> Then, in the second patch, you can add support to 'status.showuntracked';
> you pretend that it is set to 'normal' if it is not defined in the
> configuration file.
> 
> Hmm?
Sounds good to me. Will redo the patch. Thanks!

-- .marius

← back to recent threads