[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH v3] Replace entries in revisision_status.c

From: Daniel Näslund <daniel_at_longitudo.com>
Date: Thu, 11 Mar 2010 22:55:56 +0100

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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.