Hi,
> Index: subversion/clients/cmdline/cl.h
> ===================================================================
> --- subversion/clients/cmdline/cl.h (revision 14774)
> +++ subversion/clients/cmdline/cl.h (working copy)
> @@ -254,6 +254,18 @@
> svn_boolean_t repos_locks,
> apr_pool_t *pool);
>
> +
> +/* Print STATUS for PATH in XML to stdout. Unlike svn_cl__print_status,
> + prints details only in verbose mode. Checks status against the
> + latest revision in the repository if the '-u' cmdline option is
> + specified. Like svn_cl__print_status, lists missing repository locks
> + as broken WC locks. */
Hmmm... What about:
/* Print STATUS for PATH in XML to stdout. Use POOL for temporary
allocations. */
You don't need to be more specific than so. (And, it doesn't print any
locks as broken and it (should) always print last committe info
(details)). But you don't need to mention that here, since no
arguments control this.
> +
> /* Print STATUS and PATH in a format determined by DETAILED and
> SHOW_LAST_COMMITTED. */
> static svn_error_t *
> @@ -166,6 +194,142 @@
> return SVN_NO_ERROR;
> }
>
> +
> +/* Print STATUS of PATH in XML format */
You can remove this docstring, since there is already one in the .h
file.
> +svn_error_t *
> +svn_cl__print_status_xml (const char *path,
> + svn_wc_status2_t *status,
> + apr_pool_t *pool)
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> +
> + if (status->text_status == svn_wc_status_none
> + && status->repos_text_status == svn_wc_status_none)
> + return SVN_NO_ERROR;
> +
> + /* "<entry>" */
I see you removed some such comments. I think all of them can go away,
since they don't add anything. You see, this patch starts to look
good, so I have to find some really minor things:-)
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "entry",
> + "path", svn_path_local_style (path, pool), NULL);
> +
> + /* "<wc-status ... >" */
> + apr_hash_t *att_hash = svn_xml_make_att_hash (NULL, pool);
> + apr_hash_set (att_hash, "item", APR_HASH_KEY_STRING,
> + generate_status_desc (status->text_status));
> + apr_hash_set (att_hash, "props", APR_HASH_KEY_STRING,
> + generate_status_desc (status->prop_status));
> + if (status->locked)
> + apr_hash_set (att_hash, "wc-locked", APR_HASH_KEY_STRING, "true");
> + if (status->copied)
> + apr_hash_set (att_hash, "copied", APR_HASH_KEY_STRING, "true");
> + if (status->switched)
> + apr_hash_set (att_hash, "switched", APR_HASH_KEY_STRING, "true");
> + svn_xml_make_open_tag_hash (&sb, pool, svn_xml_normal, "wc-status",
> + att_hash);
> +
> + if (status->entry && status->entry->lock_token)
> + {
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "lock", NULL);
> +
> + 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");
> +
> + /* If lock_owner is NULL, assume WC is corrupt. */
> + if (status->entry->lock_owner)
> + {
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
> + "owner", NULL);
> + svn_xml_escape_cdata_cstring (&sb, status->entry->lock_owner,
> + pool);
> + svn_xml_make_close_tag (&sb, pool, "owner");
> + }
> + else
> + return svn_error_create (SVN_ERR_WC_CORRUPT, 0, NULL);
YOu might add an error message here as well, so the user gets
something more specific than the generic error string in
svn_error_codes.h. Maybe something like
"'%s' has lock token, but no lock owner" ?
> +
> + if (status->entry->lock_comment)
> + {
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "comment", NULL);
> + svn_xml_escape_cdata_cstring (&sb, status->entry->lock_comment,
> + pool);
> + svn_xml_make_close_tag (&sb, pool, "comment");
> + }
> +
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "created",
> + NULL);
> + svn_xml_escape_cdata_cstring (&sb,
> + svn_time_to_cstring
> + (status->entry->lock_creation_date, pool),
> + pool);
> + svn_xml_make_close_tag (&sb, pool, "created");
> +
> + svn_xml_make_close_tag (&sb, pool, "lock");
> + }
> +
> + /* "</wc-status>" */
More comments that could go.
> + svn_xml_make_close_tag (&sb, pool, "wc-status");
> +
> + if (status->repos_lock)
This should be
if (status->repos_text_status != svn_wc_status_none
|| status->repos_prop_status != svn_wc_status_none
|| status->repos_lock)
You print repos_status if any of these are true.
> + {
> + /* "<repos-status>" */
I won't pick on all these comments:-)
> + 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);
> +
> + /* "<lock>" */
So when you change the above, you need a specific test for
status->repos_lock here.
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "lock", NULL);
> +
> + 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");
> +
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata, "owner", NULL);
> + svn_xml_escape_cdata_cstring (&sb, status->repos_lock->owner, pool);
> + svn_xml_make_close_tag (&sb, pool, "owner");
> +
> + if (status->repos_lock->comment)
> + {
> + /* "<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, pool);
> + svn_xml_make_close_tag (&sb, pool, "comment");
> + }
> +
> + /* "<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
> + (status->repos_lock->creation_date,
> + pool),
> + pool);
> + svn_xml_make_close_tag (&sb, pool, "created");
> +
> + if (status->repos_lock->expiration_date != 0)
> + {
> + svn_xml_make_open_tag (&sb, pool, svn_xml_protect_pcdata,
> + "expires", NULL);
> + svn_xml_escape_cdata_cstring (&sb, svn_time_to_cstring
> + (status->repos_lock->expiration_date,
> + pool),
> + pool);
> + svn_xml_make_close_tag (&sb, pool, "expires");
> + }
> +
> + /* "</lock>" */
> + svn_xml_make_close_tag (&sb, pool, "lock");
> +
> + /* "</repos-status>"*/
> + svn_xml_make_close_tag (&sb, pool, "repos-status");
> + }
> +
Hmmm... I think the last committed info could be added here as well. I
> don't know if it was in the original patch and I forgot it when I
> rewrote the DTD:-( BTW, the DTD is missing from this patch.
> + /* "</entry>" */
> + svn_xml_make_close_tag (&sb, pool, "entry");
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> /* Called by status-cmd.c */
> svn_error_t *
> svn_cl__print_status (const char *path,
> Index: subversion/clients/cmdline/status-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/status-cmd.c (revision 14774)
> +++ subversion/clients/cmdline/status-cmd.c (working copy)
> +/* Prints XML target */
> +static svn_error_t *
> +print_start_target_xml (apr_pool_t *pool, const char *target)
We normally put the pool argument last.
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> +
> + /* "<target ...>" */
> + svn_xml_make_open_tag (&sb, pool, svn_xml_normal, "target",
> + "path", target, NULL);
> +
> + return svn_cl__error_checked_fputs (sb->data, stdout);
> +}
> +
> +
> +/* Prints </target> and the revision in repository against which
> + * status was checked */
> +static svn_error_t *
> +print_finish_target_xml (apr_pool_t *pool,
> + svn_revnum_t repos_rev,
> + svn_boolean_t update)
You could get rid of the update parameter and make sure repos_rev is
always set to SVN_INVALID_REVNUM if not update.
Same about pool argument placement.
> +{
> + svn_stringbuf_t *sb = svn_stringbuf_create ("", pool);
> +
> + if (SVN_IS_VALID_REVNUM (repos_rev) && update)
> + {
> + const char *repos_rev_str;
> + repos_rev_str = apr_psprintf (pool, "%" APR_OFF_T_FMT, repos_rev);
Use %ld for revision numbers. (That has to do with gettext, which
doesn't understand such macros.)
> @@ -86,6 +159,8 @@
> int i;
> svn_opt_revision_t rev;
> struct status_baton sb;
> + svn_revnum_t repos_rev;
Just initialize this to SVN_INVALID_REVNUM. (See above.)
> + svn_error_t *err;
>
> SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
> opt_state->targets, pool));
> @@ -93,9 +168,10 @@
> /* We want our -u statuses to be against HEAD. */
> rev.kind = svn_opt_revision_head;
>
> - /* The notification callback. */
> - svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2, FALSE, FALSE,
> - FALSE, pool);
> + /* The notification callback, set the notifier to NULL in XML mode */
To be pedantic, we don't *set* it to NULL here... "leave it as
NULL..." or something.
> + if (! opt_state->xml)
> + svn_cl__get_notifier (&ctx->notify_func2, &ctx->notify_baton2, FALSE,
> + FALSE, FALSE, pool);
>
> /* Add "." if user passed 0 arguments */
> svn_opt_push_implicit_dot_target(targets, pool);
> @@ -104,6 +180,23 @@
>
> sb.had_print_error = FALSE;
>
> + if (opt_state->xml)
> + {
> + /* If output is not incremental, output the XML header and wrap
> + everything in a top-level element. This makes the output in
> + its entirety a well-formed XML document. */
> + if (! opt_state->incremental)
> + SVN_ERR (print_header_xml (pool));
> +
> + }
> + else
> + {
> + if (opt_state->incremental)
> + return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("'incremental' option only valid in XML "
> + "mode"));
> + }
> +
> for (i = 0; i < targets->nelts; i++)
> {
> const char *target = ((const char **) (targets->elts))[i];
> @@ -115,21 +208,34 @@
> /* Retrieve a hash of status structures with the information
> requested by the user. */
>
> - sb.detailed = (opt_state->verbose || opt_state->update);
> - sb.show_last_committed = opt_state->verbose;
> + sb.detailed = (opt_state->verbose || opt_state->update
> + || opt_state->xml);
> + sb.show_last_committed = (opt_state->verbose || opt_state->xml);
This change isn't necessary anymore, since svn_cl__print_status_xml
doesn't take these as arguments.
> sb.skip_unrecognized = opt_state->quiet;
> sb.repos_locks = opt_state->update;
> + sb.xml_mode = opt_state->xml;
> sb.pool = subpool;
> - SVN_ERR (svn_client_status2 (NULL, target, &rev, print_status, &sb,
> - opt_state->nonrecursive ? FALSE : TRUE,
> - opt_state->verbose,
> - opt_state->update,
> - opt_state->no_ignore,
> - opt_state->ignore_externals,
> - ctx, subpool));
> +
> + if (opt_state->xml)
> + print_start_target_xml (pool, svn_path_local_style (target, pool));
> +
YOu leak the returned error here.
> + err = svn_client_status2 (&repos_rev, target, &rev, print_status, &sb,
> + opt_state->nonrecursive ? FALSE : TRUE,
> + (opt_state->verbose || opt_state->xml),
I don't think XML mode should imply --verbose *in this case*. I think
--verbose makes sense for XML, since. Otherwise status will output
lots of useless entries for unmodified paths. I normally argue that
we should print all available information in XML and that the
receiving application can filter it. I'm not consistent, I know, but
this would be extreme:-)
> + opt_state->update,
> + opt_state->no_ignore,
> + opt_state->ignore_externals,
> + ctx, subpool);
> + if (opt_state->xml)
> + err = print_finish_target_xml (pool, repos_rev, opt_state->update);
> +
And here you leak an error as well.
> + if (err)
> + break;
> }
>
> svn_pool_destroy (subpool);
> -
There is no point in trying to make the XML complete on error. You
can't guarantee that it is correct anyway, since you don't know where
svn_client_status2() fails. We have to live with the fact that if the
cmdline client returns non-zero and prints an error message, the
output might be incomplete.
> - return SVN_NO_ERROR;
> + if (opt_state->xml && (! opt_state->incremental))
> + err = print_footer_xml (pool);
> +
So just SVN_ERR here as well as above.
> + return err;
> }
I think this patch starts to look really god now. Except for the error
leaks, I think the other things are more or less details.
Please have the patience to provide another version with these
fixes:-) And don't forget the DTD.
Thanks,
//Peter
On Mon, 30 May 2005, Alexander Thomas wrote:
> Version 5 of the patch for svn status --xml is attached.
>
> [[[
> Patch to fix issue 2069 - "svn status" in xml mode
>
> * subversion/clients/cmdline/cl.h
> (svn_cl__print_status_xml): Added new function prototype.
>
> * subversion/clients/cmdline/status.c
> (generate_status_desc): New function.
> (svn_cl__print_status_xml): New function to handle xml output.
>
> * subversion/clients/cmdline/main.c
> (svn_cl__cmd_table): Added --incremental and --xml options to
> subcommand array.
>
> * subversion/clients/cmdline/status-cmd.c
> (struct status_baton): Added new boolean field.
> (print_start_target_xml): New function.
> (print_header_xml): New function.
> (print_footer_xml): New function.
> (print_finish_target_xml): New function.
> (print_status): Edited to check --xml option.
>
> * subversion/clients/cmdline/dtd/status.dtd
> New file.
>
> * subversion/tests/clients/cmdline/stat_tests.py
> (status_in_xml): New test.
> (test_list): Added status_in_xml.
>
> * tools/client-side/bash_completion
> (_svn): Add "--incremental" and "--xml" options to "status"
> for auto-completion.
> ]]]
>
> -Alexander Thomas (AT)
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon May 30 23:33:01 2005