Re: svn commit: r1872030 - /subversion/trunk/subversion/libsvn_subr/mergeinfo.c

[Daniel Shahaf <d....@daniel.shahaf.name>]

Good morning Julian,

julianfoad@apache.org wrote on Fri, Dec 27, 2019 at 15:08:31 -0000:
> Author: julianfoad
> Date: Fri Dec 27 15:08:31 2019
> New Revision: 1872030
> 
> URL: http://svn.apache.org/viewvc?rev=1872030&view=rev
> Log:
> Avoid converting invalid mergeinfo to bogus valid-looking mergeinfo.
> 
> For issue #4840, "Merge assertion failure in svn_sort__array_insert".

Right.  I did try to follow the 4840 thread when you started it on dev@.

> The internal representation of mergeinfo is not supposed to include
> empty ranges.  If a representation of an empty range did occur, perhaps
> due to a bug in mergeinfo processing or improper data passed to an API,
> the mergeinfo printing functions produced bogus valid-looking output.

I agree that this is a problem…

> For example, for a range with (.start == .end == 10), it wrote "10-11".
> This was very confusing when seen in error messages and debugging.

… and not just because it gets in the way of debugging, but due to the
more general "In the face of ambiguity, refuse the temptation to guess" …

> Instead, print a diagnostic output in the form "[empty-range@10*]".

… but I'm not sure about this part.  This stringification will be used
not only by error messages but also by svn_mergeinfo_to_string(), and
I suspect (but haven't confirmed) it could find its way into versioned
data.  Is this the desired behaviour?  Would it be better for the public
API's error out on invalid data, rather than propagate it (and hope
whoever's getting the data will parse it immediately, lest the error
remain dormant)?  We could still have a way to stringify even invalid
data, for debugging purposes.

> This output is not intended be accepted as input by the parsing
> functions.

HTH,

Daniel

Re: svn commit: r1872030 - /subversion/trunk/subversion/libsvn_subr/mergeinfo.c

[Julian Foad <ju...@apache.org>]

Daniel Shahaf wrote:
>> Instead, print a diagnostic output in the form "[empty-range@10*]".
> 
> … but I'm not sure about this part.  This stringification will be used
> not only by error messages but also by svn_mergeinfo_to_string(), and
> I suspect (but haven't confirmed) it could find its way into versioned
> data.

Good point.

http://svn.apache.org/r1872104 : Raise an error on trying to convert 
invalid mergeinfo to a string.

Thanks.
- Julian