On Fri, 20 May 2005 alexander@collab.net wrote:
> [[[
> Version 4: Patch to fix issue 2069 - "svn status" in xml mode
>
"Version 4" doesn't belong in the log message.
> * subversion/clients/cmdline/cl.h
> (svn_cl__print_status_xml): added prototype for new function
Use initial caps and end sentences with a period.
> (svn_cl__get_notifier2) : added prototype for new wrapper function
>
No space before colon.
> * subversion/clients/cmdline/status.c
> (generate_status_desc): new function for getting detailed string
> representation of status
You shouldn't describe the purpose of a new function in the log. That's
documented in the code.
> (print_status_xml): new function to print status in XML format
> to stdout
> (svn_cl__print_status_xml): new function handling xml output and calls
> print_status_xml function
>
> * subversion/clients/cmdline/notify.c
> (struct notify_baton): added new element in notify baton
> (notify): restricted printing the 'Status against revision' in xml mode
> (svn_cl__get_notifier2): new wrapper function for selecting xml output,
>
> * subversion/clients/cmdline/main.c
> (svn_cl__cmd_table[]): added --incremental and --xml options in to
"in to" -> "to". Remove the brackets.
> subcommand array for status subcommand
>
> * subversion/clients/cmdline/status-cmd.c
> (struct status_baton): added new item 'xml_mode' in struct
> for storing xml mode is requested or not
> (print_header_xml): prints xml header
> (print_footer_xml): prints xml footer
> (print_against_xml): prints status against version details in xml
The three above functions are no. Please say so in the log.
> (print_status): checks xml option and calls xml handling routine,
>
> * subversion/clients/cmdline/dtd/status.dtd
> added a new dtd file for validating status xml
>
"New file." is enough here.
> * subversion/tests/clients/cmdline/stat_tests.py
> (status_in_xml): new function to verify success when
> svn status output in xml format
> (test_list): added status_in_xml
>
> * tools/client-side/bash_completion
> (_svn): add "--incremental" and "--xml" options to "status"
> for auto-completion.
> ]]]
>
HACKING contains more guidelines and examples for writing log messages.
Now to the code. I think it is good except for a few details.
> Index: subversion/clients/cmdline/cl.h
> ===================================================================
> --- subversion/clients/cmdline/cl.h (revision 14774)
> +++ subversion/clients/cmdline/cl.h (working copy)
> @@ -254,6 +254,27 @@
> svn_boolean_t repos_locks,
> apr_pool_t *pool);
>
> +
> +/* Print STATUS for PATH in XML to stdout for human consumption. Prints in
Human consumption? Nah, that can go.
> + abbreviated format by default, or DETAILED format if flag is set.
What's abbreviated format for XML? (See below.)
> +
> + When DETAILED is set, use SHOW_LAST_COMMITTED to toggle display of
> + the last-committed-revision and last-committed-author.
> +
Since the information is available, I think you should always print it
in the XML.
> + If SKIP_UNRECOGNIZED is TRUE, this function will not print out
> + unversioned items found in the working copy.
> +
> + When DETAILED is set, and REPOS_LOCKS is set, treat missing repository locks
> + as broken WC locks. */
Not true. This docstring needs updating.
> +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.
> +
> /* Print a hash that maps property names (char *) to property values
> (svn_string_t *). The names are assumed to be in UTF-8 format;
> the values are either in UTF-8 (the special Subversion props) or
> @@ -386,7 +407,34 @@
> *
> * If don't want a summary line at the end of notifications, set
> * SUPPRESS_FINAL_LINE.
> + *
> + * If ouput need in XML format, set IN_XML to true
> */
> +void svn_cl__get_notifier2 (svn_wc_notify_func2_t *notify_func_p,
> + void **notify_baton_p,
> + svn_boolean_t is_checkout,
> + svn_boolean_t is_export,
> + svn_boolean_t suppress_final_line,
> + svn_boolean_t in_xml,
> + apr_pool_t *pool);
> +
> +
We don't revise internal functions. (That it is internal is seen by
the __ in the name.) Also, I think it is better to set the notifier to
NULL when in XML mode. You don't want stdout messed up by any
notifications.
> 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.
>
>
> + }
> +}
> +
> +
> +/* 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.
> +
> + /* "<wc-status...>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "wc-status",
> + "item", generate_status_desc (status->text_status),
> + "props", generate_status_desc (status->prop_status),
> + "wc-locked", status->locked ? "true" : "false",
> + "copied", status->copied ? "true" : "false",
> + "switched", status->switched ? "true" : "false",
> + NULL);
> +
Since it common for copied, switched and wc-locked to be false, I
think you should use the default values declared in the DTD and avoid
setting these attributes if they're false. XML is verbose, but we
don't need tomake it unreasonably so:-) Hint: Use
svn_xml_make_open_tag_hash.
> +
> + if (status->locked && status->entry)
You're confusing (and rightly so) WC adm area locks with file locks in
the repository. This test should be:
if (status->entry && status->entry->lock_token)
> + {
> + /* "<lock>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "lock", NULL);
> +
> + /* "<token>xx</token> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "token", NULL);
> + svn_xml_escape_cdata_cstring (&sb, status->entry->lock_token, pool);
> + svn_xml_make_close_tag (&sb, pool, "token");
> +
> + /* "<owner>xx</owner> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "owner", NULL);
> + svn_xml_escape_cdata_cstring (&sb, status->entry->lock_owner ?
> + status->entry->lock_owner : "",
> + pool);
It is correct to not assume that lock_owner is non-NULL, since the
entry reading functions doesn't guarantee that. I think you should
return an error in that case instead of making owner empty. It's a
corrupt WC.
> + svn_xml_make_close_tag (&sb, pool, "owner");
> +
> + /* "<comment>xx</comment> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "comment", NULL);
> + svn_xml_escape_cdata_cstring (&sb, status->entry->lock_comment ?
> + status->entry->lock_comment : "",
> + pool);
> + svn_xml_make_close_tag (&sb, pool, "comment");
> +
Since comment is optional, don't an an element if it is NULL.
> + /* "<created>xx</created> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "created", NULL);
> + svn_xml_escape_cdata_cstring (&sb, svn_time_to_cstring (
The last paren should be first on the next line.
> + status->entry->lock_creation_date,
> + pool), pool);
> + svn_xml_make_close_tag (&sb, pool, "created");
> +
> + /* "</lock>" */
> + svn_xml_make_close_tag (&sb, pool, "lock");
> + }
> +
> + /* "</wc-status>" */
> + svn_xml_make_close_tag (&sb, pool, "wc-status");
> +
> + /* "<repos-status>" */
This function needs a flag repos_status, indicating thether -u was
used. There is no need to generate an repo-status element if -u wasn't
used.
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "repos-status",
> + "item",
> + generate_status_desc (status->repos_text_status),
> + "props",
> + generate_status_desc (status->repos_prop_status),
> + NULL);
> + if (status->repos_lock && repos_locks)
> + {
> + /* "<lock>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "lock", NULL);
> + /* "<token>xx</token> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "token", NULL);
> + svn_xml_escape_cdata_cstring (&sb, status->repos_lock->token, pool);
> + svn_xml_make_close_tag (&sb, pool, "token");
> +
> + /* "<owner>xx</owner> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "owner", NULL);
> + svn_xml_escape_cdata_cstring (&sb, status->repos_lock->owner ?
> + status->repos_lock->owner : "",
Bad indentation. And here, owner is guaranteed to be non-NULL. So,
don't check it. If it is null, that's an API violation. Confusing?
Maybe, but libsvn_wc works a little differently from this.
> + pool);
> + svn_xml_make_close_tag (&sb, pool, "owner");
> +
> + /* "<comment>xx</comment> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "comment", NULL);
> + svn_xml_escape_cdata_cstring (&sb, status->repos_lock->comment ?
> + status->repos_lock->comment : ""
Align with the &.
> + , pool);
Comma should be on previous line.
> + svn_xml_make_close_tag (&sb, pool, "comment");
> +
Make no comment element if the comment is null.
> + /* "<created>xx</created> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "created", NULL);
Quoting and diff might have its impact, but this line looks longer
than 80 chars to me.
> + svn_xml_escape_cdata_cstring (&sb, svn_time_to_cstring (
Misplaced paren.
> + status->repos_lock->creation_date,
> + pool), pool);
> + svn_xml_make_close_tag (&sb, pool, "created");
> +
> + if (status->repos_lock->expiration_date != 0)
> + {
> + /* "<expires>xx</expires> */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "expires", NULL);
Long line.
> + svn_xml_escape_cdata_cstring (&sb, svn_time_to_cstring (
Paren.
> + status->repos_lock->expiration_date,
> + pool), pool);
> + svn_xml_make_close_tag (&sb, pool, "expires");
> + }
> +
> + /* "</lock>" */
> + svn_xml_make_close_tag (&sb, pool, "lock");
> + }
> +
> + /* "</wc-status>" */
Comment/code mismatch. Maybe you hsould get rid of all those comments
that don't add anything.
> + svn_xml_make_close_tag (&sb, pool, "repos-status");
> + /* "</entry>" */
> + svn_xml_make_close_tag (&sb, pool, "entry");
> +
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +
> /* Print STATUS and PATH in a format determined by DETAILED and
> SHOW_LAST_COMMITTED. */
> static svn_error_t *
> @@ -166,8 +317,30 @@
> return SVN_NO_ERROR;
> }
>
> +
> /* Called by status-cmd.c */
> 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)
> +{
> + if (! status
When can status be null? (It seems like you just followed an existing
pattern, but I still wonder.)
> + || (skip_unrecognized && ! status->entry)
> + || (status->text_status == svn_wc_status_none
> + && status->repos_text_status == svn_wc_status_none))
> + return SVN_NO_ERROR;
> +
> + return print_status_xml (svn_path_local_style (path, pool),
> + detailed, show_last_committed,
> + repos_locks, status, pool);
> +}
I find this tiny wrapper more confusing than good. I think you should
have everything in one function.
> void
> +svn_cl__get_notifier2 (svn_wc_notify_func2_t *notify_func_p,
> + void **notify_baton_p,
> + svn_boolean_t is_checkout,
> + svn_boolean_t is_export,
> + svn_boolean_t suppress_final_line,
> + svn_boolean_t in_xml,
> + apr_pool_t *pool)
> +{
> + svn_cl__get_notifier (notify_func_p, notify_baton_p,
> + is_checkout, is_export,
> + suppress_final_line,
> + pool);
> + if (notify_baton_p)
> + ((struct notify_baton*)*notify_baton_p)->in_xml = in_xml;
I'm glad this function is going away! Avoid such casts when not
absolutely necessary!
> +/* Prints XML target */
> +static svn_error_t *
> +print_target_xml (apr_pool_t *pool, const char *target)
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> +
> + /* "<target ...>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "target",
> + "path", target, NULL);
svn_path_local_style.
> +
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
Missing docstring.
> +static svn_error_t *
> +print_against_xml (apr_pool_t *pool,
> + svn_revnum_t result_rev,
> + svn_boolean_t update)
Bad indentation.
Maybe this function should be named finish_target_xml, since it does
more than print the against element.
> @@ -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.
> + err = (svn_client_status2 (&result_rev, target, &rev, print_status, &sb,
> + opt_state->nonrecursive ? FALSE : TRUE,
> + (opt_state->verbose || opt_state->xml),
> + opt_state->update,
> + opt_state->no_ignore,
> + opt_state->ignore_externals,
> + ctx, subpool));
Now I'm getting really picky, but you shouldn't have parens around the
function call.
> + if (opt_state->xml)
> + err = print_against_xml (pool, result_rev,
> + opt_state->update);
Indentation.
> + if (err) break;
break should be on its own line.
> }
>
> Index: subversion/clients/cmdline/dtd/status.dtd
> ===================================================================
> --- subversion/clients/cmdline/dtd/status.dtd (revision 0)
> +++ subversion/clients/cmdline/dtd/status.dtd (revision 0)
> @@ -0,0 +1,44 @@
Missing comment describing the purpose of the file.
> +<!ENTITY % BOOL '(true | false) "false"'>
> +
> +<!ELEMENT status (target*)>
> +
> +<!ELEMENT target (entry*, against?)>
> +<!-- target path -->
> +<!ATTLIST target
> + path CDATA #REQUIRED>
> +
> +<!ELEMENT entry (wc-status, repos-status?, wc-lock?, repos-lock?)>
The last two elemets are not used. (And yes, I know that I wrote that,
but only as a quikc example:-)
> +<!ATTLIST entry
> + path CDATA #REQUIRED>
> +
> +<!ELEMENT wc-status (lock?)>
> +<!-- WC dir locked? -->
> +<!-- Add with history? -->
> +<!-- Item switched -->
> +<!ATTLIST wc-status
> + item (added | conflicted | deleted | merged | ignored | modified |
> + replaced | external | unversioned | incomplete | obstructed |
> + normal | none) "none"
> + props (conflicted | modified | normal | none) "none"
IN retrospect, I think the default values here are useless. Make item
and props required instead.
> + wc-locked %BOOL;
> + copied %BOOL;
> + switched %BOOL;
> +>
> +
> +<!ELEMENT repos-status (lock?)>
> +<!ATTLIST repos-status
> + item (added | deleted | modified | replaced | none) #IMPLIED
> + props (modified | none) #IMPLIED
> +>
#IMPLIED -> #REQUIRED.
> +
> +<!-- Lock info stored in WC or repos. -->
> +<!ELEMENT lock (token, owner, comment?, created, expires?)>
> +
> +<!ELEMENT token (#PCDATA)> <!-- Lock token URI. -->
> +<!ELEMENT owner (#PCDATA)> <!-- Lock owner. -->
> +<!ELEMENT comment (#PCDATA)> <!-- Lock Comment. -->
> +<!ELEMENT created (#PCDATA)> <!-- Creation date in ISO format. -->
> +<!ELEMENT expires (#PCDATA)> <!-- Expiration date in ISO format. -->
> +
> +<!ELEMENT against EMPTY>
> +<!ATTLIST against revision CDATA #REQUIRED>
> Index: subversion/tests/clients/cmdline/stat_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/stat_tests.py (revision 14774)
> +++ subversion/tests/clients/cmdline/stat_tests.py (working copy)
> @@ -817,6 +817,76 @@
> svntest.actions.run_and_verify_status(wc_dir, expected_status)
>
>
> +def status_in_xml(sbox):
> + "status output in XML format"
> +
> + output, error = svntest.actions.run_and_verify_svn (None, None, [],
> + 'help', 'status')
> +
> + # Checks for --xml option in status help
> + output_omsg = "--xml"
> + output_dmsg = ": output in XML"
> +
> + for line in output:
> + if line.find (output_omsg) > 0 and \
> + line.find (output_dmsg) > 0:
> + break
> + else:
> + print "'--xml' option not found in: [svn help status]"
> + raise svntest.Failure
> +
> + # Checks for --incremental option in status help
> + output_omsg = "--incremental"
> + output_dmsg = ": give output suitable for concatenation"
> +
> + for line in output:
> + if line.find (output_omsg) > 0 and \
> + line.find (output_dmsg) > 0:
> + break
> + else:
> + print "'--incremental' option not found in: [svn help status]"
> + raise svntest.Failure
> +
You had a test like this in the blame patch, and I removed it. I don't
think we need to test this. Do we have such test elsewhere (except for
getopts_test)?
> + # Checks wheather output of the cmd 'status --xml' is in XML format
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon May 23 22:30:17 2005