You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2011/07/13 23:08:26 UTC
Re: svn commit: r1146452 - /subversion/branches/1.7.x/STATUS
gstein@apache.org wrote on Wed, Jul 13, 2011 at 20:51:17 -0000:
> Author: gstein
> Date: Wed Jul 13 20:51:17 2011
> New Revision: 1146452
>
> URL: http://svn.apache.org/viewvc?rev=1146452&view=rev
> Log:
> votes!
>
> Modified:
> subversion/branches/1.7.x/STATUS
>
> Modified: subversion/branches/1.7.x/STATUS
> URL: http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?rev=1146452&r1=1146451&r2=1146452&view=diff
> ==============================================================================
> --- subversion/branches/1.7.x/STATUS (original)
> +++ subversion/branches/1.7.x/STATUS Wed Jul 13 20:51:17 2011
> @@ -15,14 +15,6 @@ Status of 1.7.0:
> Candidate changes:
> ==================
>
> - * r1146013
> - Improve notifications of paths skipped because they were conflicted
> - Justification:
> - Improves general usability of the svn client and the libsvn_client api
> - and would have to wait for 1.8.0 if it doesn't get in 1.7.0.
> - Votes:
> - +1: rhuijben, cmpilato
> -
> * r1146131, r1146134
> Add svn_fs_verify() API.
> Justification:
> @@ -32,6 +24,10 @@ Candidate changes:
> +1: danielsh, rhuijben
> -1: stsp (no-op API change)
> -1: cmpilato (no-op API change -- will reconsider if real utility is added)
> + -1: gstein (callers have no idea if the function will do anything
> + since they don't know if they're working with 1.7.0 or
> + 1.7.10, so why bother to call it at all? cross your
> + fingers and hope?)
svn_fs_version().minor
>
> * r1146214, r1146381
> Handle NULL inputs when stringifying svn_checksum_t.
> @@ -39,7 +35,8 @@ Candidate changes:
> Avoids segfaults.
> Votes:
> +1: danielsh
> - -0: cmpilato, rhuijben (problem is with callers, not implementation)
> + -0: cmpilato, rhuijben, gstein (problem is with callers,
> + not implementation)
>
Just a reminder: this revision IS present on trunk, so someone please
suggest how to change trunk to avoid releasing this triply-(-0)ed change
in 1.8.0.
Re: svn commit: r1146452 - /subversion/branches/1.7.x/STATUS
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Shahaf wrote:
> The backport proposal now includes a fix of the original callsite
> (r1146708).
>
> Thus, 1.7.x the semantics of these API's are unchanged and the backport
> entry for the problematic caller has been patched. For 1.8.x, given
> that I'm juiced out by this thread, I'll go ahead and implement on trunk
> whatever consensus@ decides.
>
> I have observed that all users of _display() (except one in
> adm-crawler.c, where the exception to the rule is questioned by a ###
> comment) use the _display() API either for error messages or for
> stringifying the digest into constructing paths and other objects.
>
> I believe that error messages don't distinguish between a NULL return
> and a "(null)" return, and that path construction, etc, would probably
> work in a bogus manner if NULL is returned (think svn_dirent_join_many())
> and work-and-maybe-fail-later if "(null)" is returned.
Thanks for checking. So the function is being used for two different
purposes, one of which is what its name implies (where accepting null
input could be useful), and the other is not (and logically requires a
non-null input).
> Please let me know how to change trunk.
My advice is to revert it to how it was (input must be non-null).
- Julian
> Julian Foad wrote on Thu, Jul 14, 2011 at 11:45:46 +0100:
> > Daniel Shahaf wrote:
> > > Oh. And I realized that svn_checksum_to_cstring() only started to
> > > accept NULL in 1.7. Which means that, currently, in 1.7.x,
> > >
> > > * svn_checksum_to_cstring(NULL) returns NULL
> > > * svn_checksum_to_cstring_display(NULL) segfaults
> >
> > The essential and original difference between the two functions is that
> > given an all-zero input, the former returns NULL whereas the latter
> > returns a printable representation ("0000000000000000000000...").
> >
> > So it seems to me that if the _display function is going to do anything
> > with a null input, it should return some printable representation such
> > as "(null)", and never return a null pointer.
> >
> > Alternatively I think it's fine to leave it the way it was, requiring a
> > non-null input, even though the other function accepts a null input.
> >
> > - Julian
> >
> >
> > >
> > > Generally I prefer consistency before I prefer that My Way gets
> > > chosen... which means that I would prefer that these two API's behave
> > > the same way in 1.7. (In 1.6 both of them segfault.)
> > >
> > > Daniel
> > > (I don't have /too/ much energy left for this discussion...)
Re: svn commit: r1146452 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
The backport proposal now includes a fix of the original callsite
(r1146708).
Thus, 1.7.x the semantics of these API's are unchanged and the backport
entry for the problematic caller has been patched. For 1.8.x, given
that I'm juiced out by this thread, I'll go ahead and implement on trunk
whatever consensus@ decides.
I have observed that all users of _display() (except one in
adm-crawler.c, where the exception to the rule is questioned by a ###
comment) use the _display() API either for error messages or for
stringifying the digest into constructing paths and other objects.
I believe that error messages don't distinguish between a NULL return
and a "(null)" return, and that path construction, etc, would probably
work in a bogus manner if NULL is returned (think svn_dirent_join_many())
and work-and-maybe-fail-later if "(null)" is returned.
Please let me know how to change trunk.
Thanks,
Julian Foad wrote on Thu, Jul 14, 2011 at 11:45:46 +0100:
> Daniel Shahaf wrote:
> > Oh. And I realized that svn_checksum_to_cstring() only started to
> > accept NULL in 1.7. Which means that, currently, in 1.7.x,
> >
> > * svn_checksum_to_cstring(NULL) returns NULL
> > * svn_checksum_to_cstring_display(NULL) segfaults
>
> The essential and original difference between the two functions is that
> given an all-zero input, the former returns NULL whereas the latter
> returns a printable representation ("0000000000000000000000...").
>
> So it seems to me that if the _display function is going to do anything
> with a null input, it should return some printable representation such
> as "(null)", and never return a null pointer.
>
> Alternatively I think it's fine to leave it the way it was, requiring a
> non-null input, even though the other function accepts a null input.
>
> - Julian
>
>
> >
> > Generally I prefer consistency before I prefer that My Way gets
> > chosen... which means that I would prefer that these two API's behave
> > the same way in 1.7. (In 1.6 both of them segfault.)
> >
> > Daniel
> > (I don't have /too/ much energy left for this discussion...)
>
>
Re: svn commit: r1146452 - /subversion/branches/1.7.x/STATUS
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Shahaf wrote:
> Oh. And I realized that svn_checksum_to_cstring() only started to
> accept NULL in 1.7. Which means that, currently, in 1.7.x,
>
> * svn_checksum_to_cstring(NULL) returns NULL
> * svn_checksum_to_cstring_display(NULL) segfaults
The essential and original difference between the two functions is that
given an all-zero input, the former returns NULL whereas the latter
returns a printable representation ("0000000000000000000000...").
So it seems to me that if the _display function is going to do anything
with a null input, it should return some printable representation such
as "(null)", and never return a null pointer.
Alternatively I think it's fine to leave it the way it was, requiring a
non-null input, even though the other function accepts a null input.
- Julian
>
> Generally I prefer consistency before I prefer that My Way gets
> chosen... which means that I would prefer that these two API's behave
> the same way in 1.7. (In 1.6 both of them segfault.)
>
> Daniel
> (I don't have /too/ much energy left for this discussion...)
Re: svn commit: r1146452 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Oh. And I realized that svn_checksum_to_cstring() only started to
accept NULL in 1.7. Which means that, currently, in 1.7.x,
* svn_checksum_to_cstring(NULL) returns NULL
* svn_checksum_to_cstring_display(NULL) segfaults
Generally I prefer consistency before I prefer that My Way gets
chosen... which means that I would prefer that these two API's behave
the same way in 1.7. (In 1.6 both of them segfault.)
Daniel
(I don't have /too/ much energy left for this discussion...)
Re: svn commit: r1146452 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Just adding two facts to the mix:
The use case I had was representation_string() trying to stringify
md5_checksum, which is NULL for reps retrieved from the cache.
Looking at the other callers, most of them seem to pass the checksum to
'info' or to an editor function documented as ignoring the checksum if
it's NULL.
Re: svn commit: r1146452 - /subversion/branches/1.7.x/STATUS
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jul 13, 2011 at 17:08, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> gstein@apache.org wrote on Wed, Jul 13, 2011 at 20:51:17 -0000:
>> Author: gstein
>> Date: Wed Jul 13 20:51:17 2011
>> New Revision: 1146452
>>
>> URL: http://svn.apache.org/viewvc?rev=1146452&view=rev
>> Log:
>> votes!
>>
>> Modified:
>> subversion/branches/1.7.x/STATUS
>>
>> Modified: subversion/branches/1.7.x/STATUS
>> URL: http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?rev=1146452&r1=1146451&r2=1146452&view=diff
>> ==============================================================================
>> --- subversion/branches/1.7.x/STATUS (original)
>> +++ subversion/branches/1.7.x/STATUS Wed Jul 13 20:51:17 2011
>> @@ -15,14 +15,6 @@ Status of 1.7.0:
>> Candidate changes:
>> ==================
>>
>> - * r1146013
>> - Improve notifications of paths skipped because they were conflicted
>> - Justification:
>> - Improves general usability of the svn client and the libsvn_client api
>> - and would have to wait for 1.8.0 if it doesn't get in 1.7.0.
>> - Votes:
>> - +1: rhuijben, cmpilato
>> -
>> * r1146131, r1146134
>> Add svn_fs_verify() API.
>> Justification:
>> @@ -32,6 +24,10 @@ Candidate changes:
>> +1: danielsh, rhuijben
>> -1: stsp (no-op API change)
>> -1: cmpilato (no-op API change -- will reconsider if real utility is added)
>> + -1: gstein (callers have no idea if the function will do anything
>> + since they don't know if they're working with 1.7.0 or
>> + 1.7.10, so why bother to call it at all? cross your
>> + fingers and hope?)
>
> svn_fs_version().minor
>
>>
>> * r1146214, r1146381
>> Handle NULL inputs when stringifying svn_checksum_t.
>> @@ -39,7 +35,8 @@ Candidate changes:
>> Avoids segfaults.
>> Votes:
>> +1: danielsh
>> - -0: cmpilato, rhuijben (problem is with callers, not implementation)
>> + -0: cmpilato, rhuijben, gstein (problem is with callers,
>> + not implementation)
>>
>
> Just a reminder: this revision IS present on trunk, so someone please
> suggest how to change trunk to avoid releasing this triply-(-0)ed change
> in 1.8.0.
Fix the callers to avoid passing NULL.
Or leave the API change in there. Note that we are voting -0 rather
than applying a veto to your API change [on trunk]. I say that "we
disagree, but not strongly enough to raise a stink". And "we'd prefer
if you find a different solution" rather than "require to find".
Cheers,
-g