On Mon, 16 May 2005 kfogel@collab.net wrote:
> > +/* 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?
>
For the XML, we want to use stdio, since our XML routines uses \n as
newline separator and we want the platform newline. (That's for
readability, an XML parser doesn't care.)
For the traditional output, we don't want anything to mess around with our
newlines. We produce an \n, but if the file has CRLF we will have the line
to output ending with CR. stdio would convert this to CRCRLF AFAICT. But
your question indicates there is the need for a comment.
> 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.)
>
Yep.
> > 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?
>
Ooops.
> 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 :-).
>
IN this case I agree.
> > @@ -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' :-).
>
I don't. Remember that discussion we had about this on IRC? If the only
thing in a substatement is a function call, I prefer no curly braces.
That's what I'm using until we have a project policy for this:-) (Or
someone strongly objects, ofcousre.)
> > 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,
Then we would buffer up the whole XML for one file in memory
unnecessarily. Remember that blame_receiver_xml is called once per line.
> 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. */
>
There are some conrer cases here. You want a target element if the
blamed file is empty, but you don't want it if the file was skipped. So
you need to print <target> outside the blame receiver, since that's not
called if there are no lines
I'll commit some style fixes and an added comment.
Thanks, as always, for the review.
regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon May 16 21:26:09 2005