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

Re: [PATCH] [merge-tracking] Code clarity fix

From: Kamesh Jayachandran <kamesh_at_collab.net>
Date: 2006-09-11 13:58:18 CEST

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" <o2w9gs702@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<o2w9gs702@sneakemail.com>
          Kamesh Jayachandran <kamesh@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;
 }

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 11 13:59:30 2006

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.