Re: [PATCH v4 6/8] dir: fix checks on common prefix directory

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

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> ...
> Fix most these problems by making treat_leading_path() not only loop
> over each leading path component, but calling treat_path() directly on
> each.  To do so, we have to create a synthetic dir_entry, but that only
> takes a few lines.  Then, pay attention to the path_treatment result we
> get from treat_path() and don't treat path_excluded, path_untracked, and
> path_recurse all the same as path_recurse.
>
> This leaves one remaining problem, the new inconsistency from commit
> df5bcdf83ae.  That will be addressed in a subsequent commit.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  dir.c                                         | 57 +++++++++++++++----
>  ...common-prefixes-and-directory-traversal.sh |  6 +-
>  2 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 645b44ea64..1de5d7ad33 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2102,37 +2102,72 @@ static int treat_leading_path(struct dir_struct *dir,
>  			      const struct pathspec *pathspec)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> -	int baselen, rc = 0;
> +	int prevlen, baselen;
>  	const char *cp;
> +	struct cached_dir cdir;
> +	struct dirent *de;
> +	enum path_treatment state = path_none;
> +
> +	/*
> +	 * For each directory component of path, we are going to check whether
> +	 * that path is relevant given the pathspec.  For example, if path is
> +	 *    foo/bar/baz/
> +	 * then we will ask treat_path() whether we should go into foo, then
> +	 * whether we should go into bar, then whether baz is relevant.
> +	 * Checking each is important because e.g. if path is
> +	 *    .git/info/
> +	 * then we need to check .git to know we shouldn't traverse it.
> +	 * If the return from treat_path() is:
> +	 *    * path_none, for any path, we return false.
> +	 *    * path_recurse, for all path components, we return true
> +	 *    * <anything else> for some intermediate component, we make sure
> +	 *        to add that path to the relevant list but return false
> +	 *        signifying that we shouldn't recurse into it.
> +	 */
>  
>  	while (len && path[len - 1] == '/')
>  		len--;
>  	if (!len)
>  		return 1;
> +
> +	de = xcalloc(1, sizeof(struct dirent)+len+1);

That "+len+1" may deserve a comment?  If we wanted to shoot for the
minimum memory consumption (and we do not), we would probably
allocate

	(sizeof(struct dirent) - sizeof(de->d_name)) +
		max(sizeof(de->d_name), len + 1)

bytes, but unconditionally adding len+1 is simpler and easier to
understand.  Either way, we *are* relying on the assumption that
either:

 (1) the "struct dirent" would have d_name[] array at the end of the
     struct, and by over-allocating, we can safely fit and carry a
     name that is much longer than sizeof(.d_name[]); OR

 (2) the "struct dirent" has d_name[] that is large enough to hold len+1
     bytes, if the assumption (1) does not hold.

is true.

> +	memset(&cdir, 0, sizeof(cdir));
> +	cdir.de = de;
> +#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
> +	de->d_type = DT_DIR;
> +#endif
>  	baselen = 0;
> +	prevlen = 0;
>  	while (1) {
> -		cp = path + baselen + !!baselen;
> +		prevlen = baselen + !!baselen;
> +		cp = path + prevlen;
>  		cp = memchr(cp, '/', path + len - cp);
>  		if (!cp)
>  			baselen = len;
>  		else
>  			baselen = cp - path;
> -		strbuf_setlen(&sb, 0);
> +		strbuf_reset(&sb);
>  		strbuf_add(&sb, path, baselen);
>  		if (!is_directory(sb.buf))
>  			break;



> -		if (simplify_away(sb.buf, sb.len, pathspec))
> -			break;
> -		if (treat_one_path(dir, NULL, istate, &sb, baselen, pathspec,
> -				   DT_DIR, NULL) == path_none)
> +		strbuf_reset(&sb);
> +		strbuf_add(&sb, path, prevlen);
> +		memcpy(de->d_name, path+prevlen, baselen-prevlen);
> +		de->d_name[baselen-prevlen] = '\0';
> +		state = treat_path(dir, NULL, &cdir, istate, &sb, prevlen,
> +				    pathspec);

So this is the crux fo the fix---instead of doing a (poor) imitation
of what treat_path() does by calling simplify_away() and
treat_one_path() ourselves, we make a call to the real thing.

Looking good.  Thanks.