Re: Merging limitations after directory renames -- interesting test repo

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Fri, Feb 18, 2011 at 4:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Yeah, I think it is probably a good idea to really limit the rename
> detection only to renames at least inside merge-recursive.  The static
> variable does look ugly but it shouldn't be a rocket surgery to pass it
> down if we want to.

Here's a slight update. It still has the "too ugly to live"
if-statment without proper indentation, but it now changes
"for_each_hash()" to pass in a flag value and uses that.

It also fixes one of the tests that depended on the "find copies"
behavior for its result to actually ask for copies now that we don't
give them unless you do.

                                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 5616 bytes --]

 builtin/describe.c            |    4 ++--
 diffcore-rename.c             |   17 ++++++++++-------
 hash.c                        |    4 ++--
 hash.h                        |    2 +-
 t/t4008-diff-break-rewrite.sh |    4 ++--
 5 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 342129f..4f4fe3e 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -63,7 +63,7 @@ static inline struct commit_name *find_commit_name(const unsigned char *peeled)
 	return n;
 }
 
-static int set_util(void *chain)
+static int set_util(void *chain, int flag)
 {
 	struct commit_name *n;
 	for (n = chain; n; n = n->next) {
@@ -289,7 +289,7 @@ static void describe(const char *arg, int last_one)
 		fprintf(stderr, "searching to describe %s\n", arg);
 
 	if (!have_util) {
-		for_each_hash(&names, set_util);
+		for_each_hash(&names, set_util, 0);
 		have_util = 1;
 	}
 
diff --git a/diffcore-rename.c b/diffcore-rename.c
index df41be5..3a937ac 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -170,7 +170,7 @@ static int estimate_similarity(struct diff_filespec *src,
 	 * and the final score computation below would not have a
 	 * divide-by-zero issue.
 	 */
-	if (base_size * (MAX_SCORE-minimum_score) < delta_size * MAX_SCORE)
+	if (max_size * (MAX_SCORE-minimum_score) < delta_size * MAX_SCORE)
 		return 0;
 
 	if (!src->cnt_data && diff_populate_filespec(src, 0))
@@ -247,7 +247,7 @@ struct file_similarity {
 };
 
 static int find_identical_files(struct file_similarity *src,
-				struct file_similarity *dst)
+				struct file_similarity *dst, int copies)
 {
 	int renames = 0;
 
@@ -277,6 +277,8 @@ static int find_identical_files(struct file_similarity *src,
 			}
 			/* Give higher scores to sources that haven't been used already */
 			score = !source->rename_used;
+			if (source->rename_used && !copies)
+				continue;
 			score += basename_same(source, target);
 			if (score > best_score) {
 				best = p;
@@ -306,7 +308,7 @@ static void free_similarity_list(struct file_similarity *p)
 	}
 }
 
-static int find_same_files(void *ptr)
+static int find_same_files(void *ptr, int copies)
 {
 	int ret;
 	struct file_similarity *p = ptr;
@@ -329,7 +331,7 @@ static int find_same_files(void *ptr)
 	 * If we have both sources *and* destinations, see if
 	 * we can match them up
 	 */
-	ret = (src && dst) ? find_identical_files(src, dst) : 0;
+	ret = (src && dst) ? find_identical_files(src, dst, copies) : 0;
 
 	/* Free the hashes and return the number of renames found */
 	free_similarity_list(src);
@@ -377,7 +379,7 @@ static void insert_file_table(struct hash_table *table, int src_dst, int index,
  * and then during the second round we try to match
  * cache-dirty entries as well.
  */
-static int find_exact_renames(void)
+static int find_exact_renames(int copies)
 {
 	int i;
 	struct hash_table file_table;
@@ -390,7 +392,7 @@ static int find_exact_renames(void)
 		insert_file_table(&file_table, 1, i, rename_dst[i].two);
 
 	/* Find the renames */
-	i = for_each_hash(&file_table, find_same_files);
+	i = for_each_hash(&file_table, find_same_files, copies);
 
 	/* .. and free the hash data structure */
 	free_hash(&file_table);
@@ -467,7 +469,7 @@ void diffcore_rename(struct diff_options *options)
 	 * We really want to cull the candidates list early
 	 * with cheap tests in order to avoid doing deltas.
 	 */
-	rename_count = find_exact_renames();
+	rename_count = find_exact_renames(detect_rename == DIFF_DETECT_COPY);
 
 	/* Did we only want exact renames? */
 	if (minimum_score == MAX_SCORE)
@@ -551,6 +553,7 @@ void diffcore_rename(struct diff_options *options)
 		rename_count++;
 	}
 
+	if (detect_rename == DIFF_DETECT_COPY)
 	for (i = 0; i < dst_cnt * NUM_CANDIDATE_PER_DST; i++) {
 		struct diff_rename_dst *dst;
 
diff --git a/hash.c b/hash.c
index 1cd4c9d..c5dd002 100644
--- a/hash.c
+++ b/hash.c
@@ -81,7 +81,7 @@ void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table)
 	return insert_hash_entry(hash, ptr, table);
 }
 
-int for_each_hash(const struct hash_table *table, int (*fn)(void *))
+int for_each_hash(const struct hash_table *table, int (*fn)(void *, int), int flag)
 {
 	int sum = 0;
 	unsigned int i;
@@ -92,7 +92,7 @@ int for_each_hash(const struct hash_table *table, int (*fn)(void *))
 		void *ptr = array->ptr;
 		array++;
 		if (ptr) {
-			int val = fn(ptr);
+			int val = fn(ptr, flag);
 			if (val < 0)
 				return val;
 			sum += val;
diff --git a/hash.h b/hash.h
index 69e33a4..1e2de55 100644
--- a/hash.h
+++ b/hash.h
@@ -30,7 +30,7 @@ struct hash_table {
 
 extern void *lookup_hash(unsigned int hash, const struct hash_table *table);
 extern void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table);
-extern int for_each_hash(const struct hash_table *table, int (*fn)(void *));
+extern int for_each_hash(const struct hash_table *table, int (*fn)(void *, int), int flag);
 extern void free_hash(struct hash_table *table);
 
 static inline void init_hash(struct hash_table *table)
diff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh
index d79d9e1e..73b4a24 100755
--- a/t/t4008-diff-break-rewrite.sh
+++ b/t/t4008-diff-break-rewrite.sh
@@ -173,8 +173,8 @@ test_expect_success \
     'compare_diff_raw expected current'
 
 test_expect_success \
-    'run diff with -B -M' \
-    'git diff-index -B -M "$tree" >current'
+    'run diff with -B -C' \
+    'git diff-index -B -C "$tree" >current'
 
 cat >expected <<\EOF
 :100644 100644 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 08bb2fb671deff4c03a4d4a0a1315dff98d5732c C095	file0	file1