On Tue, May 3, 2011 at 12:46 PM, Hyrum K Wright <hy...@hyrumwright.org> wrote: > On Tue, May 3, 2011 at 12:33 PM, Julian Foad <julian.f...@wandisco.com> wrote: >> On Tue, 2011-05-03 at 17:10 +0100, Julian Foad wrote: >>> On Tue, 2011-05-03 at 18:02 +0200, Bert Huijben wrote: >>> > >>> > > -----Original Message----- >>> > > From: Hyrum K Wright [mailto:hy...@hyrumwright.org] >>> > > Sent: dinsdag 3 mei 2011 17:49 >>> > > To: Julian Foad >>> > > Cc: Philip Martin; dev@subversion.apache.org >>> > > Subject: Re: svn info --recursive isn't reporting tree-conflict-only >>> > > nodes >>> > > >>> > > On Tue, May 3, 2011 at 10:19 AM, Julian Foad <julian.f...@wandisco.com> >>> > > wrote: >>> > > > On Tue, 2011-05-03 at 14:36 +0100, Philip Martin wrote: >>> > > >> Julian Foad <julian.f...@wandisco.com> writes: >>> > > >> >>> > > >> > I found a bug in "svn info -R": it doesn't report a node that has a >>> > tree >>> > > >> > conflict but otherwise is nonexistent, except if this node is the >>> > root >>> > > >> > node of the requested target path. >>> > > >> >>> > > >> I think that's an actual-only node. See >>> > > >> >>> > > >> http://subversion.tigris.org/issues/show_bug.cgi?id=3779 >>> > > >> >>> > > >> which says that we need to ensure that commands behave sensibly on >>> > > >> actual-only nodes. >>> > > > >>> > > > Yes. >>> > > > >>> > > > The 'info' code uses svn_wc__internal_walk_children(), and makes a >>> > > > special case of checking for a tree conflict on the target node if >>> > > > that >>> > > > walk function returns a NOT FOUND error. But 'info' doesn't check for >>> > > > tree conflicts on other unvisited leaf nodes that may exist as >>> > > > actual-only nodes. >>> > > > >>> > > > One way or another, 'info' is going to have to walk a tree of nodes >>> > > > that >>> > > > includes actual-only nodes. It could either do this itself or we >>> > > > could >>> > > > have a walker function that does that. It's logically re-usable >>> > > > functionality (even if 'info' is currently the only user) so we should >>> > > > have some kind of walker function that does that. So I'll look at >>> > > > adding this functionality into svn_wc__internal_walk_children(). >>> > > >>> > > That's my "gut feeling" as well. ACTUAL-only children are still >>> > > children (in some sense), and should be included in the set of paths >>> > > returned by a walk of the children. >>> > >>> > In some ways it does, but then every callback has to start with a check >>> > 'is >>> > this an actual only child?' >>> > >>> > And most likely we forget that check in a dozen places, because it is so >>> > unlikely to find these actual only children in test scenarios. >>> > >>> > So the result will probably be that Subversion fails for end users... >>> > because almost none of our functions accept these actual only children. (I >>> > think only read_info with a conflict argument and the tree conflict >>> > functions do) >>> >>> I'm thinking of making the walker do this ONLY when given an additional >>> flag: "include actual-only nodes". Does that change your perspective? >> >> A deeper problem here is lack of unambiguous concepts in our API >> descriptions. When we say an API is fetching "children" or "nodes" or >> "conflict victims", exactly which combinations of metadata states >> constitute "a child" or "a node"? We don't clearly define it. Does "a >> child" always refer to a path where the visible node kind is "dir" or >> "file", or can it be a path that is scheduled for deletion? Does "a >> conflict victim" include an "actual-only" path? >> >> We seem to have some functions that are meant to be general purpose but >> then we keep finding that we need to write special-purpose functions. I >> feel like we keep switching to and fro. >> >> Starting with just a little clarification right here, let's see what >> svn_wc__internal_walk_children() is currently used for: >> >> svn_wc__get_info() >> svn_wc_prop_set4() >> svn_wc_set_changelist2() >> svn_wc_get_changelists() >> >> What set of "nodes" do these need to visit? >> >> Info - we want to report on every path that is known in the WC >> database, including paths where there is no dir/file/symlink but there >> is just a tree conflict. >> >> Props - only makes sense on paths at which a node exists (i.e. >> dir/file/symlink) in the working view (i.e. topmost layer). (Subversion >> used to allow viewing and modifying the "working" props of a >> schedule-delete node in some cases, but that was a bug.) >> >> Changelists - makes sense on paths at which a node exists in the >> working view (additional restriction: not a dir, so only file or >> symlink), and also on paths at which a node deletion is scheduled. >> >> So what to do? Make one "node walker" for each case, or add parameters >> to it to control what set of paths is walked? Actually, yes, both of >> those are appropriate, since the concept of "node walker" is to visit a >> certain set of nodes for a generic purpose. >> >> So I will suggest that we define a walker that walks "every path that is >> known to the WC database" for use by "svn info". >> >> And I will suggest that we clarify the definition of the walker used by >> "propset" and by "changelist", and split it into two (or parameterize >> it) if that helps. >> >> Just thinking. > > More thinking: We already have at least a couple other walkers: a > mergeinfo walking and a status walker (and an AT-AT walker for all I > know). Some of these could potentially disappear and move to > recursive queries in the DB, but I'd hope that we can reuse as much > code as posible, rather than having multiple independent > implementations. > > However, since all this stuff is safely tucked into libsvn_wc, we can > make changes relatively easily without too many compat concerns.
Also, as long as we're looking at walkers, I'll add another one I just discovered: the conflict resolver walker in libsvn_wc/conflicts.c. -Hyrum
