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

[Junio C Hamano]

Junio C Hamano  writes:

> Nguyễn Thái Ngọc Duy   writes:
>
>> 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.

Here is a fix-up to the test part to avoid "touch" if we are not
interested in timestamp, and to avoid "git" command in the upstream
of a pipe, plus an additional test to help further bugfix to make
sure a directory that becomes empty with this culling is not
included in the end result.

Thanks for looking into this topic.

 t/t2203-add-intent.sh | 28 +---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 12d701c..1fc8d3f 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -87,11 +87,33 @@ test_expect_success 'cache-tree does not ignore dir that 
has i-t-a entries' '
(
cd ita-in-dir &&
mkdir 2 &&
-   touch 1 2/1 2/2 3 &&
+   for f in 1 2/1 2/2 3
+   do
+   echo "$f" >"$f"
+   done &&
git add 1 2/2 3 &&
git add -N 2/1 &&
-   git commit -m comitted &&
-   git ls-tree -r HEAD | grep 2/2
+   git commit -m committed &&
+   git ls-tree -r HEAD >actual &&
+   grep 2/2 actual
+   )
+'
+
+test_expect_success 'cache-tree does skip dir that becomes empty' '
+   rm -fr ita-in-dir &&
+   git init ita-in-dir &&
+   (
+   cd ita-in-dir &&
+   mkdir 2 &&
+   for f in 1 2/1 2/2 3
+   do
+   echo "$f" >"$f"
+   done &&
+   git add 1 3 &&
+   git add -N 2/1 &&
+   git commit -m committed &&
+   git ls-tree HEAD >actual &&
+   ! grep 2 actual
)
 '
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   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 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Nguyễn Thái Ngọc Duy]

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.

Reported-by: Yuri Kanivetsky 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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
 
-- 
2.8.2.537.g0965dd9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html