kfogel@collab.net wrote:
> rooneg@tigris.org writes:
>
>>Log:
>>Implement a '--limit' switch for 'svn log', which allows you to limit
>>the number of log entries returned.
>
>
> Yay!
>
> A few small comments below. (I should really have reviewed the patch
> earlier, rather than the commit now, sorry for being late.)
Heh. Best way to get reviews is always to commit the changes ;-)
I'll reply to some of your comments now, if I don't say anything about
it just assume I agree and will fix it later tonight sometime.
>>@@ -215,6 +218,14 @@
>> lb->date,
>> lb->msg,
>> lb->subpool);
>>+
>>+ /* Compatability cruft so that we can provide limit functionality
>>+ even if the server doesn't support it. */
>>+ if (lb->limit && (++lb->count == lb->limit))
>>+ {
>>+ lb->err = NULL;
>>+ return SVN_RA_DAV__XML_INVALID;
>>+ }
>>
>> reset_log_item (lb);
>
>
> Exploratory question:
>
> Why are we returning SVN_RA_DAV__XML_INVALID here? I realize it may
> work out to have the desired effect, but to the reader, it looks like
> we're returning an error for a non-error condition. (I'm assuming a
> non-modern server is not considered an error! :-) )
>
> The only purpose of SVN_RA_DAV__XML_INVALID is to tell the XML parser
> to stop processing, so it won't take us to the 'ELEM_log_item' case
> again, right? But, this strategy doesn't save the user much wall
> clock time, because they still end up waiting for all the unuseable
> logs to come down the wire. (Stop me if I'm wrong...)
>
> At first, I thought a cleaner-looking solution might be to simply
> protect the receiver invocation with a limit check, e.g.:
>
> case ELEM_log_item:
> {
> ### if (lb->limit && (++lb->count == lb->limit))
> ### {
> svn_error_t *err = (*(lb->receiver))(lb->receiver_baton,
> lb->changed_paths,
> lb->revision,
> lb->author,
> lb->date,
> lb->msg,
> lb->subpool);
> ### }
>
> (New lines marked with "###" because I'm too lazy to generate a real
> patch.)
>
> But then I realized, the difference between that solution and yours is
> that your solution spares us the effort of parsing XML only to ignore
> the results. In which case, maybe the best thing is to just add a
> comment to your change, pointing out that we're not really generating
> an error, we're just stopping parsing, and say why this is better.
>
> If any of this analysis misses the point, please bop me. I just
> wanted to know if you had considered the other kind of conditional,
> and if so, whether you rejected it for the reasons I'm guessing you
> did.
No, you're pretty much on target. I seem to recall seeing another
example of us returning an error inside the XML parser to bail out early
somewhere in ra_dav, but I can't find it now, so it's quite possible
that I'm on crack. Some of the client side ra_dav code is a bit of a
maze, so perhaps it's just hiding.
Ideally I'd like it if we could actually totally bail out, drop the
socket connection on the floor and get on with our lives (that's what
it's doing in ra_svn, for example), but even with faking an error it
still seems to wait for all the data, which kind of sucks. If someone
knows a better way I'd be all ears.
In any event, if people feel it would be more clear I can move to
something like what you suggest, just putting a check around the call to
the receiver, since bailing out early probably isn't buying us all that
much time in the long run. Either way I'll add some comments to clarify
what's going on.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 28 18:45:27 2004