Hi Greg. Thanks for the feedback. r1180843 addresses most of this.
On Fri, 2011-10-07, Greg Stein wrote:
> On Fri, Oct 7, 2011 at 09:56, Julian Foad <julian.foad_at_wandisco.com> wrote:
> >...
> > diff_tree_with_delta(tree1, delta)
> >
> > Takes 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.
Will do.
> > typedef struct svn_client_tree__vtable_t
>
> The vtable should be private/encapsulated.
Done; danielsh mentioned this.
> > /* Fetch the node kind of the node at @a relpath.
> > * (### and other metadata? revnum? props?)
> > *
> > * Set @a *kind to the node kind.
> > */
> > svn_error_t *(*get_kind)(svn_client_tree_t *tree,
> > svn_node_kind_t *kind,
> > const char *relpath,
> > apr_pool_t *scratch_pool);
>
> 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.
Yes; this is stated in the overall doc string. May need to clarify or
cross-ref the docs.
> > /* Fetch the contents and properties of the file at @a relpath.
> > *
> > * If @a stream is non-NULL, set @a *stream to a readable stream yielding
> > * the contents of the file at @a relpath. (### ? The stream
> > * handlers for @a stream may not perform any operations on @a tree.)
> > *
> > * If @a props is non-NULL, set @a *props to contain the regular
> > * versioned properties of the file (not 'wcprops', 'entryprops', etc.).
> > * The hash maps (const char *) names to (#svn_string_t *) values.
> > */
> > svn_error_t *(*get_file)(svn_client_tree_t *tree,
> > svn_stream_t **stream,
> > apr_hash_t **props,
> > const char *relpath,
> > apr_pool_t *result_pool,
> > apr_pool_t *scratch_pool);
>
> I would like to see the notion of a symlink be a first-order item in
> this API.
[...]
> 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.
Good; done. In r1180839 I declared & defined svn_kind_t in svn_types.h,
and revved a couple of functions (svn_io_check_path, for starters).
> > /* Fetch the entries and properties of the directory at @a relpath.
> > *
> > * If @a dirents is non-NULL, set @a *dirents to contain all the entries
> > * of directory @a relpath. The keys will be (<tt>const char *</tt>)
> > * entry names, and the values (#svn_client_tree_dirent_t *) dirents.
> > * Only the @c kind and @c filesize fields are filled in.
>
> Just names are easiest.
Right, will do.
I was thinking, don't want another round trip to fetch the kind, then a
third round trip to fetch the node's contents. However, that was the
wrong way to think about it; instead, the implementation should feel
free to fetch and cache info in whatever way is efficient for it, so for
example an implementation over an RA session might fetch the node-kinds
at the same time as the entry names (using svn_ra_get_dir), and then the
get_kind() calls can look at that cached info and return fast without
another round trip.
The same argument explains why I initially added the below
'push-to-delta-editor' call ...
> > /* Push a sub-tree into an editor, as a delta against an empty tree.
> > * This is useful for efficiency when streaming a (sub-)tree from a
> > * 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).
Well, I was thinking how to stream a large chunk of a tree without
network round trips. Initially I thought we needed some kind of "send
me a whole subtree" call. But the better answer is: this interface
doesn't need to know how we do that efficiently, it's just an interface
for reading the data once we've received it. Make another interface for
setting up the request for a whole subtree, and use this 'tree' API for
examining the response.
> >...
> > struct svn_client_tree_t
> > {
> > const svn_client_tree__vtable_t *vtable;
> >
> > /* Pool used to manage this session. */
> > apr_pool_t *pool;
> >
> > /* Private data for the tree implementation. */
> > void *priv;
> > };
>
> [...] that would be "baton" (we never call it "priv").
Wrong; I copied this straight out of
/* The RA session object. */
struct svn_ra_session_t {
const svn_ra__vtable_t *vtable;
/* Pool used to manage this session. */
apr_pool_t *pool;
/* Private data for the RA implementation. */
void *priv;
};
:-)
But I'm not personally attached to this name; we can change them.
- Julian
Received on 2011-10-10 11:40:35 CEST