On 05/30/2011 07:52 AM, Avalon wrote: > Could someone of the devs please take a closer look? > I am willing to improve the patch based on concrete suggestions.
[...]
Sure thing. I can see where you're going with the patch. I haven't taken
the time to fully consider the appropriate, so consider the following a
syntactical patch review only.
> Index: log.c
> ===================================================================
> --- log.c (revision 1129130)
> +++ log.c (working copy)
> @@ -41,7 +41,6 @@
> #include "svn_private_config.h"
> #include "private/svn_wc_private.h"
>
> - ^L
Please don't remove our page breaks.
> /*** Getting misc. information ***/
>
> /* The baton for use with copyfrom_info_receiver(). */
> @@ -261,7 +260,43 @@
> return rb->receiver(rb->baton, log_entry, pool);
> }
>
> - ^L
Here, too.
> +svn_error_t *
> +svn_client__check_for_deleted_rev(svn_ra_session_t *ra_session,
[...]
This function is file-private, so should be "static" and not use the
"svn_client__" naming prefix. check_for_deleted_rev() would serve just fine
as a name.
> + SVN_ERR(svn_ra_get_session_url(ra_session,
> + &session_url,
> + pool));
> +
> + SVN_ERR(svn_client__path_relative_to_root(&rel_path,
> + ctx->wc_ctx,
> + url_or_path,
> + session_url,
> + FALSE,
> + ra_session,
> + pool,
> + pool));
> +
> + SVN_ERR(svn_ra_get_deleted_rev(ra_session,
> + rel_path,
> + peg_rev.value.number,
> + end_revnum,
> + revision_deleted,
> + pool));
peg_rev isn't guaranteed to have a valid ".value.number" component, yet you
unconditionally reference it. That field is only populated if the revision
is of type svn_opt_revision_number, and my testing of your patch this was
not always the case. (It was svn_opt_revision_working in my scenario.)
> @@ -481,11 +518,46 @@
> else
> ra_target = url_or_path;
>
> - SVN_ERR(svn_client__ra_session_from_path(&ra_session, &ignored_revnum,
> + err = svn_client__ra_session_from_path(&ra_session, &ignored_revnum,
> &actual_url, ra_target, NULL,
> &peg_rev, &session_opt_rev,
> - ctx, pool));
> + ctx, pool);
Need to correct the indentation here.
> + if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND &&
> session_opt_rev.value.number > peg_rev.value.number)
> + {
> + svn_revnum_t revision_deleted = SVN_INVALID_REVNUM;
> + svn_ra_session_t *tmp_session;
> +
> + SVN_ERR(svn_client__open_ra_session_internal(&tmp_session, NULL,
> + ra_target, NULL,
> + NULL, FALSE, TRUE,
> + ctx, pool));
> +
> + SVN_ERR(svn_client__check_for_deleted_rev(tmp_session,
> + url_or_path,
> + peg_rev,
> +
> session_opt_rev.value.number,
> + &revision_deleted,
> + ctx,
> + pool));
> +
> + if (revision_deleted != SVN_INVALID_REVNUM && revision_deleted !=
> SVN_ERR_CLIENT_BAD_REVISION)
> + {
> + svn_opt_revision_t session_mod_rev;
> +
> + svn_error_clear(err);
> +
> + session_mod_rev.kind = svn_opt_revision_number;
> + session_mod_rev.value.number = revision_deleted - 1;
> +
> + err = svn_client__ra_session_from_path(&ra_session,
> &ignored_revnum,
> + &actual_url, ra_target,
> NULL,
> + &peg_rev,
> &session_mod_rev,
> + ctx, pool);
> + }
This section needs to cleanup the tmp_session as soon as it's no longer
needed. We tend to do this by creating a subpool (called "sesspool", for
kicks), opening the temporary RA session in that pool, and then destroying
that pool as soon as we no longer need the session.
> @@ -603,6 +676,22 @@
> passed_receiver_baton = &lb;
> }
>
> + if (end_revnum > peg_rev.value.number)
> + {
> + SVN_ERR(svn_client__check_for_deleted_rev(ra_session,
> + url_or_path,
> + peg_rev,
> + end_revnum,
> + &revision_deleted,
> + ctx,
> + pool));
> + }
> +
> + if (revision_deleted != SVN_INVALID_REVNUM && revision_deleted !=
> SVN_ERR_CLIENT_BAD_REVISION)
> + {
> + end_revnum = revision_deleted - 1;
> + }
> +
SVN_ERR_CLIENT_BAD_REVISION is not a valid identifier for an svn_revnum_t,
so the "revision_deleted != SVN_ERR_CLIENT_BAD_REVISION" check is bogus.
Finally, patches are best sent to the mailing list with a "[PATCH] "
Subject: line prefix *and* a valid log message.
--
C. Michael Pilato <cmpil...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
signature.asc
Description: OpenPGP digital signature
