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 <dan...@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 <dan...@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 >
