On Wed, Mar 10, 2010 at 05:51, Daniel Näslund <daniel_at_longitudo.com> wrote:
>...
>> > * status_deleted nodes have no revision info; you may simply want to
>> > test for SVN_INVALID_REVNUM, since read_info() will return that when
>> > it is "not interesting". IOW, it does the proper work for you
>
> I'm checking that revision is not SVN_INVALID_REVNUM. As I've understood
> it, that happens when we have something in WORKING, it has been deleted
> or added and in that case there's no revision info for us to use. Right?
Anything in WORKING means there is no "current revision". You have new
nodes that the repository knows nothing about until they are
committed. They may be added/deleted/copied/moved.
>> > * do you need wc_root? how about just using
>> > svn_wc__internal_path_switched() ?
>
> I first tried to use the child_disjoint.*() func of libsvn_wc/lock.c but
> it felt strange to use that code when we already have
> svn_wc__check_wc_root() that checks if a node is switched. I can
> separate the switch part into a separate func in a follow up and
> replace the call to svn_wc__check_wc_root() with that one then. But I'm
> so terribly slow at getting my patches finished that I thought it was
> best to use _check_wc_root() as is for now.
Fair enough.
>> * You could optimize the checking for modifications to stop checking when
>> you find the first change. (You are not reporting specific changes)
>
> Done that!
>
> 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!).
>
> [[[
> As part of WC-NG, remove some uses of svn_wc_entry_t.
>
> * 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
* in the NOT_FOUND logic, no need for SVN_ERR. you *know* there is an
error. do: return svn_error_return(err);
* 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
* s/beeing/being/
* in the patch, the braces on the if (err) block look wrong. there
"should" be a close-brace after the SVN_ERR. (which we've also said
needs to change)
* check_wc_root isn't cheap, so taking a page from Bert's book, you
could do that only when switched is FALSE
* 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
* oh... the whole read_kind and error logic can be simplified. pass
allow_missing=TRUE to read_kind and check for svn_node_unknown. but...
per above, if you want to return NOT_FOUND, then just allow
walk_status to fail
Okee doke. I don't see anything else. :-P
I really like this work. The old function was a disaster area.
Cheers,
-g
Received on 2010-03-11 09:05:54 CET