lundblad@tigris.org writes:
> --- trunk/subversion/clients/cmdline/blame-cmd.c (original)
> +++ trunk/subversion/clients/cmdline/blame-cmd.c Wed May 11 09:16:51 2005
> @@ -82,6 +146,34 @@
> }
>
>
> +/* Prints XML header to standard out. */
> +static svn_error_t *
> +print_header_xml (apr_pool_t *pool)
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> +
> + /* <?xml version="1.0" encoding="utf-8"?> */
> + svn_xml_make_header (&sb, pool);
> +
> + /* "<blame>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "blame", NULL);
> +
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +
> +/* Prints XML footer to standard out. */
> +static svn_error_t *
> +print_footer_xml (apr_pool_t *pool)
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> +
> + /* "</blame>" */
> + svn_xml_make_close_tag (&sb, pool, "blame");
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
In these two functions, we print directly to stdout. But...
> @@ -124,20 +216,42 @@
> opt_state->start_revision.kind = svn_opt_revision_number;
> opt_state->start_revision.value.number = 1;
> }
> + if (! opt_state->xml)
> + {
> + SVN_ERR (svn_stream_for_stdout (&out, pool));
> + bl.out = out;
> + }
> + else
> + bl.sbuf = svn_stringbuf_create ("", pool);
>
> - SVN_ERR (svn_stream_for_stdout (&out, pool));
> bl.opt_state = opt_state;
> - bl.out = out;
...here we set up a stream for stdout (in 'bl.out' via 'out') if *not*
doing XML, whereas if doing XML then we don't touch 'bl.out'. Why the
difference? Is there a reason that our output strategy for non-XML
should be different from our output strategy for XML?
Independently of that, we don't need the 'out' variable at all. It is
only used in that one place to set up 'bl.out'; it would be clearer to
just use 'bl.out' directly, I think.
(Also, it would be nice to have a blank line before the added 'if'
above, since it's not a continuation of the previous conditional.)
> subpool = svn_pool_create (pool);
>
> + /* If output is not incremental, output the XML header and wrap
> + everything in a top-level element. This makes the output in
> + its entirety a well-formed XML document. */
> + if (opt_state->xml)
> + {
> + if (opt_state->verbose)
> + return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("'verbose' option invalid in XML mode"));
> +
> + if ( ! opt_state->incremental)
> + SVN_ERR (print_header_xml (pool));
> + }
> + else if (opt_state->incremental)
> + return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("'incremental' option only valid in XML "
> + "mode"));
The comment about incrementalness doesn't apply to the entire outer
'if', it only applies to the second inner 'if'. Maybe move the
comment?
By the way, personally I would find it more readable to put the 'else
if' body in curly braces too, both for balance with the 'if' and
because it's multiple lines. I don't think that's a project-wide
standard, though; it's just a preference I share with Mike Pilato :-).
> @@ -151,31 +265,59 @@
> /* Check for a peg revision. */
> SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
> subpool));
> -
> - err = svn_client_blame2 (truepath,
> - &peg_revision,
> - &opt_state->start_revision,
> - &opt_state->end_revision,
> - blame_receiver,
> - &bl,
> - ctx,
> - subpool);
> + if (opt_state->xml)
> + {
> + /* "<target ...>" */
> + /* We don't output this tag immediately, which avoids creating
> + a target element if this path is skipped. */
> + svn_xml_make_open_tag (&bl.sbuf, pool, svn_xml_normal, "target",
> + "path", truepath, NULL);
> +
> + err = svn_client_blame2 (truepath,
> + &peg_revision,
> + &opt_state->start_revision,
> + &opt_state->end_revision,
> + blame_receiver_xml,
> + &bl,
> + ctx,
> + subpool);
> + }
> + else
> + err = svn_client_blame2 (truepath,
> + &peg_revision,
> + &opt_state->start_revision,
> + &opt_state->end_revision,
> + blame_receiver,
> + &bl,
> + ctx,
> + subpool);
Now here I'd *really* like the curly braces for the 'else' :-).
> if (err)
> {
> if (err->apr_err == SVN_ERR_CLIENT_IS_BINARY_FILE)
> {
> - SVN_ERR (svn_cmdline_printf (subpool,
> - _("Skipping binary file: '%s'\n"),
> - target));
> svn_error_clear (err);
> + SVN_ERR (svn_cmdline_fprintf (stderr, subpool,
> + _("Skipping binary file: '%s'\n"),
> + target));
> }
> else
> {
> return err;
> }
> }
> + else if (opt_state->xml)
> + {
> + /* "</target>" */
> + svn_xml_make_close_tag (&(bl.sbuf), pool, "target");
> + SVN_ERR (svn_cl__error_checked_fputs (bl.sbuf->data, stdout));
> + }
> +
> + if (opt_state->xml)
> + svn_stringbuf_setempty (bl.sbuf);
Hmmm. This is interesting. We already did a print and a setempty
from the end of blame_receiver_xml(). I guess this means the XML
printed at that time was unbalanced: it starts with "<target>" but
does not include the closing "</target>". Then here, after the
callback has completed, we print "</target>" and (again) clear
'bl.sbuf'.
Any reason we do the printing & clearing twice? Why not eliminate it
from blame_receiver_xml() and just do it all here? Or, conversely,
why not set up the opening "<target>" element in blame_receiver_xml()?
That way you could avoid the need for this comment earlier, too:
/* "<target ...>" */
/* We don't output this tag immediately, which avoids creating
a target element if this path is skipped. */
Just to be clear: I'm not saying the code is incorrect as it stands.
I'm just asking if the code flow is as clear as it could be.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon May 16 16:23:42 2005