Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri-test.c

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri-test.c

Daniel Shahaf-2
[hidden email] wrote on Mon, 17 Dec 2018 11:26 +0000:
> * subversion/include/svn_dirent_uri.h
>   (svn_relpath__internal_style): Change prototype so that the function can
>    return an error instead of aborting if anything goes wrong.

Shall we move the declaration to subversion/include/private/ while we're
at it?  That will ensure that API consumers that called this function
--- yes, they shouldn't have, but they may have anyway --- don't
accidentally call the re-signatured function (after upgrading libsvn.so
without recompiling) and get hard-to-trace garbage.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri-test.c

Branko Čibej
On 17.12.2018 13:11, Daniel Shahaf wrote:
> [hidden email] wrote on Mon, 17 Dec 2018 11:26 +0000:
>> * subversion/include/svn_dirent_uri.h
>>   (svn_relpath__internal_style): Change prototype so that the function can
>>    return an error instead of aborting if anything goes wrong.
> Shall we move the declaration to subversion/include/private/ while we're
> at it?  That will ensure that API consumers that called this function
> --- yes, they shouldn't have, but they may have anyway --- don't
> accidentally call the re-signatured function (after upgrading libsvn.so
> without recompiling) and get hard-to-trace garbage.


I've been meaning to raise the same question. The only non-library user
is svndumpfilter, but we "cheat" in the command-line client, too.

If no-one objects, I'll move this declaration to somewhere in
subversion/include/private. I'm not sure where though, there's no really
appropriate private header there (svn_subr_private.h and
svn_string_private.h are the obvious candidates).

-- Brane

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri-test.c

Daniel Shahaf-2
Branko Čibej wrote on Mon, 17 Dec 2018 13:19 +0100:

> On 17.12.2018 13:11, Daniel Shahaf wrote:
> > [hidden email] wrote on Mon, 17 Dec 2018 11:26 +0000:
> >> * subversion/include/svn_dirent_uri.h
> >>   (svn_relpath__internal_style): Change prototype so that the function can
> >>    return an error instead of aborting if anything goes wrong.
> > Shall we move the declaration to subversion/include/private/ while we're
> > at it?  That will ensure that API consumers that called this function
> > --- yes, they shouldn't have, but they may have anyway --- don't
> > accidentally call the re-signatured function (after upgrading libsvn.so
> > without recompiling) and get hard-to-trace garbage.
>
>
> I've been meaning to raise the same question. The only non-library user
> is svndumpfilter, but we "cheat" in the command-line client, too.
>
> If no-one objects, I'll move this declaration to somewhere in
> subversion/include/private. I'm not sure where though, there's no really
> appropriate private header there (svn_subr_private.h and
> svn_string_private.h are the obvious candidates).

I'd say the obvious candidate is a new svn_dirent_uri_private.h.

But I thinko'd earlier.  Moving the declaration won't affect the ABI
compatibility issue; for that we'd have to rename the function as well.

I think it's plausible that a third party library user may be calling this
function since it doesn't specifically have a doxygen note warning that
it's private --- notwithstanding it being named with double underscores.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri-test.c

Branko Čibej
On 17.12.2018 13:28, Daniel Shahaf wrote:

> Branko Čibej wrote on Mon, 17 Dec 2018 13:19 +0100:
>> On 17.12.2018 13:11, Daniel Shahaf wrote:
>>> [hidden email] wrote on Mon, 17 Dec 2018 11:26 +0000:
>>>> * subversion/include/svn_dirent_uri.h
>>>>   (svn_relpath__internal_style): Change prototype so that the function can
>>>>    return an error instead of aborting if anything goes wrong.
>>> Shall we move the declaration to subversion/include/private/ while we're
>>> at it?  That will ensure that API consumers that called this function
>>> --- yes, they shouldn't have, but they may have anyway --- don't
>>> accidentally call the re-signatured function (after upgrading libsvn.so
>>> without recompiling) and get hard-to-trace garbage.
>>
>> I've been meaning to raise the same question. The only non-library user
>> is svndumpfilter, but we "cheat" in the command-line client, too.
>>
>> If no-one objects, I'll move this declaration to somewhere in
>> subversion/include/private. I'm not sure where though, there's no really
>> appropriate private header there (svn_subr_private.h and
>> svn_string_private.h are the obvious candidates).
> I'd say the obvious candidate is a new svn_dirent_uri_private.h.
>
> But I thinko'd earlier.  Moving the declaration won't affect the ABI
> compatibility issue; for that we'd have to rename the function as well.
>
> I think it's plausible that a third party library user may be calling this
> function since it doesn't specifically have a doxygen note warning that
> it's private --- notwithstanding it being named with double underscores.

True ...  I'll rename the function at the same time.

-- Brane
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1849080 - in /subversion/trunk/subversion: bindings/javahl/native/Path.cpp include/svn_dirent_uri.h libsvn_subr/dirent_uri.c svndumpfilter/svndumpfilter.c tests/libsvn_subr/dirent_uri-test.c

Julian Foad-5
Branko Čibej wrote:
> >>>>   (svn_relpath__internal_style): Change prototype so that the function can
> >>>>    return an error instead of aborting if anything goes wrong.
> [...]  I'll rename the function at the same time.

See IRC: I suggest a name like "svn_relpath__from_rel_dirent", and its use in JavaHL appears to be erroneous or at the very least error-prone.

--
- Julian