threads / discuss / 42772

Git doesn't always add files to a commit (amend)

Subject: Git doesn't always add files to a commit (amend)

## tl;dr

30 messages between Jul 4, 2016 and Jul 18, 2016.

replies: 29people: 3as markdown or json

Yuri Kanivetsky· Jul 4, 2016, 13:50 UTC · lore

Hi,

When intent to add a directory is made (`git add -N`), and then contents of any but the first file is staged, `git commit -v --amend` doesn't add it to the commit, see for yourself:

    #!/usr/bin/env bash
    set -eu
    rm -rf 1
    mkdir 1
    cd 1
    git init
    echo 1 > 1 && git add 1 && git commit -m 1
    mkdir 2
    echo 2/1 > 2/1
    echo 2/2 > 2/2
    git add -N 2
    # git add 2/1   # this file is added
    git add 2/2   # as opposed to this one
    git commit --amend -m 1
    git --no-pager log -p
    git reset
    git --no-pager status

I'm running git-2.9.0.

Regards, Yuri

Duy Nguyen· Jul 4, 2016, 15:48 UTC · re: Yuri Kanivetsky · lore

Re: Git doesn't always add files to a commit (amend)

On Mon, Jul 04, 2016 at 04:50:38PM +0300, Yuri Kanivetsky wrote:

Show 5 quoted lines

> Hi,
> 
> When intent to add a directory is made (`git add -N`), and then
> contents of any but the first file is staged, `git commit -v --amend`
> doesn't add it to the commit

Oops, a bug since 2012. Thanks for the report. I know what's wrong and will prepare a proper patch in a couple of hours.

Show 28 quoted lines

> , see for yourself:
> 
>     #!/usr/bin/env bash
>     set -eu
>     rm -rf 1
>     mkdir 1
>     cd 1
>     git init
>     echo 1 > 1 && git add 1 && git commit -m 1
>     mkdir 2
>     echo 2/1 > 2/1
>     echo 2/2 > 2/2
>     git add -N 2
>     # git add 2/1   # this file is added
>     git add 2/2   # as opposed to this one
>     git commit --amend -m 1
>     git --no-pager log -p
>     git reset
>     git --no-pager status
> 
> I'm running git-2.9.0.
> 
> Regards,
> Yuri
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nguyễn Thái Ngọc Duy· Jul 4, 2016, 17:48 UTC · re: Yuri Kanivetsky · lore

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

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

-- 
2.8.2.537.g0965dd9

Junio C Hamano· Jul 6, 2016, 17:43 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

Show 34 quoted lines

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

Show 52 quoted lines

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

Junio C Hamano· Jul 6, 2016, 17:55 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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
 	)
 '

Nguyễn Thái Ngọc Duy· Jul 6, 2016, 18:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 0/2] cache-tree building fix in the presence of ita entries

1/2 is the same as before with some test fixups from Junio.

2/2 fixes the empty tree generation found by Junio. I split it out because the commit message in 1/2 is already long. And because it can happen even without 1/2 (i.e. yet another bug :( though really hard to happen).

Suppose a/b/c contains all i-t-a entries. Without 1/2, we skip adding 'c' to the tree object 'a/b' (by luck). But if 'a/b' has _nothing_ else but 'a/b/c' then again we will have an empty tree object, which should not be added in 'a'. It's a cascading effect all the way up.

Nguyễn Thái Ngọc Duy (2):
  cache-tree.c: fix i-t-a entry skipping directory updates sometimes
  cache-tree: do not generate empty trees as a result of all i-t-a subentries

 cache-tree.c          | 20 ++++++++++++++++++--
 t/t2203-add-intent.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 2 deletions(-)

-- 
2.8.2.537.g0965dd9

Nguyễn Thái Ngọc Duy· Jul 6, 2016, 18:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 1/2] cache-tree.c: fix i-t-a entry skipping directory updates sometimes

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.

    a-file
    subdir/file1
    subdir/file2
    subdir/file3
    the-last-file

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 <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 | 17 +++++++++++++++++
 2 files changed, 19 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..24aed2e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -82,5 +82,22 @@ 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 &&
+		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 committed &&
+		git ls-tree -r HEAD >actual &&
+		grep 2/2 actual
+	)
+'
+
 test_done

-- 
2.8.2.537.g0965dd9

Nguyễn Thái Ngọc Duy· Jul 6, 2016, 18:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

If a subdirectory contains nothing but i-t-a entries, we generate an empty tree object and add it to its parent tree. Which is wrong. Such a subdirectory should not be added. We ignore i-t-a files _and_ directories.

Note that this has a cascading effect. If subdir 'a/b/c' contains nothing but i-t-a entries, we ignore it. But then if 'a/b' contains only (the non-existing) 'a/b/c', then we should ignore 'a/b' while building 'a' too. And it goes all the way up to top directory.

Noticed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache-tree.c          | 16 ++++++++++++++++
 t/t2203-add-intent.sh | 12 ++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/cache-tree.c b/cache-tree.c
index c2676e8..75e73d7 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -380,6 +380,13 @@ static int update_one(struct cache_tree *it,
 			continue;
 		}
 
+		/*
+		 * "sub" can be an empty tree if all subentries are i-t-a.
+		 */
+		if (sub && sub->cache_tree->entry_count < 0 &&
+		    !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
+			continue;
+
 		strbuf_grow(&buffer, entlen + 100);
 		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
 		strbuf_add(&buffer, sha1, 20);
@@ -426,6 +433,15 @@ int cache_tree_update(struct index_state *istate, int flags)
 	i = update_one(it, cache, entries, "", 0, &skip, flags);
 	if (i < 0)
 		return i;
+	/*
+	 * Top dir can become empty if all entries are i-t-a (even
+	 * from subdirs). Note that we do allow to create an empty
+	 * tree from an empty index. Only error when an empty tree is
+	 * a result of the i-t-a thing.
+	 */
+	if (it->entry_count < 0 &&
+	    !hashcmp(it->sha1, EMPTY_TREE_SHA1_BIN))
+		return error(_("cannot build a tree from just intent-to-add entries"));
 	istate->cache_changed |= CACHE_TREE_CHANGED;
 	return 0;
 }
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 24aed2e..a19f06b 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -99,5 +99,17 @@ test_expect_success 'cache-tree does not ignore dir that has i-t-a entries' '
 	)
 '
 
+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 -p 1/2/3 &&
+		echo 4 >1/2/3/4 &&
+		git add -N 1/2/3/4 &&
+		test_must_fail git commit -m committed
+	)
+'
+
 test_done

-- 
2.8.2.537.g0965dd9

Junio C Hamano· Jul 6, 2016, 19:26 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

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

Show 13 quoted lines

> @@ -426,6 +433,15 @@ int cache_tree_update(struct index_state *istate, int flags)
>  	i = update_one(it, cache, entries, "", 0, &skip, flags);
>  	if (i < 0)
>  		return i;
> +	/*
> +	 * Top dir can become empty if all entries are i-t-a (even
> +	 * from subdirs). Note that we do allow to create an empty
> +	 * tree from an empty index. Only error when an empty tree is
> +	 * a result of the i-t-a thing.
> +	 */
> +	if (it->entry_count < 0 &&
> +	    !hashcmp(it->sha1, EMPTY_TREE_SHA1_BIN))
> +		return error(_("cannot build a tree from just intent-to-add entries"));

The test would not let you tell between the two possible ways the last step "git commit" fails.

Did it fail due to the protection this change adds (i.e. you should be checking if "git write-tree" fails if that is the case we want to cover), or did it fail because you recorded an empty tree as the root commit without giving the --allow-empty option?

In any case, I am not sure about the logic in the comment, either. "git commit --allow-empty" should be able to create a new commit without any files in it, no?

Show 24 quoted lines

>  	istate->cache_changed |= CACHE_TREE_CHANGED;
>  	return 0;
>  }
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 24aed2e..a19f06b 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -99,5 +99,17 @@ test_expect_success 'cache-tree does not ignore dir that has i-t-a entries' '
>  	)
>  '
>  
> +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 -p 1/2/3 &&
> +		echo 4 >1/2/3/4 &&
> +		git add -N 1/2/3/4 &&
> +		test_must_fail git commit -m committed
> +	)
> +'
> +
>  test_done

Duy Nguyen· Jul 7, 2016, 17:12 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

On Wed, Jul 06, 2016 at 12:26:19PM -0700, Junio C Hamano wrote:

Show 27 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> 
> > @@ -426,6 +433,15 @@ int cache_tree_update(struct index_state *istate, int flags)
> >  	i = update_one(it, cache, entries, "", 0, &skip, flags);
> >  	if (i < 0)
> >  		return i;
> > +	/*
> > +	 * Top dir can become empty if all entries are i-t-a (even
> > +	 * from subdirs). Note that we do allow to create an empty
> > +	 * tree from an empty index. Only error when an empty tree is
> > +	 * a result of the i-t-a thing.
> > +	 */
> > +	if (it->entry_count < 0 &&
> > +	    !hashcmp(it->sha1, EMPTY_TREE_SHA1_BIN))
> > +		return error(_("cannot build a tree from just intent-to-add entries"));
> 
> The test would not let you tell between the two possible ways the
> last step "git commit" fails.
> 
> Did it fail due to the protection this change adds (i.e. you should
> be checking if "git write-tree" fails if that is the case we want to
> cover), or did it fail because you recorded an empty tree as the
> root commit without giving the --allow-empty option?
> 
> In any case, I am not sure about the logic in the comment, either.
> "git commit --allow-empty" should be able to create a new commit
> without any files in it, no?

You're right. If an empty index can produce an empty tree, then an index full of i-t-a entries should also be able to produce an empty tree.

git-commit not failing when --allow-empty is not given is another (known) problem, where it miscounts the number of real index entries. It's not right to "fix" it in here.

I'll deal with that separately. Let's focus on cache-tree only this time. So how about this on top?

-- 8< --

diff --git a/cache-tree.c b/cache-tree.c
index 75e73d7..2d50640 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -433,15 +433,6 @@ int cache_tree_update(struct index_state *istate, int flags)
 	i = update_one(it, cache, entries, "", 0, &skip, flags);
 	if (i < 0)
 		return i;
-	/*
-	 * Top dir can become empty if all entries are i-t-a (even
-	 * from subdirs). Note that we do allow to create an empty
-	 * tree from an empty index. Only error when an empty tree is
-	 * a result of the i-t-a thing.
-	 */
-	if (it->entry_count < 0 &&
-	    !hashcmp(it->sha1, EMPTY_TREE_SHA1_BIN))
-		return error(_("cannot build a tree from just intent-to-add entries"));
 	istate->cache_changed |= CACHE_TREE_CHANGED;
 	return 0;
 }
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index a19f06b..1a01279 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -104,10 +104,17 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
 	git init ita-in-dir &&
 	(
 		cd ita-in-dir &&
+		echo ground >ground &&
+		git add ground &&
 		mkdir -p 1/2/3 &&
 		echo 4 >1/2/3/4 &&
 		git add -N 1/2/3/4 &&
-		test_must_fail git commit -m committed
+		git commit -m committed &&
+		git ls-tree -r HEAD >actual &&
+		cat >expected <<\EOF &&
+100644 blob b649b43b0708f5604ac912f5a15f7da2bad51a1b	ground
+EOF
+		test_cmp expected actual
 	)
 '
 
-- 8< --

> > +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 -p 1/2/3 &&
> > +		echo 4 >1/2/3/4 &&
> > +		git add -N 1/2/3/4 &&
> > +		test_must_fail git commit -m committed
> > +	)
> > +'
> > +
> >  test_done

Junio C Hamano· Jul 7, 2016, 18:52 UTC · re: Duy Nguyen · lore

Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

Duy Nguyen <pclouds@gmail.com> writes:

> I'll deal with that separately. Let's focus on cache-tree only this
> time. So how about this on top?

I was hoping that you would limit the scope of the test to check if write-tree does the right thing. i.e. not test "git commit", but test "git write-tree".

Duy Nguyen· Jul 8, 2016, 15:39 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

On Thu, Jul 07, 2016 at 11:52:58AM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > I'll deal with that separately. Let's focus on cache-tree only this
> > time. So how about this on top?
> 
> I was hoping that you would limit the scope of the test to check if
> write-tree does the right thing.  i.e. not test "git commit", but
> test "git write-tree".
>

Yeah that's better. So the squash patch is something like this

-- 8< --

diff --git a/cache-tree.c b/cache-tree.c
index 75e73d7..2d50640 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -433,15 +433,6 @@ int cache_tree_update(struct index_state *istate, int flags)
 	i = update_one(it, cache, entries, "", 0, &skip, flags);
 	if (i < 0)
 		return i;
-	/*
-	 * Top dir can become empty if all entries are i-t-a (even
-	 * from subdirs). Note that we do allow to create an empty
-	 * tree from an empty index. Only error when an empty tree is
-	 * a result of the i-t-a thing.
-	 */
-	if (it->entry_count < 0 &&
-	    !hashcmp(it->sha1, EMPTY_TREE_SHA1_BIN))
-		return error(_("cannot build a tree from just intent-to-add entries"));
 	istate->cache_changed |= CACHE_TREE_CHANGED;
 	return 0;
 }
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index a19f06b..80880b7 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -107,7 +107,9 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
 		mkdir -p 1/2/3 &&
 		echo 4 >1/2/3/4 &&
 		git add -N 1/2/3/4 &&
-		test_must_fail git commit -m committed
+		git write-tree >actual &&
+		echo 4b825dc642cb6eb9a060e54bf8d69288fbee4904 >empty-tree &&
+		test_cmp empty-tree actual
 	)
 '
 
-- 8< --
--
Duy

Junio C Hamano· Jul 8, 2016, 15:53 UTC · re: Duy Nguyen · lore

Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

Duy Nguyen <pclouds@gmail.com> writes:

> Yeah that's better. So the squash patch is something like this

Rather...

Show 12 quoted lines

> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index a19f06b..80880b7 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -107,7 +107,9 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
>  		mkdir -p 1/2/3 &&
>  		echo 4 >1/2/3/4 &&
>  		git add -N 1/2/3/4 &&
> -		test_must_fail git commit -m committed
> +		git write-tree >actual &&
> +		echo 4b825dc642cb6eb9a060e54bf8d69288fbee4904 >empty-tree &&
> +		test_cmp empty-tree actual

	written=$(git write-tree) &&
        git ls-tree "$written" >actual &&
        ! grep 1 actual

That way, we have one less thing to worry about when the hash function changes in the future. You may want to rename 1/2/3 to something more readable (e.g. dir/2/3) and grep for "dir" instead, though.

Duy Nguyen· Jul 8, 2016, 16:36 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

On Fri, Jul 8, 2016 at 5:53 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> Yeah that's better. So the squash patch is something like this
>
> Rather...
>
>> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
>> index a19f06b..80880b7 100755
>> --- a/t/t2203-add-intent.sh
>> +++ b/t/t2203-add-intent.sh
>> @@ -107,7 +107,9 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
>>               mkdir -p 1/2/3 &&
>>               echo 4 >1/2/3/4 &&
>>               git add -N 1/2/3/4 &&
>> -             test_must_fail git commit -m committed
>> +             git write-tree >actual &&
>> +             echo 4b825dc642cb6eb9a060e54bf8d69288fbee4904 >empty-tree &&
>> +             test_cmp empty-tree actual
>
>         written=$(git write-tree) &&
>         git ls-tree "$written" >actual &&
>         ! grep 1 actual
>
> That way, we have one less thing to worry about when the hash
> function changes in the future.  You may want to rename 1/2/3
> to something more readable (e.g. dir/2/3) and grep for "dir"
> instead, though.

I thought about that too, then did a grep which showed empty sha1 tree was used elsewhere. And thought of sending a patch to define $EMPTY_SHA1 in test-lib-functions.sh or somewhere so we don't hard code it everywhere, but I didn't. But yeah ls-tree works. The last line could be test_must_be_empty actual (a bit stricter than grep 1)

-- 
Duy

Junio C Hamano· Jul 8, 2016, 17:23 UTC · re: Duy Nguyen · lore

Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

Duy Nguyen <pclouds@gmail.com> writes:

Show 5 quoted lines

> I thought about that too, then did a grep which showed empty sha1 tree
> was used elsewhere. And thought of sending a patch to define
> $EMPTY_SHA1 in test-lib-functions.sh or somewhere so we don't hard
> code it everywhere, but I didn't. But yeah ls-tree works. The last
> line could be test_must_be_empty actual (a bit stricter than grep 1)

I am OK if you moved $EMPTY_TREE from t4010 to test-lib.sh, added $EMPTY_BLOB next to it while at it, and remove their definition from t1501.

t0000, t1100, t3404, t4054 and t5504 have references to 4b825dc64 that may want to be updated to use $EMPTY_TREE.

t5308, t7011, and t7012 defines HI_SHA1 and NULL_SHA1, which may want to be updated to use $EMPTY_BLOB.

t/lib-pack.sh has one instance of 69de29bb2d1d that may want to be updated to use $EMPTY_BLOB.

Nguyễn Thái Ngọc Duy· Jul 9, 2016, 05:23 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 0/4] cache-tree building fix in the presence of ita entries

v3 fixes 2/2 (which is 4/4 now), allowing cache-tree to generate an empty tree if the index contains nothing but i-t-a entries.

Since empty tree SHA-1 is involved and we don't want to make it harder to move away from SHA-1 in future, 1/2 and 2/2 are added to keep SHA-1 for empty tree (and blob while we're at it) in one place.

Note that I didn't make lib-pack.sh and t5308 use $_EMPTY_BLOB because the actual SHA-1 characters matter (t5308) and I'm not so sure about variable expansion in the case/esac block and not wanting to check all the shells out there again (lib-pack.sh).

Nguyễn Thái Ngọc Duy (4):
  test-lib.sh: introduce and use $_EMPTY_TREE
  test-lib.sh: introduce and use $_EMPTY_BLOB
  cache-tree.c: fix i-t-a entry skipping directory updates sometimes
  cache-tree: do not generate empty trees as a result of all i-t-a subentries

 cache-tree.c                         | 11 +++++++++--
 t/t0000-basic.sh                     |  2 +-
 t/t1011-read-tree-sparse-checkout.sh |  8 ++++----
 t/t1100-commit-tree-options.sh       |  2 +-
 t/t1700-split-index.sh               | 24 ++++++++++++------------
 t/t2203-add-intent.sh                | 31 +++++++++++++++++++++++++++++++
 t/t3102-ls-tree-wildcards.sh         |  8 ++++----
 t/t4010-diff-pathspec.sh             |  6 ++----
 t/t4054-diff-bogus-tree.sh           | 10 ++++------
 t/t5504-fetch-receive-strict.sh      |  4 ++--
 t/t7011-skip-worktree-reading.sh     | 12 +++++-------
 t/t7012-skip-worktree-writing.sh     | 10 ++++------
 t/t7063-status-untracked-cache.sh    |  6 +++---
 t/t7508-status.sh                    |  2 +-
 t/test-lib.sh                        |  5 ++++-
 15 files changed, 87 insertions(+), 54 deletions(-)

-- 
2.8.2.537.g0965dd9

Nguyễn Thái Ngọc Duy· Jul 9, 2016, 05:23 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 2/4] test-lib.sh: introduce and use $_EMPTY_BLOB

Similar to $_EMPTY_TREE this makes it easier to recognize this special SHA-1 and change hash later.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh |  8 ++++----
 t/t1700-split-index.sh               | 24 ++++++++++++------------
 t/t3102-ls-tree-wildcards.sh         |  8 ++++----
 t/t7011-skip-worktree-reading.sh     | 12 +++++-------
 t/t7012-skip-worktree-writing.sh     | 10 ++++------
 t/t7063-status-untracked-cache.sh    |  6 +++---
 t/t7508-status.sh                    |  2 +-
 t/test-lib.sh                        |  3 ++-
 8 files changed, 35 insertions(+), 38 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 0c74bee..21cfa2e 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -15,11 +15,11 @@ test_description='sparse checkout tests
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
 test_expect_success 'setup' '
-	cat >expected <<-\EOF &&
+	cat >expected <<-EOF &&
 	100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0	init.t
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/added
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/addedtoo
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	subsub/added
+	100644 $_EMPTY_BLOB 0	sub/added
+	100644 $_EMPTY_BLOB 0	sub/addedtoo
+	100644 $_EMPTY_BLOB 0	subsub/added
 	EOF
 	cat >expected.swt <<-\EOF &&
 	H init.t
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8aef49f..305f210 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -33,14 +33,14 @@ test_expect_success 'add one file' '
 	git update-index --add one &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $_EMPTY_BLOB 0	one
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
 	test-dump-split-index .git/index | sed "/^own/d" >actual &&
 	cat >expect <<EOF &&
 base $base
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $_EMPTY_BLOB 0	one
 replacements:
 deletions:
 EOF
@@ -51,7 +51,7 @@ test_expect_success 'disable split index' '
 	git update-index --no-split-index &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $_EMPTY_BLOB 0	one
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
@@ -67,7 +67,7 @@ test_expect_success 'enable split index again, "one" now belongs to base index"'
 	git update-index --split-index &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $_EMPTY_BLOB 0	one
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
@@ -105,7 +105,7 @@ test_expect_success 'add another file, which stays index' '
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
 100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0	one
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+100644 $_EMPTY_BLOB 0	two
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
@@ -113,7 +113,7 @@ EOF
 	q_to_tab >expect <<EOF &&
 $BASE
 100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0Q
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+100644 $_EMPTY_BLOB 0	two
 replacements: 0
 deletions:
 EOF
@@ -159,14 +159,14 @@ test_expect_success 'add original file back' '
 	git update-index --add one &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $_EMPTY_BLOB 0	one
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
 	test-dump-split-index .git/index | sed "/^own/d" >actual &&
 	cat >expect <<EOF &&
 $BASE
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $_EMPTY_BLOB 0	one
 replacements:
 deletions: 0
 EOF
@@ -178,8 +178,8 @@ test_expect_success 'add new file' '
 	git update-index --add two &&
 	git ls-files --stage >actual &&
 	cat >expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+100644 $_EMPTY_BLOB 0	one
+100644 $_EMPTY_BLOB 0	two
 EOF
 	test_cmp expect actual
 '
@@ -188,8 +188,8 @@ test_expect_success 'unify index, two files remain' '
 	git update-index --no-split-index &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+100644 $_EMPTY_BLOB 0	one
+100644 $_EMPTY_BLOB 0	two
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
diff --git a/t/t3102-ls-tree-wildcards.sh b/t/t3102-ls-tree-wildcards.sh
index 4d4b02e..5cf84fa 100755
--- a/t/t3102-ls-tree-wildcards.sh
+++ b/t/t3102-ls-tree-wildcards.sh
@@ -12,16 +12,16 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'ls-tree a[a] matches literally' '
-	cat >expect <<-\EOF &&
-	100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391	a[a]/three
+	cat >expect <<-EOF &&
+	100644 blob $_EMPTY_BLOB	a[a]/three
 	EOF
 	git ls-tree -r HEAD "a[a]" >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'ls-tree outside prefix' '
-	cat >expect <<-\EOF &&
-	100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391	../a[a]/three
+	cat >expect <<-EOF &&
+	100644 blob $_EMPTY_BLOB	../a[a]/three
 	EOF
 	( cd aa && git ls-tree -r HEAD "../a[a]"; ) >actual &&
 	test_cmp expect actual
diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh
index 88d60c1..7fe0c9f 100755
--- a/t/t7011-skip-worktree-reading.sh
+++ b/t/t7011-skip-worktree-reading.sh
@@ -23,17 +23,15 @@ S sub/1
 H sub/2
 EOF
 
-NULL_SHA1=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
-
 setup_absent() {
 	test -f 1 && rm 1
 	git update-index --remove 1 &&
-	git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+	git update-index --add --cacheinfo 100644 $_EMPTY_BLOB 1 &&
 	git update-index --skip-worktree 1
 }
 
 test_absent() {
-	echo "100644 $NULL_SHA1 0	1" > expected &&
+	echo "100644 $_EMPTY_BLOB 0	1" > expected &&
 	git ls-files --stage 1 > result &&
 	test_cmp expected result &&
 	test ! -f 1
@@ -42,12 +40,12 @@ test_absent() {
 setup_dirty() {
 	git update-index --force-remove 1 &&
 	echo dirty > 1 &&
-	git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+	git update-index --add --cacheinfo 100644 $_EMPTY_BLOB 1 &&
 	git update-index --skip-worktree 1
 }
 
 test_dirty() {
-	echo "100644 $NULL_SHA1 0	1" > expected &&
+	echo "100644 $_EMPTY_BLOB 0	1" > expected &&
 	git ls-files --stage 1 > result &&
 	test_cmp expected result &&
 	echo dirty > expected
@@ -120,7 +118,7 @@ test_expect_success 'grep with skip-worktree file' '
 	test "$(git grep --no-ext-grep test)" = "1:test"
 '
 
-echo ":000000 100644 $_z40 $NULL_SHA1 A	1" > expected
+echo ":000000 100644 $_z40 $_EMPTY_BLOB A	1" > expected
 test_expect_success 'diff-index does not examine skip-worktree absent entries' '
 	setup_absent &&
 	git diff-index HEAD -- 1 > result &&
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index 9ceaa40..003e36f 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -53,17 +53,15 @@ test_expect_success 'read-tree removes worktree, dirty case' '
 	git update-index --no-skip-worktree added
 '
 
-NULL_SHA1=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
-
 setup_absent() {
 	test -f 1 && rm 1
 	git update-index --remove 1 &&
-	git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+	git update-index --add --cacheinfo 100644 $_EMPTY_BLOB 1 &&
 	git update-index --skip-worktree 1
 }
 
 test_absent() {
-	echo "100644 $NULL_SHA1 0	1" > expected &&
+	echo "100644 $_EMPTY_BLOB 0	1" > expected &&
 	git ls-files --stage 1 > result &&
 	test_cmp expected result &&
 	test ! -f 1
@@ -72,12 +70,12 @@ test_absent() {
 setup_dirty() {
 	git update-index --force-remove 1 &&
 	echo dirty > 1 &&
-	git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+	git update-index --add --cacheinfo 100644 $_EMPTY_BLOB 1 &&
 	git update-index --skip-worktree 1
 }
 
 test_dirty() {
-	echo "100644 $NULL_SHA1 0	1" > expected &&
+	echo "100644 $_EMPTY_BLOB 0	1" > expected &&
 	git ls-files --stage 1 > result &&
 	test_cmp expected result &&
 	echo dirty > expected
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index a971884..8d23259 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -53,7 +53,7 @@ A  two
 EOF
 
 cat >../dump.expect <<EOF &&
-info/exclude e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
+info/exclude $_EMPTY_BLOB
 core.excludesfile 0000000000000000000000000000000000000000
 exclude_per_dir .gitignore
 flags 00000006
@@ -137,7 +137,7 @@ EOF
 test_expect_success 'verify untracked cache dump' '
 	test-dump-untracked-cache >../actual &&
 	cat >../expect <<EOF &&
-info/exclude e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
+info/exclude $_EMPTY_BLOB
 core.excludesfile 0000000000000000000000000000000000000000
 exclude_per_dir .gitignore
 flags 00000006
@@ -184,7 +184,7 @@ EOF
 test_expect_success 'verify untracked cache dump' '
 	test-dump-untracked-cache >../actual &&
 	cat >../expect <<EOF &&
-info/exclude e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
+info/exclude $_EMPTY_BLOB
 core.excludesfile 0000000000000000000000000000000000000000
 exclude_per_dir .gitignore
 flags 00000006
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c3ed7cb..2b534ec 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -803,7 +803,7 @@ EOF
 '
 
 cat >expect <<EOF
-:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	dir1/modified
+:100644 100644 $_EMPTY_BLOB 0000000000000000000000000000000000000000 M	dir1/modified
 EOF
 test_expect_success 'status refreshes the index' '
 	touch dir2/added &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 327caa5..e1785a2 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -163,6 +163,7 @@ _x40="$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
 _z40=0000000000000000000000000000000000000000
 
 _EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+_EMPTY_BLOB=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
 
 # Line feed
 LF='
@@ -172,7 +173,7 @@ LF='
 # when case-folding filenames
 u200c=$(printf '\342\200\214')
 
-export _x05 _x40 _z40 LF u200c _EMPTY_TREE
+export _x05 _x40 _z40 LF u200c _EMPTY_TREE _EMPTY_BLOB
 
 # Each test should start with something like this, after copyright notices:
 #

-- 
2.8.2.537.g0965dd9

Nguyễn Thái Ngọc Duy· Jul 9, 2016, 05:23 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes

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.

    a-file
    subdir/file1
    subdir/file2
    subdir/file3
    the-last-file

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 <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 | 17 +++++++++++++++++
 2 files changed, 19 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..24aed2e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -82,5 +82,22 @@ 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 &&
+		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 committed &&
+		git ls-tree -r HEAD >actual &&
+		grep 2/2 actual
+	)
+'
+
 test_done

-- 
2.8.2.537.g0965dd9

Nguyễn Thái Ngọc Duy· Jul 9, 2016, 05:23 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 4/4] cache-tree: do not generate empty trees as a result of all i-t-a subentries

If a subdirectory contains nothing but i-t-a entries, we generate an empty tree object and add it to its parent tree. Which is wrong. Such a subdirectory should not be added.

Noticed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache-tree.c          |  7 +++++++
 t/t2203-add-intent.sh | 14 ++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/cache-tree.c b/cache-tree.c
index c2676e8..2d50640 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -380,6 +380,13 @@ static int update_one(struct cache_tree *it,
 			continue;
 		}
 
+		/*
+		 * "sub" can be an empty tree if subentries are i-t-a.
+		 */
+		if (sub && sub->cache_tree->entry_count < 0 &&
+		    !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
+			continue;
+
 		strbuf_grow(&buffer, entlen + 100);
 		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
 		strbuf_add(&buffer, sha1, 20);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 24aed2e..f4b2fac 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -99,5 +99,19 @@ test_expect_success 'cache-tree does not ignore dir that has i-t-a entries' '
 	)
 '
 
+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 -p 1/2/3 &&
+		echo 4 >1/2/3/4 &&
+		git add -N 1/2/3/4 &&
+		git write-tree >actual &&
+		echo $_EMPTY_TREE >expected &&
+		test_cmp expected actual
+	)
+'
+
 test_done

-- 
2.8.2.537.g0965dd9

Junio C Hamano· Jul 12, 2016, 20:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v3 4/4] cache-tree: do not generate empty trees as a result of all i-t-a subentries

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

Show 15 quoted lines

> diff --git a/cache-tree.c b/cache-tree.c
> index c2676e8..2d50640 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -380,6 +380,13 @@ static int update_one(struct cache_tree *it,
>  			continue;
>  		}
>  
> +		/*
> +		 * "sub" can be an empty tree if subentries are i-t-a.
> +		 */
> +		if (sub && sub->cache_tree->entry_count < 0 &&
> +		    !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
> +			continue;
> +

Looks sensible, except that it is unclear if it is correct to assume that "subentries are ita" always equals to "entry_count < 0" here. I _think_ it is OK, as the function itself does use the latter as the sign that it hit to_invalidate=1 case when it returns.

Thanks.

Show 26 quoted lines

>  		strbuf_grow(&buffer, entlen + 100);
>  		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
>  		strbuf_add(&buffer, sha1, 20);
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 24aed2e..f4b2fac 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -99,5 +99,19 @@ test_expect_success 'cache-tree does not ignore dir that has i-t-a entries' '
>  	)
>  '
>  
> +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 -p 1/2/3 &&
> +		echo 4 >1/2/3/4 &&
> +		git add -N 1/2/3/4 &&
> +		git write-tree >actual &&
> +		echo $_EMPTY_TREE >expected &&
> +		test_cmp expected actual
> +	)
> +'
> +
>  test_done

Duy Nguyen· Jul 13, 2016, 14:59 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 4/4] cache-tree: do not generate empty trees as a result of all i-t-a subentries

On Tue, Jul 12, 2016 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> diff --git a/cache-tree.c b/cache-tree.c
>> index c2676e8..2d50640 100644
>> --- a/cache-tree.c
>> +++ b/cache-tree.c
>> @@ -380,6 +380,13 @@ static int update_one(struct cache_tree *it,
>>                       continue;
>>               }
>>
>> +             /*
>> +              * "sub" can be an empty tree if subentries are i-t-a.
>> +              */
>> +             if (sub && sub->cache_tree->entry_count < 0 &&
>> +                 !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
>> +                     continue;
>> +
>
> Looks sensible, except that it is unclear if it is correct to assume
> that "subentries are ita" always equals to "entry_count < 0" here.
> I _think_ it is OK, as the function itself does use the latter as
> the sign that it hit to_invalidate=1 case when it returns.

I had the same concern and double checked it. If one day we have a new entry_count < 0 case that's not i-t-a, we'll need to refactor this code a bit.

-- 
Duy

Nguyễn Thái Ngọc Duy· Jul 9, 2016, 05:23 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE

This is a special SHA1. Let's keep it at one place, easier to replace later when the hash change comes, easier to recognize. Start with an underscore to reduce the chances somebody may override it without realizing it's predefined.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t0000-basic.sh                |  2 +-
 t/t1100-commit-tree-options.sh  |  2 +-
 t/t4010-diff-pathspec.sh        |  6 ++----
 t/t4054-diff-bogus-tree.sh      | 10 ++++------
 t/t5504-fetch-receive-strict.sh |  4 ++--
 t/test-lib.sh                   |  4 +++-
 6 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 60811a3..48214e9 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -834,7 +834,7 @@ test_expect_success 'git write-tree should be able to write an empty tree' '
 '
 
 test_expect_success 'validate object ID of a known tree' '
-	test "$tree" = 4b825dc642cb6eb9a060e54bf8d69288fbee4904
+	test "$tree" = $_EMPTY_TREE
 '
 
 # Various types of objects
diff --git a/t/t1100-commit-tree-options.sh b/t/t1100-commit-tree-options.sh
index b7e9b4fc..2adb123 100755
--- a/t/t1100-commit-tree-options.sh
+++ b/t/t1100-commit-tree-options.sh
@@ -15,7 +15,7 @@ Also make sure that command line parser understands the normal
 . ./test-lib.sh
 
 cat >expected <<EOF
-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
+tree $_EMPTY_TREE
 author Author Name <author@email> 1117148400 +0000
 committer Committer Name <committer@email> 1117150200 +0000
 
diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index 43c488b..7c98912 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -78,16 +78,14 @@ test_expect_success 'diff-tree pathspec' '
 	test_cmp expected current
 '
 
-EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
-
 test_expect_success 'diff-tree with wildcard shows dir also matches' '
-	git diff-tree --name-only $EMPTY_TREE $tree -- "f*" >result &&
+	git diff-tree --name-only $_EMPTY_TREE $tree -- "f*" >result &&
 	echo file0 >expected &&
 	test_cmp expected result
 '
 
 test_expect_success 'diff-tree -r with wildcard' '
-	git diff-tree -r --name-only $EMPTY_TREE $tree -- "*file1" >result &&
+	git diff-tree -r --name-only $_EMPTY_TREE $tree -- "*file1" >result &&
 	echo path1/file1 >expected &&
 	test_cmp expected result
 '
diff --git a/t/t4054-diff-bogus-tree.sh b/t/t4054-diff-bogus-tree.sh
index 1d6efab..6b48512 100755
--- a/t/t4054-diff-bogus-tree.sh
+++ b/t/t4054-diff-bogus-tree.sh
@@ -3,8 +3,6 @@
 test_description='test diff with a bogus tree containing the null sha1'
 . ./test-lib.sh
 
-empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
-
 test_expect_success 'create bogus tree' '
 	bogus_tree=$(
 		printf "100644 fooQQQQQQQQQQQQQQQQQQQQQ" |
@@ -22,13 +20,13 @@ test_expect_success 'create tree with matching file' '
 
 test_expect_success 'raw diff shows null sha1 (addition)' '
 	echo ":000000 100644 $_z40 $_z40 A	foo" >expect &&
-	git diff-tree $empty_tree $bogus_tree >actual &&
+	git diff-tree $_EMPTY_TREE $bogus_tree >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'raw diff shows null sha1 (removal)' '
 	echo ":100644 000000 $_z40 $_z40 D	foo" >expect &&
-	git diff-tree $bogus_tree $empty_tree >actual &&
+	git diff-tree $bogus_tree $_EMPTY_TREE >actual &&
 	test_cmp expect actual
 '
 
@@ -57,11 +55,11 @@ test_expect_success 'raw diff shows null sha1 (index)' '
 '
 
 test_expect_success 'patch fails due to bogus sha1 (addition)' '
-	test_must_fail git diff-tree -p $empty_tree $bogus_tree
+	test_must_fail git diff-tree -p $_EMPTY_TREE $bogus_tree
 '
 
 test_expect_success 'patch fails due to bogus sha1 (removal)' '
-	test_must_fail git diff-tree -p $bogus_tree $empty_tree
+	test_must_fail git diff-tree -p $bogus_tree $_EMPTY_TREE
 '
 
 test_expect_success 'patch fails due to bogus sha1 (modification)' '
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 44f3d5f..39cc971 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -115,8 +115,8 @@ test_expect_success 'push with transfer.fsckobjects' '
 	test_cmp exp act
 '
 
-cat >bogus-commit <<\EOF
-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
+cat >bogus-commit <<EOF
+tree $_EMPTY_TREE
 author Bugs Bunny 1234567890 +0000
 committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0055ebb..327caa5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -162,6 +162,8 @@ _x40="$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
 # Zero SHA-1
 _z40=0000000000000000000000000000000000000000
 
+_EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+
 # Line feed
 LF='
 '
@@ -170,7 +172,7 @@ LF='
 # when case-folding filenames
 u200c=$(printf '\342\200\214')
 
-export _x05 _x40 _z40 LF u200c
+export _x05 _x40 _z40 LF u200c _EMPTY_TREE
 
 # Each test should start with something like this, after copyright notices:
 #

-- 
2.8.2.537.g0965dd9

Junio C Hamano· Jul 12, 2016, 20:40 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE

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

Show 26 quoted lines

> This is a special SHA1. Let's keep it at one place, easier to replace
> later when the hash change comes, easier to recognize. Start with an
> underscore to reduce the chances somebody may override it without
> realizing it's predefined.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t0000-basic.sh                |  2 +-
>  t/t1100-commit-tree-options.sh  |  2 +-
>  t/t4010-diff-pathspec.sh        |  6 ++----
>  t/t4054-diff-bogus-tree.sh      | 10 ++++------
>  t/t5504-fetch-receive-strict.sh |  4 ++--
>  t/test-lib.sh                   |  4 +++-
>  6 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 60811a3..48214e9 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -834,7 +834,7 @@ test_expect_success 'git write-tree should be able to write an empty tree' '
>  '
>  
>  test_expect_success 'validate object ID of a known tree' '
> -	test "$tree" = 4b825dc642cb6eb9a060e54bf8d69288fbee4904
> +	test "$tree" = $_EMPTY_TREE
>  '

I doubt the point of, and I'd rather not to see, the leading underscore. Are there existing uses of the name that want it to mean something different?

Duy Nguyen· Jul 13, 2016, 15:04 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE

On Tue, Jul 12, 2016 at 10:40 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 32 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> This is a special SHA1. Let's keep it at one place, easier to replace
>> later when the hash change comes, easier to recognize. Start with an
>> underscore to reduce the chances somebody may override it without
>> realizing it's predefined.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  t/t0000-basic.sh                |  2 +-
>>  t/t1100-commit-tree-options.sh  |  2 +-
>>  t/t4010-diff-pathspec.sh        |  6 ++----
>>  t/t4054-diff-bogus-tree.sh      | 10 ++++------
>>  t/t5504-fetch-receive-strict.sh |  4 ++--
>>  t/test-lib.sh                   |  4 +++-
>>  6 files changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
>> index 60811a3..48214e9 100755
>> --- a/t/t0000-basic.sh
>> +++ b/t/t0000-basic.sh
>> @@ -834,7 +834,7 @@ test_expect_success 'git write-tree should be able to write an empty tree' '
>>  '
>>
>>  test_expect_success 'validate object ID of a known tree' '
>> -     test "$tree" = 4b825dc642cb6eb9a060e54bf8d69288fbee4904
>> +     test "$tree" = $_EMPTY_TREE
>>  '
>
> I doubt the point of, and I'd rather not to see, the leading
> underscore.  Are there existing uses of the name that want it to
> mean something different?

No. There is EMPTY_TREE in use, but it's exactly what we expect. It's probably still a good idea to separate "global" variables from per-file ones. But I don't feel strongly about this, so if a re-roll is required (or somebody votes for underscore removal, including you), I'll remove the underscore.

-- 
Duy

Nguyễn Thái Ngọc Duy· Jul 16, 2016, 05:06 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v4 0/4] cache-tree building fix in the presence of ita entries

v4 removes the leading underscore from _EMPTY_BLOB and _EMPTY_TREE and updates 4/4 slightly like this.

diff --git a/cache-tree.c b/cache-tree.c
index 2d50640..f28b1f4 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -325,6 +325,7 @@ static int update_one(struct cache_tree *it,
 		const unsigned char *sha1;
 		unsigned mode;
 		int expected_missing = 0;
+		int contains_ita = 0;
 
 		path = ce->name;
 		pathlen = ce_namelen(ce);
@@ -341,7 +342,8 @@ static int update_one(struct cache_tree *it,
 			i += sub->count;
 			sha1 = sub->cache_tree->sha1;
 			mode = S_IFDIR;
-			if (sub->cache_tree->entry_count < 0) {
+			contains_ita = sub->cache_tree->entry_count < 0;
+			if (contains_ita) {
 				to_invalidate = 1;
 				expected_missing = 1;
 			}
@@ -381,10 +383,9 @@ static int update_one(struct cache_tree *it,
 		}
 
 		/*
-		 * "sub" can be an empty tree if subentries are i-t-a.
+		 * "sub" can be an empty tree if all subentries are i-t-a.
 		 */
-		if (sub && sub->cache_tree->entry_count < 0 &&
-		    !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
+		if (contains_ita && !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
 			continue;
 
 		strbuf_grow(&buffer, entlen + 100);

Nguyễn Thái Ngọc Duy (4):
  test-lib.sh: introduce and use $EMPTY_TREE
  test-lib.sh: introduce and use $EMPTY_BLOB
  cache-tree.c: fix i-t-a entry skipping directory updates sometimes
  cache-tree: do not generate empty trees as a result of all i-t-a
    subentries

 cache-tree.c                         | 14 +++++++++++---
 t/t0000-basic.sh                     |  2 +-
 t/t1011-read-tree-sparse-checkout.sh |  8 ++++----
 t/t1100-commit-tree-options.sh       |  2 +-
 t/t1700-split-index.sh               | 24 ++++++++++++------------
 t/t2203-add-intent.sh                | 31 +++++++++++++++++++++++++++++++
 t/t3102-ls-tree-wildcards.sh         |  8 ++++----
 t/t4010-diff-pathspec.sh             |  2 --
 t/t4054-diff-bogus-tree.sh           | 10 ++++------
 t/t5504-fetch-receive-strict.sh      |  4 ++--
 t/t7011-skip-worktree-reading.sh     | 12 +++++-------
 t/t7012-skip-worktree-writing.sh     | 10 ++++------
 t/t7063-status-untracked-cache.sh    |  6 +++---
 t/t7508-status.sh                    |  2 +-
 t/test-lib.sh                        |  5 ++++-
 15 files changed, 87 insertions(+), 53 deletions(-)

-- 
2.9.1.566.gbd532d4

Nguyễn Thái Ngọc Duy· Jul 16, 2016, 05:06 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v4 1/4] test-lib.sh: introduce and use $EMPTY_TREE

This is a special SHA1. Let's keep it at one place, easier to replace later when the hash change comes, easier to recognize.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t0000-basic.sh                |  2 +-
 t/t1100-commit-tree-options.sh  |  2 +-
 t/t4010-diff-pathspec.sh        |  2 --
 t/t4054-diff-bogus-tree.sh      | 10 ++++------
 t/t5504-fetch-receive-strict.sh |  4 ++--
 t/test-lib.sh                   |  4 +++-
 6 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 60811a3..1aa5093 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -834,7 +834,7 @@ test_expect_success 'git write-tree should be able to write an empty tree' '
 '
 
 test_expect_success 'validate object ID of a known tree' '
-	test "$tree" = 4b825dc642cb6eb9a060e54bf8d69288fbee4904
+	test "$tree" = $EMPTY_TREE
 '
 
 # Various types of objects
diff --git a/t/t1100-commit-tree-options.sh b/t/t1100-commit-tree-options.sh
index b7e9b4fc..ae66ba5 100755
--- a/t/t1100-commit-tree-options.sh
+++ b/t/t1100-commit-tree-options.sh
@@ -15,7 +15,7 @@ Also make sure that command line parser understands the normal
 . ./test-lib.sh
 
 cat >expected <<EOF
-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
+tree $EMPTY_TREE
 author Author Name <author@email> 1117148400 +0000
 committer Committer Name <committer@email> 1117150200 +0000
 
diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index 43c488b..35b35a8 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -78,8 +78,6 @@ test_expect_success 'diff-tree pathspec' '
 	test_cmp expected current
 '
 
-EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
-
 test_expect_success 'diff-tree with wildcard shows dir also matches' '
 	git diff-tree --name-only $EMPTY_TREE $tree -- "f*" >result &&
 	echo file0 >expected &&
diff --git a/t/t4054-diff-bogus-tree.sh b/t/t4054-diff-bogus-tree.sh
index 1d6efab..18f42c5 100755
--- a/t/t4054-diff-bogus-tree.sh
+++ b/t/t4054-diff-bogus-tree.sh
@@ -3,8 +3,6 @@
 test_description='test diff with a bogus tree containing the null sha1'
 . ./test-lib.sh
 
-empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
-
 test_expect_success 'create bogus tree' '
 	bogus_tree=$(
 		printf "100644 fooQQQQQQQQQQQQQQQQQQQQQ" |
@@ -22,13 +20,13 @@ test_expect_success 'create tree with matching file' '
 
 test_expect_success 'raw diff shows null sha1 (addition)' '
 	echo ":000000 100644 $_z40 $_z40 A	foo" >expect &&
-	git diff-tree $empty_tree $bogus_tree >actual &&
+	git diff-tree $EMPTY_TREE $bogus_tree >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'raw diff shows null sha1 (removal)' '
 	echo ":100644 000000 $_z40 $_z40 D	foo" >expect &&
-	git diff-tree $bogus_tree $empty_tree >actual &&
+	git diff-tree $bogus_tree $EMPTY_TREE >actual &&
 	test_cmp expect actual
 '
 
@@ -57,11 +55,11 @@ test_expect_success 'raw diff shows null sha1 (index)' '
 '
 
 test_expect_success 'patch fails due to bogus sha1 (addition)' '
-	test_must_fail git diff-tree -p $empty_tree $bogus_tree
+	test_must_fail git diff-tree -p $EMPTY_TREE $bogus_tree
 '
 
 test_expect_success 'patch fails due to bogus sha1 (removal)' '
-	test_must_fail git diff-tree -p $bogus_tree $empty_tree
+	test_must_fail git diff-tree -p $bogus_tree $EMPTY_TREE
 '
 
 test_expect_success 'patch fails due to bogus sha1 (modification)' '
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 44f3d5f..9b19cff 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -115,8 +115,8 @@ test_expect_success 'push with transfer.fsckobjects' '
 	test_cmp exp act
 '
 
-cat >bogus-commit <<\EOF
-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
+cat >bogus-commit <<EOF
+tree $EMPTY_TREE
 author Bugs Bunny 1234567890 +0000
 committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0055ebb..85f4c6d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -162,6 +162,8 @@ _x40="$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
 # Zero SHA-1
 _z40=0000000000000000000000000000000000000000
 
+EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+
 # Line feed
 LF='
 '
@@ -170,7 +172,7 @@ LF='
 # when case-folding filenames
 u200c=$(printf '\342\200\214')
 
-export _x05 _x40 _z40 LF u200c
+export _x05 _x40 _z40 LF u200c EMPTY_TREE
 
 # Each test should start with something like this, after copyright notices:
 #

-- 
2.9.1.566.gbd532d4

Nguyễn Thái Ngọc Duy· Jul 16, 2016, 05:06 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v4 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes

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.

    a-file
    subdir/file1
    subdir/file2
    subdir/file3
    the-last-file

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 <yuri.kanivetsky@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache-tree.c          |  4 ++--
 t/t2203-add-intent.sh | 17 +++++++++++++++++
 2 files changed, 19 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..24aed2e 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -82,5 +82,22 @@ 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 &&
+		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 committed &&
+		git ls-tree -r HEAD >actual &&
+		grep 2/2 actual
+	)
+'
+
 test_done

-- 
2.9.1.566.gbd532d4

Nguyễn Thái Ngọc Duy· Jul 16, 2016, 05:06 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v4 4/4] cache-tree: do not generate empty trees as a result of all i-t-a subentries

If a subdirectory contains nothing but i-t-a entries, we generate an empty tree object and add it to its parent tree. Which is wrong. Such a subdirectory should not be added.

Noticed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache-tree.c          | 10 +++++++++-
 t/t2203-add-intent.sh | 14 ++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/cache-tree.c b/cache-tree.c
index c2676e8..f28b1f4 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -325,6 +325,7 @@ static int update_one(struct cache_tree *it,
 		const unsigned char *sha1;
 		unsigned mode;
 		int expected_missing = 0;
+		int contains_ita = 0;
 
 		path = ce->name;
 		pathlen = ce_namelen(ce);
@@ -341,7 +342,8 @@ static int update_one(struct cache_tree *it,
 			i += sub->count;
 			sha1 = sub->cache_tree->sha1;
 			mode = S_IFDIR;
-			if (sub->cache_tree->entry_count < 0) {
+			contains_ita = sub->cache_tree->entry_count < 0;
+			if (contains_ita) {
 				to_invalidate = 1;
 				expected_missing = 1;
 			}
@@ -380,6 +382,12 @@ static int update_one(struct cache_tree *it,
 			continue;
 		}
 
+		/*
+		 * "sub" can be an empty tree if all subentries are i-t-a.
+		 */
+		if (contains_ita && !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
+			continue;
+
 		strbuf_grow(&buffer, entlen + 100);
 		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
 		strbuf_add(&buffer, sha1, 20);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 24aed2e..8f22c43 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -99,5 +99,19 @@ test_expect_success 'cache-tree does not ignore dir that has i-t-a entries' '
 	)
 '
 
+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 -p 1/2/3 &&
+		echo 4 >1/2/3/4 &&
+		git add -N 1/2/3/4 &&
+		git write-tree >actual &&
+		echo $EMPTY_TREE >expected &&
+		test_cmp expected actual
+	)
+'
+
 test_done

-- 
2.9.1.566.gbd532d4

Nguyễn Thái Ngọc Duy· Jul 16, 2016, 05:06 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v4 2/4] test-lib.sh: introduce and use $EMPTY_BLOB

Similar to $EMPTY_TREE this makes it easier to recognize this special SHA-1 and change hash later.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t1011-read-tree-sparse-checkout.sh |  8 ++++----
 t/t1700-split-index.sh               | 24 ++++++++++++------------
 t/t3102-ls-tree-wildcards.sh         |  8 ++++----
 t/t7011-skip-worktree-reading.sh     | 12 +++++-------
 t/t7012-skip-worktree-writing.sh     | 10 ++++------
 t/t7063-status-untracked-cache.sh    |  6 +++---
 t/t7508-status.sh                    |  2 +-
 t/test-lib.sh                        |  3 ++-
 8 files changed, 35 insertions(+), 38 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 0c74bee..2563d18 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -15,11 +15,11 @@ test_description='sparse checkout tests
 . "$TEST_DIRECTORY"/lib-read-tree.sh
 
 test_expect_success 'setup' '
-	cat >expected <<-\EOF &&
+	cat >expected <<-EOF &&
 	100644 77f0ba1734ed79d12881f81b36ee134de6a3327b 0	init.t
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/added
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	sub/addedtoo
-	100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	subsub/added
+	100644 $EMPTY_BLOB 0	sub/added
+	100644 $EMPTY_BLOB 0	sub/addedtoo
+	100644 $EMPTY_BLOB 0	subsub/added
 	EOF
 	cat >expected.swt <<-\EOF &&
 	H init.t
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8aef49f..292a072 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -33,14 +33,14 @@ test_expect_success 'add one file' '
 	git update-index --add one &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $EMPTY_BLOB 0	one
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
 	test-dump-split-index .git/index | sed "/^own/d" >actual &&
 	cat >expect <<EOF &&
 base $base
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $EMPTY_BLOB 0	one
 replacements:
 deletions:
 EOF
@@ -51,7 +51,7 @@ test_expect_success 'disable split index' '
 	git update-index --no-split-index &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $EMPTY_BLOB 0	one
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
@@ -67,7 +67,7 @@ test_expect_success 'enable split index again, "one" now belongs to base index"'
 	git update-index --split-index &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $EMPTY_BLOB 0	one
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
@@ -105,7 +105,7 @@ test_expect_success 'add another file, which stays index' '
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
 100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0	one
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+100644 $EMPTY_BLOB 0	two
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
@@ -113,7 +113,7 @@ EOF
 	q_to_tab >expect <<EOF &&
 $BASE
 100644 2e0996000b7e9019eabcad29391bf0f5c7702f0b 0Q
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+100644 $EMPTY_BLOB 0	two
 replacements: 0
 deletions:
 EOF
@@ -159,14 +159,14 @@ test_expect_success 'add original file back' '
 	git update-index --add one &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $EMPTY_BLOB 0	one
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
 	test-dump-split-index .git/index | sed "/^own/d" >actual &&
 	cat >expect <<EOF &&
 $BASE
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
+100644 $EMPTY_BLOB 0	one
 replacements:
 deletions: 0
 EOF
@@ -178,8 +178,8 @@ test_expect_success 'add new file' '
 	git update-index --add two &&
 	git ls-files --stage >actual &&
 	cat >expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+100644 $EMPTY_BLOB 0	one
+100644 $EMPTY_BLOB 0	two
 EOF
 	test_cmp expect actual
 '
@@ -188,8 +188,8 @@ test_expect_success 'unify index, two files remain' '
 	git update-index --no-split-index &&
 	git ls-files --stage >ls-files.actual &&
 	cat >ls-files.expect <<EOF &&
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	one
-100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	two
+100644 $EMPTY_BLOB 0	one
+100644 $EMPTY_BLOB 0	two
 EOF
 	test_cmp ls-files.expect ls-files.actual &&
 
diff --git a/t/t3102-ls-tree-wildcards.sh b/t/t3102-ls-tree-wildcards.sh
index 4d4b02e..e804377 100755
--- a/t/t3102-ls-tree-wildcards.sh
+++ b/t/t3102-ls-tree-wildcards.sh
@@ -12,16 +12,16 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'ls-tree a[a] matches literally' '
-	cat >expect <<-\EOF &&
-	100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391	a[a]/three
+	cat >expect <<-EOF &&
+	100644 blob $EMPTY_BLOB	a[a]/three
 	EOF
 	git ls-tree -r HEAD "a[a]" >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'ls-tree outside prefix' '
-	cat >expect <<-\EOF &&
-	100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391	../a[a]/three
+	cat >expect <<-EOF &&
+	100644 blob $EMPTY_BLOB	../a[a]/three
 	EOF
 	( cd aa && git ls-tree -r HEAD "../a[a]"; ) >actual &&
 	test_cmp expect actual
diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh
index 88d60c1..84f4145 100755
--- a/t/t7011-skip-worktree-reading.sh
+++ b/t/t7011-skip-worktree-reading.sh
@@ -23,17 +23,15 @@ S sub/1
 H sub/2
 EOF
 
-NULL_SHA1=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
-
 setup_absent() {
 	test -f 1 && rm 1
 	git update-index --remove 1 &&
-	git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+	git update-index --add --cacheinfo 100644 $EMPTY_BLOB 1 &&
 	git update-index --skip-worktree 1
 }
 
 test_absent() {
-	echo "100644 $NULL_SHA1 0	1" > expected &&
+	echo "100644 $EMPTY_BLOB 0	1" > expected &&
 	git ls-files --stage 1 > result &&
 	test_cmp expected result &&
 	test ! -f 1
@@ -42,12 +40,12 @@ test_absent() {
 setup_dirty() {
 	git update-index --force-remove 1 &&
 	echo dirty > 1 &&
-	git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+	git update-index --add --cacheinfo 100644 $EMPTY_BLOB 1 &&
 	git update-index --skip-worktree 1
 }
 
 test_dirty() {
-	echo "100644 $NULL_SHA1 0	1" > expected &&
+	echo "100644 $EMPTY_BLOB 0	1" > expected &&
 	git ls-files --stage 1 > result &&
 	test_cmp expected result &&
 	echo dirty > expected
@@ -120,7 +118,7 @@ test_expect_success 'grep with skip-worktree file' '
 	test "$(git grep --no-ext-grep test)" = "1:test"
 '
 
-echo ":000000 100644 $_z40 $NULL_SHA1 A	1" > expected
+echo ":000000 100644 $_z40 $EMPTY_BLOB A	1" > expected
 test_expect_success 'diff-index does not examine skip-worktree absent entries' '
 	setup_absent &&
 	git diff-index HEAD -- 1 > result &&
diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh
index 9ceaa40..9d1abe5 100755
--- a/t/t7012-skip-worktree-writing.sh
+++ b/t/t7012-skip-worktree-writing.sh
@@ -53,17 +53,15 @@ test_expect_success 'read-tree removes worktree, dirty case' '
 	git update-index --no-skip-worktree added
 '
 
-NULL_SHA1=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
-
 setup_absent() {
 	test -f 1 && rm 1
 	git update-index --remove 1 &&
-	git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+	git update-index --add --cacheinfo 100644 $EMPTY_BLOB 1 &&
 	git update-index --skip-worktree 1
 }
 
 test_absent() {
-	echo "100644 $NULL_SHA1 0	1" > expected &&
+	echo "100644 $EMPTY_BLOB 0	1" > expected &&
 	git ls-files --stage 1 > result &&
 	test_cmp expected result &&
 	test ! -f 1
@@ -72,12 +70,12 @@ test_absent() {
 setup_dirty() {
 	git update-index --force-remove 1 &&
 	echo dirty > 1 &&
-	git update-index --add --cacheinfo 100644 $NULL_SHA1 1 &&
+	git update-index --add --cacheinfo 100644 $EMPTY_BLOB 1 &&
 	git update-index --skip-worktree 1
 }
 
 test_dirty() {
-	echo "100644 $NULL_SHA1 0	1" > expected &&
+	echo "100644 $EMPTY_BLOB 0	1" > expected &&
 	git ls-files --stage 1 > result &&
 	test_cmp expected result &&
 	echo dirty > expected
diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index a971884..a828a5f 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -53,7 +53,7 @@ A  two
 EOF
 
 cat >../dump.expect <<EOF &&
-info/exclude e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
+info/exclude $EMPTY_BLOB
 core.excludesfile 0000000000000000000000000000000000000000
 exclude_per_dir .gitignore
 flags 00000006
@@ -137,7 +137,7 @@ EOF
 test_expect_success 'verify untracked cache dump' '
 	test-dump-untracked-cache >../actual &&
 	cat >../expect <<EOF &&
-info/exclude e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
+info/exclude $EMPTY_BLOB
 core.excludesfile 0000000000000000000000000000000000000000
 exclude_per_dir .gitignore
 flags 00000006
@@ -184,7 +184,7 @@ EOF
 test_expect_success 'verify untracked cache dump' '
 	test-dump-untracked-cache >../actual &&
 	cat >../expect <<EOF &&
-info/exclude e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
+info/exclude $EMPTY_BLOB
 core.excludesfile 0000000000000000000000000000000000000000
 exclude_per_dir .gitignore
 flags 00000006
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c3ed7cb..a42aef8 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -803,7 +803,7 @@ EOF
 '
 
 cat >expect <<EOF
-:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	dir1/modified
+:100644 100644 $EMPTY_BLOB 0000000000000000000000000000000000000000 M	dir1/modified
 EOF
 test_expect_success 'status refreshes the index' '
 	touch dir2/added &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 85f4c6d..9f36091 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -163,6 +163,7 @@ _x40="$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
 _z40=0000000000000000000000000000000000000000
 
 EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+EMPTY_BLOB=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
 
 # Line feed
 LF='
@@ -172,7 +173,7 @@ LF='
 # when case-folding filenames
 u200c=$(printf '\342\200\214')
 
-export _x05 _x40 _z40 LF u200c EMPTY_TREE
+export _x05 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB
 
 # Each test should start with something like this, after copyright notices:
 #

-- 
2.9.1.566.gbd532d4

Junio C Hamano· Jul 18, 2016, 20:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v4 0/4] cache-tree building fix in the presence of ita entries

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

Show 38 quoted lines

> v4 removes the leading underscore from _EMPTY_BLOB and _EMPTY_TREE and
> updates 4/4 slightly like this.
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 2d50640..f28b1f4 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -325,6 +325,7 @@ static int update_one(struct cache_tree *it,
>  		const unsigned char *sha1;
>  		unsigned mode;
>  		int expected_missing = 0;
> +		int contains_ita = 0;
>  
>  		path = ce->name;
>  		pathlen = ce_namelen(ce);
> @@ -341,7 +342,8 @@ static int update_one(struct cache_tree *it,
>  			i += sub->count;
>  			sha1 = sub->cache_tree->sha1;
>  			mode = S_IFDIR;
> -			if (sub->cache_tree->entry_count < 0) {
> +			contains_ita = sub->cache_tree->entry_count < 0;
> +			if (contains_ita) {
>  				to_invalidate = 1;
>  				expected_missing = 1;
>  			}
> @@ -381,10 +383,9 @@ static int update_one(struct cache_tree *it,
>  		}
>  
>  		/*
> -		 * "sub" can be an empty tree if subentries are i-t-a.
> +		 * "sub" can be an empty tree if all subentries are i-t-a.
>  		 */
> -		if (sub && sub->cache_tree->entry_count < 0 &&
> -		    !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
> +		if (contains_ita && !hashcmp(sha1, EMPTY_TREE_SHA1_BIN))
>  			continue;
>  
>  		strbuf_grow(&buffer, entlen + 100);

This makes quite a lot of sense; even though I do not think it would change the behaviour within the context of current code, it definitely is easier to understand and prevents future mistakes.

Will queue.

Thanks.