[PATCH] Fix diff-pruning logic which was running prune too early.

For later stages to reorder patches, pruning logic and rename detection
logic should not decide which delete to discard (because another entry
said it will take over the file as a rename) until the very end.

Also fix some tests that were assuming the earlier "last one is rename
or keep everything else is copy" semantics of diff-raw format, which no
longer is true.

Signed-off-by: Junio C Hamano <junkio@cox.net>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/diff-cache.c b/diff-cache.c
index 10d5c955743d5095b114185e06ba6724c5919e3e..579a673f6a69e720c207083ef8cc2a97a4dd5d55 100644 (file)
--- a/diff-cache.c
+++ b/diff-cache.c
@@ -229,7 +229,6 @@ int main(int argc, const char **argv)
        ret = diff_cache(active_cache, active_nr);
        if (detect_rename)
                diffcore_rename(detect_rename, diff_score_opt);
-       diffcore_prune();
        if (pickaxe)
                diffcore_pickaxe(pickaxe);
        if (2 <= argc)

diff --git a/diff-files.c b/diff-files.c
index 17c328a807c8f12bc32bdcc91eb7db1dfb789e9a..d7cfad79e7aae77bb121cf5c678ba02c98184c3e 100644 (file)
--- a/diff-files.c
+++ b/diff-files.c
@@ -115,7 +115,6 @@ int main(int argc, const char **argv)
        }
        if (detect_rename)
                diffcore_rename(detect_rename, diff_score_opt);
-       diffcore_prune();
        if (pickaxe)
                diffcore_pickaxe(pickaxe);
        if (1 < argc)

diff --git a/diff-tree.c b/diff-tree.c
index 31f8019046f8ee24812ef57702d1484f5b563992..a7e7345bd622e1885471345c759d0fe9ae20c6d9 100644 (file)
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -266,10 +266,8 @@ static int call_diff_flush(void)
 {
        if (detect_rename)
                diffcore_rename(detect_rename, diff_score_opt);
-       diffcore_prune();
        if (pickaxe)
                diffcore_pickaxe(pickaxe);
-
        if (diff_queue_is_empty()) {
                diff_flush(DIFF_FORMAT_NO_OUTPUT, 0);
                return 0;

diff --git a/diff.c b/diff.c
index 5a2aee7acc0b54e6be8d6a884895098e0a739127..05e52779b286a8cfa25e9062fa7ce7364131766d 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -12,9 +12,6 @@ static const char *diff_opts = "-pu";
 static unsigned char null_sha1[20] = { 0, };
 
 static int reverse_diff;
-static int generate_patch;
-static int line_termination = '\n';
-static int inter_name_termination = '\t';
 
 static const char *external_diff(void)
 {
@@ -502,7 +499,9 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *queue,
        return dp;
 }
 
-static void diff_flush_raw(struct diff_filepair *p)
+static void diff_flush_raw(struct diff_filepair *p,
+                          int line_termination,
+                          int inter_name_termination)
 {
        int two_paths;
        char status[10];
@@ -566,10 +565,6 @@ static void diff_flush_patch(struct diff_filepair *p)
        const char *name, *other;
        char msg_[PATH_MAX*2+200], *msg;
 
-       /* diffcore_prune() keeps "stay" entries for diff-raw
-        * copy/rename detection, but when we are generating
-        * patches we do not need them.
-        */
        if (diff_unmodified_pair(p))
                return;
 
@@ -585,7 +580,7 @@ static void diff_flush_patch(struct diff_filepair *p)
                        "similarity index %d%%\n"
                        "copy from %s\n"
                        "copy to %s\n",
-                       (int)(0.5 + p->score * 100/MAX_SCORE),
+                       (int)(0.5 + p->score * 100.0/MAX_SCORE),
                        p->one->path, p->two->path);
                msg = msg_;
                break;
@@ -594,7 +589,7 @@ static void diff_flush_patch(struct diff_filepair *p)
                        "similarity index %d%%\n"
                        "rename old %s\n"
                        "rename new %s\n",
-                       (int)(0.5 + p->score * 100/MAX_SCORE),
+                       (int)(0.5 + p->score * 100.0/MAX_SCORE),
                        p->one->path, p->two->path);
                msg = msg_;
                break;
@@ -630,105 +625,82 @@ int diff_needs_to_stay(struct diff_queue_struct *q, int i,
        return 0;
 }
 
-static int diff_used_as_source(struct diff_queue_struct *q, int lim,
-                              struct diff_filespec *it)
+int diff_queue_is_empty(void)
 {
-       int i;
-       for (i = 0; i < lim; i++) {
-               struct diff_filepair *p = q->queue[i++];
-               if (!strcmp(p->one->path, it->path))
-                       return 1;
-       }
-       return 0;
+       struct diff_queue_struct *q = &diff_queued_diff;
+       return q->nr == 0;
 }
 
-void diffcore_prune(void)
+static void diff_resolve_rename_copy(void)
 {
-       /*
-        * Although rename/copy detection wants to have "no-change"
-        * entries fed into them, the downstream do not need to see
-        * them, unless we had rename/copy for the same path earlier.
-        * This function removes such entries.
-        *
-        * The applications that use rename/copy should:
-        *
-        * (1) feed change and "no-change" entries via diff_queue().
-        * (2) call diffcore_rename, and any other future diffcore_xxx
-        *     that would benefit by still having "no-change" entries.
-        * (3) call diffcore_prune
-        * (4) call other diffcore_xxx that do not need to see
-        *     "no-change" entries.
-        * (5) call diff_flush().
-        */
-       struct diff_queue_struct *q = &diff_queued_diff;
-       struct diff_queue_struct outq;
        int i;
-
-       outq.queue = NULL;
-       outq.nr = outq.alloc = 0;
-
+       struct diff_queue_struct *q = &diff_queued_diff;
        for (i = 0; i < q->nr; i++) {
                struct diff_filepair *p = q->queue[i];
-               if (!diff_unmodified_pair(p) ||
-                   diff_used_as_source(q, i, p->one))
-                       diff_q(&outq, p);
-               else
-                       free(p);
+               p->status = 0;
+               if (DIFF_PAIR_UNMERGED(p))
+                       p->status = 'U';
+               else if (!DIFF_FILE_VALID((p)->one))
+                       p->status = 'N';
+               else if (!DIFF_FILE_VALID((p)->two)) {
+                       /* maybe earlier one said 'R', meaning
+                        * it will take it, in which case we do
+                        * not need to keep 'D'.
+                        */
+                       int j;
+                       for (j = 0; j < i; j++) {
+                               struct diff_filepair *pp = q->queue[j];
+                               if (pp->status == 'R' &&
+                                   !strcmp(pp->one->path, p->one->path))
+                                       break;
+                       }
+                       if (j < i)
+                               continue;
+                       p->status = 'D';
+               }
+               else if (strcmp(p->one->path, p->two->path)) {
+                       /* This is rename or copy.  Which one is it? */
+                       if (diff_needs_to_stay(q, i+1, p->one))
+                               p->status = 'C';
+                       else
+                               p->status = 'R';
+               }
+               else if (memcmp(p->one->sha1, p->two->sha1, 20))
+                       p->status = 'M';
+               else {
+                       /* we do not need this one */
+                       p->status = 0;
+               }
        }
-       free(q->queue);
-       *q = outq;
-       return;
-}
-
-int diff_queue_is_empty(void)
-{
-       struct diff_queue_struct *q = &diff_queued_diff;
-       return q->nr == 0;
 }
 
 void diff_flush(int diff_output_style, int resolve_rename_copy)
 {
        struct diff_queue_struct *q = &diff_queued_diff;
        int i;
+       int line_termination = '\n';
+       int inter_name_termination = '\t';
 
-       generate_patch = 0;
-       switch (diff_output_style) {
-       case DIFF_FORMAT_HUMAN:
-               line_termination = '\n';
-               inter_name_termination = '\t';
-               break;
-       case DIFF_FORMAT_MACHINE:
+       if (diff_output_style == DIFF_FORMAT_MACHINE)
                line_termination = inter_name_termination = 0;
-               break;
-       case DIFF_FORMAT_PATCH:
-               generate_patch = 1;
-               break;
-       }
+       if (resolve_rename_copy)
+               diff_resolve_rename_copy();
+
        for (i = 0; i < q->nr; i++) {
                struct diff_filepair *p = q->queue[i];
-               if (resolve_rename_copy) {
-                       if (DIFF_PAIR_UNMERGED(p))
-                               p->status = 'U';
-                       else if (!DIFF_FILE_VALID((p)->one))
-                               p->status = 'N';
-                       else if (!DIFF_FILE_VALID((p)->two))
-                               p->status = 'D';
-                       else if (strcmp(p->one->path, p->two->path)) {
-                               /* This is rename or copy.  Which one is it? */
-                               if (diff_needs_to_stay(q, i+1, p->one))
-                                       p->status = 'C';
-                               else
-                                       p->status = 'R';
-                       }
-                       else
-                               p->status = 'M';
-               }
-               if (generate_patch)
+               if (p->status == 0)
+                       continue;
+               switch (diff_output_style) {
+               case DIFF_FORMAT_PATCH:
                        diff_flush_patch(p);
-               else
-                       diff_flush_raw(p);
+                       break;
+               case DIFF_FORMAT_HUMAN:
+               case DIFF_FORMAT_MACHINE:
+                       diff_flush_raw(p, line_termination,
+                                      inter_name_termination);
+                       break;
+               }
        }
-
        for (i = 0; i < q->nr; i++) {
                struct diff_filepair *p = q->queue[i];
                diff_free_filespec_data(p->one);
@@ -755,9 +727,9 @@ void diff_addremove(int addremove, unsigned mode,
         * with something like '=' or '*' (I haven't decided
         * which but should not make any difference).
         * Feeding the same new and old to diff_change() 
-        * also has the same effect.  diffcore_prune() should
-        * be used to filter uninteresting ones out before the
-        * final output happens.
+        * also has the same effect.
+        * Before the final output happens, they are pruned after
+        * merged into rename/copy pairs as appropriate.
         */
        if (reverse_diff)
                addremove = (addremove == '+' ? '-' :

diff --git a/diff.h b/diff.h
index 6aa412869e8e846afed653f781bddb8a30aad38f..0d1f9ae347fdaf9970a80ccc47114b474dafa35d 100644 (file)
--- a/diff.h
+++ b/diff.h
@@ -39,8 +39,6 @@ extern void diff_setup(int reverse);
 
 extern void diffcore_rename(int rename_copy, int minimum_score);
 
-extern void diffcore_prune(void);
-
 extern void diffcore_pickaxe(const char *needle);
 extern void diffcore_pathspec(const char **pathspec);
 

diff --git a/diffcore-rename.c b/diffcore-rename.c
index e8de49358667a51e9622d5423e4f8523a8f6907e..f40ab78bd1d662eac3cacfa22f6e2ce757b9fe14 100644 (file)
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -133,10 +133,7 @@ static void record_rename_pair(struct diff_queue_struct *outq,
         * The downstream diffcore transformers are free to reorder
         * the entries as long as they keep file pairs that has the
         * same p->one->path in earlier rename_rank to appear before
-        * later ones.  This ordering is used by the diff_flush()
-        * logic to tell renames from copies, and also used by the
-        * diffcore_prune() logic to omit unnecessary
-        * "no-modification" entries.
+        * later ones.
         *
         * To the final output routine, and in the diff-raw format
         * output, a rename/copy that is based on a path that has a
@@ -271,14 +268,8 @@ void diffcore_rename(int detect_rename, int minimum_score)
 
        /* We really want to cull the candidates list early
         * with cheap tests in order to avoid doing deltas.
-        *
-        * With the current callers, we should not have already
-        * matched entries at this point, but it is nonetheless
-        * checked for sanity.
         */
        for (i = 0; i < created.nr; i++) {
-               if (created.s[i]->xfrm_flags & RENAME_DST_MATCHED)
-                       continue; /* we have matched exactly already */
                for (h = 0; h < sizeof(srcs)/sizeof(srcs[0]); h++) {
                        struct diff_rename_pool *p = srcs[h];
                        for (j = 0; j < p->nr; j++) {
@@ -386,25 +377,13 @@ void diffcore_rename(int detect_rename, int minimum_score)
                }
                else if (!DIFF_FILE_VALID(p->two)) {
                        /* deleted */
-                       if (p->one->xfrm_flags & RENAME_SRC_GONE)
-                               ; /* rename/copy deleted it already */
-                       else
-                               diff_queue(q, p->one, p->two);
+                       diff_queue(q, p->one, p->two);
                }
                else if (strcmp(p->one->path, p->two->path)) {
                        /* rename or copy */
                        struct diff_filepair *dp =
                                diff_queue(q, p->one, p->two);
                        dp->score = p->score;
-
-                       /* if we have a later entry that is a rename/copy
-                        * that depends on p->one, then we copy here.
-                        * otherwise we rename it.
-                        */
-                       if (!diff_needs_to_stay(&outq, i+1, p->one))
-                               /* this is the last one, so mark it as gone.
-                                */
-                               p->one->xfrm_flags |= RENAME_SRC_GONE;
                }
                else
                        /* otherwise it is a modified (or "stay") entry */

diff --git a/diffcore.h b/diffcore.h
index c709cddc42d565fa4e68de87ffff4cf3842fa8b1..1ffc78d019c2fa61d72425d94a29a0093f606447 100644 (file)
--- a/diffcore.h
+++ b/diffcore.h
@@ -12,8 +12,6 @@
 #define DEFAULT_MINIMUM_SCORE 5000
 
 #define RENAME_DST_MATCHED 01
-#define RENAME_SRC_GONE    02
-#define RENAME_SCORE_SHIFT 8
 
 struct diff_filespec {
        unsigned char sha1[20];

diff --git a/t/t4005-diff-rename-2.sh b/t/t4005-diff-rename-2.sh
index 2009aec88cf00449c4b670d088ab7b0ecfe25d90..24d5365aa850ee4d9f8d07db6899faa761942d0c 100644 (file)
--- a/t/t4005-diff-rename-2.sh
+++ b/t/t4005-diff-rename-2.sh
@@ -147,14 +147,13 @@ test_expect_success \
 ################################################################
 
 # tree has COPYING and rezrov.  work tree has the same COPYING and
-# copy-edited COPYING.1, and unchanged rezrov.  We should see
-# unmodified COPYING in the output, so that downstream diff-helper can
-# notice.  We should not say anything about rezrov.
+# copy-edited COPYING.1, and unchanged rezrov.  We should not say
+# anything about rezrov nor COPYING, since the revised again diff-raw
+# nows how to say Copy.
 
 git-diff-cache -C $tree >current
 cat >expected <<\EOF
 :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0603b3238a076dc6c8022aedc6648fa523a17178 C1234 COPYING COPYING.1
-:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 6ff87c4664981e4397625791c8ea3bbb5f2279a3 M     COPYING
 EOF
 
 test_expect_success \