On Tue, 24 May 2005 alexander@collab.net wrote:
> 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.
>
But you don't need to. IN print_status, repos_locks is used to
differentiate between a missing repository lock in the status struct
meaning 2didn't ask the repository" and "we know there is no lock, so if
the WC is locked, it is broken". You don't output anything telling whether
the lock is broken, except what you can infer from the existence of a lock
in the wc vs. repository. There is no need to not print a repository lock
if -u wasn't specified, since in that case there will never be repository
locks in the status structs.
> > 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.
Yeah, but in generate_status_code, a space means many things, not just
"unversioned". For XML, we distinguish between all codes, so saying
"unversioned" is a more problematic lie, so to speak:-) I could argue that
generate_status_code should also abort() in the default: case, but let's
keep the scope of this patch limited to XML output.
> > > +/* 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);
>
OK.
>
> > > @@ -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);
>
>
No, please. Get rid of svn_wc_get_notifier2. In XML mode, just don't call
this function.
> Thanks for the review :)
> sorry about bad indentation and other silly mistakes :( - next time will do
> a careful patch review before sending ...
>
That's always a good idea, but such silly mistake will nearly always
happen. Just ask me:-) That's one thing reviews are for.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 24 21:40:41 2005