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/08/01 14:01:14 UTC
Re: [PATCH] 'svn info --xml' - v2
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' - 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