# [PATCH] Ensure that commit/status don't stat all files when core.ignoreStat = true 11 messages from 2008-05-27 to 2008-05-31. Participants: Marius Storm-Olsen, Junio C Hamano, Simon Hausmann. Thread: https://gitlist.dev/t/13687 ## Marius Storm-Olsen, 2008-05-27 09:29 Subject: [PATCH] Ensure that commit/status don't stat all files when core.ignoreStat = true Message-ID: <1211880549-15758-1-git-send-email-marius@trolltech.com> URL: https://gitlist.dev/e/1211880549-15758-1-git-send-email-marius%40trolltech.com ``` 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 --- read-cache.c | 10 ++++++++++ wt-status.c | 11 ++++++++++- 2 files changed, 20 insertions(+), 1 deletions(-) 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, 2008-05-27 20:00 Subject: Re: [PATCH] Ensure that commit/status don't stat all files when core.ignoreStat = true Message-ID: <7v7idf1pqy.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v7idf1pqy.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1211880549-15758-1-git-send-email-marius@trolltech.com> ``` Marius Storm-Olsen writes: > 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; } > 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, 2008-05-27 20:21 Subject: Re: [PATCH] Ensure that commit/status don't stat all files when core.ignoreStat = true Message-ID: <483C6D60.7090705@trolltech.com> URL: https://gitlist.dev/e/483C6D60.7090705%40trolltech.com In-Reply-To: <7v7idf1pqy.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano said the following on 27.05.2008 22:00: > Marius Storm-Olsen 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. > 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 ... > 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 ``` ## Simon Hausmann, 2008-05-30 08:54 Subject: [PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files. Message-ID: <3088e5a9507aa55ee55f512fb3bd554f1658d2e3.1212152269.git.marius@trolltech.com> URL: https://gitlist.dev/e/3088e5a9507aa55ee55f512fb3bd554f1658d2e3.1212152269.git.marius%40trolltech.com In-Reply-To: <043361f81946d5657217e6be0f22f4df350fd13f.1212152269.git.marius@trolltech.com> ``` 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 Signed-off-by: Marius Storm-Olsen --- 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(-) 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, 2008-05-30 11:14 Subject: [PATCH 1/3] Clearify the documentation for core.ignoreStat Message-ID: <043361f81946d5657217e6be0f22f4df350fd13f.1212152269.git.marius@trolltech.com> URL: https://gitlist.dev/e/043361f81946d5657217e6be0f22f4df350fd13f.1212152269.git.marius%40trolltech.com In-Reply-To: <483C6D60.7090705@trolltech.com> ``` 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 --- Documentation/config.txt | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-) 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 ``` ## Marius Storm-Olsen, 2008-05-30 12:38 Subject: [PATCH 3/3] Add shortcut in refresh_cache_ent() for marked entries. Message-ID: URL: https://gitlist.dev/e/c581c03d8d8bfc76753018d4399a79b02a057186.1212152269.git.marius%40trolltech.com In-Reply-To: <3088e5a9507aa55ee55f512fb3bd554f1658d2e3.1212152269.git.marius@trolltech.com> ``` 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 Signed-off-by: Marius Storm-Olsen --- read-cache.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) 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, 2008-05-30 13:10 Subject: Re: [PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files. Message-ID: <483FFCDB.9010902@trolltech.com> URL: https://gitlist.dev/e/483FFCDB.9010902%40trolltech.com In-Reply-To: <3088e5a9507aa55ee55f512fb3bd554f1658d2e3.1212152269.git.marius@trolltech.com> ``` (Err.. Sent from me, and should have had a:) From: Simon Hausmann > 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 > Signed-off-by: Marius Storm-Olsen Sorry for the confusion... -- .marius [@trolltech.com] 'if you know what you're doing, it's not research' ``` ## Marius Storm-Olsen, 2008-05-30 13:14 Subject: Re: [PATCH 3/3] Add shortcut in refresh_cache_ent() for marked entries. Message-ID: <483FFDAC.8090606@trolltech.com> URL: https://gitlist.dev/e/483FFDAC.8090606%40trolltech.com In-Reply-To: ``` Marius Storm-Olsen said the following on 30.05.2008 14:38: > 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 > Signed-off-by: Marius Storm-Olsen 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, 2008-05-30 13:16 Subject: Re: [PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files. Message-ID: <483FFE1B.1020507@trolltech.com> URL: https://gitlist.dev/e/483FFE1B.1020507%40trolltech.com In-Reply-To: <3088e5a9507aa55ee55f512fb3bd554f1658d2e3.1212152269.git.marius@trolltech.com> ``` Simon Hausmann said the following on 30.05.2008 10:54: > 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 > Signed-off-by: Marius Storm-Olsen > --- 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, 2008-05-30 20:27 Subject: Re: [PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files. Message-ID: <7viqwvk04y.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7viqwvk04y.fsf%40gitster.siamese.dyndns.org In-Reply-To: <3088e5a9507aa55ee55f512fb3bd554f1658d2e3.1212152269.git.marius@trolltech.com> ``` Simon Hausmann writes: > 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. > 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, 2008-05-31 06:41 Subject: Re: [PATCH 2/3] Introduce core.showUntrackedFiles to make it possible to disable showing of untracked files. Message-ID: <4840F31E.7090801@trolltech.com> URL: https://gitlist.dev/e/4840F31E.7090801%40trolltech.com In-Reply-To: <7viqwvk04y.fsf@gitster.siamese.dyndns.org> ``` 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)) ... > 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 ```