You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Alexander Thomas <al...@collab.net> on 2005/06/15 07:11:58 UTC
[PATCH] 'svn info --xml' - v2
[[[
Version 2: Patch to add XML output for 'svn info' and send warnings to
stderr instead of stdout.
* subversion/clients/cmdline/main.c
(svn_cl__cmd_table[]): Added --xml and --incremental options
for svn info.
* subversion/clients/cmdline/dtd/info.dtd
New dtd for info --xml.
* subversion/clients/cmdline/info-cmd.c
(print_header_xml): New function.
(print_footer_xml): New function.
(kind_str): New function.
(schedule_str): New function.
(make_tagged_cdata): New function.
(print_info_xml): New function.
(info_receiver): Edited existing callback to print xml or
traditional output depending on cmdline option.
(svn_cl__info): Modified for XML output and redirected warnings to
stderr.
* tools/client-side/bash_completion
(_svn): Added "--incremental" and "--xml" options to "info"
for auto-completion.
]]]
Re: [PATCH] 'svn info --xml' - v2
Posted by Michael W Thelen <mi...@pietdepsi.com>.
Alexander Thomas wrote:
> [[[
> Patch to add XML output for 'svn info'.
This patch has been sitting for a while without comment... are any
developers able to review it? If not, I'll file it as a patch issue in
the next day or two.
--
Michael W Thelen
It is a mistake to think you can solve any major problems just with
potatoes. -- Douglas Adams
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v2
Posted by Julian Foad <ju...@btopenworld.com>.
Alexander Thomas wrote:
> On Wed, 2005-06-15 at 12:17 +0100, Julian Foad wrote:
>>That's two logically separate changes. It would be much better if you could
>>send two separate patches.
>
> Patch to add XML output for 'svn info'.
Thank you very much. I hope someone else will review it, as I'm going to be
away for a while.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v2
Posted by Michael W Thelen <mi...@pietdepsi.com>.
Alexander Thomas wrote:
> [[[
> Patch to add XML output for 'svn info'.
Thank you for the patch... I've filed it as issue #2362.
http://subversion.tigris.org/issues/show_bug.cgi?id=2362
--
Michael W Thelen
It is a mistake to think you can solve any major problems just with
potatoes. -- Douglas Adams
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v5
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
>
> So, please find attached my version of your patch after making all of
> the above changes.
>
> I think this is ready to commit as far I am concerned, but I would
> really like someone else to review particularly the format of the XML if
> possible. If there are no further concerns in the next two or three
> days I will commit this.
No comments seen, so I've committed this in r16014.
Thanks, Alexander.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v5
Posted by Michael W Thelen <mi...@pietdepsi.com>.
Julian Foad wrote:
>>> So, please find attached my version of your patch after making all of
>>> the above changes.
>>
>> Just a ping... this patch doesn't look like it's been applied yet. Is
>> anyone else able to review it? If not, and if it hasn't been applied in
>> a day or two, I can file an issue for it so it doesn't get lost.
>
> I'm not around for another week or two. Thanks for keeping an eye on it.
No problem! I'll leave it alone then. :-)
--
Michael W Thelen
It is a mistake to think you can solve any major problems just with
potatoes. -- Douglas Adams
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v5
Posted by Julian Foad <ju...@btopenworld.com>.
Michael W Thelen wrote:
> Julian Foad wrote:
>
>>So, please find attached my version of your patch after making all of
>>the above changes.
>
> Just a ping... this patch doesn't look like it's been applied yet. Is
> anyone else able to review it? If not, and if it hasn't been applied in
> a day or two, I can file an issue for it so it doesn't get lost.
I'm not around for another week or two. Thanks for keeping an eye on it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v5
Posted by Michael W Thelen <mi...@pietdepsi.com>.
Julian Foad wrote:
> So, please find attached my version of your patch after making all of
> the above changes.
>
> I think this is ready to commit as far I am concerned, but I would
> really like someone else to review particularly the format of the XML if
> possible. If there are no further concerns in the next two or three
> days I will commit this.
>
> Please say if you don't agree with something I said or did, or if you
> would like to make further changes.
Just a ping... this patch doesn't look like it's been applied yet. Is
anyone else able to review it? If not, and if it hasn't been applied in
a day or two, I can file an issue for it so it doesn't get lost.
--
Michael W Thelen
It is a mistake to think you can solve any major problems just with
potatoes. -- Douglas Adams
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v5
Posted by Julian Foad <ju...@btopenworld.com>.
Alexander Thomas wrote:
>
> Patch to add XML output for 'svn info'.
I'm testing this patch against its DTD by running the following command in my
Subversion working copy and in my "sandbox" working copy (which has all sorts
of odd things in it):
$ svn info --xml -R * | xmllint --dtdvalid
~/src/subversion/subversion/clients/cmdline/dtd/info.dtd --noout -
I've also been looking at the other DTDs (blame, list, log, status) to see how
this compares with them, and looking at the source of the information
(svn_info_t) to see what it really means and which bits are related to each other.
In deciding which elements are optional and which are dependent on others, I
could do with some help from somebody who understands the client and WC better
than me. To me, it seems that nearly everything is optional, and it seems that
this has only been determined empirically by finding test cases in which some
bit of information is not present.
One further thing I'd like to see, but that can be done afterwards, is the DTDs
(all of them) better laid out, with replaceable entities naming data types like
"pathname", "username", "date", etc., even though all of those are just defined
as "(#PCDATA)"; and better comments that state both the meaning and the data
type (format) of each field.
[...]
> (svn_cl__node_kind_str): New prototype.
[...]
> (make_tagged_cdata): New function.
I decided to add these both as semi-public functions, and have committed them
already in r15545, the latter named "svn_cl__xml_tagged_cdata".
> Index: subversion/clients/cmdline/dtd/info.dtd
> ===================================================================
> --- subversion/clients/cmdline/dtd/info.dtd (revision 0)
> +++ subversion/clients/cmdline/dtd/info.dtd (revision 0)
> @@ -0,0 +1,47 @@
> +<!-- XML DTD for Subversion command-line client output. -->
> +
> +<!-- For "svn info" -->
> +<!ELEMENT info (entry*)>
> +
> +<!ELEMENT entry (url?, repository?, wc-info?, last-changed?, conflict?, lock?)>
> +<!-- path: local path -->
> +<!-- kind: path type -->
> +<!-- revision number of wc or repos path/url-->
> +<!ATTLIST entry
> + path CDATA #REQUIRED
> + kind (file | directory) #REQUIRED
I've changed "directory" to "dir" for consistency with "svn list --xml".
> + revision CDATA #REQUIRED
> +>
> +<!ELEMENT url (#PCDATA)> <!-- repository url -->
> +
> +<!ELEMENT repository (root?, uuid)>
It seems that the UUID needs to be optional. I have some directories that have
both root and uuid, and some with only root, and some with only uuid.
> +<!ELEMENT root (#PCDATA)> <!-- root: repository URL -->
> +<!ELEMENT uuid (#PCDATA)> <!-- uuid: repository uuid -->
> +
> +<!ELEMENT wc-info (schedule?, copy-from-url?, copy-from-rev?, checksum?)>
> +<!-- schedule: applies only for wc -->
Hmm. All of the "wc-info" fields apply only to a wc.
> +<!ELEMENT schedule (#PCDATA | normal | add | delete | replace | none)*>
This implies mixed content. I've changed this to:
<!ELEMENT schedule (#PCDATA)> <!-- normal | add | delete | replace | none -->
> +<!ELEMENT copy-from-url (#PCDATA)>
> +<!ELEMENT copy-from-rev (#PCDATA)>
The code was writing "copied-from-rev". I've fixed the code.
> +<!ELEMENT checksum (#PCDATA)>
> +
> +<!ELEMENT last-changed (author, revision, date, text-updated?, prop-updated?)>
> +<!ELEMENT author (#PCDATA)> <!-- last changed author -->
> +<!ELEMENT revision (#PCDATA)> <!-- last changed revision -->
This "last-changed" element ought to be consistent with the "commit" element of
"status.dtd" and "list.dtd", even as far as being named "commit".
> +<!ELEMENT date (#PCDATA)> <!-- last changed date in iso format -->
> +<!ELEMENT text-updated (#PCDATA)> <!-- text last updated -->
> +<!ELEMENT prop-updated (#PCDATA)> <!-- properties last updated -->
Those last two elements are WC info, not last-changed info. See the definition
of svn_info_t for details.
> +<!ELEMENT conflict (prev-base-file, prev-wc-file?, cur-base-file, prop-file?)>
> +<!ELEMENT prev-base-file (#PCDATA)> <!-- previous base file -->
> +<!ELEMENT prev-wc-file (#PCDATA)> <!-- previous wc file -->
> +<!ELEMENT cur-base-file (#PCDATA)> <!-- current base file -->
> +<!ELEMENT prop-file (#PCDATA)> <!-- current properties file -->
This conflict information is also a subset of the WC information.
> +<!ELEMENT lock (token, owner, created, expires?, comment?)>
> +<!ELEMENT token (#PCDATA)> <!-- lock token -->
> +<!ELEMENT owner (#PCDATA)> <!-- lock owner -->
> +<!ELEMENT created (#PCDATA)> <!-- lock created: date & time in iso format -->
> +<!ELEMENT expires (#PCDATA)> <!-- lock expires: date & time in iso format -->
> +<!ELEMENT comment (#PCDATA)> <!-- lock comment -->
This "lock" element should be consistent with the one in "status.dtd"; it very
nearly is, but the order of the elements is different.
[...]
> + /* "<copied-from-rev> xx </copied-from-rev>" */
> + make_tagged_cdata (&sb, pool, "copied-from-rev",
I've corrected this from "copied-from-rev" to "copy-from-rev".
> + apr_psprintf (pool, "%ld", info->copyfrom_rev));
[...]
> @@ -76,26 +315,9 @@
> if (SVN_IS_VALID_REVNUM (info->rev))
> SVN_ERR (svn_cmdline_printf (pool, _("Revision: %ld\n"), info->rev));
>
> - switch (info->kind)
> - {
> - case svn_node_file:
> - SVN_ERR (svn_cmdline_printf (pool, _("Node Kind: file\n")));
> - break;
> -
> - case svn_node_dir:
> - SVN_ERR (svn_cmdline_printf (pool, _("Node Kind: directory\n")));
> - break;
> -
> - case svn_node_none:
> - SVN_ERR (svn_cmdline_printf (pool, _("Node Kind: none\n")));
> - break;
> -
> - case svn_node_unknown:
> - default:
> - SVN_ERR (svn_cmdline_printf (pool, _("Node Kind: unknown\n")));
> - break;
> - }
>
> + SVN_ERR (svn_cmdline_printf (pool, _("Node Kind: %s\n"),
> + svn_cl__node_kind_str (info->kind)));
As I said, we probably want those strings to remain localisable, while the XML
output is fixed, so we'll leave that part of the code as it was, and not call
the new "node_kind_str" function there.
So, please find attached my version of your patch after making all of the above
changes.
I think this is ready to commit as far I am concerned, but I would really like
someone else to review particularly the format of the XML if possible. If
there are no further concerns in the next two or three days I will commit this.
Please say if you don't agree with something I said or did, or if you would
like to make further changes.
- Julian
Re: [PATCH] 'svn info --xml' - v4
Posted by Alexander Thomas <al...@collab.net>.
On Tue, 2005-08-02 at 00:06 +0100, Julian Foad wrote:
> Alexander Thomas wrote:
> >
> > * subversion/clients/cmdline/dtd/info.dtd
> > New dtd for info --xml.
>
> This DTD file is missing from the patch this time. Please could you send it.
Sorry, I missed out. Here I am posting an new version of the patch,
after changing APR_OFF_T_FMT to "%ld" along with the info.dtd file.
Regards
-Alexander Thomas(AT)
Re: [PATCH] 'svn info --xml' - v2
Posted by Julian Foad <ju...@btopenworld.com>.
Alexander Thomas wrote:
>
> * subversion/clients/cmdline/dtd/info.dtd
> New dtd for info --xml.
This DTD file is missing from the patch this time. Please could you send it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v2
Posted by Julian Foad <ju...@btopenworld.com>.
Alexander Thomas wrote:
> On Sun, 2005-07-31 at 18:52 +0100, Julian Foad wrote:
>
>>The existing plain-text "svn info" function, print_info(), should also use this
>>function, unless we want an unlocalised version for XML and a localised version
>>for plain text ... which I suppose we do. And print_info() currently also
>>printfs "none" and "unknown" which is really rather silly, so we'd have to make
>>a decision on what it really ought to be doing if it somehow encounters those
>>node kinds. OK, forget about print_info() for now.
>>
>
> I too feel same with "none" and "unknown". So I changing the code
> fragment as below.
>
> SVN_ERR (svn_cmdline_printf (pool, _("Node Kind: %s\n"),
> svn_cl__node_kind_str (info->kind)));
Hmm... as I said, I suppose we want the strings to be translated into the local
human language for plain text output but not for XML output. Don't worry, I'll
fix that up if necessary (if other people agree).
>>>+ make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
>>>+ "copied-from-rev", apr_psprintf (pool,
>>>+ "%" APR_OFF_T_FMT, info->copyfrom_rev));
>>
>>info-cmd.c:217: warning: long long int format, different type arg (arg 3)
>>
>>APR_OFF_T_FMT is not the format of a revision number. "%ld" is.
>
> Sorry I don't get this warning. I am taking this macro from apr.h where
> APR_OFF_T_FMT is defined for "ld". :(
APR_OFF_T_FMT is defined as whatever format string is appropriate for printing
values of type "apr_off_t" on your system. That may be "%ld" on your system,
but it is different on different systems. It is "%lld" on my system.
You are not printing a value of type "apr_off_t", you are printing a value of
type "svn_revnum_t". The correct format string for that is "%ld" on all
systems. We write the "%ld" literally, not using the defined constant
SVN_REVNUM_T_FMT, because use of a defined constant would interfere with the
"_(...)" macro used by "gettext" localisation. So, write:
apr_psprintf (pool, "%ld", info->copyfrom_rev)
But you don't need to submit another version of the patch just for this; I'll
make the change if there are no other problems when I review it.
Thanks for this new version of the patch. I haven't reviewed it yet.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v2
Posted by Alexander Thomas <al...@collab.net>.
On Sun, 2005-07-31 at 18:52 +0100, Julian Foad wrote:
> > + svn_xml_make_close_tag (&sb, pool, "info");
> > + return svn_cl__error_checked_fputs (sb->data, stdout);
> > +}
> > +
> > +/* Return string representation of KIND */
> > +static const char *
> > +kind_str (svn_node_kind_t kind)
> > +{
> > + switch (kind)
> > + {
> > + case svn_node_dir:
> > + return "directory";
> > + case svn_node_file:
> > + return "file";
> > + default:
> > + return "";
> > + }
> > +}
>
> This is a duplicate of a function in ls-cmd.c. Don't duplicate code, share it.
> Move it into "cl.h" and "util.c", named something like "svn_cl__node_kind_str".
>
Created svn_cl__node_kind_str() in util.c and added prototype in cl.h.
Changes to be made in ls-cmd.c to use this new function will be posted
as another patch.
>
> The existing plain-text "svn info" function, print_info(), should also use this
> function, unless we want an unlocalised version for XML and a localised version
> for plain text ... which I suppose we do. And print_info() currently also
> printfs "none" and "unknown" which is really rather silly, so we'd have to make
> a decision on what it really ought to be doing if it somehow encounters those
> node kinds. OK, forget about print_info() for now.
>
I too feel same with "none" and "unknown". So I changing the code
fragment as below.
SVN_ERR (svn_cmdline_printf (pool, _("Node Kind: %s\n"),
svn_cl__node_kind_str (info->kind)));
> > +/* 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);
> > + const char *rev_str = (SVN_IS_VALID_REVNUM(info->rev)) ?
> > + apr_psprintf (pool, "%ld", info->rev) :
> > + NULL;
> > +
> > + /* If REV_STR is NULL, assume WC is corrupt. */
> > + if (!rev_str)
>
> The only way rev_str can be null is if your conditional assignment above set it
> to null, so just move that assignment after this "if" statement and make it
> unconditional, to save having two different tests for the same thing. I know
> that would mean the variable would not be initialised at its point of
> declaration, but that's the norm in C for non-trivial initialisations.
>
> > + return svn_error_createf (SVN_ERR_WC_CORRUPT, NULL,
> > + _("'%s' has invalid revision"),
> > + svn_path_local_style (target, pool));
> > +
I changed with a single condition check.
> > + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> > + "copied-from-rev", apr_psprintf (pool,
> > + "%" APR_OFF_T_FMT, info->copyfrom_rev));
>
> info-cmd.c:217: warning: long long int format, different type arg (arg 3)
>
> APR_OFF_T_FMT is not the format of a revision number. "%ld" is.
>
Sorry I don't get this warning. I am taking this macro from apr.h where
APR_OFF_T_FMT is defined for "ld". :(
> > + }
> > +
> > + /* "<date> xx </date>" */
> > + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> > + "date", svn_time_to_cstring
> > + (info->last_changed_date, pool));
>
> (I wonder if there are any cases in which last-changed info is being printed
> but last_changed_date is absent (zero), which would result in
> "1970-01-01T00:00:00.000000Z" being printed.)
>
All svn xml (blame --xml, status --xml ...) date output is printed in
ISO format. I am adding a check on last_changed_date to resolve this
absent condition.
Regards
-Alexander Thomas (AT)
Re: [PATCH] 'svn info --xml' - v2
Posted by Julian Foad <ju...@btopenworld.com>.
Alexander Thomas wrote:
> [[[
> Patch to add XML output for 'svn info'.
Sorry I took so long to get around to reviewing this.
> * subversion/clients/cmdline/main.c
> (svn_cl__cmd_table[]): Added --xml and --incremental options
> for svn info.
>
> * subversion/clients/cmdline/dtd/info.dtd
> New dtd for info --xml.
>
> * subversion/clients/cmdline/info-cmd.c
> (print_header_xml): New function.
> (print_footer_xml): New function.
> (kind_str): New function.
> (schedule_str): New function.
> (make_tagged_cdata): New function.
> (print_info_xml): New function.
> (info_receiver): Edited existing callback to print xml or
> traditional output depending on cmdline option.
> (svn_cl__info): Modified for XML output.
>
> * tools/client-side/bash_completion
> (_svn): Added "--incremental" and "--xml" options to "info"
> for auto-completion.
> ]]]
[...]
> Index: subversion/clients/cmdline/dtd/info.dtd
> ===================================================================
> --- subversion/clients/cmdline/dtd/info.dtd (revision 0)
> +++ subversion/clients/cmdline/dtd/info.dtd (revision 0)
> @@ -0,0 +1,47 @@
> +<!-- XML DTD for Subversion command-line client output. -->
> +
> +<!-- For "svn info" -->
> +<!ELEMENT info (entry*)>
> +
> +<!ELEMENT entry (url?, repository?, wc-info?, last-changed, conflict?, lock?)>
For a simple schedule-add file (without history), and possibly other cases, no
last-changed info exists, so that element should be optional.
> +<!-- path: local path -->
> +<!-- type: path type -->
> +<!-- revision number of wc or repos path/url-->
> +<!ATTLIST entry
> + path CDATA #REQUIRED
> + type (file | directory) #REQUIRED
Call the type "kind", with values "dir" and "file", for consistency with list.dtd.
> + revision CDATA #REQUIRED
> +>
> +<!ELEMENT url (#PCDATA)> <!-- repository url -->
> +
> +<!ELEMENT repository (root?, uuid)>
> +<!ELEMENT root (#PCDATA)> <!-- root: path type -->
"path type"? Don't you mean "repository URL" or something?
> +<!ELEMENT uuid (#PCDATA)> <!-- uuid: repository uuid -->
> +
> +<!ELEMENT wc-info (schedule?, copy-from-url?, copy-from-rev?, checksum?)>
> +<!-- schedule: applies only for wc -->
> +<!ELEMENT schedule (#PCDATA | normal | add | delete | replace | none)*>
> +<!ELEMENT copy-from-url (#PCDATA)>
> +<!ELEMENT copy-from-rev (#PCDATA)>
> +<!ELEMENT checksum (#PCDATA)>
> +
> +<!ELEMENT last-changed (author, revision, date, text-updated?, prop-updated?)>
> +<!ELEMENT author (#PCDATA)> <!-- last changed author -->
> +<!ELEMENT revision (#PCDATA)> <!-- last changed revision -->
> +<!ELEMENT date (#PCDATA)> <!-- last changed date in iso format -->
> +<!ELEMENT text-updated (#PCDATA)> <!-- text last updated -->
> +<!ELEMENT prop-updated (#PCDATA)> <!-- properties last updated -->
> +
> +<!ELEMENT conflict (prev-base-file, prev-wc-file, cur-base-file, prop-file)>
In a manual test with a conflict caused by trying to merge a binary file, I
found only cur-base-file and prev-base-file.
> +<!ELEMENT prev-base-file (#PCDATA)> <!-- previous base file -->
> +<!ELEMENT prev-wc-file (#PCDATA)> <!-- previous wc file -->
> +<!ELEMENT cur-base-file (#PCDATA)> <!-- current base file -->
> +<!ELEMENT prop-file (#PCDATA)> <!-- current properties file -->
> +
> +
> +<!ELEMENT lock (token, owner, created, expires?, comment?)>
> +<!ELEMENT token (#PCDATA)> <!-- lock token -->
> +<!ELEMENT owner (#PCDATA)> <!-- lock owner -->
> +<!ELEMENT created (#PCDATA)> <!-- lock created: date & time in iso format -->
> +<!ELEMENT expires (#PCDATA)> <!-- lock expires: date & time in iso format -->
> +<!ELEMENT comment (#PCDATA)> <!-- lock comment -->
> Index: subversion/clients/cmdline/info-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/info-cmd.c (revision 14784)
> +++ subversion/clients/cmdline/info-cmd.c (working copy)
[...]
> @@ -47,8 +48,267 @@
> return SVN_NO_ERROR;
> }
>
> +/* Prints XML header */
> +static svn_error_t *
> +print_header_xml (apr_pool_t *pool)
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> + /* <?xml version="1.0" encoding="utf-8"?> */
> + svn_xml_make_header (&sb, pool);
>
> + /* "<status>" */
This comment is wrong.
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "info", NULL);
> +
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +
> +/* Prints XML footer */
> static svn_error_t *
> +print_footer_xml (apr_pool_t *pool)
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> + /* "</status>" */
And this one.
> + svn_xml_make_close_tag (&sb, pool, "info");
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +/* Return string representation of KIND */
> +static const char *
> +kind_str (svn_node_kind_t kind)
> +{
> + switch (kind)
> + {
> + case svn_node_dir:
> + return "directory";
> + case svn_node_file:
> + return "file";
> + default:
> + return "";
> + }
> +}
This is a duplicate of a function in ls-cmd.c. Don't duplicate code, share it.
Move it into "cl.h" and "util.c", named something like "svn_cl__node_kind_str".
In its usage in "svn list --xml", I see that the corresponding XML attribute is
called "kind", so that should be its name here too.
The existing plain-text "svn info" function, print_info(), should also use this
function, unless we want an unlocalised version for XML and a localised version
for plain text ... which I suppose we do. And print_info() currently also
printfs "none" and "unknown" which is really rather silly, so we'd have to make
a decision on what it really ought to be doing if it somehow encounters those
node kinds. OK, forget about print_info() for now.
[...]
> +/* 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,
> + enum svn_xml_open_tag_style style,
This doesn't need to take an (undocumented) "style" parameter, because the only
style that makes sense is svn_xml_protect_pcdata. Having this hard-coded would
have prevented the error of passing the "self-closing" style to it later in
your patch.
> + const char *tagname,
> + const char *data)
> +{
> + if (data)
> + {
> + svn_xml_make_open_tag (sb, pool, style, tagname, NULL);
> + svn_xml_escape_cdata_cstring (sb, data, pool);
> + svn_xml_make_close_tag (sb, pool, tagname);
> + }
> +}
> +
> +
> +/* 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);
> + const char *rev_str = (SVN_IS_VALID_REVNUM(info->rev)) ?
> + apr_psprintf (pool, "%ld", info->rev) :
> + NULL;
> +
> + /* If REV_STR is NULL, assume WC is corrupt. */
> + if (!rev_str)
The only way rev_str can be null is if your conditional assignment above set it
to null, so just move that assignment after this "if" statement and make it
unconditional, to save having two different tests for the same thing. I know
that would mean the variable would not be initialised at its point of
declaration, but that's the norm in C for non-trivial initialisations.
> + return svn_error_createf (SVN_ERR_WC_CORRUPT, NULL,
> + _("'%s' has invalid revision"),
> + svn_path_local_style (target, pool));
> +
> + str_kind = kind_str (info->kind);
> +
> + /* "<entry ...>" */
[...]
> + /* "<copy-from-url> xx </copy-from-url>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "copy-from-url", info->copyfrom_url);
> +
> + if (SVN_IS_VALID_REVNUM (info->copyfrom_rev))
> + {
> + /* "<copied-from-rev> xx </copied-from-rev>" */
element wc-info: validity error : Element wc-info content does not follow the
DTD, expecting (schedule? , copy-from-url? , copy-from-rev? , checksum?), got
(schedule copy-from-url copied-from-rev checksum)
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "copied-from-rev", apr_psprintf (pool,
> + "%" APR_OFF_T_FMT, info->copyfrom_rev));
info-cmd.c:217: warning: long long int format, different type arg (arg 3)
APR_OFF_T_FMT is not the format of a revision number. "%ld" is.
> + }
> +
> + /* "<checksum> xx </checksum>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "checksum", info->checksum);
> +
> + /* "</wc-info>" */
> + svn_xml_make_close_tag (&sb, pool, "wc-info");
> + }
> +
> + /* "<last-changed>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "last-changed", NULL);
This "last-changed" element needs to be omitted if it is inapplicable (such as
for a simple schedule-add file).
> +
> + /* "<author> xx </author>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "author", info->last_changed_author);
> +
> + if (SVN_IS_VALID_REVNUM (info->last_changed_rev))
> + {
> + /* "<revision> xx </revision>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "revision", apr_psprintf (pool,
> + "%" APR_OFF_T_FMT, info->last_changed_rev));
Same again.
> + }
> +
> + /* "<date> xx </date>" */
> + make_tagged_cdata (&sb, pool, svn_xml_protect_pcdata,
> + "date", svn_time_to_cstring
> + (info->last_changed_date, pool));
(I wonder if there are any cases in which last-changed info is being printed
but last_changed_date is absent (zero), which would result in
"1970-01-01T00:00:00.000000Z" being printed.)
[...]
> + /* "<comment ...> xxxx </comment>" */
> + make_tagged_cdata (&sb, pool, svn_xml_self_closing, "comment",
A self-closing element doesn't make sense in this context, as already noted.
> + info->lock->comment);
[...]
> @@ -295,5 +573,8 @@
> }
> svn_pool_destroy (subpool);
>
> + if (opt_state->xml && (! opt_state->incremental))
> + err = print_footer_xml (pool);
That's leaking an error. You probably want "SVN_ERR" instead of "err =".
> +
> return SVN_NO_ERROR;
> }
I haven't much of an opinion on the format of the XML output. It seems OK at a
quick look.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn info --xml' - v2
Posted by Alexander Thomas <al...@collab.net>.
On Wed, 2005-06-15 at 12:17 +0100, Julian Foad wrote:
> That's two logically separate changes. It would be much better if you could
> send two separate patches.
[[[
Patch to add XML output for 'svn info'.
* subversion/clients/cmdline/main.c
(svn_cl__cmd_table[]): Added --xml and --incremental options
for svn info.
* subversion/clients/cmdline/dtd/info.dtd
New dtd for info --xml.
* subversion/clients/cmdline/info-cmd.c
(print_header_xml): New function.
(print_footer_xml): New function.
(kind_str): New function.
(schedule_str): New function.
(make_tagged_cdata): New function.
(print_info_xml): New function.
(info_receiver): Edited existing callback to print xml or
traditional output depending on cmdline option.
(svn_cl__info): Modified for XML output.
* tools/client-side/bash_completion
(_svn): Added "--incremental" and "--xml" options to "info"
for auto-completion.
]]]
Re: [PATCH] 'svn info --xml' - v2
Posted by Julian Foad <ju...@btopenworld.com>.
Alexander Thomas wrote:
> Patch to add XML output for 'svn info' and send warnings to stderr
> instead of stdout.
That's two logically separate changes. It would be much better if you could
send two separate patches.
Thanks.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org