You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/12/02 01:38:17 UTC
svn commit: r1041230 - /subversion/trunk/subversion/include/svn_checksum.h
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
Modified:
subversion/trunk/subversion/include/svn_checksum.h
Modified: subversion/trunk/subversion/include/svn_checksum.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_checksum.h?rev=1041230&r1=1041229&r2=1041230&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_checksum.h (original)
+++ subversion/trunk/subversion/include/svn_checksum.h Thu Dec 2 00:38:17 2010
@@ -121,9 +121,11 @@ svn_checksum_to_cstring_display(const sv
/** Return the hex representation of @a checksum, allocating the
* string in @a pool. If @a checksum->digest is all zeros (that is,
- * 0, not '0') or NULL, then return NULL.
+ * 0, not '0') then return NULL. In 1.7+, @a checksum may be NULL
+ * and NULL will be returned in that case.
*
* @since New in 1.6.
+ * @note Passing NULL for @a checksum in 1.6 will cause a segfault.
*/
const char *
svn_checksum_to_cstring(const svn_checksum_t *checksum,
Re: svn commit: r1041230 -
/subversion/trunk/subversion/include/svn_checksum.h
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-12-06, Blair Zajac wrote:
> On 12/6/10 7:47 AM, Julian Foad wrote:
> > On Sun, 2010-12-05, Stefan Fuhrmann wrote:
> >> 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.)
>
> Nope, that's correct.
>
> > 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.
>
> Well, I don't agree, for problems that it could cause later, but don't feel that
> strongly to argue about it :)
Ok. FWIW, I'd also be very happy with a revved API, I just don't feel
strongly enough to do it myself.
- Julian
Re: svn commit: r1041230 - /subversion/trunk/subversion/include/svn_checksum.h
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/6/10 7:47 AM, Julian Foad wrote:
> On Sun, 2010-12-05, Stefan Fuhrmann wrote:
>> 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.)
Nope, that's correct.
> 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.
Well, I don't agree, for problems that it could cause later, but don't feel that
strongly to argue about it :)
Blair
Re: svn commit: r1041230 -
/subversion/trunk/subversion/include/svn_checksum.h
Posted by Julian Foad <ju...@wandisco.com>.
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
Re: svn commit: r1041230 - /subversion/trunk/subversion/include/svn_checksum.h
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
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(). For instance:
const char *
svn_checksum_to_cstring(const svn_checksum_t *checksum,
apr_pool_t *pool)
{
if (checksum == NULL)
abort();
return svn_checksum_to_cstring2(checksum, pool);
}
does not have the exact same behavior as the old
implementation. Even if we added error output etc.,
that very message may be ignored by / masked in
the calling process. Forcing a segfault via
if (checksum == NULL)
return (const char*)checksum->digest;
might not be liked by some compilers / optimizers
because they know checksum to be NULL in the
return statement.
-- Stefan^2.
Re: svn commit: r1041230 -
/subversion/trunk/subversion/include/svn_checksum.h
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-12-01, 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.
Any back-port effort of this kind needs to take account of all API
changes, not just newly created APIs. If it doesn't, then yes it may
crash. I believe that is an expected result of our compatibility rules,
and not without precedent, although I can't quickly lay my hands on a
good example.
> 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.
We should not port this change to 1.6.x. I don't believe we need to rev
the API for this reason; however, I haven't reviewed the change and so I
don't know if there are other reasons.
- Julian
Re: svn commit: r1041230 - /subversion/trunk/subversion/include/svn_checksum.h
Posted by Blair Zajac <bl...@orcaware.com>.
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.
Regards,
Blair