Skip to content

Commit 25d5ea4

Browse files
Junio C HamanoLinus Torvalds
Junio C Hamano
authored and
Linus Torvalds
committed
[PATCH] Redo rename/copy detection logic.
Earlier implementation had a major screw-up in the memory management area. Rename/copy logic sometimes borrowed a pointer to a structure without any provision for downstream to determine which pointer is shared and which is not. This resulted in the later clean-up code to sometimes double free such structure, resulting in a segfault. This made -M and -C useless. Another problem the earlier implementation had was that it reordered the patches, and forced the logic to differentiate renames and copies to depend on that particular order. This problem was fixed by teaching rename/copy detection logic not to do any reordering, and rename-copy differentiator not to depend on the order of the patches. The diffs will leave rename/copy detector in the same destination path order as the patch that was fed into it. Some test vectors have been reordered to accommodate this change. It also adds a sanity check logic to the human-readable diff-raw output to detect paths with embedded TAB and LF characters, which cannot be expressed with that format. This idea came up during a discussion with Chris Wedgwood. Signed-off-by: Junio C Hamano <junkio@cox.net> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
1 parent 1e3f6b6 commit 25d5ea4

File tree

6 files changed

+314
-252
lines changed

6 files changed

+314
-252
lines changed

‎diff.c‎

Lines changed: 98 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -493,8 +493,6 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *queue,
493493
dp->one = one;
494494
dp->two = two;
495495
dp->score = 0;
496-
dp->orig_order = queue->nr;
497-
dp->rename_rank = 0;
498496
diff_q(queue, dp);
499497
return dp;
500498
}
@@ -505,6 +503,17 @@ static void diff_flush_raw(struct diff_filepair *p,
505503
{
506504
int two_paths;
507505
char status[10];
506+
507+
if (line_termination) {
508+
const char *err = "path %s cannot be expressed without -z";
509+
if (strchr(p->one->path, line_termination) ||
510+
strchr(p->one->path, inter_name_termination))
511+
die(err, p->one->path);
512+
if (strchr(p->two->path, line_termination) ||
513+
strchr(p->two->path, inter_name_termination))
514+
die(err, p->two->path);
515+
}
516+
508517
switch (p->status) {
509518
case 'C': case 'R':
510519
two_paths = 1;
@@ -628,41 +637,110 @@ int diff_needs_to_stay(struct diff_queue_struct *q, int i,
628637
int diff_queue_is_empty(void)
629638
{
630639
struct diff_queue_struct *q = &diff_queued_diff;
631-
return q->nr == 0;
640+
int i;
641+
for (i = 0; i < q->nr; i++)
642+
if (!diff_unmodified_pair(q->queue[i]))
643+
return 0;
644+
return 1;
632645
}
633646

634-
static void diff_resolve_rename_copy(void)
647+
#if DIFF_DEBUG
648+
void diff_debug_filespec(struct diff_filespec *s, int x, const char *one)
649+
{
650+
fprintf(stderr, "queue[%d] %s (%s) %s %06o %s\n",
651+
x, one ? : "",
652+
s->path,
653+
DIFF_FILE_VALID(s) ? "valid" : "invalid",
654+
s->mode,
655+
s->sha1_valid ? sha1_to_hex(s->sha1) : "");
656+
fprintf(stderr, "queue[%d] %s size %lu flags %d\n",
657+
x, one ? : "",
658+
s->size, s->xfrm_flags);
659+
}
660+
661+
void diff_debug_filepair(const struct diff_filepair *p, int i)
662+
{
663+
diff_debug_filespec(p->one, i, "one");
664+
diff_debug_filespec(p->two, i, "two");
665+
fprintf(stderr, "score %d, status %c\n",
666+
p->score, p->status ? : '?');
667+
}
668+
669+
void diff_debug_queue(const char *msg, struct diff_queue_struct *q)
635670
{
636671
int i;
637-
struct diff_queue_struct *q = &diff_queued_diff;
672+
if (msg)
673+
fprintf(stderr, "%s\n", msg);
674+
fprintf(stderr, "q->nr = %d\n", q->nr);
638675
for (i = 0; i < q->nr; i++) {
639676
struct diff_filepair *p = q->queue[i];
677+
diff_debug_filepair(p, i);
678+
}
679+
}
680+
#endif
681+
682+
static void diff_resolve_rename_copy(void)
683+
{
684+
int i, j;
685+
struct diff_filepair *p, *pp;
686+
struct diff_queue_struct *q = &diff_queued_diff;
687+
688+
/* This should not depend on the ordering of things. */
689+
690+
diff_debug_queue("resolve-rename-copy", q);
691+
692+
for (i = 0; i < q->nr; i++) {
693+
p = q->queue[i];
640694
p->status = 0;
641695
if (DIFF_PAIR_UNMERGED(p))
642696
p->status = 'U';
643697
else if (!DIFF_FILE_VALID((p)->one))
644698
p->status = 'N';
645699
else if (!DIFF_FILE_VALID((p)->two)) {
646-
/* maybe earlier one said 'R', meaning
647-
* it will take it, in which case we do
648-
* not need to keep 'D'.
700+
/* Deletion record should be omitted if there
701+
* is another entry that is a rename or a copy
702+
* and it uses this one as the source. Then we
703+
* can say the other one is a rename.
649704
*/
650-
int j;
651-
for (j = 0; j < i; j++) {
652-
struct diff_filepair *pp = q->queue[j];
653-
if (pp->status == 'R' &&
654-
!strcmp(pp->one->path, p->one->path))
705+
for (j = 0; j < q->nr; j++) {
706+
pp = q->queue[j];
707+
if (!strcmp(pp->one->path, p->one->path) &&
708+
strcmp(pp->one->path, pp->two->path))
655709
break;
656710
}
657-
if (j < i)
658-
continue;
711+
if (j < q->nr)
712+
continue; /* has rename/copy */
659713
p->status = 'D';
660714
}
661715
else if (strcmp(p->one->path, p->two->path)) {
662-
/* This is rename or copy. Which one is it? */
663-
if (diff_needs_to_stay(q, i+1, p->one))
664-
p->status = 'C';
665-
else
716+
/* See if there is somebody else anywhere that
717+
* will keep the path (either modified or
718+
* unmodified). If so, we have to be a copy,
719+
* not a rename. In addition, if there is
720+
* some other rename or copy that comes later
721+
* than us that uses the same source, we
722+
* cannot be a rename either.
723+
*/
724+
for (j = 0; j < q->nr; j++) {
725+
pp = q->queue[j];
726+
if (strcmp(pp->one->path, p->one->path))
727+
continue;
728+
if (!strcmp(pp->one->path, pp->two->path)) {
729+
if (DIFF_FILE_VALID(pp->two)) {
730+
/* non-delete */
731+
p->status = 'C';
732+
break;
733+
}
734+
continue;
735+
}
736+
/* pp is a rename/copy ... */
737+
if (i < j) {
738+
/* ... and comes later than us */
739+
p->status = 'C';
740+
break;
741+
}
742+
}
743+
if (!p->status)
666744
p->status = 'R';
667745
}
668746
else if (memcmp(p->one->sha1, p->two->sha1, 20))
@@ -672,6 +750,7 @@ static void diff_resolve_rename_copy(void)
672750
p->status = 0;
673751
}
674752
}
753+
diff_debug_queue("resolve-rename-copy done", q);
675754
}
676755

677756
void diff_flush(int diff_output_style, int resolve_rename_copy)

0 commit comments

Comments
 (0)