Hi Paul.
Glad to see from some recent commits that you still have an eye in the
merge code.
I'm stumbling a bit on get_most_inclusive_end_rev(). The doc string says:
'If IS_ROLLBACK is true the oldest revision is considered the
"most inclusive" otherwise the youngest revision is.'
But the code says:
if (... (is_rollback && (range->end > end_rev))
|| ((! is_rollback) && (range->end < end_rev)))
end_rev = range->end;
which seems to be doing the opposite: if IS_ROLLBACK is true it is
selecting the youngest RANGE->END, else the oldest.
The first of the two call sites (both within do_directory_merge()) says:
/* Now examine NOTIFY_B->CHILDREN_WITH_MERGEINFO to find the youngest
ending revision that actually needs to be merged (for reverse
merges this is the oldest starting revision). */
svn_revnum_t end_rev =
get_most_inclusive_end_rev(notify_b->children_with_mergeinfo,
is_rollback);
Is the phrase "oldest starting revision" simply a typo for "oldest
ending revision"?
Now a question about empty ranges. The last major update to the doc
strings of get_most_inclusive_start_rev() and
get_most_inclusive_end_rev() was r872772. One further difference
between those two functions was called out prior to that change: the
former said "Skip no-op ranges on the target (they are probably
dummies)." That difference is still there in the code but not
documented. What it actually does is ignore the first child
(presumably that's the 'target') entirely if the first child's first
range is empty[1]:
get_most_inclusive_start_rev(): # pseudo-code
for (i = 0; i < children_with_mergeinfo->nelts; i++)
{
child = children_with_mergeinfo[i]
range = child->remaining_ranges[0]
if ((i == 0) && (range->start == range->end))
continue;
select the lowest/highest first-range start rev seen so far
}
I haven't yet found where and why an empty range is added or allowed
to remain on the target. Is it still relevant? Why is the skipping
only relevant for _start_rev() or should _end_rev() do it too?
I don't yet have enough comprehension of the whole merge code to know
what's right or wrong here or how to test for any practical effects.
- Julian
[1] The comment said 'no-op ranges', but I'm thinking 'empty ranges'
is less ambiguous since it's different from the no-op meaning of
remove_noop_merge_ranges(), remove_noop_subtree_ranges() and
log_noop_revs().
[2] We both seem to like footnotes so I thought I'd include one. Or
two, but one of them[2] isn't referenced except by itself.
Received on 2011-11-19 13:06:57 CET