On Tue, May 3, 2011 at 12:33 PM, Julian Foad <julian.foad_at_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:hyrum_at_hyrumwright.org]
>> > > Sent: dinsdag 3 mei 2011 17:49
>> > > To: Julian Foad
>> > > Cc: Philip Martin; dev_at_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.foad_at_wandisco.com>
>> > > wrote:
>> > > > On Tue, 2011-05-03 at 14:36 +0100, Philip Martin wrote:
>> > > >> Julian Foad <julian.foad_at_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.
-Hyrum
Received on 2011-05-03 19:46:30 CEST