You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2010/03/12 16:59:35 UTC
Valid values of svn_merge_range_t - no change number zero
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
Re: Valid values of svn_merge_range_t - no change number zero
Posted by Julian Foad <ju...@wandisco.com>.
BTW this is just a request for comments, not a finished patch.
(mergeinfo_tests.py 2 fails and I have not yet investigated.)
- Julian
I (Julian Foad) wrote:
> 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--
[...]
Re: Valid values of svn_merge_range_t - no change number zero
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-03-16, Paul Burba wrote:
> Your patch looks good to me. It passes all tests [fsfs x ra_local]
> and I can't see that it will introduce any problems. I assume the
> mergeinfo_tests.py 2 failure you mentioned at the start of the thread
> was solved in v2?
Thanks. Yes, the test failures I saw with v1 were fixed by v2.
Committed in r924201.
- Julian
Re: Valid values of svn_merge_range_t - no change number zero
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Mar 16, 2010 at 8:21 AM, Paul Burba <pt...@gmail.com> wrote:
> On Tue, Mar 16, 2010 at 7:52 AM, Julian Foad <ju...@wandisco.com> wrote:
>> On Mon, 2010-03-15, Paul Burba wrote:
>>> On Fri, Mar 12, 2010 at 12:59 PM, Julian Foad <ju...@wandisco.com> wrote:
>>> > 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?
>>>
>>> Hi Julian,
>>>
>>> It does make sense, but I can't apply this patch, seems to have a few
>>> problems, see below.
>>
>> [...]
>>> > @@ -877,24 +877,27 @@ test_remove_rangelist(apr_pool_t *pool)
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> Why is that all on one line? Those offsets are supposed to be on
>>> their own line no?
>>
>> I don't think this is the problem. This is the diff format produced by
>> the "--show-c-function" ("-p") option to GNU diff or "svn diff".
>>
>>> Also, test_remove_rangelist(apr_pool_t *pool) looks to be on line 706
>>> in https://svn.apache.org/repos/asf/subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c@922300,
>>> so it seems your from-to headers are off.
>>
>> That's just because the "--show-c-function" option is very dumb and just
>> intended as a guess. It just shows (part of) the last line it saw
>> before the current hunk that looked like a line introducing a function.
>>
>> [...]
>>> > @@ -943,12 +946,15 @@ test_rangelist_remove_randomly(apr_pool_
>>> ^^^
>>> Again, all on one line and this time the line is truncated.
>>
>> Yup, that's normal.
>
> Ok, didn't realize that was all normal.
>
>>> Anyhow, could try attaching the patch again?
>>
>> Certainly. Attached v2, which in addition has a fix for one of the
>> places that generated an "r0" merge-range.
>
> Great, the applies cleanly.
>
>> It's possible your patch application was marred by line-endings, or
>> maybe can't cope with 6 context lines (the default is 3), so I've
>> attached the patch this time, and cut the context lines back to 3.
>> (What patch-application program was it?)
>
> It's TortoiseSVN's built-in patch tool.
>
> I have to prep for a meeting right now, but I'll look at this later today.
>
> Thanks,
>
> Paul
Julian,
Your patch looks good to me. It passes all tests [fsfs x ra_local]
and I can't see that it will introduce any problems. I assume the
mergeinfo_tests.py 2 failure you mentioned at the start of the thread
was solved in v2?
Paul
Re: Valid values of svn_merge_range_t - no change number zero
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Mar 16, 2010 at 7:52 AM, Julian Foad <ju...@wandisco.com> wrote:
> On Mon, 2010-03-15, Paul Burba wrote:
>> On Fri, Mar 12, 2010 at 12:59 PM, Julian Foad <ju...@wandisco.com> wrote:
>> > 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?
>>
>> Hi Julian,
>>
>> It does make sense, but I can't apply this patch, seems to have a few
>> problems, see below.
>
> [...]
>> > @@ -877,24 +877,27 @@ test_remove_rangelist(apr_pool_t *pool)
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> Why is that all on one line? Those offsets are supposed to be on
>> their own line no?
>
> I don't think this is the problem. This is the diff format produced by
> the "--show-c-function" ("-p") option to GNU diff or "svn diff".
>
>> Also, test_remove_rangelist(apr_pool_t *pool) looks to be on line 706
>> in https://svn.apache.org/repos/asf/subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c@922300,
>> so it seems your from-to headers are off.
>
> That's just because the "--show-c-function" option is very dumb and just
> intended as a guess. It just shows (part of) the last line it saw
> before the current hunk that looked like a line introducing a function.
>
> [...]
>> > @@ -943,12 +946,15 @@ test_rangelist_remove_randomly(apr_pool_
>> ^^^
>> Again, all on one line and this time the line is truncated.
>
> Yup, that's normal.
Ok, didn't realize that was all normal.
>> Anyhow, could try attaching the patch again?
>
> Certainly. Attached v2, which in addition has a fix for one of the
> places that generated an "r0" merge-range.
Great, the applies cleanly.
> It's possible your patch application was marred by line-endings, or
> maybe can't cope with 6 context lines (the default is 3), so I've
> attached the patch this time, and cut the context lines back to 3.
> (What patch-application program was it?)
It's TortoiseSVN's built-in patch tool.
I have to prep for a meeting right now, but I'll look at this later today.
Thanks,
Paul
Re: Valid values of svn_merge_range_t - no change number zero
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-03-15, Paul Burba wrote:
> On Fri, Mar 12, 2010 at 12:59 PM, Julian Foad <ju...@wandisco.com> wrote:
> > 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?
>
> Hi Julian,
>
> It does make sense, but I can't apply this patch, seems to have a few
> problems, see below.
[...]
> > @@ -877,24 +877,27 @@ test_remove_rangelist(apr_pool_t *pool)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Why is that all on one line? Those offsets are supposed to be on
> their own line no?
I don't think this is the problem. This is the diff format produced by
the "--show-c-function" ("-p") option to GNU diff or "svn diff".
> Also, test_remove_rangelist(apr_pool_t *pool) looks to be on line 706
> in https://svn.apache.org/repos/asf/subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c@922300,
> so it seems your from-to headers are off.
That's just because the "--show-c-function" option is very dumb and just
intended as a guess. It just shows (part of) the last line it saw
before the current hunk that looked like a line introducing a function.
[...]
> > @@ -943,12 +946,15 @@ test_rangelist_remove_randomly(apr_pool_
> ^^^
> Again, all on one line and this time the line is truncated.
Yup, that's normal.
> Anyhow, could try attaching the patch again?
Certainly. Attached v2, which in addition has a fix for one of the
places that generated an "r0" merge-range.
It's possible your patch application was marred by line-endings, or
maybe can't cope with 6 context lines (the default is 3), so I've
attached the patch this time, and cut the context lines back to 3.
(What patch-application program was it?)
- Julian
Re: Valid values of svn_merge_range_t - no change number zero
Posted by Paul Burba <pt...@gmail.com>.
On Fri, Mar 12, 2010 at 12:59 PM, Julian Foad <ju...@wandisco.com> wrote:
> 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?
Hi Julian,
It does make sense, but I can't apply this patch, seems to have a few
problems, see below.
> [[[
> 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)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Why is that all on one line? Those offsets are supposed to be on
their own line no?
Also, test_remove_rangelist(apr_pool_t *pool) looks to be on line 706
in https://svn.apache.org/repos/asf/subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c@922300,
so it seems your from-to headers are off.
>
> #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_
^^^
Again, all on one line and this time the line is truncated.
Anyhow, could try attaching the patch again?
Paul