git-commit goes awry after git-add -u

git-commit goes awry after git-add -u

[Salikh Zakirov]

Hi,

I have observed incorret behaviour of git-commit after git-add -u, where
it records deletions of files not related to the files touched by commit.

Unfortunately, I was not able to create a small reproducer, and so
describing the reproducer with the codebase I've encountered this.
Fortunately, it's an open-source project, so I've uploaded the tree
to repo.or.cz.

I've reproduced the problem with both 1.5.3-rc5 and 1.5.2.4

The problematic commit is the topmost one, so it is needed to reset it
to reproduce the bug. The commit moves files:

  vm/em/src/ia32 => vm/em/src/arch/ia32
  vm/vmcore/src/util/em64t => vm/em/src/arch/em64t
  vm/vmcore/src/util/ipf => vm/em/src/arch/ipf

Problem description (short):

Sequence of commands
 git-add -u
 git-commit
produces severely damaged commit. If 'git-add -u' is changed
to using git-add and git-rm, the commit is correct.

How to reproduce (long):

$ git clone git://repo.or.cz/drlvm.git
$ cd drlvm
$ git reset HEAD^   	# undo the problematic commit
$ git status		# all is good now
...
# Changed but not updated:
...
#       deleted:    vm/em/src/ia32/...
...
#       deleted:    vm/vmcore/src/util/em64t/...
...
#       deleted:    vm/vmcore/src/util/ipf/...
...
# Untracked files:
...
#       vm/em/src/arch/

$ git add vm/em/src/arch/	# inform git about added files
$ git status		# still good...
...
# Changes to be committed:
...
#       new file:   vm/em/src/arch/...
...
# Changed but not updated:
...
#       deleted:    vm/em/src/ia32/...
...
#       deleted:    vm/vmcore/src/util/em64t/...
...
#       deleted:    vm/vmcore/src/util/ipf/...
...
$ git add -u		# inform git about deleted files as well
$ git status		# output is okay, it looks like git picked up all of the
moves, but ...
...
# Changed but not updated:
#       renamed:    vm/vmcore/src/util/em64t/... -> vm/em/src/arch/em64t/...
...
$ git diff		# is empty as expected
$ git diff -M --cached  # shows a bunch of renames as expected
$ git commit -m "move"  # BANG! something gone wrong:
Created commit 64aed27: move
 53 files changed, 23168 insertions(+), 5414 deletions(-)
 create mode 100644 vm/em/src/arch/...
 ...					# file creations are okay
 delete mode 100644 vm/vmi/Makefile	# these deletions are incorrect
 delete mode 100644 vm/vmi/src/j9vmls.cpp
 delete mode 100644 vm/vmi/src/vm_trace.h
 delete mode 100644 vm/vmi/src/vmi.cpp
 delete mode 100644 vm/vmi/src/vmi.exp
 delete mode 100644 vm/vmstart/src/compmgr/component_manager_impl.cpp
 delete mode 100644 vm/vmstart/src/compmgr/component_manager_impl.h

$ git status		# work dir is expected to be clean, but it is not
# Changes to be committed:
...
#       deleted:    vm/vmcore/src/util/em64t/...	
...
#       new file:   vm/vmi/Makefile
#       new file:   vm/vmi/src/j9vmls.cpp
#       renamed:
vm/vmcore/src/util/em64t/base_natives/java_lang_thread_em64t.cpp ->
vm/vmi/src/vm_trace.h
#       new file:   vm/vmi/src/vmi.cpp
#       new file:   vm/vmi/src/vmi.exp
#       new file:   vm/vmstart/src/compmgr/component_manager_impl.cpp
#       new file:   vm/vmstart/src/compmgr/component_manager_impl.h

So it does not committed deletion of moved files, but instead recorded
deletions of some other arbitrary files, which in fact are still intact
in the tree, and now are reported as new.

Alternative sequence of commands, not involving 'git-add -u' produces
correct results (without output for compactness)
$ git add vm/em
$ git rm -r vm/vmcore/src/util/ipf
$ git rm -r vm/vmcore/src/util/em64t
$ git rm -r vm/em/src/ia32
$ git status
# On branch master
nothing to commit (working directory clean)

Re: git-commit goes awry after git-add -u

[Salikh Zakirov]

Salikh Zakirov wrote:
> Alternative sequence of commands, not involving 'git-add -u' produces
> correct results (without output for compactness)
> $ git add vm/em
> $ git rm -r vm/vmcore/src/util/ipf
> $ git rm -r vm/vmcore/src/util/em64t
> $ git rm -r vm/em/src/ia32

Sorry, forgot an important command in the sequence:
$ git commit -m moved

> $ git status
> # On branch master
> nothing to commit (working directory clean)

Re: git-commit goes awry after git-add -u

[Junio C Hamano]

Salikh Zakirov <salikh@gmail.com> writes:

> I have observed incorret behaviour of git-commit after git-add -u, where
> it records deletions of files not related to the files touched by commit.

Does this fix the issue?

Ideally remove_file_from_cache() should have the invalidate call
inside just like add_file_to_cache() does, but there was a
technical reason it couldn't that I do not recall offhand, so I
am playing it safe here with this tentative patch to see if the
cause is a cache-tree corruption.

---
 builtin-add.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 1591171..a5fae7c 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -115,6 +115,7 @@ static void update_callback(struct diff_queue_struct *q,
 			break;
 		case DIFF_STATUS_DELETED:
 			remove_file_from_cache(path);
+			cache_tree_invalidate_path(active_cache_tree, path);
 			if (verbose)
 				printf("remove '%s'\n", path);
 			break;

[PATCH] Fix "git add -u" data corruption.

[Junio C Hamano]

This applies to 'maint' to fix a rather serious data corruption
issue.  When "git add -u" affects a subdirectory in such a way
that the only changes to its contents are path removals, the
next tree object written out of that index was bogus, as the
remove codepath forgot to invalidate the cache-tree entry.

Kodos for noticing this breakage goes to Salikh Zakirov.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-add.c         |    1 +
 t/t2200-add-update.sh |   59 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/builtin-add.c b/builtin-add.c
index 1591171..a5fae7c 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -115,6 +115,7 @@ static void update_callback(struct diff_queue_struct *q,
 			break;
 		case DIFF_STATUS_DELETED:
 			remove_file_from_cache(path);
+			cache_tree_invalidate_path(active_cache_tree, path);
 			if (verbose)
 				printf("remove '%s'\n", path);
 			break;
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 83005e7..4c7c6af 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -13,26 +13,53 @@ only the updates to dir/sub.'
 
 . ./test-lib.sh
 
-test_expect_success 'setup' '
-echo initial >top &&
-mkdir dir &&
-echo initial >dir/sub &&
-git-add dir/sub top &&
-git-commit -m initial &&
-echo changed >top &&
-echo changed >dir/sub &&
-echo other >dir/other
+test_expect_success setup '
+	echo initial >check &&
+	echo initial >top &&
+	mkdir dir1 dir2 &&
+	echo initial >dir1/sub1 &&
+	echo initial >dir1/sub2 &&
+	echo initial >dir2/sub3 &&
+	git add check dir1 dir2 top &&
+	test_tick
+	git-commit -m initial &&
+
+	echo changed >check &&
+	echo changed >top &&
+	echo changed >dir2/sub3 &&
+	rm -f dir1/sub1 &&
+	echo other >dir2/other
+'
+
+test_expect_success update '
+	git add -u dir1 dir2
 '
 
-test_expect_success 'update' 'git-add -u dir'
+test_expect_success 'update noticed a removal' '
+	test "$(git-ls-files dir1/sub1)" = ""
+'
 
-test_expect_success 'update touched correct path' \
-  'test "`git-diff-files --name-status dir/sub`" = ""'
+test_expect_success 'update touched correct path' '
+	test "$(git-diff-files --name-status dir2/sub3)" = ""
+'
 
-test_expect_success 'update did not touch other tracked files' \
-  'test "`git-diff-files --name-status top`" = "M	top"'
+test_expect_success 'update did not touch other tracked files' '
+	test "$(git-diff-files --name-status check)" = "M	check" &&
+	test "$(git-diff-files --name-status top)" = "M	top"
+'
 
-test_expect_success 'update did not touch untracked files' \
-  'test "`git-diff-files --name-status dir/other`" = ""'
+test_expect_success 'update did not touch untracked files' '
+	test "$(git-ls-files dir2/other)" = ""
+'
+
+test_expect_success 'cache tree has not been corrupted' '
+
+	git ls-files -s |
+	sed -e "s/ 0	/	/" >expect &&
+	git ls-tree -r $(git write-tree) |
+	sed -e "s/ blob / /" >current &&
+	diff -u expect current
+
+'
 
 test_done

Re: [PATCH] Fix "git add -u" data corruption.

[Zakirov Salikh]

Junio wrote:
> This applies to 'maint' to fix a rather serious data corruption
> issue.  When "git add -u" affects a subdirectory in such a way
> that the only changes to its contents are path removals, the
> next tree object written out of that index was bogus, as the
> remove codepath forgot to invalidate the cache-tree entry.

Fixes the issue, thanks.

To make this more fair to git, especially to a notorious statement
"git never lost any data in its existence", this commit corruption,
while annoying, does not lose any data from working directory,
and was easy to fix and work around once I've figured out what's happened.

Re: git-commit goes awry after git-add -u

[Salikh Zakirov]

Junio C Hamano wrote:
> Salikh Zakirov <salikh@gmail.com> writes:
> 
>> I have observed incorrect behaviour of git-commit after git-add -u, where
>> it records deletions of files not related to the files touched by commit.
> 
> Does this fix the issue?

Thanks, solved the issue nicely.