Re: [RFC] QA of manually set svn:mergeinfo

On Thu, 08 Nov 2007, Paul Burba wrote:

> > -----Original Message-----
> > From: Mark Phippard [mailto:markphip@gmail.com]
> > Sent: Wednesday, November 07, 2007 11:51 AM
> > To: Paul Burba
> > Cc: dev@subversion.tigris.org
> > Subject: Re: [RFC] QA of manually set svn:mergeinfo
> >
> > On 11/7/07, Paul Burba <pburba@collab.net> wrote:
> > > In the case of improperly ordered ranges I think we can safely say
> > > there is only one correct interpretation ("/trunk:6,3" ==
> > "/trunk:3,6"
> > > no matter how much you look at it :-).
> > >
> > > With overlapping changes with the same inheritability there is only
> > > one correct meaning as well ("/trunk:5-6,6-7" == "/trunk:5-7").
> > >
> > > *But* in both cases the fact that a user specified something odd
> > > raises a red flag for me that maybe they meant something
> > else entirely
> > > or simply don't understand what they are doing. I'm quite
> > comfortable
> > > being ultra-strict when someone is using propset to set
> > svn:mergeinfo.
> >
> > That was exactly my point. Your first example above is
> > perfect. Was the user just sloppy and ordered the value
> > wrong ... or is the ",3"
> > supposed to be ",13"? There is no way to know. The fact
> > that the format is not right is a clue that something might
> > be wrong, so maybe we should just fail it.
>
> Not entirely sure now that unordered ranges should result in an error.
> The 'Note' in the doc string for svn_mergeinfo_parse() implies that the
> input string need not have revision ranges in order:
>
> /** Parse the mergeinfo from @a input into @a *mergeinfo, mapping from
> * paths to @c apr_array_header_t *'s of @c svn_merge_range_t *
> * elements. If no mergeinfo is available, return an empty hash
> * (never @c NULL). Perform temporary allocations in @a pool.
> *
> * Note: @a *mergeinfo will contain rangelists that are guaranteed to
> * be sorted (ordered by smallest revision ranges to largest).
> * @since New in 1.5.
> */
> svn_error_t *
> svn_mergeinfo_parse(apr_hash_t **mergeinfo, const char *input,
> apr_pool_t *pool);
 
This code has always required that the rangelists in the INPUT text be
in sorted order.

> Keeping the explicit guarantee is fine, but should we accept unordered
> ranges in INPUT and correctly order them or return an error? IIUC the
> only time that a user would see this error is when manually setting
> svn:mergeinfo with propset and thus far we seem to agree that an error
> is ok in that case. We make heavy use of svn_mergeinfo_parse()
> elsewhere, but in those cases we are parsing svn:mergeinfo that has
> already been set (and *should* be ordered).
>
> > If we are going to fail it, the error message has to help.
> > They could have thousands of revisions listed. If they put
> > two of them out of order we have to be able to tell them that
> > or users are going to hate us.
> >
> > Think of the difference between saying an XML file is invalid
> > and telling you the line that is invalid. We need to try to
> > help if we are going to fail the command.