Re: [PATCH] read_directory: avoid invoking exclude machinery on tracked files
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 15, 2013, 16:52 UTC
- Message-ID
- <7vd2w1wmdo.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1360937848-4426-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 61 quoted lines
> read_directory() (and its friendly wrapper fill_directory) collects
> untracked/ignored files by traversing through the whole worktree (*),
> feeding every entry to treat_one_path(), where each entry is checked
> against .gitignore patterns.
>
> One may see that tracked files can't be excluded and we do not need to
> run them through exclude machinery. On repos where there are many
> .gitignore patterns and/or a lot of tracked files, this unnecessary
> processing can become expensive.
>
> This patch avoids it mostly for normal cases. Directories are still
> processed as before. DIR_SHOW_IGNORED and DIR_COLLECT_IGNORED are not
> normally used unless some options are given (e.g. "checkout
> --overwrite-ignore", "add -f"...) so people still need to pay penalty
> in some cases, just not as often as before.
>
> git status | webkit linux-2.6 libreoffice-core gentoo-x86
> -------------+----------------------------------------------
> before | 1.159s 0.226s 0.415s 0.597s
> after | 0.778s 0.176s 0.266s 0.556s
> nr. patterns | 89 376 19 0
> nr. tracked | 182k 40k 63k 101k
>
> (*) Not completely true. read_directory may skip recursing into a
> directory if it's entirely excluded and DIR_SHOW_OTHER_DIRECTORIES
> is not set.
>
> Tracked-down-by: Karsten Blees <karsten.blees@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> For reference:
> http://thread.gmane.org/gmane.comp.version-control.git/215820/focus=216195
>
> dir.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 57394e4..bdff256 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1244,7 +1244,19 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
> const struct path_simplify *simplify,
> int dtype, struct dirent *de)
> {
> - int exclude = is_excluded(dir, path->buf, &dtype);
> + int exclude;
> +
> + if (dtype == DT_UNKNOWN)
> + dtype = get_dtype(de, path->buf, path->len);
> +
> + if (!(dir->flags & DIR_SHOW_IGNORED) &&
> + !(dir->flags & DIR_COLLECT_IGNORED) &&
> + dtype != DT_DIR &&
> + cache_name_exists(path->buf, path->len, ignore_case))
> + return path_ignored;
> +
> + exclude = is_excluded(dir, path->buf, &dtype);
> +
> if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
> && exclude_matches_pathspec(path->buf, path->len, simplify))
> dir_add_ignored(dir, path->buf, path->len);Interesting.
In the current code, we always check if a path is excluded, and when dealing with DT_REG/DT_LNK, we call treat_file():
* When such a path is excluded, treat_file() returns true when we are not showing ignored directories. This causes treat_one_path() to return path_ignored, so for excluded DT_REG/DT_LNK paths when no DIR_*_IGNORED is in effect, this change is a correct optimization.
* When such a path is not excluded, on the ther hand, and when we are not showing ignored directories, treat_file() just returns the value of exclude_file, which is initialized to false and is not changed in the function. This causes treat_one_path() to return path_handled. However, the new code returns path_ignored in this case.
What guarantees that this change is regression free? I do not seem to be able to find anything that checks if the path is already known to the index in the original code for the case you special cased (i.e. DIR_*_IGNORED is not set and dtype is not DT_DIR). Do all the callers that reach this function in their callgraph, when they get path_ignored for a path in the index, behave as if the difference between path_ignored and path_handled does not matter?
Show 10 quoted lines
> @@ -1256,9 +1268,6 @@ static enum path_treatment treat_one_path(struct dir_struct *dir,
> if (exclude && !(dir->flags & DIR_SHOW_IGNORED))
> return path_ignored;
>
> - if (dtype == DT_UNKNOWN)
> - dtype = get_dtype(de, path->buf, path->len);
> -
> switch (dtype) {
> default:
> return path_ignored;