On Mon, 30 May 2005, Julian Foad wrote:
Just sneaking in on Julian's review here. It's not a complete review.
> Alexander Thomas wrote:
> > Index: subversion/clients/cmdline/dtd/info.dtd
> > ===================================================================
> > --- subversion/clients/cmdline/dtd/info.dtd (revision 0)
> > +++ subversion/clients/cmdline/dtd/info.dtd (revision 0)
> > +<!-- path: target supplied in cmdline -->
> > +<!ATTLIST target
> > + path CDATA #REQUIRED
> > +>
> > +
> > +<!ELEMENT entry (url?, revision, repository?, wc-info?, last-changed, conflict?, lock?)>
> > +<!-- path: local path -->
> > +<!-- type: path type -->
> > +<!ATTLIST entry
> > + path CDATA #REQUIRED
> > + type (file | directory | none | unknown) "unknown"
> > +>
I don't think a default value makes sense here.
> > +<!ELEMENT schedule (#PCDATA | normal | add | delete | replace | none)*>
The elements used on the line above are not declared.
> > +
> > +/* prints svn info in xml mode to standard out */
> > +static svn_error_t *
> > +print_info_xml (const char *target,
> > + const svn_info_t *info,
> > + apr_pool_t *pool)
> > +{
> > + const char *str_kind;
> > + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> > +
> > + switch (info->kind)
> > + {
> > + case svn_node_file:
> > + str_kind = "file";
> > + break;
> > +
> > + case svn_node_dir:
> > + str_kind = "directory";
> > + break;
> > +
> > + case svn_node_none:
> > + str_kind = "none";
> > + break;
> > +
> > + case svn_node_unknown:
> > + default:
>
> The "default" case should throw an error, I should think. (I see you just
> copied this from the existing non-XML code.)
>
I'm not sure about this. Regarding the status in XML patch, I suggested
using abort(). My reasoning is that if we have a switch that covers all
enum values, getting another value is an API violation. We have lots of
cases where we just crash when something violates the API (we even have
some abort() cases). For example, we normally don't check for a NULL
pointer, unless that's explicitly allowed. Adding error messages for such
cases that "cannot happen" just clutters the code IMO. And, as a minor
thing, it makes the translators have to translate really obscure errors.
OTOH, I can understand arguments for returning an error instead. IN that
case, maybe we should have an API violation error which always includes
file and line info, or alternatively has a message attatched that doesn't
need to be translated.
Note that I think abort is the right thing when some API returns an
illegal value, not when the WC got corrupt or something like that, but
that's obvious, isn't it?
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 31 21:27:45 2005