[PATCH] add --limit argument to svn log

[Garrett Rooney <ro...@electricjellyfish.net>]

OK, so it took me a long time to get back to this, but I'm working on my 
'svn log --limit' patch again (aka issue 1992).  This lets you run 'svn 
log --limit 5' to limit your log output to the last 5 log entries.

Here's the current iteration of the code.  It adds a svn_repos_get_logs3 
function that adds a limit argument and then bubbles it back up through 
the RA layers into the client.  Both ra_dav and ra_svn implement this in 
such a way that it will 'work' with servers that don't support the 
functionality, although for ra_dav it seems to still download all the 
log entries, even though I tried to get the XML parser to error out.

The ra_dav code is untested, but looks like it should work.  I will of 
course test it before committing.

A log message is prepended to the diff.

Please let me know what you think, it's been a while since I looked at 
this stuff, so it's quite possible that there's some kind of problem 
that I've missed.

-garrett

Re: [PATCH] add --limit argument to svn log

[Garrett Rooney <ro...@electricjellyfish.net>]

Greg Hudson wrote:
> The biggest problem I see:
> 
> -  SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)(?r)(?r)bb)", start, end,
> +  SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)(?r)(?r)bbr)", start, end,
> 
> I know I left a little gap in the ra_svn infrastructure by not allowing
> 'n's in the optional part of a tuple, but you don't get to gloss over
> that by pretending that your limit argument is a revision.  That's a big
> giant -1.
> 
> I suggest introducing SVN_RA_SVN_UNSPECIFIED_NUMBER, #defined to
> ~((unsigned apr_uint64_t) 0), and using that as the distinguished value
> for an unspecified optional number.

That makes sense.  I agree, the current way of doing it (passing a 
revision) is a hack.

> Other issues:
> 
> In libsvn_repos/log.c, I would add a counter to the for loop rather than
> adjusting start or end.  It's more straightforward, and I'm a little
> concerned about the correctness of your code in light of the continue
> clause in the loop body.  Since the loop conditions are already
> complicated, the counter check might be best written as an "if (++count
>>= limit) break;" (or whatever) in the loop body, after the continue
> 
> clause has been given a chance to fire.

I'll take a look.

> I think the limit argument should just be an int, not a "long int".  Our
> code base only very rarely uses the "long" type.  The only real
> difference between "int" and "long" is that long is guaranteed to be 32
> bits, and if ints are 16 bits (you have to go back pretty far to find an
> environment like that), we're already very sunk.

It was a long int so I could use the code for passing around revisions 
in ra_svn ;-)  That can certainly go away once that hack is removed.

> In three places you wrote "compatability", which is a misspelling of
> "compatibility".

Oops.  Good catch.  I'm too used to working with a copy editor who 
catches those sort of things for me ;-)

Thanks for the review!

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] add --limit argument to svn log

[Garrett Rooney <ro...@electricjellyfish.net>]

Garrett Rooney wrote:
> OK, here's an updated patch.  I think it fixes all the problems Greg 
> pointed out.  Still needs to be tested with mod_dav_svn/ra_svn, 
> hopefully I'll get to do that tomorrow.

This was committed in revision 11155, after some minor modifications to 
make the ra_dav portions actually compile and work (hey, I said the 
previous version was untested!).

Please let me know if you notice any problems.

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] add --limit argument to svn log

[Garrett Rooney <ro...@electricjellyfish.net>]

OK, here's an updated patch.  I think it fixes all the problems Greg 
pointed out.  Still needs to be tested with mod_dav_svn/ra_svn, 
hopefully I'll get to do that tomorrow.

Comments welcome, as always.

-garrett

Implement a '--limit' switch for 'svn log', which allows you to limit
the number of log entries returned.

* subversion/include/svn_repos.h
   (svn_repos_get_logs3): new prototype, adds a new 'limit' argument to
    svn_repos_get_logs2.
   (svn_repos_get_logs2): deprecate.

* subversion/libsvn_repos/log.c
   (svn_repos_get_logs3): new, implements limit functionality.
   (svn_repos_get_logs2, svn_repos_get_logs): implement in terms of new
    svn_repos_get_logs3 function.

* subversion/include/svn_client.h
   (svn_client_log2): new prototype, adds a new 'limit' argument to
    svn_client_log.
   (svn_client_log): deprecate.

* subversion/libsvn_client/log.c
   (svn_client_log2): new, like old svn_client_log, but now calls the
    get_log2 method of the RA layer.
   (svn_client_log): reimplement in terms of svn_client_log2.

* subversion/clients/cmdline/cl.h
   (svn_cl__opt_state_t): add limit field.

* subversion/clients/cmdline/log-cmd.c
   (svn_cl__log): call svn_client_log2 instead of svn_client_log.

* subversion/clients/cmdline/main.c
   (svn_cl__options): add limit option.
   (main): handle limit option.

* 
subversion/tests/clients/cmdline/getopt_tests_data/svn_help_log_switch_stdout
   (svn help log): update for new --limit flag.

* subversion/include/svn_ra.h
   (svn_ra_plugin_t::get_log2): new, adds a 'limit' argument to get_log.
   (svn_ra_plugin_t::get_log): deprecate.

* subversion/libsvn_ra_local/ra_plugin.c
   (svn_ra_local__get_log2): new, like svn_ra_local__get_log, but with an
    additional 'limit' argument.
   (svn_ra_local__get_log): reimplement in terms of svn_ra_local_get_log2.
   (ra_local_plugin): add svn_ra_local__get_log2.

* subversion/include/svn_ra_svn.h
   (SVN_RA_SVN_UNSPECIFIED_NUMBER): new constant, indicates an optional
    number that was not found.

* subversion/libsvn_ra_svn/marshal.c
   (vparse_tuple): handle the 'n' case for optional entries in a tuple.

* subversion/libsvn_ra_svn/client.c
   (ra_svn_log2): add limit functionality, with fallback in case the server
    doesn't respect the limit request.
   (ra_svn_log): implement in terms of ra_svn_log2.
   (ra_svn_plugin): add ra_svn_log2.

* subversion/svnserve/serve.c
   (log_cmd): add limit functionality.

* subversion/libsvn_ra_dav/log.c
   (log_baton): add limit and count fields for keeping track of logs we
    receive versus how many we're willing to display.
   (log_end_element): bail out if we go over the limit of entries we're
    going to show.
   (svn_ra_dav__get_log2): new, adds limit to old svn_ra_dav__get_log impl.
   (svn_ra_dav__get_log): reimplement in terms of svn_ra_dav__get_log2.

* subversion/libsvn_ra_dav/ra_dav.h
   (svn_ra_dav__get_log2): prototype.

* subversion/libsvn_ra_dav/session.c
   (dav_plugin): add svn_ra_dav__get_log2.

* subversion/mod_dav_svn/log.c
   (dav_svn__log_report): parse limit out of request and pass it on to
    svn_repos_get_logs2.

Re: [PATCH] add --limit argument to svn log

[Greg Hudson <gh...@MIT.EDU>]

The biggest problem I see:

-  SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)(?r)(?r)bb)", start, end,
+  SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)(?r)(?r)bbr)", start, end,

I know I left a little gap in the ra_svn infrastructure by not allowing
'n's in the optional part of a tuple, but you don't get to gloss over
that by pretending that your limit argument is a revision.  That's a big
giant -1.

I suggest introducing SVN_RA_SVN_UNSPECIFIED_NUMBER, #defined to
~((unsigned apr_uint64_t) 0), and using that as the distinguished value
for an unspecified optional number.

Other issues:

In libsvn_repos/log.c, I would add a counter to the for loop rather than
adjusting start or end.  It's more straightforward, and I'm a little
concerned about the correctness of your code in light of the continue
clause in the loop body.  Since the loop conditions are already
complicated, the counter check might be best written as an "if (++count
>= limit) break;" (or whatever) in the loop body, after the continue
clause has been given a chance to fire.

I think the limit argument should just be an int, not a "long int".  Our
code base only very rarely uses the "long" type.  The only real
difference between "int" and "long" is that long is guaranteed to be 32
bits, and if ints are 16 bits (you have to go back pretty far to find an
environment like that), we're already very sunk.

In three places you wrote "compatability", which is a misspelling of
"compatibility".


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org