On 9/17/07, Eric Gillespie <epg@pretzelnet.org> wrote:
> > > Index: subversion/include/svn_client.h
> > > /**
> > > * Similar to svn_client_log4(), but using @c svn_log_message_receiver_t
> > > - * instead of @c svn_log_message_receiver2_t. Also, @a
> > > - * include_merged_revisions and @a omit_log_text are set to @c FALSE
> > > + * instead of @c svn_log_entry_receiver_t. Also, @a
> > > + * include_merged_revisions is set to @c FALSE and @a revprops is
> >
> > Perhaps "contains" instead of "is" at the end here?
>
> To me, "is" means that's all there is, but "contains" may mean it
> contains other things as well.
Sure.
> > > Index: subversion/libsvn_client/log.c
> > > +pre_15_receiver(void *baton, svn_log_entry_t *log_entry, apr_pool_t *pool)
>
> > It might be better to make the fallback implementation use
> > svn_ra_rev_proplist for any request which requires non-standard
> > revprops instead of potentially making multiple svn_ra_rev_prop
> > requests. The tradeoff is minimizing the number of round-trip
> > requests vs. minimizing the amount of unrequested data sent; I'm not
> > sure what the best alternative in practice would be.
>
> I wrote it to minimize the amount of data rather than the number
> of requests, since property values may be arbitrarily large. It
> may be that most revprop values in the wild are small, but I was
> afraid of the diabolical behavior in the other case. I'm willing
> to go either way.
Yeah, I don't have a good idea as to which is going to be better in
practice.
> > > + else if (state == REVPROP_VALUE &&
> > > + strcmp(name.name, "revprop-value") == 0)
> > > + {
> >
> > Possibly sanity-check here that info->revprop_name is non-NULL?
>
> Hm? No version of the server sends revprop-name elements but not
> revprop-value elements.
Well, sure, but just because no version of the server sends this data
doesn't mean we should blindly assume that whatever we get over the
network is valid... (Unless this is typical for the DAV clients.)
> > (Also, I noticed that in the output of "svn log --xml" revprop names
> > are attributes on an element, whereas here they are elements
> > alternating with the values; any reason not to make them attributes in
> > the DAV request too?)
>
> In both cases, I copied and pasted from nearby code. I see now
> that copyfrom info is attributes, so I can change that if you
> like. I am no DAV expert and have no opinion. I suppose there's
> an argument to be made for not transmitting the same word twice
> and going with an attribute, but then we should have used a
> better format than XML in the first place...
I am no DAV expert either. Using attributes does seem like a more
XML-y way of doing a key-value mapping like this (and would alleviate
my previous comment), but it doesn't really matter.
> > > + else if (strcmp(child->name, "revprop") == 0)
> > > + {
> > > + /* XXX should we raise an error if all-revprops and revprop
> > > + elements are given? */
> >
> > Sure!
>
> Which error should I raise? SVN_ERR_RA_DAV_MALFORMED_DATA?
I guess.
> > Index: subversion/tests/cmdline/log_tests.py
>
> > > + def ignore(self, *args, **kwargs):
> > > + del self.cdata[:]
> >
> > Maybe it's more efficient or something, but "self.cdata = []" is more
> > clear to me.
>
> That's the idiomatic way to clear a list. self.cdata = [] is
> entirely different:
>
> some_obj.some_list = [3]
> foo = some_obj.some_list
> ...
> some_obj.clear_list()
> # foo is [3] in your case, should be []
>
> Granted, that's not going to happen in this case, but they remain
> different operations, and list-clearing is what I'm after here.
Ah, sure. Here it's the same result (modulo GC), but it's a common
enough idiom, I guess.
> > I would refactor this test into three tests (with a common setup
> > function): one which should work against any server, one for 1.5
> > servers, and one for older server, and then use conditional Skips to
> > only run two of them.
>
> I pointed only you at the diff rather than sending it to the list
> because it wasn't ready for publication yet ;->. You've caught
> me in the middle of reworking the test. We no longer have
> separate pre- and post- 1.5 behavior, thanks to you suggestion of
> requesting the revprops, so no need for separate tests.
Heh, sorry for airing your dirty laundry :)
> > Perhaps check here to disallow use of both --with-all-revprops and
> > --with-revprop simultaneously?
>
> I prefer commands that allow later options to override earlier
> ones, so if I append --with-all-revprops to a command that
> already has some --with-revprop options, it just works. I'm not
> sure off-hand if we have any kind of precedence for that in svn.
Hmm, sure, but is that what the code will do? Looks like
--with-all-revprops always wins, not the latest one. Which I guess
makes sense.
> > Here (or actually, better in parse_revprop), use
> > svn_prop_name_is_valid to make sure the property name is legal.
>
> Ah, I didn't know about that one. Its documentation confirms
> what I thought about property names, so we need take no special
> care in the DAV code. It looks like only propedit-cmd.c uses
> this right now; I'll add it to parse_revprop and some other
> commands in a separate change, before this one.
Yup, it's a recent API (and a very nice one).
--dave
--
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 18 19:37:21 2007