On Tue, May 3, 2011 at 12:46 PM, Hyrum K Wright <hyrum_at_hyrumwright.org> wrote:
> 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.
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
Received on 2011-05-03 20:01:30 CEST