Re: Bug: write-tree corrupts intent-to-add index state
On Tue, Nov 06, 2012 at 12:53:27AM -0800, Jonathon Mah wrote: > It appears the index forgot that those files were new. So not only > has the intent-to-add status been lost, but git status shows a file > existing in neither HEAD nor the index as not-new-but-modified. > > Tracing through it, I narrowed it down to git write-tree affecting > the index state. As far as I can tell, it's incorrect for that > command to change the index. I'm now out of my (shallow) depth in > the git codebase, so I'm throwing it out there for someone to tackle > (hopefully). I've attached a failing test case. I played with your test a bit and found that if we skip the commit step, the bug disappears. I checked and believe that i-t-a flag in index is preserved, which leaves cache-tree as the culprit. If cache-tree is (incorrectly) valid, diff operations won't bother checking index. We used to not allow writing tree with i-t-a entries present. Then we allowed it, but forgot that we need to invalidate any paths that contain i-t-a entries, otherwise cache-tree won't truly reflect index. The below patch seems to do the trick, but I'd rather do the invalidation inside update_one() so we don't need to traverse over the index again. I haven't succesfully put cache-tree.c back in my head again to make it happen. Anybody is welcome to step up, or I'll finish it this weekend. -- 8< -- diff --git a/cache-tree.c b/cache-tree.c index 28ed657..30a8018 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -381,6 +381,9 @@ int cache_tree_update(struct cache_tree *it, i = update_one(it, cache, entries, "", 0, flags); if (i < 0) return i; + for (i = 0; i < entries; i++) + if (cache[i]->ce_flags & CE_INTENT_TO_ADD) + cache_tree_invalidate_path(it, cache[i]->name); return 0; } diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index ec35409..3ddbd89 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -62,5 +62,25 @@ test_expect_success 'can "commit -a" with an i-t-a entry' ' git commit -a -m all ' +test_expect_success 'cache-tree invalidates i-t-a paths' ' + git reset --hard && + mkdir dir && + : >dir/foo && + git add dir/foo && + git commit -m foo && + + : >dir/bar && + git add -N dir/bar && + git diff --cached --name-only >actual && + echo dir/bar >expect && + test_cmp expect actual && + + git write-tree >/dev/null && + + git diff --cached --name-only >actual && + echo dir/bar >expect && + test_cmp expect actual +' + test_done -- 8< -- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bug: write-tree corrupts intent-to-add index state
Hi revisionaries, I came across what appears to be a bug while working today. I had several changes, both staged and unstaged, and two files that I had marked with intent-to-add (git add -N). $ git status -sb ## Library-3.x...origin/Library-3.x [ahead 4] M Library/LIRootSource.m M Library/Library.xcodeproj/project.pbxproj [...] M Library/SceneKit/LISceneKitBookshelfScene.m AM Library/SceneKit/LISceneKitDisplayedMediumProxy.h AM Library/SceneKit/LISceneKitDisplayedMediumProxy.m (The last two files were added with -N.) I then attempted to stash my work, but that failed: $ git stash -k error: Entry 'Library/SceneKit/LISceneKitDisplayedMediumProxy.h' not uptodate. Cannot merge. Cannot save the current worktree state Re-checking the status showed that the intent-to-add files were now simply modified, yet they did not appear in git ls-tree -r HEAD. $ git status -sb ## Library-3.x...origin/Library-3.x [ahead 4] M Library/LIRootSource.m M Library/Library.xcodeproj/project.pbxproj [...] M Library/SceneKit/LISceneKitBookshelfScene.m M Library/SceneKit/LISceneKitDisplayedMediumProxy.h M Library/SceneKit/LISceneKitDisplayedMediumProxy.m It appears the index forgot that those files were new. So not only has the intent-to-add status been lost, but git status shows a file existing in neither HEAD nor the index as not-new-but-modified. Tracing through it, I narrowed it down to git write-tree affecting the index state. As far as I can tell, it's incorrect for that command to change the index. I'm now out of my (shallow) depth in the git codebase, so I'm throwing it out there for someone to tackle (hopefully). I've attached a failing test case. Additional notes for bug hunters: - I've only seen files in already-existing subdirectories lose intent-to-add status, whereas marking a file in the top-level as i-t-a is preserved during write-tree. - The 'git ls-files -s --debug' output doesn't change across the write-tree: 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 dir/baz ctime: 0:0 mtime: 0:0 dev: 0ino: 0 uid: 0gid: 0 size: 0 flags: 20004000 --- t/t2203-add-intent.sh | 22 ++ 1 file changed, 22 insertions(+) diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index ec35409..fcc67c0 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -62,5 +62,27 @@ test_expect_success 'can "commit -a" with an i-t-a entry' ' git commit -a -m all ' +test_expect_success 'i-t-a status unaffected by write-tree' ' + git reset --hard && + mkdir dir && + echo frotz >dir/bar && + git add dir && + git commit -m "establish dir" && + echo fizfaz >foo && + echo fragz >dir/baz && + git add rezrov && + git add -N foo && + git add -N dir/baz && + cat >expect <<-\EOF && + AM dir/baz + AM foo + EOF + git status --untracked-files=no --porcelain >actual && + test_cmp actual expect && + git write-tree && + git status --untracked-files=no --porcelain >actual && + test_cmp actual expect +' + test_done -- 1.8.0 Jonathon Mah m...@jonathonmah.com -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
