On Mon, 2005-05-23 at 22:39 +0200, Peter N. Lundblad wrote:
On Fri, 20 May 2005 alexander@collab.net wrote:
>
> +svn_error_t *svn_cl__print_status_xml (const char *path,
> > + svn_wc_status2_t *status,
> > + svn_boolean_t detailed,
> > + svn_boolean_t
> > show_last_committed,> > + svn_boolean_t skip_unrecognized,
> > + svn_boolean_t repos_locks,
> > + apr_pool_t *pool);
> > +
>
> So, you can remove all the bools except skip_unrecognized. And when I
> think about it, it seems like skip_unrecognized can go as well. Not
> sure about that one.
>
> Keeping repos_locks, because I am using this in print_status_xml.
> Index: subversion/clients/cmdline/status.c
> > ===================================================================
> > --- subversion/clients/cmdline/status.c (revision 14774)
> > +++ subversion/clients/cmdline/status.c (working copy)
> > @@ -51,6 +53,155 @@
> > }
> > }
> >
> > +
> > +/* Return the detailed string representation of STATUS */
> > +static const char*
> > +generate_status_desc (enum svn_wc_status_kind status)
> > +{
> > + switch (status)
> > + {
> > + case svn_wc_status_none: return "none";
> > + case svn_wc_status_normal: return "normal";
> > + case svn_wc_status_added: return "added";
> > + case svn_wc_status_missing: return "missing";
> > + case svn_wc_status_incomplete: return "incomplete";
> > + case svn_wc_status_deleted: return "deleted";
> > + case svn_wc_status_replaced: return "replaced";
> > + case svn_wc_status_modified: return "modified";
> > + case svn_wc_status_merged: return "merged";
> > + case svn_wc_status_conflicted: return "conflicted";
> > + case svn_wc_status_obstructed: return "obstructed";
> > + case svn_wc_status_ignored: return "ignored";
> > + case svn_wc_status_external: return "external";
> > + case svn_wc_status_unversioned: return "unversioned";
> > + default: return "unversioned";
>
> I think abort() is better in the default: case. It may sound drastic,
> but if we add another value to the enum, we will notice that faster
> than if it is "unversioned". If you don't want to be that drastic,
> change the function to return an error instead.
>
> I don't understand :) this function is similar to existing
generate_status_code function, I created it to avoid messing
print_status_xml.
>
> >
> > + }
> > +}
> > +
> > +
> > +/* Print status in XML format to stdout */
> > +static svn_error_t *
> > +print_status_xml (const char *path,
> > + svn_boolean_t detailed,
> > + svn_boolean_t show_last_committed,
> > + svn_boolean_t repos_locks,
> > + svn_wc_status2_t *status,
> > + apr_pool_t *pool)
> > +{
> > + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> > +
> > + /* "<entry>" */
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "entry",
> > + "path", path[0] == '\0' ? "." : path, NULL);
>
> svn_path_local_style takes care of this special casing for the empty
> path. You should use that, since this comes from a local path
> originally.
>
> I will change it to
svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "entry",
"path", path, NULL);
> +
> > + /* "<wc-status...>" */
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "wc-status",
> > + "item", generate_status_desc
(status->text_status),
> > + "props", generate_status_desc
(status->prop_status),
> > + "wc-locked", status->locked ? "true" : "false",
> > + "copied", status->copied ? "true" : "false",
> > + "switched", status->switched ? "true" :
> > "false",> > + NULL);
> > +
>
> Since it common for copied, switched and wc-locked to be false, I
> think you should use the default values declared in the DTD and avoid
> setting these attributes if they're false. XML is verbose, but we
> don't need tomake it unreasonably so:-) Hint: Use
> svn_xml_make_open_tag_hash.
>
> I will look into it
> +
> > + if (status->locked && status->entry)
>
> You're confusing (and rightly so) WC adm area locks with file locks in
> the repository. This test should be:
> if (status->entry && status->entry->lock_token)
>
> You are right, terrible mistake :)
> + {
> > + /* "<lock>" */
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "lock", NULL);
> > +
> > + /* "<token>xx</token> */
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
"token", NULL);
> > + svn_xml_escape_cdata_cstring (&sb, status->entry->lock_token,
> > pool);> > + svn_xml_make_close_tag (&sb, pool, "token");
> > +
> > + /* "<owner>xx</owner> */
> > + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
"owner", NULL);
> > + svn_xml_escape_cdata_cstring (&sb, status->entry->lock_owner ?
> > + status->entry->lock_owner : "",
> > + pool);
>
> It is correct to not assume that lock_owner is non-NULL, since the
> entry reading functions doesn't guarantee that. I think you should
> return an error in that case instead of making owner empty. It's a
> corrupt WC.
>
> Yep I agree
> /* Called by status-cmd.c */
> > svn_error_t *
> > +svn_cl__print_status_xml (const char *path,
> > + svn_wc_status2_t *status,
> > + svn_boolean_t detailed,
> > + svn_boolean_t show_last_committed,
> > + svn_boolean_t skip_unrecognized,
> > + svn_boolean_t repos_locks,
> > + apr_pool_t *pool)
> > +{
> > + if (! status
>
> When can status be null? (It seems like you just followed an existing
> pattern, but I still wonder.)
>
> I too thought of it, just followed the existing pattern. will be removed
> > @@ -94,8 +171,12 @@
> > rev.kind = svn_opt_revision_head;
> >
> > /* The notification callback. */
> > - svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2, FALSE,
FALSE,
> > - FALSE, pool);
> > + if (opt_state->xml)
> > + svn_cl__get_notifier2 (&ctx->notify_func2, &ctx->notify_baton2,
FALSE, FALSE,
> > + FALSE, TRUE, pool);
> > + else
> > + svn_cl__get_notifier2 (&ctx->notify_func2, &ctx->notify_baton2,
FALSE, FALSE,
> > + FALSE, FALSE, pool);
> >
>
> If you don't call this if opt_state->xml is true, the notifier doesn't
> need to know about XML. You could ad a comment clarifying this.
>
>
> changing this to
svn_cl__get_notifier2 (&ctx->notify_func2, &ctx->notify_baton2, FALSE,
FALSE, FALSE, opt_state->xml, pool);
Thanks for the review :)
sorry about bad indentation and other silly mistakes :( - next time will do
a careful patch review before sending ...
-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 24 11:19:13 2005