Re: [PATCH] svn log url -rXXX should display proper error message

On Wed, 2008-03-19 at 11:00 -0400, Karl Fogel wrote:
> "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?

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