On Sat, 15 Jan 2005 kfogel@collab.net wrote:
> "Peter N. Lundblad" <peter@famlundblad.se> writes:
> > With "fixing symptoms" I was referring to the use of a function that
> > doesn't actually make sure that the paths are canonical. It fixes the
> > assertion failures (symptom), but doesn't fix the real problem (making
> > sure that paths are in canonical form). When you use a function, you
> > should make sure it does what you want. Since this function is internal,
> > and undocumented, this means reading the code:-)
>
> Okay, okay -- no need to lecture Mike Pilato on how to be a good
> programmer, he's plenty thorough already. Anyone can misread some
> code, all you need to do is point out the bug and let him fix it. No
> need to go beyond that.
>
I didn't mean to lecture anyone anything. I was critisizing the actual
commit, nothing more. This happen every day. People make mistakes, others
point that out. That was what I thought I did. Maybe I used a bad tone.
In that case, it was not my intention.
> Regarding the overall approach: I feel that Julian Foad's point about
> being strict now and loosening later if we want is a good one.
>
IN general, yes. IN this particular case, I think it isn't a good idea.
For example, "//" in the beginning of a path has a well-defined meaning on
some platform as a path and on all platforms as an URL. NOte that I am not
very much *against* enforcing the strict rule. I just think it gains us
very little (if anything) in this case and it is more work.
> > BTW, when I said we might still have other bugs in this fix, I menat that
> > something in svn_path might depend on other properties of a canonical path
> > than is_canonical enforces. These might be crashes as well. I don't
> > know, because I haven't checked.
>
> Erp -- sorry, I read this three times but don't understand what you
> meant. Try explaining another way?
>
OK. We have a definition of a canonical path. We have
svn_path_canonicalize (and, at least currently, svn_path_is_canonical()).
It is reasonalbe to assume that, if you call svn_path_is_canonical on a
path, you can pass that path to any API function that expects a canonical
path and get the expected result. My point is that we *might* have APIs
that rely on other properties of a canonical path than what
svn_path_is_canonical checks currently. For example, svn_path_is_canonical
doesn't check for multiple consecutive slashes. Some function might depend
on each component being separated by exactly one slash. This might lead to
other bugs or crashes that this fix pretends to prevent, but doesn't.
Ofcourse, if svn_path_is_canonical gets fixed, this point isn't anymore.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jan 15 21:53:21 2005