Re: [PATCHv2] git-mv: Keep moved index entries inact

Petr Baudis [off-list ref] writes:

The rewrite of git-mv from a shell script to a builtin was perhaps
a little too straightforward: the git add and git rm queues were
emulated directly, which resulted in a rather complicated code and
caused an inconsistent behaviour when moving dirty index entries;
git mv would update the entry based on working tree state,
except in case of overwrites, where the new entry would still have
sha1 of the old file.

This patch introduces rename_index_entry_at() into the index toolkit,
which will rename an entry while removing any entries the new entry
might render duplicate. This is then used in git mv instead
of all the file queues, resulting in a major simplification
of the code and an inevitable change in git mv -n output format.
...

Thanks.  I think I've managed to fix the rename_index_entry_at() in a
satisfactory way, and also made builtin-mv to allow "mv -f symlink file"
and "mv -f file symlink".

I do not agree with the semantics of this test seems to want, though.

+test_expect_failure 'git mv should follow symlink to a directory' '
+
+	rm -fr .git &&
+	git init &&
+	echo 1 >moved &&
+	mkdir -p dir &&
+	touch dir/.keep &&
+	ln -s dir symlink &&
+	git add moved dir/.keep symlink &&
+	git mv moved symlink &&
+	[ ! -e moved ] &&
+	[ -f symlink/moved ] &&
+	[ $(cat symlink/moved) = 1 ] &&
+	[ "$(ls dir)" = "$(ls symlink)" ] &&
+	git diff-files --quiet
+
+'

A symlink to us is just a different kind of blob, and by definition a blob
is the leaf level of a tree structure that represents the working tree in
the index. There won't be anything hanging below it, and when adding
things to the index we should not dereference the symlink to see where it
leads to.

Traditionally we have been loose about this check, and the normal "git
add" and "git update-index" codepath is still forever broken, and we
allow:

	$ mkdir dir
        $ >dir/file
        $ ln -s dir symlink
        $ git add symlink/file

but some codepaths that matter more (because they do larger damage
unattended, as opposed to the above command sequence that can be avoided
by user education a bit more easily), such as "git apply" and "git
read-tree", have been corrected using has_symlink_leading_path() since mid
2007.  We would need to follow through c40641b (Optimize symlink/directory
detection, 2008-05-09) and further fix "git add" and "git update-index"
codepaths to forbid the above command sequence.

So my take on the above test piece is that after:

	>moved
	mkdir dir
        >dir/file
        ln -s dir symlink
	git add moved dir symlink

This should fail, as it is an overwrite:

	git mv moved symlink

and with "-f", the command should simply remove symlink and replace it
with a regular file whose contents come from the original "moved".

IOW, what a symlink points at should not matter.

On Fri, Jul 25, 2008 at 11:46:02PM -0700, Junio C Hamano wrote:

Thanks.  I think I've managed to fix the rename_index_entry_at() in a
satisfactory way, and also made builtin-mv to allow "mv -f symlink file"
and "mv -f file symlink".

Oh, sorry, I didn't realize there were still problems with the original
one, I would try it on my own in that case.

So my take on the above test piece is that after:

	>moved
	mkdir dir
        >dir/file
        ln -s dir symlink
	git add moved dir symlink

This should fail, as it is an overwrite:

	git mv moved symlink

and with "-f", the command should simply remove symlink and replace it
with a regular file whose contents come from the original "moved".

IOW, what a symlink points at should not matter.

You convinced me, yes. (Especially since I started actually using
symlinks in some of my projects very recently and this would be the
exact semantic I would eventually expect as well.)

-- 
				Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name.  -- Ken Thompson and Dennis M. Ritchie

Currently, git-mv will declare "not under source control" on an attempt
to move a conflicted index entry. This patch adds an expect_failure
testcase for this case, since this is an artificial restriction. (However,
the scenario is not critical enough for the author to fix right now.)

Signed-off-by: Petr Baudis <redacted>
---

I don't really know if it is ok to make "feature requests" like this by
adding failing testcases...

 t/t7001-mv.sh |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 7e47931..241e9a2 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh

@@ -173,6 +173,33 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '
 
 rm -f dirty dirty2
 
+cat >multistage <<EOT
+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 1	staged
+100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 2	staged
+100755 d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 3	staged
+EOT
+
+# Rationale: I cannot git mv around a conflicted file. This is unnecessary
+# restriction in case another part of conflict resolution requires me to
+# move the file around.
+test_expect_failure 'git mv should move all stages of cache entry' '
+
+	rm -fr .git &&
+	git init &&
+	# git mv requires object to exist in working tree (bug?)
+	touch staged &&
+	git update-index --index-info <multistage &&
+	git ls-files --stage >lsf_output &&
+	test_cmp multistage lsf_output &&
+	git mv staged staged-mv &&
+	sed "s/staged/staged-mv/" <multistage >multistage-mv &&
+	git ls-files --stage >lsf_output &&
+	test_cmp multistage-mv lsf_output
+
+'
+
+rm -f multistage multistage-mv lsf_output staged
+
 test_expect_failure 'git mv should overwrite symlink to a file' '
 
 	rm -fr .git &&

Hi,

there is a race somewhere in these 'git-mv: Keep moved index entries
inact' changes.

The test cases 'git mv should overwrite symlink to a file' or 'git mv
should overwrite file with a symlink' fail occasionaly.  It's quite
non-deterministic:  I have run t7001-mv.sh in a loop (see below) and
one or the other usually fails around 50 runs (but sometimes only
after 150).  Adding some tracing echos to the tests shows that both
tests fail when running 'git diff-files' at the end.

Regards,
Gábor


---
#!/bin/bash

ret=0
i=0
while test $ret = 0 ; do
        GIT_TEST_OPTS='--verbose --debug' make t7001-mv.sh
        ret=$?
        i=$((i+1))
done
echo "Failed at ${i}th run"

Hi,

On Mon, 28 Jul 2008, SZEDER Gábor wrote:

there is a race somewhere in these 'git-mv: Keep moved index entries
inact' changes.

The test cases 'git mv should overwrite symlink to a file' or 'git mv
should overwrite file with a symlink' fail occasionaly.  It's quite
non-deterministic:  I have run t7001-mv.sh in a loop (see below) and
one or the other usually fails around 50 runs (but sometimes only
after 150).  Adding some tracing echos to the tests shows that both
tests fail when running 'git diff-files' at the end.

To make it more convenient to test: with this patch it fails all the time:

-- snipsnap --

 t/t7001-mv.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index b0fa407..6699abd 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh

@@ -180,6 +180,7 @@ test_expect_success 'git mv should overwrite symlink to a file' '
 	echo 1 >moved &&
 	ln -s moved symlink &&
 	git add moved symlink &&
+	sleep 1 &&
 	test_must_fail git mv moved symlink &&
 	git mv -f moved symlink &&
 	! test -e moved &&

Hi,

On Mon, 28 Jul 2008, Johannes Schindelin wrote:

On Mon, 28 Jul 2008, SZEDER Gábor wrote:

quoted

there is a race somewhere in these 'git-mv: Keep moved index entries
inact' changes.

The test cases 'git mv should overwrite symlink to a file' or 'git mv
should overwrite file with a symlink' fail occasionaly.  It's quite
non-deterministic:  I have run t7001-mv.sh in a loop (see below) and
one or the other usually fails around 50 runs (but sometimes only
after 150).  Adding some tracing echos to the tests shows that both
tests fail when running 'git diff-files' at the end.

To make it more convenient to test: with this patch it fails all the time:

Ooops.  Seems like I changed the test 23 to fail, instead of test 24.  
However, I think it is the same bug: the index is newer by one second, so 
it seems that the patch for builtin-mv.c did not really keep the data 
"intact".

Note that a test case should use test-chmtime to force this scenario, not 
sleep a second.

Unfortunately, I already spent my Git time budget for today, so the ball 
is out of my half for now.

Ciao,
Dscho

Hi,

On Mon, 28 Jul 2008, Johannes Schindelin wrote:

On Mon, 28 Jul 2008, Johannes Schindelin wrote:

quoted

On Mon, 28 Jul 2008, SZEDER Gábor wrote:

quoted

there is a race somewhere in these 'git-mv: Keep moved index entries 
inact' changes.

The test cases 'git mv should overwrite symlink to a file' or 'git 
mv should overwrite file with a symlink' fail occasionaly.  It's 
quite non-deterministic:  I have run t7001-mv.sh in a loop (see 
below) and one or the other usually fails around 50 runs (but 
sometimes only after 150).  Adding some tracing echos to the tests 
shows that both tests fail when running 'git diff-files' at the end.

To make it more convenient to test: with this patch it fails all the 
time:

Ooops.  Seems like I changed the test 23 to fail, instead of test 24.  
However, I think it is the same bug: the index is newer by one second, 
so it seems that the patch for builtin-mv.c did not really keep the data 
"intact".

Note that a test case should use test-chmtime to force this scenario, 
not sleep a second.

Unfortunately, I already spent my Git time budget for today, so the ball 
is out of my half for now.

Hah!  I had a few minutes, and this is my analysis:

Just try to "mv" a file, and look at the _ctime_ before and after.  Yes, 
that is right, at least on my system (ext3) it _changes_.

So the test 23 and 24 in t7001-mv.sh are totally bogus.  They purport to 
test that git-mv retains the whole meta-information in the cache and 
therefore the index does not need to be updated.

However, it _does_ need to be updated, exactly because ctime changed.

Only that the test failed to test what it tried to test, instead 
succeeding erroneously, just because the index was racy most of the time 
and got silently updated.

So, this is the analysis.  The fixes will have to be done by somebody 
else, because /me goes running now.

(Possible fixes I envisage: update ctime via stat() after rename(), or 
just give up and scrap the whole "leave cache_entry inact" thing.)

Ciao,
Dscho