subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julian.f...@wandisco.com>
Subject Re: svn commit: r1041230 - /subversion/trunk/subversion/include/svn_checksum.h
Date Mon, 06 Dec 2010 15:47:36 GMT
On Sun, 2010-12-05, Stefan Fuhrmann wrote:
> On 02.12.2010 07:57, Blair Zajac wrote:
> > On 12/1/10 4:38 PM, stefan2@apache.org wrote:
> >> Author: stefan2
> >> Date: Thu Dec  2 00:38:17 2010
> >> New Revision: 1041230
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1041230&view=rev
> >> Log:
> >> Fix the svn_checksum_to_cstring() docstring to actually say what
> >> was intended. Also, make clear that the behavior is new for 1.7 and
> >> trying to use it in 1.6 will cause segfaults.
> >>
> >> * subversion/include/svn_checksum.h
> >>    (svn_checksum_to_cstring): fix docstring
> >
> > What happens if somebody makes a svn tool that is compiled and built 
> > against the new 1.7 behavior and then it is backported to 1.6, it may 
> > core dump.
> >
> > Should we add a svn_checksum_to_cstring2() instead with the new 
> > behavior or backport this change to 1.6?  But even then we'll have 1.6 
> > versions with different behavior.  It seems making a new 
> > svn_checksum_to_cstring2() is better.

> I've felt kind of uneasy about that issue as well.
> However, it would have been difficult to implement
> a deprecated svn_checksum_to_cstring() in terms
> of svn_checksum_to_cstring2().

I don't think Blair was concerned about how the function is implemented,
but about the API promises.  (He can correct me if I'm wrong.)

> For instance: [...]
> {
>    if (checksum == NULL)
>      abort();
> 
>    return svn_checksum_to_cstring2(checksum, pool);
> }
> 
> does not have the exact same behavior as the old
> implementation. [...]

That doesn't matter.  Passing checksum=NULL was not a supported usage in
1.6.x so there is no need to preserve compatibility with that failure
mode.  It would be perfectly acceptable to implement it as:

const char *
svn_checksum_to_cstring(const svn_checksum_t *checksum,
                         apr_pool_t *pool)
{
   return svn_checksum_to_cstring2(checksum, pool);
}

But, as I said in my previous reply in this thread, I think it's fine to
just change the existing API as you have done.

- Julian



Mime
View raw message