"Bhuvaneswaran Arumugam" <bhuvan_at_collab.net> writes:
> I found this issue while I was investigating the effort involved in
> handling multiple working copy targets for 'svn log' command, issue
> 1550. Right now, if we pass working copy dependent revision kind using
> 'svn log url_at_REV' command, it displays an error message. Ex:
> $ svn log url_at_BASE
> svn: Revision type requires a working copy path, not a URL
>
> But, if we pass using 'svn log -rREV url' command, it errors out. Ex:
> $ svn log url -rBASE
> svn: 'url' is not a working copy
> svn: Can't open file 'url/.svn/entries': No such file or directory
I see the problem, but your description of it confused me a bit :-).
In both cases, we display an error message. In the first case
(url_at_BASE), the error is caught by an explicit check for this case, and
the error message is pretty clear. In the second case, (-rBASE url),
the error is caught by a generic internal check, and the error message
is more confusing. The goal is to fix the second case, right?
So your patch adds another explicit check, and gives a better error
message. Some comments:
> [[
> If working copy dependent revision kind is used for 'svn log url -rXXX'
> command, display an error message. Add svn_opt_revision_working to list
> of revision kinds we should check.
The first sentence of this should say "display a better error message"
or something. After all, the code *already* displays an error message,
before your patch.
> * subversion/include/svn_client.h
> (SVN_CLIENT_IS_WC_DEPENDENT_REVKIND): New macro to check if revision
> kind is dependent on working copy. If yes, return TRUE; otherwise,
> return FALSE.
You don't need the second sentence there -- that goes in the doc string
for the macro, in the code. Really, you can just write "New macro." and
leave it at that, since it's obvious what it does from its name.
> * subversion/libsvn_client/log.c
> (svn_client_log4): Use the new macro. Even if start/end revision kind
> is dependent on working copy, display an error message.
> ]]
No need for the word "Even" there; there's nothing unexpected about that
part of the change.
> --- subversion/include/svn_client.h (revision 29943)
> +++ subversion/include/svn_client.h (working copy)
> @@ -4303,6 +4303,16 @@
>
> /** @} */
>
> +/** Return TRUE iff revision kind is dependent on the working copy.
> + * Otherwise, return FALSE.
> + *
> + * @since New in 1.5.
> + */
> +#define SVN_CLIENT_IS_WC_DEPENDENT_REVKIND(kind) \
> + ((kind == svn_opt_revision_base || kind == svn_opt_revision_previous || \
> + kind == svn_opt_revision_working || kind == svn_opt_revision_committed) \
> + ? TRUE : FALSE)
> +
Suggest "SVN_CLIENT_REVKIND_IS_WC_DEPENDENT" for stylistic consistency
with the rest of the code. Or maybe "SVN_CLIENT_REVKIND_NEEDS_WC",
because shorter.
Are you sure this macro needs to be public, anyway? How about
SVN_CLIENT__REVKIND_NEEDS_WC in subversion/libsvn_client/client.h?
Remember, we can always make it public later, but we can't unpublish it
once we've published it.
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/libsvn_client/log.c
> ===================================================================
> --- subversion/libsvn_client/log.c (revision 29943)
> +++ subversion/libsvn_client/log.c (working copy)
> @@ -326,9 +326,10 @@
> /* Use the passed URL, if there is one. */
> if (svn_path_is_url(url_or_path))
> {
> - if (peg_revision->kind == svn_opt_revision_base
> - || peg_revision->kind == svn_opt_revision_committed
> - || peg_revision->kind == svn_opt_revision_previous)
> + if (SVN_CLIENT_IS_WC_DEPENDENT_REVKIND(peg_revision->kind) ||
> + SVN_CLIENT_IS_WC_DEPENDENT_REVKIND(start->kind) ||
> + SVN_CLIENT_IS_WC_DEPENDENT_REVKIND(end->kind))
> +
> return svn_error_create
> (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> _("Revision type requires a working copy path, not a URL"));
> @@ -432,10 +433,7 @@
> /* If this is a revision type that requires access to the working copy,
> * we use our initial target path to figure out where to root the RA
> * session, otherwise we use our URL. */
> - if (peg_revision->kind == svn_opt_revision_base
> - || peg_revision->kind == svn_opt_revision_committed
> - || peg_revision->kind == svn_opt_revision_previous
> - || peg_revision->kind == svn_opt_revision_working)
> + if (SVN_CLIENT_IS_WC_DEPENDENT_REVKIND(peg_revision->kind))
> SVN_ERR(svn_path_condense_targets(&ra_target, NULL, targets, TRUE, pool));
> else
> ra_target = url_or_path;
Good, but you should also change the place in
subversion/libsvn_client/copy.c:setup_copy()
where the "Revision type requires a working copy path, not a URL" error
is generated. It might be good to search for "svn_opt_revision_base"
throughout the code, too, just to make sure there are no other places
that should use the macro.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-19 16:00:36 CET