Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (athena.apache.org: domain of gstein@gmail.com designates
 74.125.82.47 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <1317995802.6194.16233.camel@edith>
References: <1317995802.6194.16233.camel@edith>
Date: Fri, 7 Oct 2011 16:07:17 -0400
Message-ID: 
 <CABD8fLUdmNAev5_gvaGULhdqN4QA=YJSssGrEmf9D9+d7FWn1Q@mail.gmail.com>
Subject: Re: [RFC] API for reading trees from repo or WC
From: Greg Stein <gstein@gmail.com>
To: Julian Foad <julian.foad@wandisco.com>
Cc: Subversion Development <dev@subversion.apache.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

On Fri, Oct 7, 2011 at 09:56, Julian Foad <julian.foad@wandisco.com> wrote:
>...
> =A0diff_tree_with_delta(tree1, delta)
>
> =A0 =A0Takes a reference to a base tree, and an svn_delta_editor_t type
> delta based on tree1, and reads dirs and files from tree1 as necessary
> to present the delta as a unidiff (or whatever kind of output).

You should be using Ev2 rather than the delta editor.

>...
> /**
> =A0* A readable tree. =A0This object is used to perform read requests to =
a
> =A0* repository tree or a working-copy (base or working) tree or any othe=
r
> =A0* readable tree.
> =A0*
> =A0* @since New in 1.8.
> =A0*/
> typedef struct svn_client_tree_t svn_client_tree_t;
>
> /* */
> typedef svn_io_dirent2_t svn_client_tree_dirent_t;
>
> /* V-table for #svn_client_tree_t.
> =A0*
> =A0* Paths are relpaths, relative to the tree root.
> =A0* Revision numbers and repository ids are #SVN_INVALID_REVNUM and NULL
> =A0* for an unversioned node (including a node that is a local add/copy/m=
ove
> =A0* in a WC working tree).
> =A0*/
> typedef struct svn_client_tree__vtable_t

The vtable should be private/encapsulated. See how svn_editor_t
handles its vtable, along with the various callback-setters. The
code/feature-set is much easier to extend, and the API is much easier
to modify when the vtable is hidden.

> {
> =A0/* Fetch the node kind of the node at @a relpath.
> =A0 * (### and other metadata? revnum? props?)
> =A0 *
> =A0 * Set @a *kind to the node kind.
> =A0 */
> =A0svn_error_t *(*get_kind)(svn_client_tree_t *tree,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 svn_node_kind_t *kind=
,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 const char *relpath,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 apr_pool_t *scratch_p=
ool);

relpath against what? I presume the root of the tree is defined at
construction time. Thus, all API calls are relative to that implied
root.

>
> =A0/* Fetch the contents and properties of the file at @a relpath.
> =A0 *
> =A0 * If @a stream is non-NULL, set @a *stream to a readable stream yield=
ing
> =A0 * the contents of the file at @a relpath. =A0(### ? The stream
> =A0 * handlers for @a stream may not perform any operations on @a tree.)
> =A0 *
> =A0 * If @a props is non-NULL, set @a *props to contain the regular
> =A0 * versioned properties of the file (not 'wcprops', 'entryprops', etc.=
).
> =A0 * The hash maps (const char *) names to (#svn_string_t *) values.
> =A0 */
> =A0svn_error_t *(*get_file)(svn_client_tree_t *tree,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 svn_stream_t **stream=
,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 apr_hash_t **props,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 const char *relpath,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 apr_pool_t *result_po=
ol,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 apr_pool_t *scratch_p=
ool);

I would like to see the notion of a symlink be a first-order item in
this API. The hope is to move away from exposing svn:special and
treating all symlinks as their own type. (and yes, this kinda messes
with your get_kind using svn_node_kind_t)

>
> =A0/* Fetch the entries and properties of the directory at @a relpath.
> =A0 *
> =A0 * If @a dirents is non-NULL, set @a *dirents to contain all the entri=
es
> =A0 * of directory @a relpath. =A0The keys will be (<tt>const char *</tt>=
)
> =A0 * entry names, and the values (#svn_client_tree_dirent_t *) dirents.
> =A0 * Only the @c kind and @c filesize fields are filled in.

Just names are easiest. ie. return an array of child names. If you
start returning structures, then you're going to get into versioning
hell for those structures. They become giant gloms of random data.
(ref: the old svn_wc_entry_t, the various svn_info_t structures, and
the haphazard svn_wc_status_t structures).

> =A0 * ### @c special would be useful too.

Screw special. Use kind properly.

I would suggest svn_kind_t. We can then get rid of svn_wc__db_kind_t,
and various APIs can be versioned from svn_node_kind_t over to the new
svn_kind_t.

> =A0 *
> =A0 * If @a props is non-NULL, set @a *props to contain the regular
> =A0 * versioned properties of the file (not 'wcprops', 'entryprops', etc.=
).
> =A0 * The hash maps (const char *) names to (#svn_string_t *) values.
> =A0 */
> =A0svn_error_t *(*get_dir)(svn_client_tree_t *tree,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0apr_hash_t **dirents,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0apr_hash_t **props,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const char *relpath,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0apr_pool_t *result_poo=
l,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0apr_pool_t *scratch_po=
ol);
>
> =A0/* Push a sub-tree into an editor, as a delta against an empty tree.
> =A0 * This is useful for efficiency when streaming a (sub-)tree from a
> =A0 * remote source. */

I don't see the utility here. Every single node becomes an add_* call
into the Ev2 interface. Not very complicated, so I'm not sure how this
helps (or what the problem it is trying to solve).

>...
> struct svn_client_tree_t
> {
> =A0const svn_client_tree__vtable_t *vtable;
>
> =A0/* Pool used to manage this session. */
> =A0apr_pool_t *pool;
>
> =A0/* Private data for the tree implementation. */
> =A0void *priv;
> };

This should be encapsulated. See svn_editor_t. And that would be
"baton" (we never call it "priv").

>...

Cheers,
-g