"S.Ramaswamy" <srsy70@gmail.com> writes:
> I tried this patch to get an idea about peg revisions.
> .While the tests pass, and log is now able to follow files
> across renames, I am not sure about the approach.
>
> A couple of the log tests fail that run 'svn log' on older
> revisions of the repos url fail:
> FAIL: log_tests.py 7: 'svn log -r N URL' when URL is not in HEAD
> FAIL: log_tests.py 8: 'svn log TGT' with copyfrom history
>
> That could be because a peg revision is now required by log, when
> listing files not in HEAD
>
> Any suggestions ?
Ben and I just talked this over, and decided the old behavior
(succeeding) was a "bug", and that the new failure is correct. The
URL does not exist in HEAD, period. Therefore, this command
$ svn log -rN url_not_present_in_head
which has an implied peg rev
$ svn log -rN url_not_present_in_head@HEAD
should fail.
However, this is an incompatible change that users are likely to
notice. I think we should have a separate discussion about it. In
order to do so, I will follow up to this mail with another mail, one
with a more attention-getting subject, and refer people back to this
thread.
On with the review:
> Log:
>
> Fix issue #2287 - Make svn_client_log() take a peg revision.
>
> * subversion/include/svn_client.h:
> (svn_client_log3): New prototype.
> (svn_client_log2): Deprecate.
>
> * subversion/libsvn_client/log.c:
> (svn_client_log3): New function.
> (svn_client_log2): Re-implemented using new function
> svn_client_log3().
> (svn_client_log): Re-implemented using new function
> svn_client_log3().
>
> * subversion/clients/cmdline/log-cmd.c:
> (svn_cl__log): Call svn_client_log3().
>
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 15190)
> +++ subversion/include/svn_client.h (working copy)
> @@ -915,7 +915,8 @@
> /**
> * Invoke @a receiver with @a receiver_baton on each log message from @a
> * start to @a end in turn, inclusive (but never invoke @a receiver on a
> - * given log message more than once).
> + * given log message more than once). @a peg_revision indicates in which
> + * revision @a targets are valid.
> *
> * @a targets contains either a URL followed by zero or more relative
> * paths, or a list of working copy paths, as <tt>const char *</tt>,
> @@ -947,10 +948,11 @@
> * If @a ctx->notify_func2 is non-null, then call @a ctx->notify_func2/baton2
> * with a 'skip' signal on any unversioned targets.
> *
> - * @since New in 1.2.
> + * @since New in 1.3.
> */
> svn_error_t *
> -svn_client_log2 (const apr_array_header_t *targets,
> +svn_client_log3 (const apr_array_header_t *targets,
> + const svn_opt_revision_t *peg_revision,
> const svn_opt_revision_t *start,
> const svn_opt_revision_t *end,
> int limit,
> @@ -963,6 +965,23 @@
>
>
> /**
> + * Similar to svn_client_log3(), but with the @a peg_revision
> + * parameter always set to @c svn_opt_revision_unspecified.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + */
> +svn_error_t *
> +svn_client_log2 (const apr_array_header_t *targets,
> + const svn_opt_revision_t *start,
> + const svn_opt_revision_t *end,
> + int limit,
> + svn_boolean_t discover_changed_paths,
> + svn_boolean_t strict_node_history,
> + svn_log_message_receiver_t receiver,
> + void *receiver_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +/**
> * Similar to svn_client_log2(), but with the @a limit parameter set to 0,
> * and the following special case:
> *
> Index: subversion/libsvn_client/log.c
> ===================================================================
> --- subversion/libsvn_client/log.c (revision 15190)
> +++ subversion/libsvn_client/log.c (working copy)
> @@ -45,7 +45,8 @@
>
>
> svn_error_t *
> -svn_client_log2 (const apr_array_header_t *targets,
> +svn_client_log3 (const apr_array_header_t *targets,
> + const svn_opt_revision_t *peg_revision,
> const svn_opt_revision_t *start,
> const svn_opt_revision_t *end,
> int limit,
> @@ -63,6 +64,9 @@
> apr_array_header_t *condensed_targets;
> svn_revnum_t start_revnum, end_revnum;
> svn_error_t *err = SVN_NO_ERROR; /* Because we might have no targets. */
> + svn_revnum_t rev;
> + const char *url_p;
> + svn_opt_revision_t revision;
Since we're already taking a parameter called 'peg_revision', a
comment here explaining what 'revision' is and how it differs would be
helpful. Also, a comment could explain the relationship of 'rev' to
these two. You can see how things start to get confusing pretty
quickly! :-)
> if ((start->kind == svn_opt_revision_unspecified)
> || (end->kind == svn_opt_revision_unspecified))
> @@ -157,13 +161,17 @@
> targets = real_targets;
> }
>
> - /* Open a repository session to the BASE_URL. */
> - SVN_ERR (svn_path_condense_targets (&base_name, NULL, targets, TRUE, pool));
> - SVN_ERR (svn_client__open_ra_session_internal (&ra_session, base_url,
> - base_name, NULL, NULL,
> - (NULL != base_name), TRUE,
> - ctx, pool));
> + if (start->kind == svn_opt_revision_number &&
> + end->kind == svn_opt_revision_number)
> + revision = (start->value.number > end->value.number) ? *start : *end;
> + else
> + revision.kind = svn_opt_revision_unspecified;
>
> + SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
> + &url_p, path,
> + peg_revision, &revision, ctx,
> + pool));
> +
> /* It's a bit complex to correctly handle the special revision words
> * such as "BASE", "COMMITTED", and "PREV". For example, if the
> * user runs
> @@ -270,6 +278,30 @@
> }
>
> svn_error_t *
> +svn_client_log2 (const apr_array_header_t *targets,
> + const svn_opt_revision_t *start,
> + const svn_opt_revision_t *end,
> + int limit,
> + svn_boolean_t discover_changed_paths,
> + svn_boolean_t strict_node_history,
> + svn_log_message_receiver_t receiver,
> + void *receiver_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + svn_error_t *err = SVN_NO_ERROR;
> + svn_opt_revision_t peg_revision;
> +
> + peg_revision.kind = svn_opt_revision_unspecified;
> +
> + err = svn_client_log3 (targets, &peg_revision, start, end, limit,
> + discover_changed_paths, strict_node_history,
> + receiver, receiver_baton, ctx, pool);
> +
> + return err;
> +}
> +
> +svn_error_t *
> svn_client_log (const apr_array_header_t *targets,
> const svn_opt_revision_t *start,
> const svn_opt_revision_t *end,
> @@ -281,11 +313,14 @@
> apr_pool_t *pool)
> {
> svn_error_t *err = SVN_NO_ERROR;
> + svn_opt_revision_t peg_revision;
>
> - err = svn_client_log2 (targets, start, end, 0, discover_changed_paths,
> - strict_node_history, receiver, receiver_baton, ctx,
> - pool);
> -
> + peg_revision.kind = svn_opt_revision_unspecified;
> +
> + err = svn_client_log3 (targets, &peg_revision, start, end, 0,
> + discover_changed_paths, strict_node_history,
> + receiver, receiver_baton, ctx, pool);
> +
> /* Special case: If there have been no commits, we'll get an error
> * for requesting log of a revision higher than 0. But the
> * default behavior of "svn log" is to give revisions HEAD through
All looks good.
> Index: subversion/clients/cmdline/log-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/log-cmd.c (revision 15190)
> +++ subversion/clients/cmdline/log-cmd.c (working copy)
> @@ -398,6 +398,8 @@
> struct log_receiver_baton lb;
> const char *target;
> int i;
> + svn_opt_revision_t peg_revision;
> + const char *truepath;
>
> SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
> opt_state->targets, pool));
> @@ -408,6 +410,10 @@
> /* Retrieve the first target in the list. */
> target = APR_ARRAY_IDX (targets, 0, const char *);
>
> + SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target, pool));
> +
> + APR_ARRAY_IDX (targets, 0, const char *) = truepath;
> +
What if other targets (besides the first) have peg revs on them?
The old code was able to get away with checking just the first target
because it checked the other targets for inconsistencies against the
first target: if the first target was a URL, then everything had to be
a URL, or else error. Or if it was a wc path, everything had to be a
wc path.
But here, you're parsing off a peg rev from the first target, while
not addressing the question of what happens if there are peg revs on
other targets.
> if ((opt_state->start_revision.kind != svn_opt_revision_unspecified)
> && (opt_state->end_revision.kind == svn_opt_revision_unspecified))
> {
> @@ -460,6 +466,7 @@
> }
> }
>
> + APR_ARRAY_IDX (targets, 0, const char *) = truepath;
> lb.cancel_func = ctx->cancel_func;
> lb.cancel_baton = ctx->cancel_baton;
> lb.omit_log_message = opt_state->quiet;
Why the redundant reassignment to targets[0]?
> @@ -486,7 +493,8 @@
> SVN_ERR (svn_cl__error_checked_fputs (sb->data, stdout));
> }
>
> - SVN_ERR (svn_client_log2 (targets,
> + SVN_ERR (svn_client_log3 (targets,
> + &peg_revision,
> &(opt_state->start_revision),
> &(opt_state->end_revision),
> opt_state->limit,
> @@ -516,7 +524,8 @@
> * is concerned, the result of 'svn log --quiet' is the same
> * either way.
> */
> - SVN_ERR (svn_client_log2 (targets,
> + SVN_ERR (svn_client_log3 (targets,
> + &peg_revision,
> &(opt_state->start_revision),
> &(opt_state->end_revision),
> opt_state->limit,
Aside from the above concerns, the code looks good to me!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jul 1 23:15:53 2005