[PATCH 1/3] for_each_hash: allow passing a 'void *data' pointer to callback

Subsystems: the rest

DORMANTno replies

3 messages, 1 author, 2016-06-15 · open the first message on its own page

[PATCH 1/3] for_each_hash: allow passing a 'void *data' pointer to callback

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:50:36

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Fri, 18 Feb 2011 19:55:19 -0800

For the find_exact_renames() function, this allows us to pass the
diff_options structure pointer to the low-level routines.  We will use
that to distinguish between the "rename" and "copy" cases.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

So I changed the "int flag" to a "void *data". That should be more 
flexible, with the slight downside that there are now two "void *" 
pointers, and the callback routine obviously has to know that the first 
one is the one from the hash chain, and the other one is the "data" 
helper.

This does NOT change any semantics. It is purely setup code that passes 
the new pointer around, but doesn't actually use it.

 builtin/describe.c |    4 ++--
 diffcore-rename.c  |   14 ++++++++------
 hash.c             |    4 ++--
 hash.h             |    2 +-
 4 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 342129f..3ba26dc 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, void *data)
 {
 	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, NULL);
 		have_util = 1;
 	}
 
diff --git a/diffcore-rename.c b/diffcore-rename.c
index df41be5..e5e88fe 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -247,7 +247,8 @@ struct file_similarity {
 };
 
 static int find_identical_files(struct file_similarity *src,
-				struct file_similarity *dst)
+				struct file_similarity *dst,
+				struct diff_options *options)
 {
 	int renames = 0;
 
@@ -306,11 +307,12 @@ static void free_similarity_list(struct file_similarity *p)
 	}
 }
 
-static int find_same_files(void *ptr)
+static int find_same_files(void *ptr, void *data)
 {
 	int ret;
 	struct file_similarity *p = ptr;
 	struct file_similarity *src = NULL, *dst = NULL;
+	struct diff_options *options = data;
 
 	/* Split the hash list up into sources and destinations */
 	do {
@@ -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, options) : 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(struct diff_options *options)
 {
 	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, options);
 
 	/* .. 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(options);
 
 	/* Did we only want exact renames? */
 	if (minimum_score == MAX_SCORE)
diff --git a/hash.c b/hash.c
index 1cd4c9d..749ecfe 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 *, void *), void *data)
 {
 	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, data);
 			if (val < 0)
 				return val;
 			sum += val;
diff --git a/hash.h b/hash.h
index 69e33a4..b875ce6 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 *, void *), void *data);
 extern void free_hash(struct hash_table *table);
 
 static inline void init_hash(struct hash_table *table)
-- 
1.7.4.1.51.g2bf8a

[PATCH 2/3] diffcore-rename: propler honor the difference between -M and -C

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:50:36

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Fri, 18 Feb 2011 20:10:32 -0800

We would allow rename detection to do copy detection even when asked
purely for renames.  That confuses users, but more importantly it can
terminally confuse the recursive merge rename logic.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
Hey, it actually deletes one more line than it adds, because I wanted to 
avoid having to indent that big for-loop that was largely duplicated, and 
decided to instead de-duplicate it and make it a helper function that is 
called a second time for the "find copies" case.

And it now uses the new "options" pointer in find_identical_files() to 
decide if it wants to find copies or just renames.

So this is the patch that actually changes semantics, and makes "-M" means 
"only renames" and "-C" mean "find copies too". Like it was always 
intended, but I don't know if we've ever implemented it that way. The 
"we'll find copies even with -M" is certainly not some new regression.

 diffcore-rename.c              |   53 +++++++++++++++++++--------------------
 t/t4003-diff-rename-1.sh       |    2 +-
 t/t4004-diff-rename-symlink.sh |    2 +-
 t/t4005-diff-rename-2.sh       |    2 +-
 t/t4008-diff-break-rewrite.sh  |    4 +-
 t/t4009-diff-rename-4.sh       |    2 +-
 6 files changed, 32 insertions(+), 33 deletions(-)
diff --git a/diffcore-rename.c b/diffcore-rename.c
index e5e88fe..b9b039d 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -278,6 +278,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 && options->detect_rename != DIFF_DETECT_COPY)
+				continue;
 			score += basename_same(source, target);
 			if (score > best_score) {
 				best = p;
@@ -416,6 +418,27 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)
 		m[worst] = *o;
 }
 
+static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, int copies)
+{
+	int count = 0, i;
+
+	for (i = 0; i < dst_cnt * NUM_CANDIDATE_PER_DST; i++) {
+		struct diff_rename_dst *dst;
+
+		if ((mx[i].dst < 0) ||
+		    (mx[i].score < minimum_score))
+			break; /* there is no more usable pair. */
+		dst = &rename_dst[mx[i].dst];
+		if (dst->pair)
+			continue; /* already done, either exact or fuzzy. */
+		if (!copies && rename_src[mx[i].src].one->rename_used)
+			continue;
+		record_rename_pair(mx[i].dst, mx[i].src, mx[i].score);
+		count++;
+	}
+	return count;
+}
+
 void diffcore_rename(struct diff_options *options)
 {
 	int detect_rename = options->detect_rename;
@@ -538,33 +561,9 @@ void diffcore_rename(struct diff_options *options)
 	/* cost matrix sorted by most to least similar pair */
 	qsort(mx, dst_cnt * NUM_CANDIDATE_PER_DST, sizeof(*mx), score_compare);
 
-	for (i = 0; i < dst_cnt * NUM_CANDIDATE_PER_DST; i++) {
-		struct diff_rename_dst *dst;
-
-		if ((mx[i].dst < 0) ||
-		    (mx[i].score < minimum_score))
-			break; /* there is no more usable pair. */
-		dst = &rename_dst[mx[i].dst];
-		if (dst->pair)
-			continue; /* already done, either exact or fuzzy. */
-		if (rename_src[mx[i].src].one->rename_used)
-			continue;
-		record_rename_pair(mx[i].dst, mx[i].src, mx[i].score);
-		rename_count++;
-	}
-
-	for (i = 0; i < dst_cnt * NUM_CANDIDATE_PER_DST; i++) {
-		struct diff_rename_dst *dst;
-
-		if ((mx[i].dst < 0) ||
-		    (mx[i].score < minimum_score))
-			break; /* there is no more usable pair. */
-		dst = &rename_dst[mx[i].dst];
-		if (dst->pair)
-			continue; /* already done, either exact or fuzzy. */
-		record_rename_pair(mx[i].dst, mx[i].src, mx[i].score);
-		rename_count++;
-	}
+	rename_count += find_renames(mx, dst_cnt, minimum_score, 0);
+	if (detect_rename == DIFF_DETECT_COPY)
+		rename_count += find_renames(mx, dst_cnt, minimum_score, 1);
 	free(mx);
 
  cleanup:
diff --git a/t/t4003-diff-rename-1.sh b/t/t4003-diff-rename-1.sh
index c6130c4..bfa8835 100755
--- a/t/t4003-diff-rename-1.sh
+++ b/t/t4003-diff-rename-1.sh
@@ -29,7 +29,7 @@ test_expect_success \
 # copy-and-edit one, and rename-and-edit the other.  We do not say
 # anything about rezrov.
 
-GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current
+GIT_DIFF_OPTS=--unified=0 git diff-index -C -p $tree >current
 cat >expected <<\EOF
 diff --git a/COPYING b/COPYING.1
 copy from COPYING
diff --git a/t/t4004-diff-rename-symlink.sh b/t/t4004-diff-rename-symlink.sh
index 92a65f4..6e562c8 100755
--- a/t/t4004-diff-rename-symlink.sh
+++ b/t/t4004-diff-rename-symlink.sh
@@ -35,7 +35,7 @@ test_expect_success SYMLINKS \
 # a new creation.
 
 test_expect_success SYMLINKS 'setup diff output' "
-    GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current &&
+    GIT_DIFF_OPTS=--unified=0 git diff-index -C -p $tree >current &&
     cat >expected <<\EOF
 diff --git a/bozbar b/bozbar
 new file mode 120000
diff --git a/t/t4005-diff-rename-2.sh b/t/t4005-diff-rename-2.sh
index 1ba359d..77d7f49 100755
--- a/t/t4005-diff-rename-2.sh
+++ b/t/t4005-diff-rename-2.sh
@@ -29,7 +29,7 @@ test_expect_success \
 # and COPYING.2 are based on COPYING, and do not say anything about
 # rezrov.
 
-git diff-index -M $tree >current
+git diff-index -C $tree >current
 
 cat >expected <<\EOF
 :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0603b3238a076dc6c8022aedc6648fa523a17178 C1234	COPYING	COPYING.1
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
diff --git a/t/t4009-diff-rename-4.sh b/t/t4009-diff-rename-4.sh
index de3f174..f22c8e3 100755
--- a/t/t4009-diff-rename-4.sh
+++ b/t/t4009-diff-rename-4.sh
@@ -29,7 +29,7 @@ test_expect_success \
 # and COPYING.2 are based on COPYING, and do not say anything about
 # rezrov.
 
-git diff-index -z -M $tree >current
+git diff-index -z -C $tree >current
 
 cat >expected <<\EOF
 :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0603b3238a076dc6c8022aedc6648fa523a17178 C1234
-- 
1.7.4.1.51.g2bf8a

[PATCH 3/3] diffcore-rename: improve estimate_similarity() heuristics

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:50:36

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Fri, 18 Feb 2011 20:12:06 -0800

The logic to quickly dismiss potential rename pairs was broken.  It
would too eagerly dismiss possible renames when all of the difference
was due to pure new data (or deleted data).

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This is the trivial one-liner, now with a changelog and sign-off. It 
changes the percentage of the size difference (delta_size) to be compared 
against the larger file size rather than the smaller file size.

 diffcore-rename.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/diffcore-rename.c b/diffcore-rename.c
index b9b039d..0cd4c13 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))
-- 
1.7.4.1.51.g2bf8a
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help