On Sat, Nov 19, 2011 at 7:06 AM, Julian Foad <julian.f...@wandisco.com> wrote:
> 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.
Hi Julian,
You are correct, the doc string had it backwards. I fixed that.
> 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"?
Doh, that was backwards too. It should read:
/* Now examine NOTIFY_B->CHILDREN_WITH_MERGEINFO to find the oldest
ending revision that actually needs to be merged (for reverse
merges this is the youngest ending revision). */
svn_revnum_t end_rev =
get_most_inclusive_end_rev(notify_b->children_with_mergeinfo,
is_rollback);
Fixed that too.
> 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?
Neither should do it. That check (range->start == range->end) was
once necessary when we engaged in some special case abuse of
svn_merge_range_t. I did away with that a long time back but missed
this check while cleaning up -- see the log for r872890, it should
answer your questions.
I removed the check and then, since get_most_inclusive_start_rev() and
get_most_inclusive_end_rev() are almost identical, I combined the two
into a new function get_most_inclusive_rev.
All the above changes were made in r1204617.
Paul
> 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.
Guilty as changed -- I love footnotes!
