Hi Philip,
Summary
To save everyone time, I'm going to treat your suggestions as
non-negotiable. This time, I'm only dealing with the fact that
svn_client_log was exitting silently. The patch enclosed below
fixes that. Comments inline.
On 26 Jul 2002, Philip Martin wrote:
> > + if (start_revnum > head_revnum && end_revnum <= head_revnum)
> > + {
> > + nonfatal_err = svn_error_createf(SVN_ERR_BAD_REVISION, 0, NULL,
>
> I wonder if this should be SVN_ERR_CLIENT_BAD_REVISION?
Fixed
>
> I don't think this fatal/non-fatal stuff should be here, not all
> applications will want it. svn_client_log should simply ensure that
> the revisions are valid and if not should return an error. Allowing
> "sloppy" parameters doesn't really make for a good interface.
>
Fixed
> The place to put the fatal/non-fatal stuff is in the application. So
> svn_cl__log, in log-cmd.c, could test for SVN_ERR_CLIENT_BAD_REVISION,
> print a warning, and substitute the higher numerical revision with
> HEAD and retry the operation.
>
Not done: thought I'd get the svn_client_log stuff done first.
> Whatever policy is used, the documentation for svn_client_log in
> svn_client.h should be updated.
>
The policy I've adopted is:
* if either rev is greater than head or negative, return the error,
along with a message which identifies the case of the problem
* if both are greater or negative, return the error and the message for
the "too large" error.
* if they're equal, return the error. I'm wondering if we could use an
error called SVN_ERR_CLIENT_INVALID_REV_RANGE here?
<SNIP>
> > + if (start_revnum > head_revnum && end_revnum > head_revnum)
> > + {
> > + err = svn_error_createf(SVN_ERR_BAD_REVISION, 0, NULL, pool,
> > + "Revision numbers too high: %d, %d. Head
> > is %d. Exitting",
> > + (int)start_revnum, (int)end_revnum,
> > (int)head_revnum);
>
> Revision numbers are not ints. Drop the casts and use something like
>
> "... too high: %" SVN_REVNUM_T_FMT " %" SVN_REVNUM_T_FMT ". Head is..."
>
Done.
<SNIP>
> > + if (start_revnum == end_revnum)
> > + {
> > + err = svn_error_createf(SVN_ERR_BAD_REVISION, 0, NULL, pool,
> > + "Revision numbers cannot match: %d, %d.
> > Exitting",
> > + (int)start_revnum, (int)end_revnum);
> > + return err;
> > + }
>
> Again this looks like application stuff to me. The application can
> trap this before calling svn_client_log. Personally I am quite happy
> with the single log message.
>
Done, although see question about ranges above. Thanks for the
comments.
Third cut at the patch below :)
Regards
Kieran
Index: subversion/libsvn_client/log.c
===================================================================
--- subversion/libsvn_client/log.c
+++ subversion/libsvn_client/log.c Sat Jul 27 00:53:27 2002
@@ -62,9 +62,11 @@
const char *URL;
const char *base_name = NULL;
apr_array_header_t *condensed_targets;
- svn_revnum_t start_revnum, end_revnum;
+ svn_revnum_t start_revnum, end_revnum, head_revnum;
svn_error_t *err;
+ svn_client_revision_t head;
+ head.kind = svn_client_revision_head;
if ((start->kind == svn_client_revision_unspecified)
|| (end->kind == svn_client_revision_unspecified))
{
@@ -73,7 +75,7 @@
"svn_client_log: caller failed to supply revision");
}
- start_revnum = end_revnum = SVN_INVALID_REVNUM;
+ start_revnum = end_revnum = head_revnum = SVN_INVALID_REVNUM;
path = (APR_ARRAY_IDX(targets, 0, const char *));
@@ -151,7 +153,60 @@
(&start_revnum, ra_lib, session, start, base_name, pool));
SVN_ERR (svn_client__get_revision_number
(&end_revnum, ra_lib, session, end, base_name, pool));
+ SVN_ERR (svn_client__get_revision_number
+ (&head_revnum, ra_lib, session, &head, base_name, pool));
+
+ /* Here, we check that the versions input were valid. If not, we
+ return an error */
+ if (start_revnum > head_revnum && end_revnum > head_revnum)
+ {
+ err = svn_error_createf(SVN_ERR_CLIENT_BAD_REVISION, 0, NULL,
pool,
+ "Both revision numbers too high: %"
SVN_REVNUM_T_FMT
+ " and %" SVN_REVNUM_T_FMT ". Head is %"
+ SVN_REVNUM_T_FMT ".",
+ start_revnum, end_revnum, head_revnum);
+ return err;
+ }
+
+ if (start_revnum > head_revnum || end_revnum > head_revnum)
+ {
+ err = svn_error_createf(SVN_ERR_CLIENT_BAD_REVISION, 0, NULL,
pool,
+ "Revision number too high: %"
SVN_REVNUM_T_FMT
+ ". Head is %" SVN_REVNUM_T_FMT ".",
+ (start_revnum>end_revnum)?
start_revnum:end_revnum,
+ head_revnum);
+ return err;
+ }
+ if (start_revnum < 0 && end_revnum < 0)
+ {
+ err = svn_error_createf(SVN_ERR_CLIENT_BAD_REVISION, 0, NULL,
pool,
+ "Both revision numbers are negative: %"
SVN_REVNUM_T_FMT
+ " and %" SVN_REVNUM_T_FMT ". Negative
revision number are not allowed.",
+ start_revnum, end_revnum);
+ return err;
+ }
+
+ if (start_revnum < 0 || end_revnum < 0)
+ {
+ err = svn_error_createf(SVN_ERR_CLIENT_BAD_REVISION, 0, NULL,
pool,
+ "Revision number negative: %"
SVN_REVNUM_T_FMT
+ ". Negative revision numbers are not
allowed.",
+ (start_revnum<end_revnum)?
start_revnum:end_revnum);
+ return err;
+ }
+
+
+ /* Previous to this change, "svn log -r x:x" exitted silently. It
will now give
+ diagnostic. */
+ if (start_revnum == end_revnum)
+ {
+ err = svn_error_createf(SVN_ERR_CLIENT_BAD_REVISION, 0, NULL,
pool,
+ "Revision numbers cannot match: %"
SVN_REVNUM_T_FMT
+ ", %" SVN_REVNUM_T_FMT ". Exitting",
+ start_revnum, end_revnum);
+ return err;
+ }
err = ra_lib->get_log (session,
condensed_targets,
start_revnum,
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h
+++ subversion/include/svn_client.h Sat Jul 27 00:45:29 2002
@@ -556,6 +556,17 @@
If START->kind or END->kind is svn_client_revision_unspecified,
return the error SVN_ERR_CLIENT_BAD_REVISION.
+ If the version number returned after canonicalising either START or
+ FINISH is greater than that of head, return the error
+ SVN_ERR_CLIENT_BAD_REVISION
+
+ If the version number returned after canonicalising either START or
+ FINISH is negative, return the error SVN_ERR_CLIENT_BAD_REVISION
+
+
+ If the version numbers returned from canonicalising START and FINISH
+ are equal, return SVN_ERR_CLIENT_BAD_REVISION
+
Use POOL for any temporary allocation.
Special case for repositories at revision 0:
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jul 27 02:02:32 2002