"wangsh.f...@fujitsu.com" <wangsh.f...@fujitsu.com> writes:
> I don't know this. After some test, I think it's better to consider
> '/hoge/foo/bar'
> as a absolute path.
Agreed. I think we are considering "absolute path" here as a
syntactic concept; Windows' weird rules about drive letters
don't really matter for the purposes of path canonicalization.
> 0001 and 0002 are the are the bugfix patches.
> 0003 is the test patch what I have tested on Linux and Windows.
> Waiting for some comment.
I tried to read 0001 but really couldn't make sense of the logic
at all, because it's seriously underdocumented. At minimum you
need an API spec comment for canonicalize_path_sub, explaining
what it's supposed to do and why. This is a significant rewrite
of what was already tricky code, so we can't skimp on
documentation. I'd put some effort into choosing more descriptive
names, too ("sub" doesn't mean much, especially here where it's
not clear if it means "subroutine" or "path component").
I did notice that you dropped the separate step to collapse
adjacent separators (i.e, reduce "foo//bar" to "foo/bar"), which
seems like probably a bad idea. I think such cases might confuse
canonicalize_path_sub, and even if it manages to do the right
thing, that requirement will complicate its invariants won't it?
Another thing I happened to notice is that join_path_components
is going out of its way to not generate "foo/./bar", but if
we are fixing canonicalize_path to be able to delete the "./",
that seems like a waste of code now.
I am not entirely convinced that 0002 isn't re-introducing the
security hole that the existing code seeks to plug. That one
is going to require more justification.
I concur with the upthread comments that there's little chance
we'll commit 0003 as-is; the code-to-benefit ratio is too high.
Instead, you might consider adding test_canonicalize_path in
src/test/regress/regress.c, and then adding a smaller number
of regression test cases using that.
regards, tom lane
