Alexander Thomas wrote:
> On Mon, 2005-05-30 at 22:31 +0100, Julian Foad wrote:
>>>+<!-- For "svn info" -->
>>>+<!ELEMENT info (target*)>
>>>+
>>>+<!ELEMENT target (entry*)>
>>
>>The "info" command's output is not (logically) grouped according to how the
>>targets were specified on the command line. Each item for which information is
>>printed is a self-contained item. So there is no need for a "target" element:
>> <!ELEMENT info (entry*)>
>
> No objection in removing "target". But before that let me say my
> conclusion about this.
>
> Info command support multiple target and also output multiple entries
> for a single target (try 'svn info *').
I have tried it with your patch. Now you try it. The "*" is expanded by the
shell or C run-time library into a list of file names. The "svn" program sees
a list of file names as its "targets".
"svn info" can generate multiple entries for a single target by using
"--recursive". Then it is "svn" that expands a single specified target into a
list of entries. However, I don't see that it is useful to keep this
meta-information (i.e. the grouping structure) in the XML output. After all,
that information is not in the plain text output, and the sub-grouping
information of the directories within a directory tree would not be not present
in your XML output.
> So in such cases I think its
> better to keep "target" in-tact.
>
>>>+ 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.)
>
> OK I will change this to abort()
Within the Subversion libraries, we must return errors rather than aborting,
but in the command-line client application I'm not sure what our policy is. I
get the impression that most people don't want it to abort.
>>>+ str_kind = "unknown";
>>>+ break;
>>>+ }
>>
>>Would it make sense to factor out this node-kind-to-text conversion and use it
>>for both the XML and plain-text outputs? Also other enum-to-text conversions
>>below.
>
> Yep I agree, What about creating two separate function for schedule and
> node-kind enums-to-text conversion ?
Yes, that will be good, at least for the schedule type.
Now that I look more closely at the node kind, I see that the only useful
values are "file" and "directory". In the "svn list" XML output, the node-kind
attribute is defined as being only "file" or "dir"; I think we should use
exactly the same definition for "svn info", and the other cases ("none" and
"unknown") should throw an error.
From "list.dtd":
<!ATTLIST entry kind (dir | file) #REQUIRED>
I would also suggest you factor out the creation of simple CDATA elements by
adding a function like this:
+/* If the string DATA is non-null, format it in a simple XML CDATA element
+ * named TAGNAME and append the result to the string SB.
+ * Use POOL for temporary allocations. */
+static void
+make_tagged_cdata (svn_stringbuf_t **sb,
+ apr_pool_t *pool,
+ const char *tagname,
+ const char *data)
+{
+ if (data)
+ {
+ svn_xml_make_open_tag (sb, pool, svn_xml_protect_pcdata, tagname, NULL);
+ svn_xml_escape_cdata_cstring (sb, data, pool);
+ svn_xml_make_close_tag (sb, pool, tagname);
+ }
+}
Then you can use it in (I think) 21 places, condensing examples like these:
+ if (info->URL)
+ {
+ /* "<url> xx </url>" */
+ svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "url", NULL);
+ svn_xml_escape_cdata_cstring (&sb, info->URL, pool);
+ svn_xml_make_close_tag (&sb, pool, "url");
+ }
+
+ if (SVN_IS_VALID_REVNUM (info->rev))
+ {
+ /* "<revision> xx </revision>" */
+ svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
+ "revision", NULL);
+ svn_xml_escape_cdata_cstring (&sb, apr_psprintf (pool,
+ "%" APR_OFF_T_FMT, info->rev), pool);
+ svn_xml_make_close_tag (&sb, pool, "revision");
+ }
to these:
+ /* "<url> xx </url>" */
+ make_tagged_cdata (&sb, pool, "url", info->URL);
+
+ if (SVN_IS_VALID_REVNUM (info->rev))
+ {
+ /* "<revision> xx </revision>" */
+ make_tagged_cdata (&sb, pool,
+ apr_psprintf (pool, "%" APR_OFF_T_FMT, info->rev));
+ }
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 31 13:58:42 2005