On Mon, Dec 07, 2009 at 11:33:40AM +0530, Kannan wrote:
> Kannan wrote:
> > Stefan Sperling wrote:
> >> On Mon, Nov 30, 2009 at 09:30:58PM +0530, Kannan wrote:
> >>> Stefan Sperling wrote:
> >>> @@ -991,7 +991,10 @@
> >>>
> >>> /* maybe return bc_url to the caller */
> >>> if (bc_url)
> >>> - *bc_url = *my_bc_url;
> >>> + {
> >>> + bc_url->data = svn_uri_canonicalize(my_bc_url->data, pool);
> >>> + bc_url->len = my_bc_url->len;
> >>> + }
> >> It would be nicer to have svn_ra_neon__get_baseline_props()
> >> do the canonicalisation, at this spot:
> >
> >> /* don't forget to tack on the parts we lopped off in order to find
> >> the VCC... We are expected to return a URI decoded relative
> >> path, so decode the lopped path first. */
> >> my_bc_relative = svn_path_join(relative_path->data,
> >> svn_path_uri_decode(lopped_path, pool),
> >> pool);
> >
> >> Then the caller would not need to worry about canonicalisation
> >> and your above change would not be needed.
> >
> >> Also we could replace the svn_path_join() while there.
> >
> >
> > The above code initialises `my_bc_rel' right? And `my_bc_url' is
> > getting initialised in `svn_ra_neon__get_baseline_info()' here :
> >
> > <snip>
> > /* Allocate our own copy of bc_url regardless. */
> > my_bc_url = apr_hash_get(baseline_rsrc->propset,
> > SVN_RA_NEON__PROP_BASELINE_COLLECTION,
> > APR_HASH_KEY_STRING);
> > </snip>
> >
> > Please correct me if I'm wrong. Thank you for your feedback. If the
> > above case seems ok, then shall I send the updated patch?
>
> Any updates on this thread?
You're correct, the place you point out is the place where my_bc_url
is initialised. If we could canonicalise it right there without causing
breakage elsewhere (make check over ra_neon should tell us) then please
send an updated patch. I'd say if changing this ends up causing breakage
we can apply your original patch, and then try to fix the breakage and
canonicalise my_bc_url earlier in a separate patch.
Thanks,
Stefan
Received on 2009-12-07 13:44:35 CET