@matheusbsbs / matheus-bagliano / commits / 31c6191831

log -L: store the path instead of a diff_filespec

line_log_data has held a diff_filespec* since the very early versions of the code. However, the only place in the code where we actually need the full filespec is parse_range_arg(); in all other cases, we are only interested in the path, so there is hardly a reason to store a filespec. Even worse, it causes a lot of redundant ->spec->path pointer dereferencing. And *even* worse, it caused the following bug. If you merge a rename with a modification to the old filename, like so: * Merge | \ | * Modify foo | | * | Rename foo->bar | / * Create foo we internally -- in process_ranges_merge_commit() -- scan all parents. We are mainly looking for one that doesn't have any modifications, so that we can assign all the blame to it and simplify away the merge. In doing so, we run the normal machinery on all parents in a loop. For each parent, we prepare a "working set" line_log_data by making a copy with line_log_data_copy(), which does *not* make a copy of the spec. Now suppose the rename is the first parent. The diff machinery tells us that the filepair is ('foo', 'bar'). We duly update the path we are interested in: rg->spec->path = xstrdup(pair->one->path); But that 'struct spec' is shared between the output line_log_data and the original input line_log_data. So we just wrecked the state of process_ranges_merge_commit(). When we get around to the second parent, the ranges tell us we are interested in a file 'foo' while the commits touch 'bar'. So most of this patch is just s/->spec->path/->path/ and associated management changes. This implicitly fixes the bug because we removed the shared parts between input and output of line_log_data_copy(); it is now safe to overwrite the path in the copy. There's one only somewhat related change: the comment in process_all_files() explains the reasoning behind using 'range' there. That bit of half-correct code had me sidetracked for a while. Signed-off-by: Thomas Rast <trast@inf.ethz.ch> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thomas Rast committed Apr 12, 2013 at 18:05 UTC 31c61918313c7057a48ac939d8699860e8bfff99

3 files changed +31 -24

line-log.c

+24 -21

@@ -265,7 +265,7 @@ search_line_log_data(struct line_log_data *list, const char *path,

265 if (insertion_point)

266 *insertion_point = NULL;

267 while (p) {

268 - int cmp = strcmp(p->spec->path, path);

268 + int cmp = strcmp(p->path, path);

269 if (!cmp)

270 return p;

271 if (insertion_point && cmp < 0)

@@ -275,22 +275,26 @@ search_line_log_data(struct line_log_data *list, const char *path,

275 return NULL;

276 }

277

278 +/*

279 + * Note: takes ownership of 'path', which happens to be what the only

280 + * caller needs.

281 + */

282 static void line_log_data_insert(struct line_log_data **list,

279 - struct diff_filespec *spec,

283 + char *path,

284 long begin, long end)

285 {

286 struct line_log_data *ip;

283 - struct line_log_data *p = search_line_log_data(*list, spec->path, &ip);

287 + struct line_log_data *p = search_line_log_data(*list, path, &ip);

288

289 if (p) {

290 range_set_append_unsafe(&p->ranges, begin, end);

291 sort_and_merge_range_set(&p->ranges);

288 - free_filespec(spec);

292 + free(path);

293 return;

294 }

295

296 p = xcalloc(1, sizeof(struct line_log_data));

293 - p->spec = spec;

297 + p->path = path;

298 range_set_append(&p->ranges, begin, end);

299 if (ip) {

300 p->next = ip->next;

@@ -354,7 +358,7 @@ static void dump_line_log_data(struct line_log_data *r)

358 {

359 char buf[4096];

360 while (r) {

357 - snprintf(buf, 4096, "file %s\n", r->spec->path);

361 + snprintf(buf, 4096, "file %s\n", r->path);

362 dump_range_set(&r->ranges, buf);

363 r = r->next;

364 }

@@ -561,7 +565,7 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)

565

566 for_each_string_list_item(item, args) {

567 const char *name_part, *range_part;

564 - const char *full_name;

568 + char *full_name;

569 struct diff_filespec *spec;

570 long begin = 0, end = 0;

571

@@ -584,7 +588,7 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)

588

589 if (parse_range_arg(range_part, nth_line, &cb_data,

590 lines, &begin, &end,

587 - spec->path))

591 + full_name))

592 die("malformed -L argument '%s'", range_part);

593 if (begin < 1)

594 begin = 1;

@@ -593,8 +597,9 @@ parse_lines(struct commit *commit, const char *prefix, struct string_list *args)

597 begin--;

598 if (lines < end || lines < begin)

599 die("file %s has only %ld lines", name_part, lines);

596 - line_log_data_insert(&ranges, spec, begin, end);

600 + line_log_data_insert(&ranges, full_name, begin, end);

601

602 + free_filespec(spec);

603 free(ends);

604 ends = NULL;

605 }

@@ -610,9 +615,7 @@ static struct line_log_data *line_log_data_copy_one(struct line_log_data *r)

615 line_log_data_init(ret);

616 range_set_copy(&ret->ranges, &r->ranges);

617

613 - ret->spec = r->spec;

614 - assert(ret->spec);

615 - ret->spec->count++;

618 + ret->path = xstrdup(r->path);

619

620 return ret;

621 }

@@ -652,7 +655,7 @@ static struct line_log_data *line_log_data_merge(struct line_log_data *a,

655 else if (!b)

656 cmp = -1;

657 else

655 - cmp = strcmp(a->spec->path, b->spec->path);

658 + cmp = strcmp(a->path, b->path);

659 if (cmp < 0) {

660 src = a;

661 a = a->next;

@@ -667,8 +670,7 @@ static struct line_log_data *line_log_data_merge(struct line_log_data *a,

670 }

671 d = xmalloc(sizeof(struct line_log_data));

672 line_log_data_init(d);

670 - d->spec = src->spec;

671 - d->spec->count++;

673 + d->path = xstrdup(src->path);

674 *pp = d;

675 pp = &d->next;

676 if (src2)

@@ -741,7 +743,7 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list

743 paths = xmalloc((count+1)*sizeof(char *));

744 r = range;

745 for (i = 0; i < count; i++) {

744 - paths[i] = xstrdup(r->spec->path);

746 + paths[i] = xstrdup(r->path);

747 r = r->next;

748 }

749 paths[count] = NULL;

@@ -797,7 +799,7 @@ static void filter_diffs_for_paths(struct line_log_data *range, int keep_deletio

799 continue;

800 }

801 for (rg = range; rg; rg = rg->next) {

800 - if (!strcmp(rg->spec->path, p->two->path))

802 + if (!strcmp(rg->path, p->two->path))

803 break;

804 }

805 if (rg)

@@ -1021,8 +1023,8 @@ static int process_diff_filepair(struct rev_info *rev,

1023

1024 assert(pair->two->path);

1025 while (rg) {

1024 - assert(rg->spec->path);

1025 - if (!strcmp(rg->spec->path, pair->two->path))

1026 + assert(rg->path);

1027 + if (!strcmp(rg->path, pair->two->path))

1028 break;

1029 rg = rg->next;

1030 }

@@ -1050,7 +1052,8 @@ static int process_diff_filepair(struct rev_info *rev,

1052 collect_diff(&file_parent, &file_target, &diff);

1053

1054 /* NEEDSWORK should apply some heuristics to prevent mismatches */

1053 - rg->spec->path = xstrdup(pair->one->path);

1055 + free(rg->path);

1056 + rg->path = xstrdup(pair->one->path);

1057

1058 range_set_init(&tmp, 0);

1059 range_set_map_across_diff(&tmp, &rg->ranges, &diff, diff_out);

@@ -1096,7 +1099,7 @@ static int process_all_files(struct line_log_data **range_out,

1099 struct line_log_data *rg = range;

1100 changed++;

1101 /* NEEDSWORK tramples over data structures not owned here */

1099 - while (rg && strcmp(rg->spec->path, queue->queue[i]->two->path))

1102 + while (rg && strcmp(rg->path, queue->queue[i]->two->path))

1103 rg = rg->next;

1104 assert(rg);

1105 rg->pair = diff_filepair_dup(queue->queue[i]);

line-log.h

+6 -2

@@ -26,10 +26,14 @@ struct diff_ranges {

26 };

28 /* Linked list of interesting files and their associated ranges. The

29 - * list must be kept sorted by spec->path */

29 + * list must be kept sorted by path.

30 + *

31 + * For simplicity, even though this is highly redundant, each

32 + * line_log_data owns its 'path'.

33 + */

34 struct line_log_data {

35 struct line_log_data *next;

32 - struct diff_filespec *spec;

36 + char *path;

37 char status;

38 struct range_set ranges;

39 int arg_alloc, arg_nr;

t/t4211-line-log.sh

+1 -1

@@ -45,7 +45,7 @@ canned_test "-L '/long f/',/^}/:a.c -L /main/,/^}/:a.c simple" two-ranges

45 canned_test "-L 24,+1:a.c simple" vanishes-early

47 canned_test "-M -L '/long f/,/^}/:b.c' move-support" move-support-f

48 -canned_test_failure "-M -L ':f:b.c' parallel-change" parallel-change-f-to-main

48 +canned_test "-M -L ':f:b.c' parallel-change" parallel-change-f-to-main

50 canned_test "-L 4,12:a.c -L :main:a.c simple" multiple

51 canned_test "-L 4,18:a.c -L :main:a.c simple" multiple-overlapping