Re: [PATCH] read_directory: avoid invoking exclude machinery on tracked files

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

> @@ -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;