Michael W Thelen <thelenm@cs.utah.edu> writes:
> Here's an attempt at fixing issue #692 the right way.
Okay, I'm looking carefully at issue #692 now, and the history of this
problem. I have some questions:
In the issue, Greg Hudson writes:
> The workaround in r1864 is ridiculous. We have every ability to
> avoid requesting a log of r0 through r1 when the head revision is 0,
> so there is no reason to issue a bogus request and catch the error.
> I'm reopening this issue until there is a saner fix.
First, note that r1864 didn't introduce the error-catch. It used an
unconditional extra network turnaround to determine the youngest
revision. Then r1874 changed that into a conditional: *if* we got
back a certain error from the server, *then* do the extra network
turnaround to determine youngest rev, and enter the special case.
So I'll assume Greg's really talking about r1864 and r1874.
But in any case, I don't understand his comment: "We have every
ability to avoid requesting a log of r0 through r1 when the head
revision is 0, so there is no reason to issue a bogus request and
catch the error."
Uh, how exactly do we have this ability?
The client cannot know the HEAD revision without asking the server.
Therefore, if we want to avoid issuing a bogus request, we must ask
the server for the HEAD revision first. That causes an extra network
turnaround (see r1864), which in most cases is a useless burden,
because it's a rare repository where HEAD == 0
Thus, the solution is to issue the possibly-bogus request, see if we
get an error indicating this particular kind of bogosity, and only
*then* fall into a network-inefficient behavior.
This seems eminently reasonable to me, not at all "ridiculous".
Maybe there's something I'm missing here, but I'll need a more
detailed explanation from Greg or someone to see what it is.
Okay, that brings us to your patch.
I have no conceptual problem with part (b) of your patch. That WC
BASE special case deserves to be handled better.
But part (a) reintroduces the extra network turnaround that I got rid
of in r1874.
I'd like to understand why r1874 is not a good solution, and why it is
preferable to have an extra network turnaround instead?
(I haven't reviewed the part (b) code carefully yet, since it's still
mixed in with the part (a) code. When we all get on the same page
about exactly *what* we're trying to do here, then it'll be time for
line-level code review. :-) )
Thanks,
-Karl
> Log:
> Fix issue #692 the right way. There are two special cases to be handled:
>
> (a) The repository has 0 revisions and the user runs "svn log URL": it
> is as if the user had requested -rHEAD:1. We don't want to error
> out just because r1 doesn't exist.
>
> (b) The working copy is at BASE revision 0 but the repository is not
> empty, and the user runs "svn log" in the working copy: it is as if
> the user requested -rBASE:1. We don't want to print the log message
> for r1, since the working copy is at r0.
>
> * subversion/include/svn_client.h
> (svn_client_log): Add documentation for special case (b).
>
> * subversion/libsvn_client/log.c
> (svn_client_log): Detect both special case conditions before requesting
> logs from the server. If either one occurs, invoke the log message
> receiver manually with a message for revision 0.
>
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 10111)
> +++ subversion/include/svn_client.h (working copy)
> @@ -753,6 +753,19 @@
> * revision 1. That works fine, except when there are no commits in
> * the repository, hence this special case.
> *
> + * Special case for working copies at revision 0:
> + *
> + * If @a start->kind is @c svn_opt_revision_base, and @a end->kind is
> + * @c svn_opt_revision_number && @a end->number is @c 1, then handle an
> + * a working copy at revision 0 specially: instead of invoking @a receiver
> + * on revisions 0 through 1, only invoke @a receiver on revision 0, passing
> + * @c NULL for changed paths and empty strings for the author and date. This
> + * is because that particular combination of @a start and @a end usually
> + * indicates the common case of log invocation from the working copy -- the
> + * user wants to see all log messages from the BASE revision back through
> + * revision 1. That works fine, except when the BASE revision is 0, hence
> + * this special case.
> + *
> * If @a ctx->notify_func is non-null, then call @a ctx->notify_func/baton
> * with a 'skip' signal on any unversioned targets.
> *
> Index: subversion/libsvn_client/log.c
> ===================================================================
> --- subversion/libsvn_client/log.c (revision 10111)
> +++ subversion/libsvn_client/log.c (working copy)
> @@ -113,7 +113,7 @@
> apr_array_header_t *target_urls;
> apr_array_header_t *real_targets;
> int i;
> -
> +
> /* Get URLs for each target */
> target_urls = apr_array_make (pool, 1, sizeof (const char *));
> real_targets = apr_array_make (pool, 1, sizeof (const char *));
> @@ -135,7 +135,7 @@
> svn_wc_notify_state_unknown,
> SVN_INVALID_REVNUM);
>
> -
> +
> continue;
> }
> if (! entry->url)
> @@ -175,6 +175,33 @@
> (NULL != base_name), TRUE,
> 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" when given a URL is to give revisions HEAD
> + * through 1, on the assumption that HEAD >= 1. So if we see a start
> + * revision of HEAD and an end revision of 1, then we just invoke the
> + * receiver manually on a hand-constructed log message for revision 0.
> + *
> + * See also http://subversion.tigris.org/issues/show_bug.cgi?id=692.
> + */
> + {
> + svn_revnum_t youngest_rev;
> + SVN_ERR (ra_lib->get_latest_revnum (session, &youngest_rev, pool));
> + if ((start->kind == svn_opt_revision_head)
> + && (youngest_rev == 0)
> + && (end->kind == svn_opt_revision_number)
> + && (end->value.number == 1))
> + {
> + /* Log receivers are free to handle revision 0 specially... But
> + just in case some don't, we make up a message here. */
> + SVN_ERR (receiver (receiver_baton,
> + NULL, 0, "", "", _("No commits in repository"),
> + pool));
> +
> + return SVN_NO_ERROR;
> + }
> + }
> +
> /* It's a bit complex to correctly handle the special revision words
> * such as "BASE", "COMMITTED", and "PREV". For example, if the
> * user runs
> @@ -243,20 +270,50 @@
> if (start_is_local)
> SVN_ERR (svn_client__get_revision_number
> (&start_revnum, ra_lib, session, start, target, pool));
> -
> +
> if (end_is_local)
> SVN_ERR (svn_client__get_revision_number
> (&end_revnum, ra_lib, session, end, target, pool));
>
> - err = ra_lib->get_log (session,
> - condensed_targets,
> - start_revnum,
> - end_revnum,
> - discover_changed_paths,
> - strict_node_history,
> - receiver,
> - receiver_baton,
> - pool);
> + /* Special case: If the BASE revision is 0 but the HEAD revision
> + * of the repository is > 0, then we'll get the log message for
> + * revision 1 when running "svn log" with no arguments in the
> + * working copy. That's because the default behavior of "svn log"
> + * in a working copy is to give revisions BASE through 1, on the
> + * assumption that BASE >= 1. So if we see a start revision of
> + * BASE and an end revision of 1, then we just invoke the receiver
> + * manually on a hand-constructed log message for revision 0.
> + *
> + * See also
> + * http://subversion.tigris.org/issues/show_bug.cgi?id=692.
> + */
> + if ((start->kind == svn_opt_revision_base)
> + && (start_revnum == 0)
> + && (end->kind == svn_opt_revision_number)
> + && (end->value.number == 1))
> + {
> + err = SVN_NO_ERROR;
> +
> + /* Log receivers are free to handle revision 0 specially... But
> + just in case some don't, we make up a message here. */
> + SVN_ERR (receiver (receiver_baton,
> + NULL, 0, "", "",
> + _("No commits in BASE revision"),
> + pool));
> + }
> + else
> + {
> + err = ra_lib->get_log (session,
> + condensed_targets,
> + start_revnum,
> + end_revnum,
> + discover_changed_paths,
> + strict_node_history,
> + receiver,
> + receiver_baton,
> + pool);
> + }
> +
> if (err)
> break;
> }
> @@ -273,40 +330,7 @@
> receiver_baton,
> 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
> - * 1, on the assumption that HEAD >= 1.
> - *
> - * So if we got that error for that reason, and it looks like the
> - * user was just depending on the defaults (rather than explicitly
> - * requesting the log for revision 1), then we don't error. Instead
> - * we just invoke the receiver manually on a hand-constructed log
> - * message for revision 0.
> - *
> - * See also http://subversion.tigris.org/issues/show_bug.cgi?id=692.
> - */
> - if (err && (err->apr_err == SVN_ERR_FS_NO_SUCH_REVISION)
> - && (start->kind == svn_opt_revision_head)
> - && ((end->kind == svn_opt_revision_number)
> - && (end->value.number == 1)))
> - {
> - svn_revnum_t youngest_rev;
> -
> - SVN_ERR (ra_lib->get_latest_revnum (session, &youngest_rev, pool));
> - if (youngest_rev == 0)
> - {
> - err = SVN_NO_ERROR;
> -
> - /* Log receivers are free to handle revision 0 specially... But
> - just in case some don't, we make up a message here. */
> - SVN_ERR (receiver (receiver_baton,
> - NULL, 0, "", "", _("No commits in repository."),
> - pool));
> - }
> - }
> }
> -
> +
> return err;
> }
>
> -- Mike
>
> --
> Michael W. Thelen
> It is a painful thing
> To look at your own trouble and know
> That you yourself and no one else has made it
> -- Sophocles, "Ajax"
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jul 2 20:30:33 2004