[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r12422 - trunk/subversion/libsvn_client

From: <kfogel_at_collab.net>
Date: 2004-12-20 18:32:55 CET

jpieper@tigris.org writes:
> Log:
> After renaming 'URL' to 'url' in the parameter list of
> svn_client__checkout_internal (r12365), the assertion testing for its
> non-NULLness should be adjusted as well.
> * subversion/libsvn_client/checkout.c
> (svn_client__checkout_internal): We want to assert that 'url' is
> non-NULL, not 'URL', which is not initialized at that point.

(I haven't reviewed r12365 itself yet, so some of these comments are
probably really about that commit.)

May I suggest that having an incoming parameter named 'url' with an
internal variable named 'URL' is asking for trouble anyway? :-)

Also, looking at the code in svn_client__checkout_internal:

After this line

  /* Canonicalize the URL. */
  URL = svn_path_canonicalize (url, pool);

...there comes immediately an open-curly-brace (misindented, btw),
whose first bit of non-declaratory code is this:

      /* Get the RA connection. */
      SVN_ERR (svn_client__ra_lib_from_path (&ra_lib, &session, &revnum,
                                             &URL, url, peg_revision,
                                             revision, ctx, pool));

So, URL is being passed as the "return parameter" there, right after
we assigned URL a value (which will now be lost?). There's no
conditional around the call to svn_client__ra_lib_from_path(), and
nothing in that function's doc string indicates that the value of the
incoming URL is used in any way, nor that its assignment into URL is

As I said, this properly belongs in a review of r12365, which will be
forthcoming. I just happened to notice it now, so am posting.


> Modified: trunk/subversion/libsvn_client/checkout.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_client/checkout.c?view=diff&rev=12422&p1=trunk/subversion/libsvn_client/checkout.c&r1=12421&p2=trunk/subversion/libsvn_client/checkout.c&r2=12422
> ==============================================================================
> --- trunk/subversion/libsvn_client/checkout.c (original)
> +++ trunk/subversion/libsvn_client/checkout.c Sun Dec 19 19:23:56 2004
> @@ -60,7 +60,7 @@
> /* Sanity check. Without these, the checkout is meaningless. */
> assert (path != NULL);
> - assert (URL != NULL);
> + assert (url != NULL);
> /* Fulfill the docstring promise of svn_client_checkout: */
> if ((revision->kind != svn_opt_revision_number)
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org

To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Dec 20 18:35:33 2004

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.