Re: [RFC] use typechange as rename source

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

>> In your test case, the "new" symlink won't have any similar symlink that
>> is removed from the preimage, so registering it as a rename destination
>> would not make a difference (it will say "no match found, so create this
>> as usual"), but I am not convinced if that would work well in general.
>
> I don't know that it makes a difference. We are impacting only a
> 'typechange', which implies that we have a filepair in which both p->one
> and p->two are valid; thus, the current code doesn't use it as a rename
> dst at all.

I think it does make a difference, if you have a cross rename that
swaps:

	$ ls -F foo bar
        bar	foo@
        $ mv foo tmp; mv bar foo; mv tmp bar
        $ ls -F foo bar
        bar@	foo

The attached patch does not automatically break a filepair that is a
typechange, so the new t4023 test asks for diffcore-break with -B
explicitly.

I suspect your patch alone would not pass t4023 (with or without -B).

We may want to do the autobreak in diffcore-rename even when -B is not
in effect on top of this patch, but that is a separate topic.

-- >8 --
Subject: [PATCH] rename: Break filepairs with different types.

When we consider if a path has been totally rewritten, we did not
touch changes from symlinks to files or vice versa.  But a change
that modifies even the type of a blob surely should count as a
complete rewrite.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h                           |    7 +++
 diffcore-break.c                  |    7 ++-
 t/t4023-diff-rename-typechange.sh |   86 +++++++++++++++++++++++++++++++++++++
 tree-walk.h                       |    7 ---
 4 files changed, 97 insertions(+), 10 deletions(-)
 create mode 100755 t/t4023-diff-rename-typechange.sh

diff --git a/cache.h b/cache.h
index aaa135b..d0e7a71 100644
--- a/cache.h
+++ b/cache.h
@@ -192,6 +192,13 @@ enum object_type {
 	OBJ_MAX,
 };
 
+static inline enum object_type object_type(unsigned int mode)
+{
+	return S_ISDIR(mode) ? OBJ_TREE :
+		S_ISGITLINK(mode) ? OBJ_COMMIT :
+		OBJ_BLOB;
+}
+
 #define GIT_DIR_ENVIRONMENT "GIT_DIR"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
diff --git a/diffcore-break.c b/diffcore-break.c
index c71a226..69afc07 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -52,8 +52,8 @@ static int should_break(struct diff_filespec *src,
 			     * is the default.
 			     */
 
-	if (!S_ISREG(src->mode) || !S_ISREG(dst->mode))
-		return 0; /* leave symlink rename alone */
+	if (object_type(src->mode) != object_type(dst->mode))
+		return 1; /* even their types are different */
 
 	if (src->sha1_valid && dst->sha1_valid &&
 	    !hashcmp(src->sha1, dst->sha1))
diff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh
new file mode 100755
index 0000000..255604e
--- /dev/null
+++ b/t/t4023-diff-rename-typechange.sh
@@ -0,0 +1,86 @@
+#!/bin/sh
+
+test_description='typechange rename detection'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	rm -f foo bar &&
+	cat ../../COPYING >foo &&
+	ln -s linklink bar &&
+	git add foo bar &&
+	git commit -a -m Initial &&
+	git tag one &&
+
+	rm -f foo bar &&
+	cat ../../COPYING >bar &&
+	ln -s linklink foo &&
+	git add foo bar &&
+	git commit -a -m Second &&
+	git tag two &&
+
+	rm -f foo bar &&
+	cat ../../COPYING >foo &&
+	git add foo &&
+	git commit -a -m Third &&
+	git tag three &&
+
+	mv foo bar &&
+	ln -s linklink foo &&
+	git add foo bar &&
+	git commit -a -m Fourth &&
+	git tag four &&
+
+	# This is purely for sanity check
+
+	rm -f foo bar &&
+	cat ../../COPYING >foo &&
+	cat ../../Makefile >bar &&
+	git add foo bar &&
+	git commit -a -m Fifth &&
+	git tag five &&
+
+	rm -f foo bar &&
+	cat ../../Makefile >foo &&
+	cat ../../COPYING >bar &&
+	git add foo bar &&
+	git commit -a -m Sixth &&
+	git tag six
+
+'
+
+test_expect_success 'cross renames to be detected for regular files' '
+
+	git diff-tree five six -r --name-status -B -M | sort >actual &&
+	{
+		echo "R100	foo	bar"
+		echo "R100	bar	foo"
+	} | sort >expect &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cross renames to be detected for typechange' '
+
+	git diff-tree one two -r --name-status -B -M | sort >actual &&
+	{
+		echo "R100	foo	bar"
+		echo "R100	bar	foo"
+	} | sort >expect &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'moves and renames' '
+
+	git diff-tree three four -r --name-status -B -M | sort >actual &&
+	{
+		echo "R100	foo	bar"
+		echo "T100	foo"
+	} | sort >expect &&
+	diff -u expect actual
+
+'
+
+test_done
diff --git a/tree-walk.h b/tree-walk.h
index 903a7b0..db0fbdc 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -7,13 +7,6 @@ struct name_entry {
 	unsigned int mode;
 };
 
-static inline enum object_type object_type(unsigned int mode)
-{
-	return S_ISDIR(mode) ? OBJ_TREE :
-		S_ISGITLINK(mode) ? OBJ_COMMIT :
-		OBJ_BLOB;
-}
-
 struct tree_desc {
 	const void *buffer;
 	struct name_entry entry;
-- 
1.5.3.6.2090.g4ece0