On Thu, Mar 11, 2010 at 03:05:23AM -0500, Greg Stein wrote:
> On Wed, Mar 10, 2010 at 05:51, Daniel Näslund <daniel_at_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
Received on 2010-03-11 22:57:35 CET