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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 15, 2010, 19:10 UTC
Message-ID: <20101115191041.GE16385@burratino>
In-Reply-To: <1289817410-32470-5-git-send-email-pclouds@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.

Show 11 quoted lines

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

[...]

Show 10 quoted lines

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

Show 8 quoted lines

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

Show 10 quoted lines

> +'
> +
> +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.)

[...]

Show 12 quoted lines

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

[...]

Show 9 quoted lines

> @@ -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
	 */

[...]

Show 8 quoted lines

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