Excellent patch! A few comments below:
Nuutti Kotivuori <naked@iki.fi> writes:
> 2002-05-01 Nuutti Kotivuori <naked@iki.fi>
>
> * main.c: Add --xml option to svn_cl__options.
> (svn_cl__cmd_table): Add svn_cl__xml_opt to log.
> (main): Add handling for svn_cl__xml_opt.
Typically we say "(svn_cl__options): Add --xml option.".
This is only a minor nit, however.
> * cl.h: Add svn_cl__xml_opt to enum svn_cl__longopt_t.
> (svn_cl__opt_state_t): Add xml option.
Same minor nit.
> Index: ./subversion/clients/cmdline/cl.h
> ===================================================================
> --- ./subversion/clients/cmdline/cl.h
> +++ ./subversion/clients/cmdline/cl.h Wed May 1 17:48:22 2002
> @@ -52,6 +52,7 @@
> svn_cl__auth_username_opt,
> svn_cl__auth_password_opt,
> svn_cl__targets_opt,
> + svn_cl__xml_opt,
> } svn_cl__longopt_t;
Greg Stein pointed out the possibility of a long option "--format"
$ svn --format xml log
$ svn --format=xml log
and the "--xml" could/would be a synonym for it. There might be other
formats in the future. But then we thought, the point of xml is so
that svn doesn't need to know those other formats... After all, both
the default (human-readable) log format and the xml format are machine
parseable. If someone wants something else, they can just convert one
of those, especially given how cheap and ubiquitous xml parsers are
these days.
So, --xml it is. :-)
> +/* This implements `svn_log_message_receiver_t' aswell,
> + but outputs XML instead. */
> +static svn_error_t *
> +log_message_receiver_xml (void *baton,
> + apr_hash_t *changed_paths,
> + svn_revnum_t rev,
> + const char *author,
> + const char *date,
> + const char *msg)
> +{
> + struct log_message_receiver_baton *lb = baton;
> + /* New pool for every received message. */
> + apr_pool_t *subpool = svn_pool_create(lb->pool);
> + /* Collate whole log message into sb before printing. */
> + svn_stringbuf_t *sb = svn_stringbuf_create("", subpool);
> + char *revstr;
This is not a problem with your patch, but rather with the
`svn_log_message_receiver_t' prototype. It was written before we hit
on the right way of using apr pools. It should (like other, more
modern functions) take a pool argument. Its *caller* would choose
whether or not to keep passing the same pool, perhaps using
apr_pool_clear(), because the caller knows whether or not
log_message_receiver is being called in a loop.
So, mental note: before or after this patch is applied, we should
change that interface, and then adjust this code to not create its own
subpool anymore.
> + /* <files> */
> + svn_xml_make_open_tag (&sb, subpool, svn_xml_normal, "files",
> + NULL);
These might not just be files, could be dirs (property changes).
Suggest "paths" instead of "files".
> + /* </files> */
> + svn_xml_make_close_tag (&sb, subpool, "files");
And "paths" here too, of course.
> + if(!opt_state->xml)
> + {
> + SVN_ERR (svn_client_log (auth_baton,
> + targets,
> + &(opt_state->start_revision),
> + &(opt_state->end_revision),
> + opt_state->verbose,
> + log_message_receiver,
> + &lb,
> + pool));
> + }
> + else
Might want to put a comment somewhere around here that we are in the
XML case. I know it should be obvious to anyone reading the code, but
it can help people browsing backwards, or who jumped here in a tags
operation, etc.
> + {
> + svn_stringbuf_t *sb;
> +
> + sb = NULL;
> +
> + /* The header generation is commented out because it might not
> + be certain that the log messages are indeed the advertised
> + encoding, UTF-8. The absence of the header should not matter
> + to people processing the output, and they should add it
> + themselves if storing the output as a fully-formed XML
> + document. */
> + /* <?xml version="1.0" encoding="utf-8"?> */
Interesting thought! I guess this is the best solution we can do,
isn't it? Oh well.
> + /* svn_xml_make_header (&sb, pool); */
> +
> + /* <log> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "log",
> + NULL);
> + printf ("%s", sb->data);
> +
> + SVN_ERR (svn_client_log (auth_baton,
> + targets,
> + &(opt_state->start_revision),
> + &(opt_state->end_revision),
> + opt_state->verbose,
> + log_message_receiver_xml,
> + &lb,
> + pool));
Yup, looks good!
Mental note (to self more than anyone else): svn_client_log() is where
the pool/subpool decision will be made, after we fix that interface.
I'd like to just apply this, with the tweaks above, and then do the
interface fix. No need to resubmit, I'll take care of it. Thanks for
the patch.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed May 1 23:49:15 2002