On Fri, Mar 12, 2010 at 12:59 PM, Julian Foad <julian.foad_at_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
Received on 2010-03-15 19:45:40 CET