You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bhuvaneswaran Arumugam <bh...@collab.net> on 2008/03/19 05:08:38 UTC
[PATCH] svn log url -rXXX should display proper error message
Hi,
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@REV' command, it displays an error message. Ex:
$ svn log url@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
Please find attached a patch to handle this case.
[[
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.
* 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.
* 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.
]]
Please review and let me know your comments. Thank you!
--
Regards,
Bhuvaneswaran
Re: [PATCH] svn log url -rXXX should display proper error message
Posted by Bhuvaneswaran Arumugam <bh...@collab.net>.
On Wed, 2008-03-19 at 18:03 +0530, Kamesh Jayachandran wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Bhuvan,
>
> Thanks for the patch.
>
> > +/** Return TRUE iff revision kind is dependent on the working copy.
> > + * Otherwise, return FALSE.
> > + *
> > + * @since New in 1.5.
> > + */
>
> It should be 1.6
Thank you Kamesh! Fix committed in r29948.
> > +#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)
> > +
>
>
> +1.
>
> With regards
> Kamesh Jayachandran
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFH4QgK3WHvyO0YTCwRAgRxAJ9Fx59zTQnR3LlGZuLjhMs3xk4O7ACfVqGS
> Vh7qAgP4GgbtZeh2All4x6I=
> =7qMi
> -----END PGP SIGNATURE-----
--
Regards,
Bhuvaneswaran
Re: [PATCH] svn log url -rXXX should display proper error message
Posted by Kamesh Jayachandran <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi Bhuvan,
Thanks for the patch.
> +/** Return TRUE iff revision kind is dependent on the working copy.
> + * Otherwise, return FALSE.
> + *
> + * @since New in 1.5.
> + */
It should be 1.6
> +#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)
> +
+1.
With regards
Kamesh Jayachandran
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFH4QgK3WHvyO0YTCwRAgRxAJ9Fx59zTQnR3LlGZuLjhMs3xk4O7ACfVqGS
Vh7qAgP4GgbtZeh2All4x6I=
=7qMi
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn log url -rXXX should display proper error message
Posted by Bhuvaneswaran Arumugam <bh...@collab.net>.
On Wed, 2008-03-19 at 13:44 -0400, Karl Fogel wrote:
> "Bhuvaneswaran Arumugam" <bh...@collab.net> writes:
> > --- subversion/libsvn_client/client.h (revision 29949)
> > +++ subversion/libsvn_client/client.h (working copy)
> > @@ -1059,6 +1059,19 @@
> > svn_client_ctx_t *ctx,
> > apr_pool_t *pool);
> >
> > +
> > +/** Return TRUE iff revision kind is dependent on the working copy.
> > + * Otherwise, return FALSE.
> > + *
> > + * @since New in 1.6.
> > + */
> > +#define SVN_CLIENT__REVKIND_NEEDS_WC(kind) \
> > + (((kind) == svn_opt_revision_base || \
> > + (kind) == svn_opt_revision_previous || \
> > + (kind) == svn_opt_revision_working || \
> > + (kind) == svn_opt_revision_committed) \
> > + ? TRUE : FALSE)
> > +
>
> Don't need the "@since New in 1.6." tag for internal namespaces :-).
> (Sorry, I should have pointed that out before.)
>
> Otherwise, +1.
Committed the revised fix in r29961. Thanks for your time Karl!
--
Regards,
Bhuvaneswaran
Re: [PATCH] svn log url -rXXX should display proper error message
Posted by Karl Fogel <kf...@red-bean.com>.
"Bhuvaneswaran Arumugam" <bh...@collab.net> writes:
> --- subversion/libsvn_client/client.h (revision 29949)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -1059,6 +1059,19 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> +
> +/** Return TRUE iff revision kind is dependent on the working copy.
> + * Otherwise, return FALSE.
> + *
> + * @since New in 1.6.
> + */
> +#define SVN_CLIENT__REVKIND_NEEDS_WC(kind) \
> + (((kind) == svn_opt_revision_base || \
> + (kind) == svn_opt_revision_previous || \
> + (kind) == svn_opt_revision_working || \
> + (kind) == svn_opt_revision_committed) \
> + ? TRUE : FALSE)
> +
Don't need the "@since New in 1.6." tag for internal namespaces :-).
(Sorry, I should have pointed that out before.)
Otherwise, +1.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn log url -rXXX should display proper error message
Posted by Bhuvaneswaran Arumugam <bh...@collab.net>.
On Wed, 2008-03-19 at 13:15 -0400, Karl Fogel wrote:
> "Bhuvaneswaran Arumugam" <bh...@collab.net> writes:
> > [[
> > Rename the macro which checks if revision kind is dependent on a WC. Use
> > the new macro wherever it is applicable.
> >
> > * subversion/include/svn_client.h
> > (SVN_CLIENT_IS_WC_DEPENDENT_REVKIND): Move this macro ...
> > * subversion/libsvn_client/client.h
> > (SVN_CLIENT__REVKIND_NEEDS_WC) ... to here and rename it.
> > * subversion/libsvn_client/copy.c
> > (setup_copy): Use the macro.
> > * subversion/libsvn_client/log.c
> > (svn_client_log4): Use the new macro.
> >
> > Suggested by: kfogel
> > ]]
>
> Should say it follows up to r29948.
Yep.
> > --- subversion/libsvn_client/client.h (revision 29949)
> > +++ subversion/libsvn_client/client.h (working copy)
> > @@ -1059,6 +1059,17 @@
> > svn_client_ctx_t *ctx,
> > apr_pool_t *pool);
> >
> > +
> > +/** Return TRUE iff revision kind is dependent on the working copy.
> > + * Otherwise, return FALSE.
> > + *
> > + * @since New in 1.6.
> > + */
> > +#define SVN_CLIENT__REVKIND_NEEDS_WC(kind) \
> > + ((kind == svn_opt_revision_base || kind == svn_opt_revision_previous || \
> > + kind == svn_opt_revision_working || kind == svn_opt_revision_committed) \
> > + ? TRUE : FALSE)
> > +
> >
>
> The uses of "kind" should be "(kind)", that's a super-safe way to write
> macros, for reasons you probably remember now that I'm saying it :-).
I've incorporated this change. Please find attached the revised patch.
[[
This is a follow-up for r29948. Rename the macro which checks if
revision kind is dependent on a WC. Use the new macro wherever it is
applicable.
* subversion/include/svn_client.h
(SVN_CLIENT_IS_WC_DEPENDENT_REVKIND): Move this macro ...
* subversion/libsvn_client/client.h
(SVN_CLIENT__REVKIND_NEEDS_WC) ... to here and rename it.
* subversion/libsvn_client/copy.c
(setup_copy): Use the macro.
* subversion/libsvn_client/log.c
(svn_client_log4): Use the new macro.
Suggested by: kfogel
]]
--
Regards,
Bhuvaneswaran
Re: [PATCH] svn log url -rXXX should display proper error message
Posted by Karl Fogel <kf...@red-bean.com>.
"Bhuvaneswaran Arumugam" <bh...@collab.net> writes:
> [[
> Rename the macro which checks if revision kind is dependent on a WC. Use
> the new macro wherever it is applicable.
>
> * subversion/include/svn_client.h
> (SVN_CLIENT_IS_WC_DEPENDENT_REVKIND): Move this macro ...
> * subversion/libsvn_client/client.h
> (SVN_CLIENT__REVKIND_NEEDS_WC) ... to here and rename it.
> * subversion/libsvn_client/copy.c
> (setup_copy): Use the macro.
> * subversion/libsvn_client/log.c
> (svn_client_log4): Use the new macro.
>
> Suggested by: kfogel
> ]]
Should say it follows up to r29948.
> --- subversion/libsvn_client/client.h (revision 29949)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -1059,6 +1059,17 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> +
> +/** Return TRUE iff revision kind is dependent on the working copy.
> + * Otherwise, return FALSE.
> + *
> + * @since New in 1.6.
> + */
> +#define SVN_CLIENT__REVKIND_NEEDS_WC(kind) \
> + ((kind == svn_opt_revision_base || kind == svn_opt_revision_previous || \
> + kind == svn_opt_revision_working || kind == svn_opt_revision_committed) \
> + ? TRUE : FALSE)
> +
>
The uses of "kind" should be "(kind)", that's a super-safe way to write
macros, for reasons you probably remember now that I'm saying it :-).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn log url -rXXX should display proper error message
Posted by Bhuvaneswaran Arumugam <bh...@collab.net>.
On Wed, 2008-03-19 at 11:00 -0400, Karl Fogel wrote:
> "Bhuvaneswaran Arumugam" <bh...@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@REV' command, it displays an error message. Ex:
> > $ svn log url@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@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.
Yeah, it sounds like the best way to go. I've incorporated this change
in the attached patch.
> > #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.
I do not find any other instance wherein we check for this revkind
combination: BASE, PREV, COMMITTED & WORKING.
Please find attached the revised patch for review. Thank you!
[[
Rename the macro which checks if revision kind is dependent on a WC. Use
the new macro wherever it is applicable.
* subversion/include/svn_client.h
(SVN_CLIENT_IS_WC_DEPENDENT_REVKIND): Move this macro ...
* subversion/libsvn_client/client.h
(SVN_CLIENT__REVKIND_NEEDS_WC) ... to here and rename it.
* subversion/libsvn_client/copy.c
(setup_copy): Use the macro.
* subversion/libsvn_client/log.c
(svn_client_log4): Use the new macro.
Suggested by: kfogel
]]
--
Regards,
Bhuvaneswaran
Re: [PATCH] svn log url -rXXX should display proper error message
Posted by Bhuvaneswaran Arumugam <bh...@collab.net>.
On Wed, 2008-03-19 at 11:00 -0400, Karl Fogel wrote:
> "Bhuvaneswaran Arumugam" <bh...@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@REV' command, it displays an error message. Ex:
> > $ svn log url@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@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?
Karl, thank you for the detailed review comment.
Yes, the goal is to fix the second case. I updated the log message and
incorporated your feedback related to the log message. I'll address your
feedback related to the code and submit it as a new patch.
> 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
--
Regards,
Bhuvaneswaran
Re: [PATCH] svn log url -rXXX should display proper error message
Posted by Karl Fogel <kf...@red-bean.com>.
"Bhuvaneswaran Arumugam" <bh...@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@REV' command, it displays an error message. Ex:
> $ svn log url@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@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org