Fabien COELHO wrote:
>
> This patch is a follow-up to a previous submission which dealt with
> moving 'svnversion' as a 'svn' subcommand, see:
>
> http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=107766
>
> This submission does NOT do that, but just moves the functionnality in
> libsvn_wc as a prerequisite to such a change. Anyway, it seems more
> interesting to have the function in a library than as a special command
> only.
OK, basically I like this.
> [[[
> - move 'svnversion' functionnality as a function in libsvn_wc,
> so that it can be used by other client applications,
> eg possibly 'svn' later.
> - there has been a little rewrite of the function so that is just
> uses in libsvn_wc, although the initial implementation was
> based on libsvn_client.
> - 'svnversion' calls the new function, and does not use 'libsvn_client'
> anymore, so the dependency is removed from 'build.conf'.
>
> * subversion/include/svn_wc.h:
> added new structs and function.
> * subversion/libsvn_wc/revision_status.c:
> new file which implements 'svn_wc_revision_status' function.
> * subversion/svnversion/main.c:
> call new function instead of doing the job.
> * build.conf:
> fix dependencies of svnversion.
In this part of the log message, we list each top-level symbol (function,
typedef name, etc.) in parentheses before its change description for, something
like this:
* subversion/include/svn_wc.h
(svn_wc_revision_status_t): New structure.
(svn_wc_revision_status): New function.
* subversion/libsvn_wc/revision_status.c
New file, implementing svn_wc_revision_status().
* subversion/svnversion/main.c
(status_baton, analyze_status): Remove.
(version): Don't require "svn_client".
(main): Call svn_wc_revision_status() instead of doing the job.
* build.conf
Remove svn_client from the dependencies of svnversion.
I can do this for you (oh look - I have!), I'm just letting you know so you can
do that next time to make it easier for me.
> ]]]
>
>
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 17390)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -3299,6 +3299,45 @@
> +/** This structure reports the mix of revisions found within
> + * a working copy, including whether some parts are switched
> + * or currently modified. If no working copy is found, the
> + * directory is assumed to have been 'exported'. In the later
> + * case, other fieds do not contain any useful values.
> + */
> +typedef struct svn_wc_revision_status_t
> +{
> + svn_boolean_t exported; /* not a wc, other fields are not relevant */
I recommend not having this field. Instead, return an error if the supposed WC
is not actually a WC. The caller can report a non-WC as "exported" if it wants
(and the "svnversion" program does want to do that).
> +
> + svn_revnum_t min_rev; /* lowest revision found. */
> + svn_revnum_t max_rev; /* highest revision found. */
> +
> + /* is anything ... */
> + svn_boolean_t switched;
> + svn_boolean_t touched; /* any type of modification... */
Keep this field's old name ("modified") for the moment. (I saw that the name
"touched" is from your patch to extend the amount of detail reported.)
> +}
> +svn_wc_revision_status_t;
> +
> +/** Compute the revision summary status in the @a result structure
> + * for a working copy at @a wc_path. The summary may address the
> + * last changed vs the current revisions if @a committed. Default
> + * and svn:ignore property ignores are disregarded if @a no_ignore.
> + * Configuration @a config is passed downwards.
This needs to mention @a trail_url.
> + *
> + * @since New in 1.4
> + */
> +svn_error_t *
> +svn_wc_revision_status (const char *wc_path,
> + const char *trail_url,
> + svn_boolean_t committed,
> + svn_boolean_t no_ignore,
> + svn_wc_revision_status_t *result,
> + apr_hash_t *config,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
I see that no_ignore, config, and the cancellation feature are not used in this
patch. I'm not sure whether these should be provided. I suppose I can see why
no_ignore and the cancellation are likely to be wanted. Why might "config" be
wanted? Ah, because it defines the global ignores?
It would be good to move "result" to the beginning of the argument list, partly
because we generally put output arguments first when convenient. Then
"no_ignore" and "config" would be next to each other. (I know "config" could
control other things as well as ignores.)
> + apr_pool_t *pool);
> +
> Index: subversion/libsvn_wc/revision_status.c
> ===================================================================
[...]
> +svn_error_t *
> +svn_wc_revision_status (const char *wc_path,
> + const char *trail_url,
> + svn_boolean_t committed,
> + svn_boolean_t no_ignore,
> + svn_wc_revision_status_t *result,
> + apr_hash_t *config,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool)
> {
[...]
> + SVN_ERR (svn_utf_cstring_to_utf8(&wc_path_utf8, wc_path, subpool));
No: all text going through Subversion APIs should be in UTF-8 (with a few
exceptions). The UTF-8 conversion, if needed, should be done by the caller.
> Index: subversion/svnversion/main.c
> ===================================================================
[...]
>
> - SVN_INT_ERR (svn_utf_cstring_to_utf8 (&wc_path,
> - (os->ind == argc) ? "." : os->argv[os->ind++],
> - pool));
> - wc_path = svn_path_internal_style (wc_path, pool);
> - SVN_INT_ERR (svn_wc_check_wc (wc_path, &wc_format, pool));
> - if (! wc_format)
> + SVN_INT_ERR (svn_wc_revision_status
> + ((os->ind == argc) ? "." : os->argv[os->ind],
> + (os->ind < argc)? os->argv[os->ind+1]: NULL,
That line looks wrong: indexing beyond the end of the array.
> + committed, FALSE, &res, NULL, NULL, NULL, pool));
I have not reviewed this thoroughly, but this is what I have found so far.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 21 00:50:23 2005