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>
raw | patch | inline | side by side (parent: d51c527 )
if (insertion_point)
*insertion_point = NULL;
while (p) {
if (insertion_point)
*insertion_point = NULL;
while (p) {
- int cmp = strcmp(p->spec-> path, path);
+ int cmp = strcmp(p->path, path);
if (!cmp)
return p;
if (insertion_point && cmp < 0)
if (!cmp)
return p;
if (insertion_point && cmp < 0)
+/*
+ * Note: takes ownership of 'path', which happens to be what the only
+ * caller needs.
+ */
static void line_log_data_insert(struct line_log_data **list,
static void line_log_data_insert(struct line_log_data **list,
- struct diff_filespec *spec ,
long begin, long end)
{
struct line_log_data *ip;
long begin, long end)
{
struct line_log_data *ip;
- struct line_log_data *p = search_line_log_data(*list, spec-> path, &ip);
+ struct line_log_data *p = search_line_log_data(*list, path, &ip);
if (p) {
range_set_append_unsafe(&p->ranges, begin, end);
sort_and_merge_range_set(&p->ranges);
if (p) {
range_set_append_unsafe(&p->ranges, begin, end);
sort_and_merge_range_set(&p->ranges);
return;
}
p = xcalloc(1, sizeof(struct line_log_data));
return;
}
p = xcalloc(1, sizeof(struct line_log_data));
range_set_append(&p->ranges, begin, end);
if (ip) {
p->next = ip->next;
range_set_append(&p->ranges, begin, end);
if (ip) {
p->next = ip->next;
{
char buf[4096];
while (r) {
{
char buf[4096];
while (r) {
- snprintf(buf, 4096, "file %s\n", r->spec-> path);
+ snprintf(buf, 4096, "file %s\n", r->path);
dump_range_set(&r->ranges, buf);
r = r->next;
}
dump_range_set(&r->ranges, buf);
r = r->next;
}
for_each_string_list_item(item, args) {
const char *name_part, *range_part;
for_each_string_list_item(item, args) {
const char *name_part, *range_part;
struct diff_filespec *spec;
long begin = 0, end = 0;
struct diff_filespec *spec;
long begin = 0, end = 0;
if (parse_range_arg(range_part, nth_line, &cb_data,
lines, &begin, &end,
if (parse_range_arg(range_part, nth_line, &cb_data,
lines, &begin, &end,
die("malformed -L argument '%s'", range_part);
if (begin < 1)
begin = 1;
die("malformed -L argument '%s'", range_part);
if (begin < 1)
begin = 1;
begin--;
if (lines < end || lines < begin)
die("file %s has only %ld lines", name_part, lines);
begin--;
if (lines < end || lines < begin)
die("file %s has only %ld lines", name_part, lines);
- line_log_data_insert(&ranges, spec , begin, end);
+ line_log_data_insert(&ranges, full_name , begin, end);
free(ends);
ends = NULL;
}
free(ends);
ends = NULL;
}
line_log_data_init(ret);
range_set_copy(&ret->ranges, &r->ranges);
line_log_data_init(ret);
range_set_copy(&ret->ranges, &r->ranges);
- ret->spec = r->spec;
- assert(ret->spec);
- ret->spec->count++;
+ ret->path = xstrdup(r->path);
else if (!b)
cmp = -1;
else
else if (!b)
cmp = -1;
else
- cmp = strcmp(a->spec->path, b->spec ->path);
+ cmp = strcmp(a->path, b ->path);
if (cmp < 0) {
src = a;
a = a->next;
if (cmp < 0) {
src = a;
a = a->next;
}
d = xmalloc(sizeof(struct line_log_data));
line_log_data_init(d);
}
d = xmalloc(sizeof(struct line_log_data));
line_log_data_init(d);
- d->spec = src->spec;
- d->spec->count++;
+ d->path = xstrdup(src->path);
*pp = d;
pp = &d->next;
if (src2)
*pp = d;
pp = &d->next;
if (src2)
paths = xmalloc((count+1)*sizeof(char *));
r = range;
for (i = 0; i < count; i++) {
paths = xmalloc((count+1)*sizeof(char *));
r = range;
for (i = 0; i < count; i++) {
- paths[i] = xstrdup(r->spec-> path);
+ paths[i] = xstrdup(r->path);
r = r->next;
}
paths[count] = NULL;
r = r->next;
}
paths[count] = NULL;
continue;
}
for (rg = range; rg; rg = rg->next) {
continue;
}
for (rg = range; rg; rg = rg->next) {
- if (!strcmp(rg->spec-> path, p->two->path))
+ if (!strcmp(rg->path, p->two->path))
assert(pair->two->path);
while (rg) {
assert(pair->two->path);
while (rg) {
- assert(rg->spec-> path);
- if (!strcmp(rg->spec-> path, pair->two->path))
+ assert(rg->path);
+ if (!strcmp(rg->path, pair->two->path))
collect_diff(&file_parent, &file_target, &diff);
/* NEEDSWORK should apply some heuristics to prevent mismatches */
collect_diff(&file_parent, &file_target, &diff);
/* NEEDSWORK should apply some heuristics to prevent mismatches */
- rg->spec->path = xstrdup(pair->one->path);
+ free(rg->path);
+ rg->path = xstrdup(pair->one->path);
range_set_init(&tmp, 0);
range_set_map_across_diff(&tmp, &rg->ranges, &diff, diff_out);
range_set_init(&tmp, 0);
range_set_map_across_diff(&tmp, &rg->ranges, &diff, diff_out);
struct line_log_data *rg = range;
changed++;
/* NEEDSWORK tramples over data structures not owned here */
struct line_log_data *rg = range;
changed++;
/* NEEDSWORK tramples over data structures not owned here */
- while (rg && strcmp(rg->spec-> path, queue->queue[i]->two->path))
+ while (rg && strcmp(rg->path, queue->queue[i]->two->path))
rg = rg->next;
assert(rg);
rg->pair = diff_filepair_dup(queue->queue[i]);
rg = rg->next;
assert(rg);
rg->pair = diff_filepair_dup(queue->queue[i]);
};
/* Linked list of interesting files and their associated ranges. The
};
/* Linked list of interesting files and their associated ranges. The
- * list must be kept sorted by spec->path */
+ * list must be kept sorted by path.
+ *
+ * For simplicity, even though this is highly redundant, each
+ * line_log_data owns its 'path'.
+ */
struct line_log_data {
struct line_log_data *next;
struct line_log_data {
struct line_log_data *next;
- struct diff_filespec *spec ;
char status;
struct range_set ranges;
int arg_alloc, arg_nr;
char status;
struct range_set ranges;
int arg_alloc, arg_nr;
canned_test "-L 24,+1:a.c simple" vanishes-early
canned_test "-M -L '/long f/,/^}/:b.c' move-support" move-support-f
canned_test "-L 24,+1:a.c simple" vanishes-early
canned_test "-M -L '/long f/,/^}/:b.c' move-support" move-support-f
-canned_test_failure "-M -L ':f:b.c' parallel-change" parallel-change-f-to-main
+canned_test "-M -L ':f:b.c' parallel-change" parallel-change-f-to-main
canned_test "-L 4,12:a.c -L :main:a.c simple" multiple
canned_test "-L 4,18:a.c -L :main:a.c simple" multiple-overlapping
canned_test "-L 4,12:a.c -L :main:a.c simple" multiple
canned_test "-L 4,18:a.c -L :main:a.c simple" multiple-overlapping