On Wed, 2009-11-25, Kannan wrote: > Log: > Include COMMITTED and PREVIOUS also as invalid revision keywords for a > url diff. > > * subversion/libsvn_client/diff.c > (svn_client_diff_peg5): Check also for the COMMITTED and PREVIOUS > revision keywords to error out as invalid diff for a url and also make > error message compliant with that of the one in `svn merge'. Refer: > > http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=2417716 > > Patch by: Kannan R <[email protected]>
Hi Kannan. That looks good, but I noticed that exactly the same check should also apply to svn_client_diff_summarize_peg2() as well. Based on your patch, in r884576 I added the checks in the check_path() function, as both the non-summarizing and the summarizing diffs use it. [[[ Make "diff" fail gracefully if a revision kind COMMITTED or PREVIOUS or BASE is used with a URL. Previously only BASE was handled gracefully, and only in the non-summarizing diff; any other case resulted in a cryptic error message. Also make the error message the same as the one in "merge" (see: <http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=2417716>). Suggested by: Kannan R <kannanr{_AT_}collab.net> * subversion/libsvn_client/diff.c (check_paths): If a URL is used with a revision kind that requires a WC, throw an error with an appropriate message. (svn_client_diff_peg5): Remove the check for the revision kind BASE, as it is now included in check_paths(). Index: subversion/libsvn_client/diff.c =================================================================== --- subversion/libsvn_client/diff.c (revision 884570) +++ subversion/libsvn_client/diff.c (working copy) @@ -959,6 +959,14 @@ check_paths(const struct diff_parameters return svn_error_create(SVN_ERR_CLIENT_BAD_REVISION, NULL, _("Not all required revisions are specified")); + if ((svn_path_is_url(params->path1) + && SVN_CLIENT__REVKIND_NEEDS_WC(params->revision1->kind)) + || (svn_path_is_url(params->path2) + && SVN_CLIENT__REVKIND_NEEDS_WC(params->revision2->kind))) + return svn_error_create( + SVN_ERR_CLIENT_BAD_REVISION, NULL, + _("PREV, BASE, or COMMITTED revision keywords are invalid for URL")); + /* Revisions can be said to be local or remote. BASE and WORKING, for example, are local. */ is_local_rev1 = @@ -1762,13 +1770,6 @@ svn_client_diff_peg5(const apr_array_hea struct diff_cmd_baton diff_cmd_baton; svn_wc_diff_callbacks4_t diff_callbacks; - if (svn_path_is_url(path) && - (start_revision->kind == svn_opt_revision_base - || end_revision->kind == svn_opt_revision_base) ) - return svn_error_create(SVN_ERR_CLIENT_BAD_REVISION, NULL, - _("Revision type requires a working copy " - "path, not a URL")); - /* fill diff_param */ diff_params.path1 = path; diff_params.revision1 = start_revision; ]]] - Julian
