From 366175ef8c3b1e145f4ba846e63a1dea3ec3cacc Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 19 Jun 2005 13:17:50 -0700 Subject: [PATCH] [PATCH] Rework -B output. Patch for a completely rewritten file detected by the -B flag was shown as a pair of creation followed by deletion in earlier versions. This was an misguided attempt to make reviewing such a complete rewrite easier, and unnecessarily ended up confusing git-apply. Instead, show the entire contents of old version prefixed with '-', followed by the entire contents of new version prefixed with '+'. This gives the same easy-to-review for human consumer while keeping it a single, regular modification patch for machine consumption, something that even GNU patch can grok. Signed-off-by: Junio C Hamano Signed-off-by: Linus Torvalds --- Documentation/diffcore.txt | 9 ++ diff.c | 229 +++++++++++++++++++++++----------- diffcore-break.c | 7 +- t/t4008-diff-break-rewrite.sh | 36 +++--- 4 files changed, 186 insertions(+), 95 deletions(-) diff --git a/Documentation/diffcore.txt b/Documentation/diffcore.txt index 7627453d9a..6c474d1c0c 100644 --- a/Documentation/diffcore.txt +++ b/Documentation/diffcore.txt @@ -191,6 +191,15 @@ like these: -B/60 (the same as above, since diffcore-break defautls to 50%). +Note that earlier implementation left a broken pair as a separate +creation and deletion patches. This was unnecessary hack and +the latest implementation always merges all the broken pairs +back into modifications, but the resulting patch output is +formatted differently to still let the reviewing easier for such +a complete rewrite by showing the entire contents of old version +prefixed with '-', followed by the entire contents of new +version prefixed with '+'. + diffcore-pickaxe ---------------- diff --git a/diff.c b/diff.c index e9936016c6..5cb340ca09 100644 --- a/diff.c +++ b/diff.c @@ -83,10 +83,88 @@ static struct diff_tempfile { char tmp_path[50]; } diff_temp[2]; +static int count_lines(const char *filename) +{ + FILE *in; + int count, ch, completely_empty = 1, nl_just_seen = 0; + in = fopen(filename, "r"); + count = 0; + while ((ch = fgetc(in)) != EOF) + if (ch == '\n') { + count++; + nl_just_seen = 1; + completely_empty = 0; + } + else { + nl_just_seen = 0; + completely_empty = 0; + } + fclose(in); + if (completely_empty) + return 0; + if (!nl_just_seen) + count++; /* no trailing newline */ + return count; +} + +static void print_line_count(int count) +{ + switch (count) { + case 0: + printf("0,0"); + break; + case 1: + printf("1"); + break; + default: + printf("1,%d", count); + break; + } +} + +static void copy_file(int prefix, const char *filename) +{ + FILE *in; + int ch, nl_just_seen = 1; + in = fopen(filename, "r"); + while ((ch = fgetc(in)) != EOF) { + if (nl_just_seen) + putchar(prefix); + putchar(ch); + if (ch == '\n') + nl_just_seen = 1; + else + nl_just_seen = 0; + } + fclose(in); + if (!nl_just_seen) + printf("\n\\ No newline at end of file\n"); +} + +static void emit_rewrite_diff(const char *name_a, + const char *name_b, + struct diff_tempfile *temp) +{ + /* Use temp[i].name as input, name_a and name_b as labels */ + int lc_a, lc_b; + lc_a = count_lines(temp[0].name); + lc_b = count_lines(temp[1].name); + printf("--- %s\n+++ %s\n@@ -", name_a, name_b); + print_line_count(lc_a); + printf(" +"); + print_line_count(lc_b); + printf(" @@\n"); + if (lc_a) + copy_file('-', temp[0].name); + if (lc_b) + copy_file('+', temp[1].name); +} + static void builtin_diff(const char *name_a, const char *name_b, struct diff_tempfile *temp, - const char *xfrm_msg) + const char *xfrm_msg, + int complete_rewrite) { int i, next_at, cmd_size; const char *diff_cmd = "diff -L'%s%s' -L'%s%s'"; @@ -149,12 +227,16 @@ static void builtin_diff(const char *name_a, } if (xfrm_msg && xfrm_msg[0]) puts(xfrm_msg); - if (strncmp(temp[0].mode, temp[1].mode, 3)) /* we do not run diff between different kind * of objects. */ exit(0); + if (complete_rewrite) { + fflush(NULL); + emit_rewrite_diff(name_a, name_b, temp); + exit(0); + } } fflush(NULL); execlp("/bin/sh","sh", "-c", cmd, NULL); @@ -474,7 +556,8 @@ static void run_external_diff(const char *pgm, const char *other, struct diff_filespec *one, struct diff_filespec *two, - const char *xfrm_msg) + const char *xfrm_msg, + int complete_rewrite) { struct diff_tempfile *temp = diff_temp; pid_t pid; @@ -524,7 +607,8 @@ static void run_external_diff(const char *pgm, * otherwise we use the built-in one. */ if (one && two) - builtin_diff(name, other ? : name, temp, xfrm_msg); + builtin_diff(name, other ? : name, temp, xfrm_msg, + complete_rewrite); else printf("* Unmerged path %s\n", name); exit(0); @@ -547,29 +631,75 @@ static void run_external_diff(const char *pgm, remove_tempfile(); } -static void run_diff(const char *name, - const char *other, - struct diff_filespec *one, - struct diff_filespec *two, - const char *xfrm_msg) +static void run_diff(struct diff_filepair *p) { const char *pgm = external_diff(); + char msg_[PATH_MAX*2+200], *xfrm_msg; + struct diff_filespec *one; + struct diff_filespec *two; + const char *name; + const char *other; + int complete_rewrite = 0; + + if (DIFF_PAIR_UNMERGED(p)) { + /* unmerged */ + run_external_diff(pgm, p->one->path, NULL, NULL, NULL, NULL, + 0); + return; + } + + name = p->one->path; + other = (strcmp(name, p->two->path) ? p->two->path : NULL); + one = p->one; two = p->two; + switch (p->status) { + case 'C': + sprintf(msg_, + "similarity index %d%%\n" + "copy from %s\n" + "copy to %s", + (int)(0.5 + p->score * 100.0/MAX_SCORE), + name, other); + xfrm_msg = msg_; + break; + case 'R': + sprintf(msg_, + "similarity index %d%%\n" + "rename from %s\n" + "rename to %s", + (int)(0.5 + p->score * 100.0/MAX_SCORE), + name, other); + xfrm_msg = msg_; + break; + case 'M': + if (p->score) { + sprintf(msg_, + "dissimilarity index %d%%", + (int)(0.5 + p->score * 100.0/MAX_SCORE)); + xfrm_msg = msg_; + complete_rewrite = 1; + break; + } + /* fallthru */ + default: + xfrm_msg = NULL; + } + if (!pgm && - one && two && DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) && (S_IFMT & one->mode) != (S_IFMT & two->mode)) { /* a filepair that changes between file and symlink * needs to be split into deletion and creation. */ struct diff_filespec *null = alloc_filespec(two->path); - run_external_diff(NULL, name, other, one, null, xfrm_msg); + run_external_diff(NULL, name, other, one, null, xfrm_msg, 0); free(null); null = alloc_filespec(one->path); - run_external_diff(NULL, name, other, null, two, xfrm_msg); + run_external_diff(NULL, name, other, null, two, xfrm_msg, 0); free(null); } else - run_external_diff(pgm, name, other, one, two, xfrm_msg); + run_external_diff(pgm, name, other, one, two, xfrm_msg, + complete_rewrite); } void diff_setup(int flags) @@ -693,26 +823,22 @@ static void diff_flush_raw(struct diff_filepair *p, die(err, p->two->path); } + if (p->score) + sprintf(status, "%c%03d", p->status, + (int)(0.5 + p->score * 100.0/MAX_SCORE)); + else { + status[0] = p->status; + status[1] = 0; + } switch (p->status) { case 'C': case 'R': two_paths = 1; - sprintf(status, "%c%03d", p->status, - (int)(0.5 + p->score * 100.0/MAX_SCORE)); break; case 'N': case 'D': two_paths = 0; - if (p->score) - sprintf(status, "%c%03d", p->status, - (int)(0.5 + p->score * 100.0/MAX_SCORE)); - else { - status[0] = p->status; - status[1] = 0; - } break; default: two_paths = 0; - status[0] = p->status; - status[1] = 0; break; } printf(":%06o %06o %s ", @@ -763,55 +889,14 @@ int diff_unmodified_pair(struct diff_filepair *p) static void diff_flush_patch(struct diff_filepair *p) { - const char *name, *other; - char msg_[PATH_MAX*2+200], *msg; - if (diff_unmodified_pair(p)) return; - name = p->one->path; - other = (strcmp(name, p->two->path) ? p->two->path : NULL); if ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) || (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode))) return; /* no tree diffs in patch format */ - switch (p->status) { - case 'C': - sprintf(msg_, - "similarity index %d%%\n" - "copy from %s\n" - "copy to %s", - (int)(0.5 + p->score * 100.0/MAX_SCORE), - p->one->path, p->two->path); - msg = msg_; - break; - case 'R': - sprintf(msg_, - "similarity index %d%%\n" - "rename from %s\n" - "rename to %s", - (int)(0.5 + p->score * 100.0/MAX_SCORE), - p->one->path, p->two->path); - msg = msg_; - break; - case 'D': case 'N': - if (DIFF_PAIR_BROKEN(p)) { - sprintf(msg_, - "dissimilarity index %d%%", - (int)(0.5 + p->score * 100.0/MAX_SCORE)); - msg = msg_; - } - else - msg = NULL; - break; - default: - msg = NULL; - } - - if (DIFF_PAIR_UNMERGED(p)) - run_diff(name, NULL, NULL, NULL, NULL); - else - run_diff(name, other, p->one, p->two, msg); + run_diff(p); } int diff_queue_is_empty(void) @@ -972,8 +1057,10 @@ static void diffcore_apply_filter(const char *filter) int found; for (i = found = 0; !found && i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if ((p->broken_pair && strchr(filter, 'B')) || - (!p->broken_pair && strchr(filter, p->status))) + if (((p->status == 'M') && + ((p->score && strchr(filter, 'B')) || + (!p->score && strchr(filter, 'M')))) || + ((p->status != 'M') && strchr(filter, p->status))) found++; } if (found) @@ -991,8 +1078,10 @@ static void diffcore_apply_filter(const char *filter) /* Only the matching ones */ for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - if ((p->broken_pair && strchr(filter, 'B')) || - (!p->broken_pair && strchr(filter, p->status))) + if (((p->status == 'M') && + ((p->score && strchr(filter, 'B')) || + (!p->score && strchr(filter, 'M')))) || + ((p->status != 'M') && strchr(filter, p->status))) diff_q(&outq, p); else diff_free_filepair(p); diff --git a/diffcore-break.c b/diffcore-break.c index 082e4e5962..920062bfd9 100644 --- a/diffcore-break.c +++ b/diffcore-break.c @@ -214,7 +214,7 @@ static void merge_broken(struct diff_filepair *p, struct diff_queue_struct *outq) { /* p and pp are broken pairs we want to merge */ - struct diff_filepair *c = p, *d = pp; + struct diff_filepair *c = p, *d = pp, *dp; if (DIFF_FILE_VALID(p->one)) { /* this must be a delete half */ d = p; c = pp; @@ -229,7 +229,8 @@ static void merge_broken(struct diff_filepair *p, if (!DIFF_FILE_VALID(c->two)) die("internal error in merge #4"); - diff_queue(outq, d->one, c->two); + dp = diff_queue(outq, d->one, c->two); + dp->score = p->score; diff_free_filespec_data(d->two); diff_free_filespec_data(c->one); free(d); @@ -251,7 +252,6 @@ void diffcore_merge_broken(void) /* we already merged this with its peer */ continue; else if (p->broken_pair && - p->score == 0 && !strcmp(p->one->path, p->two->path)) { /* If the peer also survived rename/copy, then * we merge them back together. @@ -259,7 +259,6 @@ void diffcore_merge_broken(void) for (j = i + 1; j < q->nr; j++) { struct diff_filepair *pp = q->queue[j]; if (pp->broken_pair && - p->score == 0 && !strcmp(pp->one->path, pp->two->path) && !strcmp(p->one->path, pp->two->path)) { /* Peer survived. Merge them */ diff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh index 94205b94ae..040d0ddbd4 100644 --- a/t/t4008-diff-break-rewrite.sh +++ b/t/t4008-diff-break-rewrite.sh @@ -44,13 +44,12 @@ test_expect_success \ cat >expected <<\EOF :100644 000000 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 0000000000000000000000000000000000000000 D file0 -:100644 000000 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0000000000000000000000000000000000000000 D100 file1 -:000000 100644 0000000000000000000000000000000000000000 11e331465a89c394dc25c780de230043750c1ec8 N100 file1 +:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 11e331465a89c394dc25c780de230043750c1ec8 M100 file1 EOF test_expect_success \ 'validate result of -B (#1)' \ - 'compare_diff_raw current expected' + 'compare_diff_raw expected current' test_expect_success \ 'run diff with -B and -M' \ @@ -62,7 +61,7 @@ EOF test_expect_success \ 'validate result of -B -M (#2)' \ - 'compare_diff_raw current expected' + 'compare_diff_raw expected current' test_expect_success \ 'swap file0 and file1' \ @@ -79,15 +78,13 @@ test_expect_success \ 'git-diff-cache -B "$tree" >current' cat >expected <<\EOF -:100644 000000 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 0000000000000000000000000000000000000000 D100 file0 -:000000 100644 0000000000000000000000000000000000000000 6ff87c4664981e4397625791c8ea3bbb5f2279a3 N100 file0 -:100644 000000 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0000000000000000000000000000000000000000 D100 file1 -:000000 100644 0000000000000000000000000000000000000000 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 N100 file1 +:100644 100644 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 6ff87c4664981e4397625791c8ea3bbb5f2279a3 M100 file0 +:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 M100 file1 EOF test_expect_success \ 'validate result of -B (#3)' \ - 'compare_diff_raw current expected' + 'compare_diff_raw expected current' test_expect_success \ 'run diff with -B and -M' \ @@ -100,7 +97,7 @@ EOF test_expect_success \ 'validate result of -B -M (#4)' \ - 'compare_diff_raw current expected' + 'compare_diff_raw expected current' test_expect_success \ 'make file0 into something completely different' \ @@ -114,13 +111,12 @@ test_expect_success \ cat >expected <<\EOF :100644 120000 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 67be421f88824578857624f7b3dc75e99a8a1481 T file0 -:100644 000000 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0000000000000000000000000000000000000000 D100 file1 -:000000 100644 0000000000000000000000000000000000000000 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 N100 file1 +:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 M100 file1 EOF test_expect_success \ 'validate result of -B (#5)' \ - 'compare_diff_raw current expected' + 'compare_diff_raw expected current' test_expect_success \ 'run diff with -B' \ @@ -130,13 +126,12 @@ test_expect_success \ # due to type differences. cat >expected <<\EOF :100644 120000 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 67be421f88824578857624f7b3dc75e99a8a1481 T file0 -:100644 000000 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0000000000000000000000000000000000000000 D100 file1 -:000000 100644 0000000000000000000000000000000000000000 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 N100 file1 +:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 M100 file1 EOF test_expect_success \ 'validate result of -B -M (#6)' \ - 'compare_diff_raw current expected' + 'compare_diff_raw expected current' test_expect_success \ 'run diff with -M' \ @@ -151,7 +146,7 @@ EOF test_expect_success \ 'validate result of -M (#7)' \ - 'compare_diff_raw current expected' + 'compare_diff_raw expected current' test_expect_success \ 'file1 edited to look like file0 and file0 rename-edited to file2' \ @@ -169,14 +164,13 @@ test_expect_success \ cat >expected <<\EOF :100644 000000 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 0000000000000000000000000000000000000000 D file0 -:100644 000000 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0000000000000000000000000000000000000000 D100 file1 -:000000 100644 0000000000000000000000000000000000000000 08bb2fb671deff4c03a4d4a0a1315dff98d5732c N100 file1 +:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 08bb2fb671deff4c03a4d4a0a1315dff98d5732c M100 file1 :000000 100644 0000000000000000000000000000000000000000 f5deac7be59e7eeab8657fd9ae706fd6a57daed2 N file2 EOF test_expect_success \ 'validate result of -B (#8)' \ - 'compare_diff_raw current expected' + 'compare_diff_raw expected current' test_expect_success \ 'run diff with -B -M' \ @@ -189,6 +183,6 @@ EOF test_expect_success \ 'validate result of -B -M (#9)' \ - 'compare_diff_raw current expected' + 'compare_diff_raw expected current' test_done