Re: [PATCH] cache-tree.c: fix i-t-a check skipping directory updates sometimes

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

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

> Commit 3cf773e (cache-tree: fix writing cache-tree when CE_REMOVE is
> present - 2012-12-16) skips i-t-a entries when building trees objects
> from the index. Unfortunately it may skip too much.
>
> The code in question checks if an entry is an i-t-a one, then no tree
> entry will be written. But it does not take into account that
> directories can also be written with the same code. Suppose we have
> this in the index.
>
>     a-file
>     subdir/file1
>     subdir/file2
>     subdir/file3
>     the-last-file
>
> We write an entry for a-file as normal and move on to subdir/file1,
> where we realize the entry name for this level is simply just
> "subdir", write down an entry for "subdir" then jump three items ahead
> to the-last-file.
>
> That is what happens normally when the first file in subdir is not an
> i-t-a entry. If subdir/file1 is an i-t-a, because of the broken
> condition in this code, we still think "subdir" is an i-t-a file and
> not writing "subdir" down and jump to the-last-file. The result tree
> now only has two items: a-file and the-last-file. subdir should be
> there too (even though it only records two sub-entries, file2 and
> file3).
>
> If the i-t-a entry is subdir/file2 or subdir/file3, this is not a
> problem because we jump over them anyway. Which may explain why the
> bug is hidden for nearly four years.
>
> Fix it by making sure we only skip i-t-a entries when the entry in
> question is actual an index entry, not a directory.

Aha.  Good catch.

However, this makes me wonder if subdir has only files all of which
are i-t-a.  The resulting top-level tree object should not record
subdir/ as an empty (sub)directory in that case.  I do not see where
you are ensuring it in the patch below, though.

>
> Reported-by: Yuri Kanivetsky <yuri.kanivetsky@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  cache-tree.c          |  4 ++--
>  t/t2203-add-intent.sh | 13 +++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index ddf0cc9..c2676e8 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -319,7 +319,7 @@ static int update_one(struct cache_tree *it,
>  	i = 0;
>  	while (i < entries) {
>  		const struct cache_entry *ce = cache[i];
> -		struct cache_tree_sub *sub;
> +		struct cache_tree_sub *sub = NULL;
>  		const char *path, *slash;
>  		int pathlen, entlen;
>  		const unsigned char *sha1;
> @@ -375,7 +375,7 @@ static int update_one(struct cache_tree *it,
>  		 * they are not part of generated trees. Invalidate up
>  		 * to root to force cache-tree users to read elsewhere.
>  		 */
> -		if (ce_intent_to_add(ce)) {
> +		if (!sub && ce_intent_to_add(ce)) {
>  			to_invalidate = 1;
>  			continue;
>  		}
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 2a4a749..12d701c 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -82,5 +82,18 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'cache-tree does not ignore dir that has i-t-a entries' '
> +	git init ita-in-dir &&
> +	(
> +		cd ita-in-dir &&
> +		mkdir 2 &&
> +		touch 1 2/1 2/2 3 &&
> +		git add 1 2/2 3 &&
> +		git add -N 2/1 &&
> +		git commit -m comitted &&
> +		git ls-tree -r HEAD | grep 2/2
> +	)
> +'
> +
>  test_done