Re: [PATCH v2] cache-tree: invalidate i-t-a paths after generating trees

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

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

> diff --git a/cache-tree.c b/cache-tree.c
> index 28ed657..989a7ff 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,
>  	int missing_ok = flags & WRITE_TREE_MISSING_OK;
>  	int dryrun = flags & WRITE_TREE_DRY_RUN;
>  	int i;
> +	int to_invalidate = 0;
>  
>  	if (0 <= it->entry_count && has_sha1_file(it->sha1))
>  		return it->entry_count;
> @@ -324,7 +325,13 @@ static int update_one(struct cache_tree *it,
>  			if (!sub)
>  				die("cache-tree.c: '%.*s' in '%s' not found",
>  				    entlen, path + baselen, path);
> -			i += sub->cache_tree->entry_count - 1;
> +			i--; /* this entry is already counted in "sub" */
> +			if (sub->cache_tree->entry_count < 0) {
> +				i -= sub->cache_tree->entry_count;
> +				to_invalidate = 1;
> +			}
> +			else
> +				i += sub->cache_tree->entry_count;

Hrm.  update_one() is prepared to see a cache-tree whose entry count
is zero (see the context lines in the previous hunk) and the
invariant for the rest of the code is "if 0 <= entry_count, the
cached tree is valid; invalid cache-tree has -1 in entry_count.
More importantly, entry_count negated does not in general express
how many entries there are in the subtree and does not tell us how
many index entries to skip.

> @@ -339,8 +346,23 @@ static int update_one(struct cache_tree *it,
>  				mode, sha1_to_hex(sha1), entlen+baselen, path);
>  		}
>  
> -		if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
> -			continue; /* entry being removed or placeholder */
> +		/*
> +		 * CE_REMOVE entries are removed before the index is
> +		 * written to disk. Skip them to remain consistent
> +		 * with the future on-disk index.
> +		 */
> +		if (ce->ce_flags & CE_REMOVE)
> +			continue;
> +
> +		/*
> +		 * CE_INTENT_TO_ADD entries exist on on-disk index but
> +		 * they are not part of generated trees. Invalidate up
> +		 * to root to force cache-tree users to read elsewhere.
> +		 */
> +		if (ce->ce_flags & CE_INTENT_TO_ADD) {
> +			to_invalidate = 1;
> +			continue;
> +		}

Thanks for documenting these.

> @@ -360,7 +382,7 @@ static int update_one(struct cache_tree *it,
>  	}
>  
>  	strbuf_release(&buffer);
> -	it->entry_count = i;
> +	it->entry_count = to_invalidate ? -i : i;

See above.  I am not fundamentally opposed to a change to redefine
entry_count so that it always maintains how many index entries the
subtree covers, even for invalidated subtree, but I do not think
this patch alone is sufficient to maintain such invariant.

>  #if DEBUG
>  	fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n",
>  		it->entry_count, it->subtree_nr,
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index ec35409..2a4a749 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -62,5 +62,25 @@ test_expect_success 'can "commit -a" with an i-t-a entry' '
>  	git commit -a -m all
>  '
>  
> +test_expect_success 'cache-tree invalidates i-t-a paths' '
> +	git reset --hard &&
> +	mkdir dir &&
> +	: >dir/foo &&
> +	git add dir/foo &&
> +	git commit -m foo &&
> +
> +	: >dir/bar &&
> +	git add -N dir/bar &&
> +	git diff --cached --name-only >actual &&
> +	echo dir/bar >expect &&
> +	test_cmp expect actual &&
> +
> +	git write-tree >/dev/null &&
> +
> +	git diff --cached --name-only >actual &&
> +	echo dir/bar >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_done