RE: svn info --recursive isn't reporting tree-conflict-only nodes

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.

- Julian
Received on 2011-05-03 19:34:21 CEST