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 <daniel_at_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 <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 23:45:53 CET