Questions about get_most_inclusive_end_rev()

[Julian Foad <ju...@wandisco.com>]

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.

Re: Questions about get_most_inclusive_end_rev()

[Julian Foad <ju...@wandisco.com>]

Paul Burba wrote:
> You are correct, the doc string had it backwards.  I fixed that.
[...]
> 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.

Thanks, Paul.  That's much clearer.

- Julian

Re: Questions about get_most_inclusive_end_rev()

[Paul Burba <pt...@gmail.com>]

On Sat, Nov 19, 2011 at 7:06 AM, Julian Foad <ju...@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!