[PATCH] Fix diff-pruning logic which was running prune too early.
authorJunio C Hamano <junkio@cox.net>
Tue, 24 May 2005 01:14:03 +0000 (18:14 -0700)
committerLinus Torvalds <torvalds@ppc970.osdl.org>
Tue, 24 May 2005 02:17:06 +0000 (19:17 -0700)
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-cache.c
diff-files.c
diff-tree.c
diff.c
diff.h
diffcore-rename.c
diffcore.h
t/t4005-diff-rename-2.sh

index 10d5c955743d5095b114185e06ba6724c5919e3e..579a673f6a69e720c207083ef8cc2a97a4dd5d55 100644 (file)
@@ -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)
index 17c328a807c8f12bc32bdcc91eb7db1dfb789e9a..d7cfad79e7aae77bb121cf5c678ba02c98184c3e 100644 (file)
@@ -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)
index 31f8019046f8ee24812ef57702d1484f5b563992..a7e7345bd622e1885471345c759d0fe9ae20c6d9 100644 (file)
@@ -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);
 
index e8de49358667a51e9622d5423e4f8523a8f6907e..f40ab78bd1d662eac3cacfa22f6e2ce757b9fe14 100644 (file)
@@ -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 */
index c709cddc42d565fa4e68de87ffff4cf3842fa8b1..1ffc78d019c2fa61d72425d94a29a0093f606447 100644 (file)
@@ -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];
index 2009aec88cf00449c4b670d088ab7b0ecfe25d90..24d5365aa850ee4d9f8d07db6899faa761942d0c 100644 (file)
@@ -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 \