[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Questions about get_most_inclusive_end_rev()

From: Paul Burba <ptburba_at_gmail.com>
Date: Mon, 21 Nov 2011 12:41:48 -0500

On Sat, Nov 19, 2011 at 7:06 AM, Julian Foad <julian.foad_at_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!
Received on 2011-11-21 18:42:23 CET

This is an archived mail posted to the Subversion Dev mailing list.