On 25 Oct 2005 15:46:19 -0500, kfogel@collab.net <kfogel@collab.net> wrote:
> > * subversion/libsvn_ra_dav/fetch.c
> > (svn_ra_dav__get_dir2): new version of get_dir, takes a dirent fields
> > arg, dynamically figure out which props we need to ask for to fill in
> > the dirent fields that were asked for. fall back to asking for all
> > of them in the has_props case, since we currently have no other way
> > to get that data. only fill in the parts of the dirent we were asked
> > to get. create and destroy a new subpool, which is used to allocate
> > the temporary neon properties array.
>
> Wow, this part reads like the function's doc string. Sure you want
> all that in the log message? :-)
Yeah, I'll see if some of that can be moved into someplace more useful...
> > + * @a dirent_fields controls which fields in the @c svn_dirent_t's are
> > + * filled in. To have them totally filled in use @c SVN_DIRENT_ALL,
> > + * otherwise simply binary or together the combination of @c SVN_DIRENT_
> > + * fields you care about.
> > + *
> > + * @since New in 1.4.
> > + */
>
> If you say "binary OR" it looks less like a typo. Actually, "bitwise OR"
> might be even better.
I agree, bitwise OR would be better.
> > +svn_ra_local__get_dir2 (svn_ra_session_t *session,
> > + const char *path,
> > + svn_revnum_t revision,
> > + apr_uint32_t dirent_fields,
> > + apr_hash_t **dirents,
> > + svn_revnum_t *fetched_rev,
> > + apr_hash_t **props,
> > + apr_pool_t *pool)
> > {
> > svn_fs_root_t *root;
> > svn_revnum_t youngest_rev;
>
> A "###" comment here indicating that dirent_fields is currently
> ignored might be a good idea.
Good call.
> I just realized that, formally, we don't need to use the deprecation
> conventions for all these double-underscore functions. But since
> they're mapping directly to public APIs, I guess following the same
> convention internally is the clearest thing to do, so, yay.
Actually, Peter Lundblad pointed out that a bunch of the get_dir2
stuff can go away, since these days we can rev the internal API
without a problem. My next rev of the patch avoids introducing a
get_dir2 method, and thus a bunch of that goes away entirely.
> > + SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-dir", "c(?r)bbn", path,
> > + rev, (props != NULL), (dirents != NULL),
> > + (apr_uint32_t) dirent_fields));
> > SVN_ERR(handle_auth_request(sess_baton, pool));
> > SVN_ERR(svn_ra_svn_read_cmd_response(conn, pool, "rll", &rev, &proplist,
> > &dirlist));
>
> You've extended the protocol to transmit dirent_fields as a number ("n").
>
> This is fine as long as both sides use apr_uint32_t, I guess. But the
> doc string for svn_ra_svn_write_cmd() says "n" means apr_uint64_t...
> Since dirent_fields is apr_uint32_t everywhere already, did you just
> mistype the cast?
That's a mistype of the cast. It should be apr_uint64_t.
> But stepping back for a moment: might it not be better just to make
> all 'dirent_fields' parameters apr_uint64_t, since that doubles the
> amount of room for future expansion? As a minor side benefit, that
> would allow us to remove the cast here entirely.
I don't feel strongly one way or the other on that. It seems unlikely
that svn_dirent_t will ever grow that large, but I suppose stranger
things have happened.
It's also been suggested that instead of sending the parameter over
the wire as a bitfield it might make sense to send a list of words,
since ra_svn doesn't currently send bitfields over the wire anywhere.
I'm not sure how I feel about that.
> > + assert (num_props == 0);
> > + }
>
> The assertion is a nice touch :-).
Had to make sure the code worked ;-)
> > + else
> > + {
> > + /* get all props, since we need them all to do has_props */
> > + which_props = NULL;
> > + }
> > +
>
> Why the subpool? There's no loop here, does it really save us anything?
> True, we allocate an array whose length is proportional to num_props,
> but num_props has a low maximum -- it's not like its magnitude is
> controlled by the client or anything.
In retrospect there's no real reason for it. I'll drop it in the next
version of the patch.
> Odd, why not just one conditional with an "||" ? <shrug> The shape
> of the code wasn't introduced by you, of course.
Yeah, I suppose that could be rewritten more cleanly.
> > + apr_uint64_t dirent_fields = SVN_DIRENT_ALL;
> >
> > - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb", &path, &rev,
> > - &want_props, &want_contents));
> > + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c(?r)bb?n", &path, &rev,
> > + &want_props, &want_contents, &dirent_fields));
>
> Good, here dirent_fields is an apr_uint64_t, which I guess it needs to
> be no matter how the parameter is casted back on the client side.
>
> Nice patch!
Thanks! There's still a problem with the kind field not getting
passed correctly down in ra_dav that I need to track down, but other
than that it seems to be working pretty well...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Oct 26 00:27:37 2005