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 Näslund <da...@longitudo.com> on 2010/03/11 22:55:56 UTC
Re: [PATCH v3] Replace entries in revisision_status.c
On Thu, Mar 11, 2010 at 03:05:23AM -0500, Greg Stein wrote:
> On Wed, Mar 10, 2010 at 05:51, Daniel N�slund <da...@longitudo.com> wrote:
[...]
> > I've removed the comments in the end about sparse_checkout not detecting
> > all cases. The walker reaches all nodes beneath local_abspath and if we
> > check for absent too I can't come up with any more cases where we won't
> > detect sparse_checkouts. Is that assumption correct?
> >
> > I'm catching the _WC_PATH_NOT_FOUND error. Before my changes the code
> > didn't return that error. BUT, is that the _right_ thing to do? Are we
> > counting on the svn_wc_.* functions to always return _WC_PATH_NOT_FOUND
> > error for unversioned resources and such? Since it's a new revision I
> > could just as well return it, right?
>
> As long as you alter the docstring to reflect the (new) error
> condition, then yes: it would be best to propagate that error to the
> caller. It totally sucks where functions silently do nothing when
> asked to operate on a non-existent node (where the caller should have
> known better!).
Done that!
[[[
As part of WC-NG, remove some uses of svn_wc_entry_t.
* subversion/include/svn_wc.h
(svn_wc_revision_status2): Add note about us returning
SVN_ERR_WC_PATH_NOT_FOUND.
* subversion/libsvn_wc/revision_status.c
�(status_baton): Rename this...
�(walk_baton): .. to this.
�(analyze_status): Change this to implement svn_wc__node_func_t and use
� �WC-NG funcs instead of information in svn_wc_entry_t.
�(svn_wc_revision_status2): Use svn_wc__node_walk_children() instead of
� �svn_wc_walk_status() to spare us from the overhead of invoking an
� �editor.
]]]
> My client isn't inlining the patch for a specific review, so I'll
> bullet-list things again:
>
> * sparse_checkout |= TRUE should just be sparse_checkout = TRUE
Fixed.
> * the URL stuff is wonky. you may *never* get repos info during the
> walk. just have the outer function recover the URL via node_get_url()
> or somesuch. it certainly beats testing that thing for every node
> during the walk, and is much more explicit/obvious to the reader. this
> also means you can toss wb->pool
I was planning to do that in an follow-up but ok, fixed!
> * check_wc_root isn't cheap, so taking a page from Bert's book, you
> could do that only when switched is FALSE
Premature optimization is the root of all evil. Done!
> * the logic in the outer function about getting the URL and computing
> the switched flag... you could do that before the walk in order to set
> ->switched early on
Premature opti... Done!
I've tested the patch on the tests I know is using the code, but not a
full make check on this version yet. Waiting 'til I know there will be
no more fixes needed.
Daniel
Re: [PATCH v3] Replace entries in revisision_status.c
Posted by Daniel Näslund <da...@longitudo.com>.
On Thu, Mar 11, 2010 at 05:44:18PM -0500, Greg Stein wrote:
> > [[[
> > As part of WC-NG, remove some uses of svn_wc_entry_t.
> >
> > * subversion/include/svn_wc.h
> > (svn_wc_revision_status2): Add note about us returning
> > SVN_ERR_WC_PATH_NOT_FOUND.
> > * subversion/libsvn_wc/revision_status.c
> > (status_baton): Rename this...
> > (walk_baton): .. to this.
> > (analyze_status): Change this to implement svn_wc__node_func_t and use
> > WC-NG funcs instead of information in svn_wc_entry_t.
> > (svn_wc_revision_status2): Use svn_wc__node_walk_children() instead of
> > svn_wc_walk_status() to spare us from the overhead of invoking an
> > editor.
> > ]]]
> Per IRC, you've got my +1 to commit.
>
> There are some other minor improvements, but let's get this in first.
> I can respond to the commit message.
Committed in r922176.
Re: [PATCH v3] Replace entries in revisision_status.c
Posted by Daniel Näslund <da...@longitudo.com>.
On Thu, Mar 11, 2010 at 05:44:18PM -0500, Greg Stein wrote:
> > [[[
> > As part of WC-NG, remove some uses of svn_wc_entry_t.
> >
> > * subversion/include/svn_wc.h
> > (svn_wc_revision_status2): Add note about us returning
> > SVN_ERR_WC_PATH_NOT_FOUND.
> > * subversion/libsvn_wc/revision_status.c
> > (status_baton): Rename this...
> > (walk_baton): .. to this.
> > (analyze_status): Change this to implement svn_wc__node_func_t and use
> > WC-NG funcs instead of information in svn_wc_entry_t.
> > (svn_wc_revision_status2): Use svn_wc__node_walk_children() instead of
> > svn_wc_walk_status() to spare us from the overhead of invoking an
> > editor.
> > ]]]
> Per IRC, you've got my +1 to commit.
>
> There are some other minor improvements, but let's get this in first.
> I can respond to the commit message.
Committed in r922176.
Re: [PATCH v3] Replace entries in revisision_status.c
Posted by Greg Stein <gs...@gmail.com>.
Per IRC, you've got my +1 to commit.
There are some other minor improvements, but let's get this in first.
I can respond to the commit message.
Cheers,
-g
On Thu, Mar 11, 2010 at 16:55, Daniel Näslund <da...@longitudo.com> wrote:
> On Thu, Mar 11, 2010 at 03:05:23AM -0500, Greg Stein wrote:
>> On Wed, Mar 10, 2010 at 05:51, Daniel Näslund <da...@longitudo.com> wrote:
>
> [...]
>
>> > I've removed the comments in the end about sparse_checkout not detecting
>> > all cases. The walker reaches all nodes beneath local_abspath and if we
>> > check for absent too I can't come up with any more cases where we won't
>> > detect sparse_checkouts. Is that assumption correct?
>> >
>> > I'm catching the _WC_PATH_NOT_FOUND error. Before my changes the code
>> > didn't return that error. BUT, is that the _right_ thing to do? Are we
>> > counting on the svn_wc_.* functions to always return _WC_PATH_NOT_FOUND
>> > error for unversioned resources and such? Since it's a new revision I
>> > could just as well return it, right?
>>
>> As long as you alter the docstring to reflect the (new) error
>> condition, then yes: it would be best to propagate that error to the
>> caller. It totally sucks where functions silently do nothing when
>> asked to operate on a non-existent node (where the caller should have
>> known better!).
>
> Done that!
>
> [[[
> As part of WC-NG, remove some uses of svn_wc_entry_t.
>
> * subversion/include/svn_wc.h
> (svn_wc_revision_status2): Add note about us returning
> SVN_ERR_WC_PATH_NOT_FOUND.
> * subversion/libsvn_wc/revision_status.c
> (status_baton): Rename this...
> (walk_baton): .. to this.
> (analyze_status): Change this to implement svn_wc__node_func_t and use
> WC-NG funcs instead of information in svn_wc_entry_t.
> (svn_wc_revision_status2): Use svn_wc__node_walk_children() instead of
> svn_wc_walk_status() to spare us from the overhead of invoking an
> editor.
> ]]]
>
>> My client isn't inlining the patch for a specific review, so I'll
>> bullet-list things again:
>>
>> * sparse_checkout |= TRUE should just be sparse_checkout = TRUE
>
> Fixed.
>
>> * the URL stuff is wonky. you may *never* get repos info during the
>> walk. just have the outer function recover the URL via node_get_url()
>> or somesuch. it certainly beats testing that thing for every node
>> during the walk, and is much more explicit/obvious to the reader. this
>> also means you can toss wb->pool
>
> I was planning to do that in an follow-up but ok, fixed!
>
>> * check_wc_root isn't cheap, so taking a page from Bert's book, you
>> could do that only when switched is FALSE
>
> Premature optimization is the root of all evil. Done!
>
>> * the logic in the outer function about getting the URL and computing
>> the switched flag... you could do that before the walk in order to set
>> ->switched early on
>
> Premature opti... Done!
>
> I've tested the patch on the tests I know is using the code, but not a
> full make check on this version yet. Waiting 'til I know there will be
> no more fixes needed.
>
> Daniel
>