Re: path.c:113: svn_path_join: Assertion `is_canonical (base, blen)' failed.

[Greg Hudson <gh...@MIT.EDU>]

On Thu, 2004-08-26 at 04:52, Ben Reser wrote:
> Honestly, I think it was a mistake to make our internal APIs accept URIs
> and local file paths interchangebly.  Our code that looks for URIs
> thinks that foo://localhost is a valid URI (it technically is even
> though SVN has no knowledge of a foo schema) so we treat it as a URI.

There's no real problem here, since such a path is non-canonical (and
thus unusual) and in the same form as a URI.  Certainly, if we were to
add support for a foo URI scheme, it would be better that foo://bar
pathnames already didn't work.

> Thus I propose that svn_path_dirname() be fixed to understand that the
> hostname portion of a URI is the root and can not be split.
> svn_path_basename() will also have to be adjusted so it can recognize
> when this occurs and return no basename.  

-1.  svn_path_dirname accepts a canonical path (not URL;
svn_path_canonicalize is documented as accepting a URL, but
svn_path_dirname is not).  Canonical paths do not have double slashes,
contrary to what you said in your response to yourself.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: path.c:113: svn_path_join: Assertion `is_canonical (base, blen)' failed.

[Greg Hudson <gh...@MIT.EDU>]

On Thu, 2004-08-26 at 14:43, Ben Reser wrote:
> Well it's not canonical if you're treating it as a local path.  But it
> is canonical as a URI.  Which is what I realized a little bit further
> down in the email.  The problem is we have no way to say to
> svn_path_canonicalize() "We never accepts URLs.  Canonicalize as though
> everything were a local path."

I agree, that should be fixed.  There should be an entirely separate set
of functions to operate on URIs.

> IMHO canonical path is defined by the output of svn_path_canonicalize().

Only if what you passed to svn_path_canonicalize() is a path.  If it's a
URI, then what you have is a canonicalized URI.

> But ignoring who's right or wrong let's look at the implications of your
> view here.  If svn_path_dirname() does not accept URLs that means every
> app that uses it *MUST* call svn_path_is_url() prior to calling it to
> ensure they won't get an assert.

Yes, because svn_path_canonicalize() has a broken contract.  But that's
good for usability anyway; if a function only operates on paths, not
URIs, then it should produce an intelligent error if handed a URI.

> Additionally, svn_path_is_url is documented as:
> /** Return @c TRUE iff @a path looks like a valid URL, @c FALSE
> otherwise. */
> 
> Uhh so paths can be URLs!?!?  This totally doesn't agree with what you
> seem to be saying.  Neither does the naming of svn_path_is_url().
> In the end apps are going to have to be writing:

That's a poorly-named function and a poorly-named parameter, perhaps
because there's no simple word for path-or-URL.  But it's not really a
compelling argument for the idea that URLs are always valid arguments to
the svn_path functions.  And there are other arguments against; for
instance, the svn_path_join docstring says:

 * Note that the contents of @a base are not examined, so it is possible to
 * use this function for constructing URLs, or for relative URLs or
 * repository paths.

(Of course, that turns out to be false; svn_path_join yields an
assertion failure if base is not canonical.)  Most of the svn_path
docstrings are quite explicit when they accept URLs.

> if (svn_path_is_url (path))
>   {
>     // their own path splitting stuff for URLs that works properly
>   }

Do we ever deliberately call svn_path_dirname on a URI currently?  Is
there any reason to do so?  It seems like a layering violation to me.

Incidentally, if you call svn_path_canonical() on "http://hostname/", it
looks like the result will be http://hostname/, which will fail the
canonical check.  Does that seem correct to you?


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: path.c:113: svn_path_join: Assertion `is_canonical (base, blen)' failed.

[Ben Reser <be...@reser.org>]

On Thu, Aug 26, 2004 at 11:20:36AM -0400, Greg Hudson wrote:
> There's no real problem here, since such a path is non-canonical (and
> thus unusual) and in the same form as a URI.  Certainly, if we were to
> add support for a foo URI scheme, it would be better that foo://bar
> pathnames already didn't work.

Well it's not canonical if you're treating it as a local path.  But it
is canonical as a URI.  Which is what I realized a little bit further
down in the email.  The problem is we have no way to say to
svn_path_canonicalize() "We never accepts URLs.  Canonicalize as though
everything were a local path."

> > Thus I propose that svn_path_dirname() be fixed to understand that the
> > hostname portion of a URI is the root and can not be split.
> > svn_path_basename() will also have to be adjusted so it can recognize
> > when this occurs and return no basename.  
> 
> -1.  svn_path_dirname accepts a canonical path (not URL;
> svn_path_canonicalize is documented as accepting a URL, but
> svn_path_dirname is not).  Canonical paths do not have double slashes,
> contrary to what you said in your response to yourself.

Okay you're right canonical paths do not have double slashes except for
in the case of a URL.  However, svn_path_dirname() is documented as
accepting any canonical path.  IMHO canonical path is defined by the
output of svn_path_canonicalize().  Thus svn_path_dirname() accepts URLs
provided they've been canonicalized.  

Either I'm right and svn_path_dirname() should be fixed or you're right
and svn_path.h's documentation needs some work.

But ignoring who's right or wrong let's look at the implications of your
view here.  If svn_path_dirname() does not accept URLs that means every
app that uses it *MUST* call svn_path_is_url() prior to calling it to
ensure they won't get an assert.  Why?  Because as you point out a
double slash is technically not a canonical path, thus
svn_path_dirname() should end up in a non-canonical result for
"foo//bar", but r10732 stops that from happening and is wrong.

Additionally, svn_path_is_url is documented as:
/** Return @c TRUE iff @a path looks like a valid URL, @c FALSE
otherwise. */

Uhh so paths can be URLs!?!?  This totally doesn't agree with what you
seem to be saying.  Neither does the naming of svn_path_is_url().

In the end apps are going to have to be writing:
if (svn_path_is_url (path))
  {
    // their own path splitting stuff for URLs that works properly
  }
else
  {
    dirname = svn_path_dirname (path,pool);
  }

And that seems a tad silly when some pretty minimal changes could just
let them do:
dirname = svn_path_dirname (path,pool);

-- 
Ben Reser <be...@reser.org>
http://ben.reser.org

"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org