On Mon, 2005-05-30 at 23:41 +0200, Peter N. Lundblad wrote:
> > +svn_error_t *
> > +svn_cl__print_status_xml (const char *path,
> > + svn_wc_status2_t *status,
> > + apr_pool_t *pool)
> > +{
> > + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> > +
> > + if (status->text_status == svn_wc_status_none
> > + && status->repos_text_status == svn_wc_status_none)
> > + return SVN_NO_ERROR;
> > +
> > + /* "<entry>" */
>
> I see you removed some such comments. I think all of them can go away,
> since they don't add anything. You see, this patch starts to look
> good, so I have to find some really minor things:-)
>
[...]
> > +
> > + if (status->entry->lock_comment)
> > + {
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "comment", NULL);
> > + svn_xml_escape_cdata_cstring (&sb, status->entry->lock_comment,
> > + pool);
> > + svn_xml_make_close_tag (&sb, pool, "comment");
> > + }
> > +
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "created",
> > + NULL);
> > + svn_xml_escape_cdata_cstring (&sb,
> > + svn_time_to_cstring
> > + (status->entry->lock_creation_date, pool),
> > + pool);
> > + svn_xml_make_close_tag (&sb, pool, "created");
> > +
> > + svn_xml_make_close_tag (&sb, pool, "lock");
> > + }
> > +
> > + /* "</wc-status>" */
>
> More comments that could go.
[...]
> > + {
> > + /* "<repos-status>" */
>
> I won't pick on all these comments:-)
>
Limiting comments only to group tags.
> > + svn_xml_make_close_tag (&sb, pool, "wc-status");
> > +
> > + if (status->repos_lock)
>
> This should be
> if (status->repos_text_status != svn_wc_status_none
> || status->repos_prop_status != svn_wc_status_none
> || status->repos_lock)
>
> You print repos_status if any of these are true.
OK, done.
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "repos-status",
> > + "item",
> > + generate_status_desc (status->repos_text_status),
> > + "props",
> > + generate_status_desc (status->repos_prop_status),
> > + NULL);
> > +
> > + /* "<lock>" */
>
> So when you change the above, you need a specific test for
> status->repos_lock here.
>
Yep ...
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "lock", NULL);
> > +
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "token", NULL);
> > + svn_xml_escape_cdata_cstring (&sb, status->repos_lock->token, pool);
> > + svn_xml_make_close_tag (&sb, pool, "token");
> > +
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "owner", NULL);
> > + svn_xml_escape_cdata_cstring (&sb, status->repos_lock->owner, pool);
> > + svn_xml_make_close_tag (&sb, pool, "owner");
> > +
> > + if (status->repos_lock->comment)
> > + {
> > + /* "<comment>xx</comment>" */
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "comment", NULL);
> > + svn_xml_escape_cdata_cstring (&sb, status->repos_lock->comment, pool);
> > + svn_xml_make_close_tag (&sb, pool, "comment");
> > + }
> > +
> > + /* "<created>xx</created>" */
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
> > + "created", NULL);
> > + svn_xml_escape_cdata_cstring (&sb, svn_time_to_cstring
> > + (status->repos_lock->creation_date,
> > + pool),
> > + pool);
> > + svn_xml_make_close_tag (&sb, pool, "created");
> > +
> > + if (status->repos_lock->expiration_date != 0)
> > + {
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
> > + "expires", NULL);
> > + svn_xml_escape_cdata_cstring (&sb, svn_time_to_cstring
> > + (status->repos_lock->expiration_date,
> > + pool),
> > + pool);
> > + svn_xml_make_close_tag (&sb, pool, "expires");
> > + }
> > +
> > + /* "</lock>" */
> > + svn_xml_make_close_tag (&sb, pool, "lock");
> > +
> > + /* "</repos-status>"*/
> > + svn_xml_make_close_tag (&sb, pool, "repos-status");
> > + }
> > +
> Hmmm... I think the last committed info could be added here as well. I
> > don't know if it was in the original patch and I forgot it when I
> > rewrote the DTD:-( BTW, the DTD is missing from this patch.
>
last commited info ?
> > +{
> > + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> > +
> > + if (SVN_IS_VALID_REVNUM (repos_rev) && update)
> > + {
> > + const char *repos_rev_str;
> > + repos_rev_str = apr_psprintf (pool, "%" APR_OFF_T_FMT, repos_rev);
>
> Use %ld for revision numbers. (That has to do with gettext, which
> doesn't understand such macros.)
>
I think %ld needed not be identified by gettext, after all this is
going in to xml output.
> > + err = svn_client_status2 (&repos_rev, target, &rev, print_status, &sb,
> > + opt_state->nonrecursive ? FALSE : TRUE,
> > + (opt_state->verbose || opt_state->xml),
>
> I don't think XML mode should imply --verbose *in this case*. I think
> --verbose makes sense for XML, since. Otherwise status will output
> lots of useless entries for unmodified paths. I normally argue that
> we should print all available information in XML and that the
> receiving application can filter it. I'm not consistent, I know, but
> this would be extreme:-)
>
Why, what is so exceptional about status ?
So you want me to remove opt_state->xml and if user wants detailed xml
output, they had to give explicit --verbose cmdline option
-Alexander Thomas (AT)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 31 14:23:46 2005