Re: [PATCH] Re: [Issue 816] - Silent failure in svn log -r

Kieran <kieran@esperi.demon.co.uk> writes:

> Index: subversion/libsvn_client/log.c
> ===================================================================
> --- subversion/libsvn_client/log.c
> +++ subversion/libsvn_client/log.c Fri Jul 26 20:38:16 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_error_t *err;
> + svn_revnum_t start_revnum, end_revnum, head_revnum;
> + svn_error_t *err, *nonfatal_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,47 @@
> (&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));
>
> + /* To deal with the case where one of the revision numbers provided
> + is greater than the current head, we calculate the head revision
> + number in the line above, and use that value instead of the
> + problem value. */
> + 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?

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.

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.

Whatever policy is used, the documentation for svn_client_log in
svn_client.h should be updated.

> pool,
> + "Revision number too high: %d. Using head
> (%d)",
> + (int)start_revnum, (int)head_revnum);
> + printf("DEBUG: Revision number too high: %d. Using head (%d)\n",
> + (int)start_revnum, (int)head_revnum);
> + start_revnum = head_revnum;
> + }
> + if (start_revnum <= head_revnum && end_revnum > head_revnum)
> + {
> + nonfatal_err = svn_error_createf(SVN_ERR_BAD_REVISION, 0, NULL,
> pool,
> + "Revision number too high: %d. Using head
> (%d)",
> + (int)end_revnum, (int)head_revnum);
> + end_revnum = head_revnum;
> + }
> + 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..."

> + return err;
> + }
> +
> + /* Previous to this change, "svn log -r x:x" exitted silently. It
> will now give
> + diagnostic, although it's not clear to me that an error is better
> than simply
> + providing the log message for that particular revision */
> + 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.

> err = ra_lib->get_log (session,
> condensed_targets,
> start_revnum,
> @@ -196,8 +238,8 @@
>
> /* We're done with the RA session. */
> SVN_ERR (ra_lib->close (session));
> -
> - return err;
> + if (err) return err;
> + return nonfatal_err;
> }
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org