* Git doesn't always add files to a commit (amend)
@ 2016-07-04 13:50 Yuri Kanivetsky
2016-07-04 15:48 ` Duy Nguyen
2016-07-04 17:48 ` [PATCH] cache-tree.c: fix i-t-a check skipping directory updates sometimes Nguyễn Thái Ngọc Duy
0 siblings, 2 replies; 30+ messages in thread
From: Yuri Kanivetsky @ 2016-07-04 13:50 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Git doesn't always add files to a commit (amend) 2016-07-04 13:50 Git doesn't always add files to a commit (amend) Yuri Kanivetsky @ 2016-07-04 15:48 ` Duy Nguyen 2016-07-04 17:48 ` [PATCH] cache-tree.c: fix i-t-a check skipping directory updates sometimes Nguyễn Thái Ngọc Duy 1 sibling, 0 replies; 30+ messages in thread From: Duy Nguyen @ 2016-07-04 15:48 UTC (permalink / raw) To: Yuri Kanivetsky; +Cc: git On Mon, Jul 04, 2016 at 04:50:38PM +0300, Yuri Kanivetsky wrote: > 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. > , 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH] cache-tree.c: fix i-t-a check skipping directory updates sometimes 2016-07-04 13:50 Git doesn't always add files to a commit (amend) Yuri Kanivetsky 2016-07-04 15:48 ` Duy Nguyen @ 2016-07-04 17:48 ` Nguyễn Thái Ngọc Duy 2016-07-06 17:43 ` Junio C Hamano 2016-07-06 18:48 ` [PATCH v2 0/2] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 1 sibling, 2 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-04 17:48 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, 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 <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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] cache-tree.c: fix i-t-a check skipping directory updates sometimes 2016-07-04 17:48 ` [PATCH] cache-tree.c: fix i-t-a check skipping directory updates sometimes Nguyễn Thái Ngọc Duy @ 2016-07-06 17:43 ` Junio C Hamano 2016-07-06 17:55 ` Junio C Hamano 2016-07-06 18:48 ` [PATCH v2 0/2] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 1 sibling, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2016-07-06 17:43 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, yuri.kanivetsky 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] cache-tree.c: fix i-t-a check skipping directory updates sometimes 2016-07-06 17:43 ` Junio C Hamano @ 2016-07-06 17:55 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2016-07-06 17:55 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, yuri.kanivetsky Junio C Hamano <gitster@pobox.com> writes: > 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 ) ' ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 0/2] cache-tree building fix in the presence of ita entries 2016-07-04 17:48 ` [PATCH] cache-tree.c: fix i-t-a check skipping directory updates sometimes Nguyễn Thái Ngọc Duy 2016-07-06 17:43 ` Junio C Hamano @ 2016-07-06 18:48 ` Nguyễn Thái Ngọc Duy 2016-07-06 18:48 ` [PATCH v2 1/2] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy ` (2 more replies) 1 sibling, 3 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-06 18:48 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] cache-tree.c: fix i-t-a entry skipping directory updates sometimes 2016-07-06 18:48 ` [PATCH v2 0/2] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy @ 2016-07-06 18:48 ` Nguyễn Thái Ngọc Duy 2016-07-06 18:48 ` [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 2016-07-09 5:23 ` [PATCH v3 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2 siblings, 0 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-06 18:48 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, 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 <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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-06 18:48 ` [PATCH v2 0/2] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2016-07-06 18:48 ` [PATCH v2 1/2] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy @ 2016-07-06 18:48 ` Nguyễn Thái Ngọc Duy 2016-07-06 19:26 ` Junio C Hamano 2016-07-09 5:23 ` [PATCH v3 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2 siblings, 1 reply; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-06 18:48 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-06 18:48 ` [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 @ 2016-07-06 19:26 ` Junio C Hamano 2016-07-07 17:12 ` Duy Nguyen 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2016-07-06 19:26 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, yuri.kanivetsky 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? > 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-06 19:26 ` Junio C Hamano @ 2016-07-07 17:12 ` Duy Nguyen 2016-07-07 18:52 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Duy Nguyen @ 2016-07-07 17:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, yuri.kanivetsky On Wed, Jul 06, 2016 at 12:26:19PM -0700, Junio C Hamano wrote: > 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-07 17:12 ` Duy Nguyen @ 2016-07-07 18:52 ` Junio C Hamano 2016-07-08 15:39 ` Duy Nguyen 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2016-07-07 18:52 UTC (permalink / raw) To: Duy Nguyen; +Cc: git, yuri.kanivetsky 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". ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-07 18:52 ` Junio C Hamano @ 2016-07-08 15:39 ` Duy Nguyen 2016-07-08 15:53 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Duy Nguyen @ 2016-07-08 15:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, yuri.kanivetsky On Thu, Jul 07, 2016 at 11:52:58AM -0700, Junio C Hamano wrote: > 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-08 15:39 ` Duy Nguyen @ 2016-07-08 15:53 ` Junio C Hamano 2016-07-08 16:36 ` Duy Nguyen 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2016-07-08 15:53 UTC (permalink / raw) To: Duy Nguyen; +Cc: git, yuri.kanivetsky 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-08 15:53 ` Junio C Hamano @ 2016-07-08 16:36 ` Duy Nguyen 2016-07-08 17:23 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Duy Nguyen @ 2016-07-08 16:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Yuri Kanivetsky On Fri, Jul 8, 2016 at 5:53 PM, Junio C Hamano <gitster@pobox.com> wrote: > 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-08 16:36 ` Duy Nguyen @ 2016-07-08 17:23 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2016-07-08 17:23 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Yuri Kanivetsky Duy Nguyen <pclouds@gmail.com> writes: > 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 0/4] cache-tree building fix in the presence of ita entries 2016-07-06 18:48 ` [PATCH v2 0/2] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2016-07-06 18:48 ` [PATCH v2 1/2] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy 2016-07-06 18:48 ` [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 @ 2016-07-09 5:23 ` Nguyễn Thái Ngọc Duy 2016-07-09 5:23 ` [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE Nguyễn Thái Ngọc Duy ` (4 more replies) 2 siblings, 5 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-09 5:23 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE 2016-07-09 5:23 ` [PATCH v3 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy @ 2016-07-09 5:23 ` Nguyễn Thái Ngọc Duy 2016-07-12 20:40 ` Junio C Hamano 2016-07-09 5:23 ` [PATCH v3 2/4] test-lib.sh: introduce and use $_EMPTY_BLOB Nguyễn Thái Ngọc Duy ` (3 subsequent siblings) 4 siblings, 1 reply; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-09 5:23 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE 2016-07-09 5:23 ` [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE Nguyễn Thái Ngọc Duy @ 2016-07-12 20:40 ` Junio C Hamano 2016-07-13 15:04 ` Duy Nguyen 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2016-07-12 20:40 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, yuri.kanivetsky 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? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE 2016-07-12 20:40 ` Junio C Hamano @ 2016-07-13 15:04 ` Duy Nguyen 0 siblings, 0 replies; 30+ messages in thread From: Duy Nguyen @ 2016-07-13 15:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Yuri Kanivetsky On Tue, Jul 12, 2016 at 10:40 PM, Junio C Hamano <gitster@pobox.com> wrote: > 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 2/4] test-lib.sh: introduce and use $_EMPTY_BLOB 2016-07-09 5:23 ` [PATCH v3 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2016-07-09 5:23 ` [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE Nguyễn Thái Ngọc Duy @ 2016-07-09 5:23 ` Nguyễn Thái Ngọc Duy 2016-07-09 5:23 ` [PATCH v3 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy ` (2 subsequent siblings) 4 siblings, 0 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-09 5:23 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes 2016-07-09 5:23 ` [PATCH v3 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2016-07-09 5:23 ` [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE Nguyễn Thái Ngọc Duy 2016-07-09 5:23 ` [PATCH v3 2/4] test-lib.sh: introduce and use $_EMPTY_BLOB Nguyễn Thái Ngọc Duy @ 2016-07-09 5:23 ` Nguyễn Thái Ngọc Duy 2016-07-09 5:23 ` [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 2016-07-16 5:06 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 4 siblings, 0 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-09 5:23 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, 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 <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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 4/4] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-09 5:23 ` [PATCH v3 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy ` (2 preceding siblings ...) 2016-07-09 5:23 ` [PATCH v3 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy @ 2016-07-09 5:23 ` Nguyễn Thái Ngọc Duy 2016-07-12 20:49 ` Junio C Hamano 2016-07-16 5:06 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 4 siblings, 1 reply; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-09 5:23 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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. 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 | 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-09 5:23 ` [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 @ 2016-07-12 20:49 ` Junio C Hamano 2016-07-13 14:59 ` Duy Nguyen 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2016-07-12 20:49 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, yuri.kanivetsky 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. Thanks. > 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 4/4] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-12 20:49 ` Junio C Hamano @ 2016-07-13 14:59 ` Duy Nguyen 0 siblings, 0 replies; 30+ messages in thread From: Duy Nguyen @ 2016-07-13 14:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Yuri Kanivetsky On Tue, Jul 12, 2016 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote: > 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v4 0/4] cache-tree building fix in the presence of ita entries 2016-07-09 5:23 ` [PATCH v3 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy ` (3 preceding siblings ...) 2016-07-09 5:23 ` [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 @ 2016-07-16 5:06 ` Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 1/4] test-lib.sh: introduce and use $EMPTY_TREE Nguyễn Thái Ngọc Duy ` (4 more replies) 4 siblings, 5 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-16 5:06 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 1/4] test-lib.sh: introduce and use $EMPTY_TREE 2016-07-16 5:06 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy @ 2016-07-16 5:06 ` Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 2/4] test-lib.sh: introduce and use $EMPTY_BLOB Nguyễn Thái Ngọc Duy ` (3 subsequent siblings) 4 siblings, 0 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-16 5:06 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 2/4] test-lib.sh: introduce and use $EMPTY_BLOB 2016-07-16 5:06 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 1/4] test-lib.sh: introduce and use $EMPTY_TREE Nguyễn Thái Ngọc Duy @ 2016-07-16 5:06 ` Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy ` (2 subsequent siblings) 4 siblings, 0 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-16 5:06 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes 2016-07-16 5:06 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 1/4] test-lib.sh: introduce and use $EMPTY_TREE Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 2/4] test-lib.sh: introduce and use $EMPTY_BLOB Nguyễn Thái Ngọc Duy @ 2016-07-16 5:06 ` Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 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 2016-07-18 20:48 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Junio C Hamano 4 siblings, 0 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-16 5:06 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, 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 <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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 4/4] cache-tree: do not generate empty trees as a result of all i-t-a subentries 2016-07-16 5:06 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy ` (2 preceding siblings ...) 2016-07-16 5:06 ` [PATCH v4 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy @ 2016-07-16 5:06 ` Nguyễn Thái Ngọc Duy 2016-07-18 20:48 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Junio C Hamano 4 siblings, 0 replies; 30+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2016-07-16 5:06 UTC (permalink / raw) To: git Cc: Junio C Hamano, yuri.kanivetsky, Nguyễn Thái Ngọc Duy 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. 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 | 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v4 0/4] cache-tree building fix in the presence of ita entries 2016-07-16 5:06 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy ` (3 preceding siblings ...) 2016-07-16 5:06 ` [PATCH v4 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 @ 2016-07-18 20:48 ` Junio C Hamano 4 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2016-07-18 20:48 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, yuri.kanivetsky Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2016-07-18 20:48 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-04 13:50 Git doesn't always add files to a commit (amend) Yuri Kanivetsky 2016-07-04 15:48 ` Duy Nguyen 2016-07-04 17:48 ` [PATCH] cache-tree.c: fix i-t-a check skipping directory updates sometimes Nguyễn Thái Ngọc Duy 2016-07-06 17:43 ` Junio C Hamano 2016-07-06 17:55 ` Junio C Hamano 2016-07-06 18:48 ` [PATCH v2 0/2] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2016-07-06 18:48 ` [PATCH v2 1/2] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy 2016-07-06 18:48 ` [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 2016-07-06 19:26 ` Junio C Hamano 2016-07-07 17:12 ` Duy Nguyen 2016-07-07 18:52 ` Junio C Hamano 2016-07-08 15:39 ` Duy Nguyen 2016-07-08 15:53 ` Junio C Hamano 2016-07-08 16:36 ` Duy Nguyen 2016-07-08 17:23 ` Junio C Hamano 2016-07-09 5:23 ` [PATCH v3 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2016-07-09 5:23 ` [PATCH v3 1/4] test-lib.sh: introduce and use $_EMPTY_TREE Nguyễn Thái Ngọc Duy 2016-07-12 20:40 ` Junio C Hamano 2016-07-13 15:04 ` Duy Nguyen 2016-07-09 5:23 ` [PATCH v3 2/4] test-lib.sh: introduce and use $_EMPTY_BLOB Nguyễn Thái Ngọc Duy 2016-07-09 5:23 ` [PATCH v3 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy 2016-07-09 5:23 ` [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 2016-07-12 20:49 ` Junio C Hamano 2016-07-13 14:59 ` Duy Nguyen 2016-07-16 5:06 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 1/4] test-lib.sh: introduce and use $EMPTY_TREE Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 2/4] test-lib.sh: introduce and use $EMPTY_BLOB Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 3/4] cache-tree.c: fix i-t-a entry skipping directory updates sometimes Nguyễn Thái Ngọc Duy 2016-07-16 5:06 ` [PATCH v4 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 2016-07-18 20:48 ` [PATCH v4 0/4] cache-tree building fix in the presence of ita entries Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.