You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2006/09/01 22:53:23 UTC
Re: [PATCH] [merge-tracking] Code clarity fix
On Thu, 31 Aug 2006, Kamesh Jayachandran wrote:
> Daniel Berlin wrote:
> >>>
> >>I am not sure about the speed.
> >>I thought copying 8 bytes at one shot should be faster than dereferncing
> >>4 integer items and assigning them.
> >
> >Again, the compiler will turn one into the other depending on a number
> >of factors, including register pressure, etc.
> >
> >>If I understand this segment of code it just takes the 'forward merge
> >>ranges', which this '*(*output) = *in1; ' should be more meaningful with
> >>the associated commit message.
> >
> >At least to me, it's less meaningful to do structure copies in a lot
> >of cases.
> >Others may disagree.
> >
> Barring the (confusing???)structure copy stuff, the code segment
> essentially assigns 'in1' or 'in2' to output.
> Initially seeing a function having 6 else if blocks doing some checks
> with 'in1' and 'in2's members and extracting the same and assigning to
> some other structure, I felt bit difficult to differentiate this
> particular 'else if' blocks with others on the first glance. After all
> mental imaging of 'in1' and 'in2', I found that 'foo... this is just
> avoiding revert the range'
In this case, I find the structure copy style very slightly more
comprehensible.
> Anyways..........
> What is our stance on 'revert range' combining.
IIRC, we don't support it, which explains the (undefined) behavior you
describe below.
> Currently 'combine_ranges' for in1(5,9) in2(10, 6) sets *output to
> (5,9), is it correct? I feel we should return FALSE without changing the
> '*output' like we do for (5, 9) and (17, 26).
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Daniel Berlin <db...@dberlin.org>.
On 9/2/06, Kamesh Jayachandran <ka...@collab.net> wrote:
> Daniel Berlin wrote:
> >> In this case, I find the structure copy style very slightly more
> >> comprehensible.
> >>
> > If y'all feel that way, feel free to commit it.
> >
> Not needed now as the corresponding code is removed because we don't
> support revert range merges.
>
I'm aware.
I was making a general statement about code style.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Kamesh Jayachandran <ka...@collab.net>.
Daniel Berlin wrote:
>> In this case, I find the structure copy style very slightly more
>> comprehensible.
>>
> If y'all feel that way, feel free to commit it.
>
Not needed now as the corresponding code is removed because we don't
support revert range merges.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Daniel Berlin <db...@dberlin.org>.
> In this case, I find the structure copy style very slightly more
> comprehensible.
>
If y'all feel that way, feel free to commit it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 12 Sep 2006, Kamesh Jayachandran wrote:
> Sorry Dan overlooked at it.
> Philip's patch seems to be very concise and seems perfect to me.
Thanks guys, r21459.
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Kamesh Jayachandran <ka...@collab.net>.
Sorry Dan overlooked at it.
Philip's patch seems to be very concise and seems perfect to me.
Attaching the patch and log for the same.
With regards
Kamesh Jayachandran
Daniel Rall wrote:
> Hey Kamesh, did you see Philip's patch for this thread, and his
> subsequent comments? Was the consensus was that Philip's fix was both
> simpler and fully correct?
>
> Thanks, Dan
>
> On Mon, 11 Sep 2006, Kamesh Jayachandran wrote:
>
>
>> Thanks Jonathan,
>> Your patch looks perfect to me.
>>
>> I am attaching my tweaks to it.(Adding the MIN macro and a log message.)
>>
>> Someone with a karma please check in the same.
>>
>> It is not about optimizing the code, rather easy comprehension.
>>
>>
>> With regards
>> Kamesh Jayachandran
>> Jonathan Gilbert wrote:
>>
>>> At 09:40 PM 06/09/2006 +0100, Philip Martin wrote:
>>>
>>>
>>>> "Jonathan Gilbert" <o2...@sneakemail.com> writes:
>>>>
>>>>
>>>>> static svn_boolean_t
>>>>> combine_ranges(svn_merge_range_t **output, svn_merge_range_t *in1,
>>>>> svn_merge_range_t *in2)
>>>>> {
>>>>> if (range_contains_revision(in1, in2->start)
>>>>> || range_contains_revision(in2, in1->start)
>>>>> || ranges_are_adjacent(in1, in2))
>>>>> {
>>>>>
>>>>>
>>> [snip]
>>>
>>>
>>>> I haven't been following the merge tracking stuff, but detecting
>>>> overlapping ranges is notorious for being both tricky and simple: it's
>>>> tricky to get the algorithm right, but the right algorithm is simple.
>>>>
>>>> static svn_boolean_t
>>>> combine_ranges(svn_merge_range_t **output,
>>>> svn_merge_range_t *in1,
>>>> svn_merge_range_t *in2)
>>>> {
>>>> if (in1->start <= in2->end + 1 && in2->start <= in1->end + 1)
>>>> {
>>>>
>>>>
>>> [snip]
>>>
>>> Wow, that _is_ concise :-) I actually had a feeling something like that
>>> would be possible when I was composing my message, but it didn't spring
>>> into my head at the time. In any case, though my example is more verbose
>>> and adds two extra functions, I think it's more maintainable, and it is
>>> certainly more self-documenting.
>>>
>>> The added functions could also probably be used elsewhere to improve the
>>> readability/maintainability of other code that needs similar operations.
>>> Certainly, there's no reason to overoptimize something that is only
>>> O(number of revisions/ranges of revisions to be merged). :-)
>>>
>>> Jonathan Gilbert
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>>> For additional commands, e-mail: dev-help@subversion.tigris.org
>>>
>>>
>>>
>
>
>> [[[
>> Patch by: Jonathan Gilbert<o2...@sneakemail.com>
>> Kamesh Jayachandran <ka...@collab.net>
>>
>> Simplifying the code.
>>
>> * subversion/libsvn_subr/mergeinfo.c
>> (MIN): New macro.
>> (range_contains_revision): New function.
>> (ranges_are_adjacent): New function.
>> (combine_ranges):
>> New simplified equivalent implementation.
>> Removes the revert range combining.
>> ]]]
>>
>
>
>> Index: subversion/libsvn_subr/mergeinfo.c
>> ===================================================================
>> --- subversion/libsvn_subr/mergeinfo.c (revision 21420)
>> +++ subversion/libsvn_subr/mergeinfo.c (working copy)
>> @@ -32,61 +32,37 @@
>> #define MAX(a, b) ((a) < (b) ? (b) : (a))
>> #endif
>>
>> +/* Define a MIN macro if we don't already have one */
>> +#ifndef MIN
>> +#define MIN(a, b) ((a) < (b) ? (a) : (b))
>> +#endif
>> +
>> +static svn_boolean_t
>> +range_contains_revision(svn_merge_range_t *range, svn_revnum_t revision)
>> +{
>> + return (range->start <= revision) && (range->end >= revision);
>> +}
>> +
>> +static svn_boolean_t
>> +ranges_are_adjacent(svn_merge_range_t *in1, svn_merge_range_t *in2)
>> +{
>> + return (in1->end + 1 == in2->start) || (in2->end + 1 == in1->start);
>> +}
>> +
>> /* Attempt to combine two ranges, IN1 and IN2, and put the result in
>> OUTPUT. Return whether they could be combined. */
>> static svn_boolean_t
>> combine_ranges(svn_merge_range_t **output, svn_merge_range_t *in1,
>> svn_merge_range_t *in2)
>> {
>> - if (in1->start == in2->start)
>> + if (range_contains_revision(in1, in2->start)
>> + || range_contains_revision(in2, in1->start)
>> + || ranges_are_adjacent(in1, in2))
>> {
>> - (*output)->start = in1->start;
>> + (*output)->start = MIN(in1->start, in2->start);
>> (*output)->end = MAX(in1->end, in2->end);
>> return TRUE;
>> }
>> - /* [1,4] U [5,9] = [1,9] in subversion revisions */
>> - else if (in2->start == in1->end
>> - || in2->start == in1->end + 1)
>> - {
>> - (*output)->start = in1->start;
>> - (*output)->end = in2->end;
>> - return TRUE;
>> - }
>> - else if (in1->start == in2->end
>> - || in1->start == in2->end + 1)
>> - {
>> - (*output)->start = in2->start;
>> - (*output)->end = in1->end;
>> - return TRUE;
>> - }
>> - else if (in1->start <= in2->start
>> - && in1->end >= in2->start)
>> - {
>> - (*output)->start = in1->start;
>> - (*output)->end = MAX(in1->end, in2->end);
>> - return TRUE;
>> - }
>> - else if (in2->start <= in1->start
>> - && in2->end >= in1->start)
>> - {
>> - (*output)->start = in2->start;
>> - (*output)->end = MAX(in1->end, in2->end);
>> - return TRUE;
>> - }
>> - else if (in1->start >= in2->start
>> - && in1->end <= in2->end)
>> - {
>> - (*output)->start = in2->start;
>> - (*output)->end = in2->end;
>> - return TRUE;
>> - }
>> - else if (in2->start >= in1->start
>> - && in2->end <= in1->end)
>> - {
>> - (*output)->start = in1->start;
>> - (*output)->end = in1->end;
>> - return TRUE;
>> - }
>>
>> return FALSE;
>> }
>>
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Daniel Rall <dl...@collab.net>.
Hey Kamesh, did you see Philip's patch for this thread, and his
subsequent comments? Was the consensus was that Philip's fix was both
simpler and fully correct?
Thanks, Dan
On Mon, 11 Sep 2006, Kamesh Jayachandran wrote:
> Thanks Jonathan,
> Your patch looks perfect to me.
>
> I am attaching my tweaks to it.(Adding the MIN macro and a log message.)
>
> Someone with a karma please check in the same.
>
> It is not about optimizing the code, rather easy comprehension.
>
>
> With regards
> Kamesh Jayachandran
> Jonathan Gilbert wrote:
> >At 09:40 PM 06/09/2006 +0100, Philip Martin wrote:
> >
> >>"Jonathan Gilbert" <o2...@sneakemail.com> writes:
> >>
> >>>static svn_boolean_t
> >>>combine_ranges(svn_merge_range_t **output, svn_merge_range_t *in1,
> >>> svn_merge_range_t *in2)
> >>>{
> >>> if (range_contains_revision(in1, in2->start)
> >>> || range_contains_revision(in2, in1->start)
> >>> || ranges_are_adjacent(in1, in2))
> >>> {
> >>>
> >[snip]
> >
> >>I haven't been following the merge tracking stuff, but detecting
> >>overlapping ranges is notorious for being both tricky and simple: it's
> >>tricky to get the algorithm right, but the right algorithm is simple.
> >>
> >>static svn_boolean_t
> >>combine_ranges(svn_merge_range_t **output,
> >> svn_merge_range_t *in1,
> >> svn_merge_range_t *in2)
> >>{
> >> if (in1->start <= in2->end + 1 && in2->start <= in1->end + 1)
> >> {
> >>
> >[snip]
> >
> >Wow, that _is_ concise :-) I actually had a feeling something like that
> >would be possible when I was composing my message, but it didn't spring
> >into my head at the time. In any case, though my example is more verbose
> >and adds two extra functions, I think it's more maintainable, and it is
> >certainly more self-documenting.
> >
> >The added functions could also probably be used elsewhere to improve the
> >readability/maintainability of other code that needs similar operations.
> >Certainly, there's no reason to overoptimize something that is only
> >O(number of revisions/ranges of revisions to be merged). :-)
> >
> >Jonathan Gilbert
> >
> >---------------------------------------------------------------------
> >To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> >For additional commands, e-mail: dev-help@subversion.tigris.org
> >
> >
>
> [[[
> Patch by: Jonathan Gilbert<o2...@sneakemail.com>
> Kamesh Jayachandran <ka...@collab.net>
>
> Simplifying the code.
>
> * subversion/libsvn_subr/mergeinfo.c
> (MIN): New macro.
> (range_contains_revision): New function.
> (ranges_are_adjacent): New function.
> (combine_ranges):
> New simplified equivalent implementation.
> Removes the revert range combining.
> ]]]
> Index: subversion/libsvn_subr/mergeinfo.c
> ===================================================================
> --- subversion/libsvn_subr/mergeinfo.c (revision 21420)
> +++ subversion/libsvn_subr/mergeinfo.c (working copy)
> @@ -32,61 +32,37 @@
> #define MAX(a, b) ((a) < (b) ? (b) : (a))
> #endif
>
> +/* Define a MIN macro if we don't already have one */
> +#ifndef MIN
> +#define MIN(a, b) ((a) < (b) ? (a) : (b))
> +#endif
> +
> +static svn_boolean_t
> +range_contains_revision(svn_merge_range_t *range, svn_revnum_t revision)
> +{
> + return (range->start <= revision) && (range->end >= revision);
> +}
> +
> +static svn_boolean_t
> +ranges_are_adjacent(svn_merge_range_t *in1, svn_merge_range_t *in2)
> +{
> + return (in1->end + 1 == in2->start) || (in2->end + 1 == in1->start);
> +}
> +
> /* Attempt to combine two ranges, IN1 and IN2, and put the result in
> OUTPUT. Return whether they could be combined. */
> static svn_boolean_t
> combine_ranges(svn_merge_range_t **output, svn_merge_range_t *in1,
> svn_merge_range_t *in2)
> {
> - if (in1->start == in2->start)
> + if (range_contains_revision(in1, in2->start)
> + || range_contains_revision(in2, in1->start)
> + || ranges_are_adjacent(in1, in2))
> {
> - (*output)->start = in1->start;
> + (*output)->start = MIN(in1->start, in2->start);
> (*output)->end = MAX(in1->end, in2->end);
> return TRUE;
> }
> - /* [1,4] U [5,9] = [1,9] in subversion revisions */
> - else if (in2->start == in1->end
> - || in2->start == in1->end + 1)
> - {
> - (*output)->start = in1->start;
> - (*output)->end = in2->end;
> - return TRUE;
> - }
> - else if (in1->start == in2->end
> - || in1->start == in2->end + 1)
> - {
> - (*output)->start = in2->start;
> - (*output)->end = in1->end;
> - return TRUE;
> - }
> - else if (in1->start <= in2->start
> - && in1->end >= in2->start)
> - {
> - (*output)->start = in1->start;
> - (*output)->end = MAX(in1->end, in2->end);
> - return TRUE;
> - }
> - else if (in2->start <= in1->start
> - && in2->end >= in1->start)
> - {
> - (*output)->start = in2->start;
> - (*output)->end = MAX(in1->end, in2->end);
> - return TRUE;
> - }
> - else if (in1->start >= in2->start
> - && in1->end <= in2->end)
> - {
> - (*output)->start = in2->start;
> - (*output)->end = in2->end;
> - return TRUE;
> - }
> - else if (in2->start >= in1->start
> - && in2->end <= in1->end)
> - {
> - (*output)->start = in1->start;
> - (*output)->end = in1->end;
> - return TRUE;
> - }
>
> return FALSE;
> }
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Kamesh Jayachandran <ka...@collab.net>.
Thanks Jonathan,
Your patch looks perfect to me.
I am attaching my tweaks to it.(Adding the MIN macro and a log message.)
Someone with a karma please check in the same.
It is not about optimizing the code, rather easy comprehension.
With regards
Kamesh Jayachandran
Jonathan Gilbert wrote:
> At 09:40 PM 06/09/2006 +0100, Philip Martin wrote:
>
>> "Jonathan Gilbert" <o2...@sneakemail.com> writes:
>>
>>> static svn_boolean_t
>>> combine_ranges(svn_merge_range_t **output, svn_merge_range_t *in1,
>>> svn_merge_range_t *in2)
>>> {
>>> if (range_contains_revision(in1, in2->start)
>>> || range_contains_revision(in2, in1->start)
>>> || ranges_are_adjacent(in1, in2))
>>> {
>>>
> [snip]
>
>> I haven't been following the merge tracking stuff, but detecting
>> overlapping ranges is notorious for being both tricky and simple: it's
>> tricky to get the algorithm right, but the right algorithm is simple.
>>
>> static svn_boolean_t
>> combine_ranges(svn_merge_range_t **output,
>> svn_merge_range_t *in1,
>> svn_merge_range_t *in2)
>> {
>> if (in1->start <= in2->end + 1 && in2->start <= in1->end + 1)
>> {
>>
> [snip]
>
> Wow, that _is_ concise :-) I actually had a feeling something like that
> would be possible when I was composing my message, but it didn't spring
> into my head at the time. In any case, though my example is more verbose
> and adds two extra functions, I think it's more maintainable, and it is
> certainly more self-documenting.
>
> The added functions could also probably be used elsewhere to improve the
> readability/maintainability of other code that needs similar operations.
> Certainly, there's no reason to overoptimize something that is only
> O(number of revisions/ranges of revisions to be merged). :-)
>
> Jonathan Gilbert
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Jonathan Gilbert <o2...@sneakemail.com>.
At 09:40 PM 06/09/2006 +0100, Philip Martin wrote:
>"Jonathan Gilbert" <o2...@sneakemail.com> writes:
>> static svn_boolean_t
>> combine_ranges(svn_merge_range_t **output, svn_merge_range_t *in1,
>> svn_merge_range_t *in2)
>> {
>> if (range_contains_revision(in1, in2->start)
>> || range_contains_revision(in2, in1->start)
>> || ranges_are_adjacent(in1, in2))
>> {
[snip]
>
>I haven't been following the merge tracking stuff, but detecting
>overlapping ranges is notorious for being both tricky and simple: it's
>tricky to get the algorithm right, but the right algorithm is simple.
>
>static svn_boolean_t
>combine_ranges(svn_merge_range_t **output,
> svn_merge_range_t *in1,
> svn_merge_range_t *in2)
>{
> if (in1->start <= in2->end + 1 && in2->start <= in1->end + 1)
> {
[snip]
Wow, that _is_ concise :-) I actually had a feeling something like that
would be possible when I was composing my message, but it didn't spring
into my head at the time. In any case, though my example is more verbose
and adds two extra functions, I think it's more maintainable, and it is
certainly more self-documenting.
The added functions could also probably be used elsewhere to improve the
readability/maintainability of other code that needs similar operations.
Certainly, there's no reason to overoptimize something that is only
O(number of revisions/ranges of revisions to be merged). :-)
Jonathan Gilbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Philip Martin <ph...@codematters.co.uk>.
"Daniel Berlin" <db...@dberlin.org> writes:
> But you do realize a good compiler will optimize the longhand version
> into this, right?
> :)
Only if the bugs in the optimizer cancel out the bugs in the longhand
code ;)
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Daniel Berlin <db...@dberlin.org>.
> I haven't been following the merge tracking stuff, but detecting
> overlapping ranges is notorious for being both tricky and simple: it's
> tricky to get the algorithm right, but the right algorithm is simple.
>
> static svn_boolean_t
> combine_ranges(svn_merge_range_t **output,
> svn_merge_range_t *in1,
> svn_merge_range_t *in2)
> {
> if (in1->start <= in2->end + 1 && in2->start <= in1->end + 1)
> {
> (*output)->start = MIN(in1->start, in2->start);
> (*output)->end = MAX(in1->end, in2->end);
> return TRUE;
> }
> else
> return FALSE;
> }
>
Yeah, this would work.
But you do realize a good compiler will optimize the longhand version
into this, right?
:)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Philip Martin <ph...@codematters.co.uk>.
"Jonathan Gilbert" <o2...@sneakemail.com> writes:
> I have been watching this thread for some time now, and it seems that a
> much simpler algorithm could be used for what's needed. Basically, we want
> to merge two ranges if they are touching in any way, right? And it's safe
> to assume that the "end" field will always be >= the "start" field, right?
>
> If so, then consider the following code:
>
> static svn_boolean_t
> range_contains_revision(svn_merge_range_t *range, svn_revnum_t revision)
> {
> return (range->start <= revision) && (range->end >= revision);
> }
>
> static svn_boolean_t
> ranges_are_adjacent(svn_merge_range_t *in1, svn_merge_range_t *in2)
> {
> return (in1->end + 1 == in2->start) || (in2->end + 1 == in1->start);
> }
>
> static svn_boolean_t
> combine_ranges(svn_merge_range_t **output, svn_merge_range_t *in1,
> svn_merge_range_t *in2)
> {
> if (range_contains_revision(in1, in2->start)
> || range_contains_revision(in2, in1->start)
> || ranges_are_adjacent(in1, in2))
> {
> (*output)->start = MIN(in1->start, in2->start);
> (*output)->end = MAX(in1->end, in2->end);
> return TRUE;
> }
>
> return FALSE;
> }
>
> Basically, if the ranges are "touching", then either one of them will start
> in the middle of the other one, or one of them will start immediately after
> the other one. In either case, the start & end can be computed in the same
> way.
>
> Are there any holes in this? If not, I think it's a lot more readable than
> what's currently in mergeinfo.c on the merge-tracking branch. :-)
I haven't been following the merge tracking stuff, but detecting
overlapping ranges is notorious for being both tricky and simple: it's
tricky to get the algorithm right, but the right algorithm is simple.
static svn_boolean_t
combine_ranges(svn_merge_range_t **output,
svn_merge_range_t *in1,
svn_merge_range_t *in2)
{
if (in1->start <= in2->end + 1 && in2->start <= in1->end + 1)
{
(*output)->start = MIN(in1->start, in2->start);
(*output)->end = MAX(in1->end, in2->end);
return TRUE;
}
else
return FALSE;
}
A discussion of the algorithm is here:
http://c2.com/cgi-bin/wiki/fullSearch?TestIfDateRangesOverlap
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] [merge-tracking] Code clarity fix
Posted by Jonathan Gilbert <o2...@sneakemail.com>.
At 7:32 PM 06/09/2006, Daniel Berlin wrote:
> I'm not sure what you think this patch does, but it's not right to
> remove these cases.
>
> The cases below that you are removing combine ranges in case you have
> the following:
>
> in1 = [0, 10]
> in2 = [4, 7]
> and
> in1= [1, 3]
> in2 = [0, 10]
>
> In both cases, one range completely subsumes the other, so we just use
> one of the ranges.
>
> It has nothing to do with revert ranges.
At 10:03 PM 06/09/2006 +0530, Kamesh Jayachandran wrote:
> //Takes care of in1 = [0, 10] in2 = [4, 7]
> else if (in1->start <= in2->start
> && in1->end >= in2->start)
> {
> (*output)->start = in1->start;
> (*output)->end = MAX(in1->end, in2->end);
> return TRUE;
> }
> //Takes care of in1= [1, 3] in2 = [0, 10]
>
> else if (in2->start <= in1->start
> && in2->end >= in1->start)
> {
> (*output)->start = in2->start;
> (*output)->end = MAX(in1->end, in2->end);
> return TRUE;
> }
>
> The code that is removed by the patch try to handle revert ranges which
> should be a no-op.
I have been watching this thread for some time now, and it seems that a
much simpler algorithm could be used for what's needed. Basically, we want
to merge two ranges if they are touching in any way, right? And it's safe
to assume that the "end" field will always be >= the "start" field, right?
If so, then consider the following code:
static svn_boolean_t
range_contains_revision(svn_merge_range_t *range, svn_revnum_t revision)
{
return (range->start <= revision) && (range->end >= revision);
}
static svn_boolean_t
ranges_are_adjacent(svn_merge_range_t *in1, svn_merge_range_t *in2)
{
return (in1->end + 1 == in2->start) || (in2->end + 1 == in1->start);
}
static svn_boolean_t
combine_ranges(svn_merge_range_t **output, svn_merge_range_t *in1,
svn_merge_range_t *in2)
{
if (range_contains_revision(in1, in2->start)
|| range_contains_revision(in2, in1->start)
|| ranges_are_adjacent(in1, in2))
{
(*output)->start = MIN(in1->start, in2->start);
(*output)->end = MAX(in1->end, in2->end);
return TRUE;
}
return FALSE;
}
Basically, if the ranges are "touching", then either one of them will start
in the middle of the other one, or one of them will start immediately after
the other one. In either case, the start & end can be computed in the same
way.
Are there any holes in this? If not, I think it's a lot more readable than
what's currently in mergeinfo.c on the merge-tracking branch. :-)
Jonathan Gilbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] [merge-tracking] Code clarity fix
Posted by Kamesh Jayachandran <ka...@collab.net>.
//Takes care of in1 = [0, 10] in2 = [4, 7]
else if (in1->start <= in2->start
&& in1->end >= in2->start)
{
(*output)->start = in1->start;
(*output)->end = MAX(in1->end, in2->end);
return TRUE;
}
//Takes care of in1= [1, 3] in2 = [0, 10]
else if (in2->start <= in1->start
&& in2->end >= in1->start)
{
(*output)->start = in2->start;
(*output)->end = MAX(in1->end, in2->end);
return TRUE;
}
The code that is removed by the patch try to handle revert ranges which should be a no-op.
With regards
Kamesh Jayachandran
-----Original Message-----
From: Daniel Berlin [mailto:dberlin@dberlin.org]
Sent: Wed 06/09/2006 19:32
To: Kamesh Jayachandran
Cc: Daniel Rall; SVN Dev
Subject: Re: [PATCH] [merge-tracking] Code clarity fix
> >
> > [[[
> > Patch by: Kamesh Jayachandran <ka...@collab.net>
> >
> > Combining the revert ranges is undefined.
> >
> > * subversion/libsvn_subr/mergeinfo.c
> > (combine_ranges):
> > Combining the revert ranges is undefined.
> > ]]]
> >
> > ------------------------------------------------------------------------
> >
I'm not sure what you think this patch does, but it's not right to
remove these cases.
The cases below that you are removing combine ranges in case you have
the following:
in1 = [0, 10]
in2 = [4, 7]
and
in1= [1, 3]
in2 = [0, 10]
In both cases, one range completely subsumes the other, so we just use
one of the ranges.
It has nothing to do with revert ranges.
> > Index: subversion/libsvn_subr/mergeinfo.c
> > ===================================================================
> > --- subversion/libsvn_subr/mergeinfo.c (revision 21340)
> > +++ subversion/libsvn_subr/mergeinfo.c (working copy)
> > @@ -73,20 +73,6 @@
> > (*output)->end = MAX(in1->end, in2->end);
> > return TRUE;
> > }
> > - else if (in1->start >= in2->start
> > - && in1->end <= in2->end)
> > - {
> > - (*output)->start = in2->start;
> > - (*output)->end = in2->end;
> > - return TRUE;
> > - }
> > - else if (in2->start >= in1->start
> > - && in2->end <= in1->end)
> > - {
> > - (*output)->start = in1->start;
> > - (*output)->end = in1->end;
> > - return TRUE;
> > - }
> >
> > return FALSE;
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Daniel Berlin <db...@dberlin.org>.
> >
> > [[[
> > Patch by: Kamesh Jayachandran <ka...@collab.net>
> >
> > Combining the revert ranges is undefined.
> >
> > * subversion/libsvn_subr/mergeinfo.c
> > (combine_ranges):
> > Combining the revert ranges is undefined.
> > ]]]
> >
> > ------------------------------------------------------------------------
> >
I'm not sure what you think this patch does, but it's not right to
remove these cases.
The cases below that you are removing combine ranges in case you have
the following:
in1 = [0, 10]
in2 = [4, 7]
and
in1= [1, 3]
in2 = [0, 10]
In both cases, one range completely subsumes the other, so we just use
one of the ranges.
It has nothing to do with revert ranges.
> > Index: subversion/libsvn_subr/mergeinfo.c
> > ===================================================================
> > --- subversion/libsvn_subr/mergeinfo.c (revision 21340)
> > +++ subversion/libsvn_subr/mergeinfo.c (working copy)
> > @@ -73,20 +73,6 @@
> > (*output)->end = MAX(in1->end, in2->end);
> > return TRUE;
> > }
> > - else if (in1->start >= in2->start
> > - && in1->end <= in2->end)
> > - {
> > - (*output)->start = in2->start;
> > - (*output)->end = in2->end;
> > - return TRUE;
> > - }
> > - else if (in2->start >= in1->start
> > - && in2->end <= in1->end)
> > - {
> > - (*output)->start = in1->start;
> > - (*output)->end = in1->end;
> > - return TRUE;
> > - }
> >
> > return FALSE;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Kamesh Jayachandran <ka...@collab.net>.
Can someone look into this?
With regards
Kamesh Jayachandran
Kamesh Jayachandran wrote:
> Daniel Rall wrote:
>> On Thu, 31 Aug 2006, Kamesh Jayachandran wrote:
>>
>> In this case, I find the structure copy style very slightly more
>> comprehensible.
>>
>>
>>
>>> Anyways..........
>>> What is our stance on 'revert range' combining.
>>>
>>
>> IIRC, we don't support it, which explains the (undefined) behavior you
>> describe below.
>>
>>
> Attached log/patch achieves the same.
>
> With regards
> Kamesh Jayachandran
> ------------------------------------------------------------------------
>
> [[[
> Patch by: Kamesh Jayachandran <ka...@collab.net>
>
> Combining the revert ranges is undefined.
>
> * subversion/libsvn_subr/mergeinfo.c
> (combine_ranges):
> Combining the revert ranges is undefined.
> ]]]
>
> ------------------------------------------------------------------------
>
> Index: subversion/libsvn_subr/mergeinfo.c
> ===================================================================
> --- subversion/libsvn_subr/mergeinfo.c (revision 21340)
> +++ subversion/libsvn_subr/mergeinfo.c (working copy)
> @@ -73,20 +73,6 @@
> (*output)->end = MAX(in1->end, in2->end);
> return TRUE;
> }
> - else if (in1->start >= in2->start
> - && in1->end <= in2->end)
> - {
> - (*output)->start = in2->start;
> - (*output)->end = in2->end;
> - return TRUE;
> - }
> - else if (in2->start >= in1->start
> - && in2->end <= in1->end)
> - {
> - (*output)->start = in1->start;
> - (*output)->end = in1->end;
> - return TRUE;
> - }
>
> return FALSE;
> }
>
>
> ------------------------------------------------------------------------
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] [merge-tracking] Code clarity fix
Posted by Kamesh Jayachandran <ka...@collab.net>.
Daniel Rall wrote:
> On Thu, 31 Aug 2006, Kamesh Jayachandran wrote:
>
>
> In this case, I find the structure copy style very slightly more
> comprehensible.
>
>
>
>> Anyways..........
>> What is our stance on 'revert range' combining.
>>
>
> IIRC, we don't support it, which explains the (undefined) behavior you
> describe below.
>
>
Attached log/patch achieves the same.
With regards
Kamesh Jayachandran