Re: svn commit: r1082658 - in /subversion/trunk/subversion: include/private/ libsvn_client/ libsvn_fs_base/ libsvn_fs_base/util/ libsvn_subr/ libsvn_wc/ tests/cmdline/

[Greg Stein <gs...@gmail.com>]

On Mar 17, 2011 7:40 PM, <pb...@apache.org> wrote:
>
> Author: pburba
> Date: Thu Mar 17 19:40:07 2011
> New Revision: 1082658
>
> URL: http://svn.apache.org/viewvc?rev=1082658&view=rev
> Log:
> Leverage the recent improvements to svn proplist -R[1] so that svn propget
-R
> can do away with svn_wc__node_walk_children().
>
> [1] See r1066541 and r1071283.
>
> * subversion/include/private/svn_skel.h
> * subversion/libsvn_subr/skel.c
>  (svn_skel__parse_proplist): Optionally parse only a particular property.

I think this is a truly awful API. You have two entirely different
semantics, keyed by a parameter, rather than two functions with appropriate
param lists. And the "parse one" can just return a string_t rather than a
hash-of-one-element.

Internally, the parse code might look like this, shared between the two, but
exposing it thus way is really ugly.

Could you please revert this change, and redo it with the two-function
approach? (or, of course, explain why that doesn't make sense)

Cheers,
-g

Re: svn commit: r1082658 - in /subversion/trunk/subversion: include/private/ libsvn_client/ libsvn_fs_base/ libsvn_fs_base/util/ libsvn_subr/ libsvn_wc/ tests/cmdline/

[Paul Burba <pt...@gmail.com>]

On Thu, Mar 17, 2011 at 3:52 PM, Greg Stein <gs...@gmail.com> wrote:
> On Mar 17, 2011 7:40 PM, <pb...@apache.org> wrote:
>>
>> Author: pburba
>> Date: Thu Mar 17 19:40:07 2011
>> New Revision: 1082658
>>
>> URL: http://svn.apache.org/viewvc?rev=1082658&view=rev
>> Log:
>> Leverage the recent improvements to svn proplist -R[1] so that svn propget
> -R
>> can do away with svn_wc__node_walk_children().
>>
>> [1] See r1066541 and r1071283.
>>
>> * subversion/include/private/svn_skel.h
>> * subversion/libsvn_subr/skel.c
>>  (svn_skel__parse_proplist): Optionally parse only a particular property.
>
> I think this is a truly awful API. You have two entirely different
> semantics, keyed by a parameter, rather than two functions with appropriate
> param lists. And the "parse one" can just return a string_t rather than a
> hash-of-one-element.
>
> Internally, the parse code might look like this, shared between the two, but
> exposing it thus way is really ugly.
>
> Could you please revert this change, and redo it with the two-function
> approach? (or, of course, explain why that doesn't make sense)

Hi Greg,

Point taken, I'll redo it with two functions.

Paul

> Cheers,
> -g
>