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

Valid values of svn_merge_range_t - no change number zero

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Fri, 12 Mar 2010 16:59:35 +0000

Hi Paul.

I think we can tighten the validation of svn_merge_range_t to exclude
change number "r0" (RANGE->start == -1) as in the following patch.

My reasoning is that a change numbered "r0" is not a valid concept in
any Subversion system because the state (tree-snapshot) numbered r0 is
by definition the beginning. (It also happens to be empty by
definition, but that's not so relevant.) We can say the same in a
different way: change "r0" would mean the change from "r(-1)" to "r0",
and "r(-1)" is not a valid concept.

Makes sense?

[[[
Tighten merge-range validation to not allow "change number r0" aka "revision
range -1:Y".

* subversion/libsvn_subr/mergeinfo.c
  (IS_VALID_FORWARD_RANGE): New macro.
  (get_type_of_intersection): Use IS_VALID_FORWARD_RANGE() for tighter
    validation of arguments than before: it previously accepted "change 0".
  (range_intersect, range_contains): Validate arguments. Add doc strings.

* subversion/tests/libsvn_subr/mergeinfo-test.c
  (randomly_fill_rev_array, rev_array_to_rangelist): Expand doc strings.
  (test_rangelist_remove_randomly, test_rangelist_intersect_randomly): Don't
    ever include change number r0 in a merge range.
--This line, and those below, will be ignored--

Index: subversion/tests/libsvn_subr/mergeinfo-test.c
===================================================================
--- subversion/tests/libsvn_subr/mergeinfo-test.c (revision 922300)
+++ subversion/tests/libsvn_subr/mergeinfo-test.c (working copy)
@@ -877,24 +877,27 @@ test_remove_rangelist(apr_pool_t *pool)

 #define RANDOM_REV_ARRAY_LENGTH 100

 /* Random number seed. */
 static apr_uint32_t random_rev_array_seed;

-/* Fill 3/4 of the array with 1s. */
+/* Set a random 3/4-ish of the elements of array REVS[RANDOM_REV_ARRAY_LENGTH]
+ * to TRUE and the rest to FALSE. */
 static void
 randomly_fill_rev_array(svn_boolean_t *revs)
 {
   int i;
   for (i = 0; i < RANDOM_REV_ARRAY_LENGTH; i++)
     {
       apr_uint32_t next = svn_test_rand(&random_rev_array_seed);
       revs[i] = (next < 0x40000000) ? 0 : 1;
     }
 }

+/* Set *RANGELIST to a rangelist representing the revisions that are marked
+ * with TRUE in the array REVS[RANDOM_REV_ARRAY_LENGTH]. */
 static svn_error_t *
 rev_array_to_rangelist(apr_array_header_t **rangelist,
                        svn_boolean_t *revs,
                        apr_pool_t *pool)
 {
   svn_stringbuf_t *buf = svn_stringbuf_create("/trunk: ", pool);
@@ -943,12 +946,15 @@ test_rangelist_remove_randomly(apr_pool_
       int j;

       svn_pool_clear(iterpool);

       randomly_fill_rev_array(first_revs);
       randomly_fill_rev_array(second_revs);
+ /* There is no change numbered "r0" */
+ first_revs[0] = FALSE;
+ second_revs[0] = FALSE;
       for (j = 0; j < RANDOM_REV_ARRAY_LENGTH; j++)
         expected_revs[j] = second_revs[j] && !first_revs[j];

       SVN_ERR(rev_array_to_rangelist(&first_rangelist, first_revs, iterpool));
       SVN_ERR(rev_array_to_rangelist(&second_rangelist, second_revs, iterpool));
       SVN_ERR(rev_array_to_rangelist(&expected_rangelist, expected_revs,
@@ -998,12 +1004,15 @@ test_rangelist_intersect_randomly(apr_po
       int j;

       svn_pool_clear(iterpool);

       randomly_fill_rev_array(first_revs);
       randomly_fill_rev_array(second_revs);
+ /* There is no change numbered "r0" */
+ first_revs[0] = FALSE;
+ second_revs[0] = FALSE;
       for (j = 0; j < RANDOM_REV_ARRAY_LENGTH; j++)
         expected_revs[j] = second_revs[j] && first_revs[j];

       SVN_ERR(rev_array_to_rangelist(&first_rangelist, first_revs, iterpool));
       SVN_ERR(rev_array_to_rangelist(&second_rangelist, second_revs, iterpool));
       SVN_ERR(rev_array_to_rangelist(&expected_rangelist, expected_revs,
Index: subversion/libsvn_subr/mergeinfo.c
===================================================================
--- subversion/libsvn_subr/mergeinfo.c (revision 922300)
+++ subversion/libsvn_subr/mergeinfo.c (working copy)
@@ -112,12 +112,22 @@ parse_pathname(const char **input,

   *input = last_colon;

   return SVN_NO_ERROR;
 }

+/* Return TRUE iff (svn_merge_range_t *) RANGE describes a valid, forward
+ * revision range.
+ *
+ * Note: The smallest valid value of RANGE->start is 0 because it is an
+ * exclusive endpoint, being one less than the revision number of the first
+ * change described by the range, and the oldest possible change is "r1" as
+ * there cannot be a change "r0". */
+#define IS_VALID_FORWARD_RANGE(range) \
+ (SVN_IS_VALID_REVNUM((range)->start) && ((range)->start < (range)->end))
+
 /* Ways in which two svn_merge_range_t can intersect or adjoin, if at all. */
 typedef enum
 {
   /* Ranges don't intersect and don't adjoin. */
   svn__no_intersection,

@@ -141,22 +151,14 @@ static svn_error_t *
 get_type_of_intersection(const svn_merge_range_t *r1,
                          const svn_merge_range_t *r2,
                          intersection_type_t *intersection_type)
 {
   SVN_ERR_ASSERT(r1);
   SVN_ERR_ASSERT(r2);
-
- /* Why not use SVN_IS_VALID_REVNUM here? Because revision 0
- is described START = -1, END = 0. See svn_merge_range_t. */
- SVN_ERR_ASSERT(r1->start >= -1);
- SVN_ERR_ASSERT(r2->start >= -1);
-
- SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(r1->end));
- SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(r2->end));
- SVN_ERR_ASSERT(r1->start < r1->end);
- SVN_ERR_ASSERT(r2->start < r2->end);
+ SVN_ERR_ASSERT(IS_VALID_FORWARD_RANGE(r1));
+ SVN_ERR_ASSERT(IS_VALID_FORWARD_RANGE(r2));

   if (!(r1->start <= r2->end && r2->start <= r1->end))
     *intersection_type = svn__no_intersection;
   else if (r1->start == r2->start && r1->end == r2->end)
     *intersection_type = svn__equal_intersection;
   else if (r1->end == r2->start || r2->end == r1->start)
@@ -760,26 +762,37 @@ svn_rangelist_merge(apr_array_header_t *
     }

   *rangelist = output;
   return SVN_NO_ERROR;
 }

+/* Return TRUE iff the forward revision ranges FIRST and SECOND overlap and
+ * (if CONSIDER_INHERITANCE is TRUE) have the same inheritability. */
 static svn_boolean_t
 range_intersect(const svn_merge_range_t *first, const svn_merge_range_t *second,
                 svn_boolean_t consider_inheritance)
 {
+ SVN_ERR_ASSERT_NO_RETURN(IS_VALID_FORWARD_RANGE(first));
+ SVN_ERR_ASSERT_NO_RETURN(IS_VALID_FORWARD_RANGE(second));
+
   return (first->start + 1 <= second->end)
     && (second->start + 1 <= first->end)
     && (!consider_inheritance
         || (!(first->inheritable) == !(second->inheritable)));
 }

+/* Return TRUE iff the forward revision range FIRST wholly contains the
+ * forward revision range SECOND and (if CONSIDER_INHERITANCE is TRUE) has
+ * the same inheritability. */
 static svn_boolean_t
 range_contains(const svn_merge_range_t *first, const svn_merge_range_t *second,
                svn_boolean_t consider_inheritance)
 {
+ SVN_ERR_ASSERT_NO_RETURN(IS_VALID_FORWARD_RANGE(first));
+ SVN_ERR_ASSERT_NO_RETURN(IS_VALID_FORWARD_RANGE(second));
+
   return (first->start <= second->start) && (second->end <= first->end)
     && (!consider_inheritance
         || (!(first->inheritable) == !(second->inheritable)));
 }

 /* Swap start and end fields of RANGE. */
]]]

- Julian
Received on 2010-03-12 18:00:13 CET

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