Re: [PATCH 04/10] unpack-trees: fix sparse checkout's "unable to match directories" fault

[Jonathan Nieder <jrnieder@gmail.com>]

Nguyễn Thái Ngọc Duy wrote:

> excluded_from_list() is designed to be work with a directory
> structure, where there are files and directories. Index however is a
                                                   ^
                                                   The
> list of files, no directories, not really fit in excluded_from_list()
                 ^with no separate entries for directories, so it does
> design.
[...]
> clear_ce_flags() is now used to match the index against sparse
> patterns. It does generate directories for excluded_from_list()

Sounds exciting.  I can't believe I missed that, but I guess what
was needed is a simple example caller.  Let's see how this patch
works in that vein.

> --- a/Documentation/git-read-tree.txt
> +++ b/Documentation/git-read-tree.txt
> @@ -416,13 +416,6 @@ turn `core.sparseCheckout` on in order to have sparse checkout
>  support.
>  
>  
> -BUGS
> -----
> -In order to match a directory with $GIT_DIR/info/sparse-checkout,
> -trailing slash must be used. The form without trailing slash, while
> -works with .gitignore, does not work with sparse checkout.

Nice.

[...]
> diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
> index 9a07de1..50f7dfe 100755
> --- a/t/t1011-read-tree-sparse-checkout.sh
> +++ b/t/t1011-read-tree-sparse-checkout.sh
> @@ -91,12 +91,20 @@ test_expect_success 'match directories with trailing slash' '
>  	test -f sub/added
>  '
>  
> -test_expect_failure 'match directories without trailing slash' '
> +test_expect_success 'match directories without trailing slash' '

Also nice.

> -	echo init.t >.git/info/sparse-checkout &&
>  	echo sub >>.git/info/sparse-checkout &&
>  	git read-tree -m -u HEAD &&
>  	git ls-files -t >result &&
> -	test_cmp expected.swt result &&
> +	test_cmp expected.swt-noinit result &&
> +	test ! -f init.t &&
> +	test -f sub/added

Maybe the new

	test_path_is_missing

and

	test_path_is_file

would be a good fit here, for better behavior when the path is a directory in
the former case and better debugging output for failures in the latter.

> +'
> +
> +test_expect_success 'match directory pattern' '
> +	echo "s?b" >>.git/info/sparse-checkout &&
> +	git read-tree -m -u HEAD &&
> +	git ls-files -t >result &&
> +	test_cmp expected.swt-noinit result &&
>  	test ! -f init.t &&
>  	test -f sub/added
>  '

A pattern change that changes the list of matched paths would be more
impressive.  (Though I understand, this version already works for
demonstrating progress.)

[...]
> --- a/dir.c
> +++ b/dir.c
> @@ -359,12 +359,6 @@ int excluded_from_list(const char *pathname,
>  			int to_exclude = x->to_exclude;
>  
>  			if (x->flags & EXC_FLAG_MUSTBEDIR) {
> -				if (!dtype) {
> -					if (!prefixcmp(pathname, exclude))
> -						return to_exclude;
> -					else
> -						continue;
> -				}

Removing the no longer used !dtype codepath.

This hunk could be a separate patch (simplifying the diff and
decoupling this series from patch 1).

> --- a/unpack-trees.c
> +++ b/unpack-trees.c
[...]
> @@ -926,14 +917,19 @@ static void set_new_skip_worktree_1(struct unpack_trees_options *o)
>  {
>  	int i;
>  
> +	/* Mark everything out (except staged entries) */
>  	for (i = 0;i < o->src_index->cache_nr;i++) {
>  		struct cache_entry *ce = o->src_index->cache[i];
>  		ce->ce_flags &= ~CE_ADDED;
> +		if (!ce_stage(ce))

Not a fan of this new definition of "staged".  The following might be
clearer:

	/*
	 * The WILL_BE_SKIP_WORKTREE bit is initialized in two stages:
	 *
	 * 1. Pretend the checkout is so narrow it contains no files.
	 *    Only unresolved entries participating in a merge would
	 *    get written to the worktree.
	 *
	 * 2. Add back entries according to .git/info/sparse-checkout
	 */

[...]
> @@ -943,19 +939,26 @@ static int set_new_skip_worktree_2(struct unpack_trees_options *o)
>  
>  	/*
>  	 * CE_ADDED marks new index entries. These have not been processed
> -	 * by set_new_skip_worktree_1() so we do it here.
> +	 * by set_new_skip_worktree_1() so we do it here. Mark every new
> +	 * entries out.
>  	 */

Nit: "every" wants a singular noun.  But even better would be to move
the description of effect to the call site.

The description of strategy can stay here so it doesn't go out of date
when the code changes:

	/*
	 * Just like set_new_skip_worktree_1.
	 * unpack-trees never writes conflicted entries to the worktree,
	 * so there is no need to worry about conflicted files.
	 *
	 * 1. Set WILL_BE_SKIP_WORKTREE for all new entries.
	 * 2. Clear it again for paths within the sparse checkout.
	 */

Is it possible to make the two code paths share code without
sacrificing speed?