Michael Haggerty [off-list ref] writes:
longest_ancestor_length() relies on a textual comparison of directory
parts to find the part of path that overlaps with one of the paths in
prefix_list. But this doesn't work if any of the prefixes involves a
symbolic link, because the directories will look different even though
they might logically refer to the same directory. So canonicalize the
paths listed in prefix_list using real_path_if_valid() before trying
to find matches.
I somehow feel that this is making the logic unnecessarily
convoluted by solving the problem at a wrong level.
If longest_ancestor_length() takes a single string and a list of
candidate string prefixes, conceptually it should be usable for any
hierarchy-looking string that uses slashes as hierarchy separator
(e.g. refs that may be stored in packed-refs that you cannot expect
lstat(2) or readlink(2) to give you any sensible results).
The real problem is that the list given from the environment may
contain a path that violates that "it suffices to take the longest
string-prefix taking slashes into account" assumption in such a
generic l-a-l implementation, no? And this patch solves it by
making l-a-l _less_ generic and forcing it to be aware of the glitch
of its caller (you can either blame clueless user who lies when
setting the GIT_CEILING_DIRECTORIES by including paths contaminated
with symlinks, or blame the calling setup_git_directory_gently_1()
function that does not resolve the symbolic links before calling
this function).
As l-a-l is only used by the "stop at the ceiling" logic, isn't it a
far simpler solution to keep the function work at the string level,
and make the caller fix its input, i.e. the value taken from the
environment variable, before calling it? That is, grab the value of
GIT_CEILING_DIRECTORIES, split it into a list at PATH_SEP (while
rejecting any non-absolute paths), normalize the elements of that
list by resolving symbolic links, and then pass the cwd[] and the
normalized string list to l-a-l?
The resulting callsite in setup_git_directory_gently_1() would pass
cwd[] and the ceiling_dirs (which now is a string list), all of
whose elements would happen to begin with "/" (or dos drive prefix
followed by the "root" symbol), but l-a-l can be written in such a
way that it does not even require that all the input has to begin at
root, which would later make it usable with things that are not
paths (references, for example, all of which begin with "refs/" and
not "/refs/").
Hrm?
On 09/30/2012 10:00 AM, Junio C Hamano wrote:
Michael Haggerty [off-list ref] writes:
quoted
longest_ancestor_length() relies on a textual comparison of directory
parts to find the part of path that overlaps with one of the paths in
prefix_list. But this doesn't work if any of the prefixes involves a
symbolic link, because the directories will look different even though
they might logically refer to the same directory. So canonicalize the
paths listed in prefix_list using real_path_if_valid() before trying
to find matches.
I somehow feel that this is making the logic unnecessarily
convoluted by solving the problem at a wrong level.
If longest_ancestor_length() takes a single string and a list of
candidate string prefixes, conceptually it should be usable for any
hierarchy-looking string that uses slashes as hierarchy separator
(e.g. refs that may be stored in packed-refs that you cannot expect
lstat(2) or readlink(2) to give you any sensible results).
The real problem is that the list given from the environment may
contain a path that violates that "it suffices to take the longest
string-prefix taking slashes into account" assumption in such a
generic l-a-l implementation, no? And this patch solves it by
making l-a-l _less_ generic and forcing it to be aware of the glitch
of its caller (you can either blame clueless user who lies when
setting the GIT_CEILING_DIRECTORIES by including paths contaminated
with symlinks, or blame the calling setup_git_directory_gently_1()
function that does not resolve the symbolic links before calling
this function).
As l-a-l is only used by the "stop at the ceiling" logic, isn't it a
far simpler solution to keep the function work at the string level,
and make the caller fix its input, i.e. the value taken from the
environment variable, before calling it? That is, grab the value of
GIT_CEILING_DIRECTORIES, split it into a list at PATH_SEP (while
rejecting any non-absolute paths), normalize the elements of that
list by resolving symbolic links, and then pass the cwd[] and the
normalized string list to l-a-l?
The resulting callsite in setup_git_directory_gently_1() would pass
cwd[] and the ceiling_dirs (which now is a string list), all of
whose elements would happen to begin with "/" (or dos drive prefix
followed by the "root" symbol), but l-a-l can be written in such a
way that it does not even require that all the input has to begin at
root, which would later make it usable with things that are not
paths (references, for example, all of which begin with "refs/" and
not "/refs/").
I agree that longest_ancestor_length() is not very generic or
interesting anymore. Nearly all of its "added value" comes from the
normalize_path_callback() helper function. One possibility would be to
inline it at the one place it is called.
The function string_list_longest_prefix() was my attempt to isolate the
kernel of generic functionality from longest_ancestor_path(). It is
*almost* the function that you are proposing, with the exception that it
does not ensure that the prefix match ends at a "/" boundary. So
another alternative could be to change this function to respect "/"
boundaries. (After the change, the function might not belong in the
string-list API anymore.)
However, the semantics of a function that matches prefixes at "/"
boundaries is not entirely obvious. Suppose we would implement the test
via a function like path_prefixcmp(path, prefix). I can think of a few
policy questions that would have to be answered:
* Is a trailing slash on the prefix argument required, optional, or
prohibited? What if the prefix is "/" or "//" or "c:/"?
* Is a trailing slash on the path argument optional/prohibited? Are "/"
or "//" allowed as path arguments?
* Is a path its own prefix?
path_prefixcmp("a/b", "a/b") -> true or false?
(For the implementation of longest_ancestor_path(), we would prefer this
to return "false".) Or does the answer depend on whether the prefix has
a trailing slash?
path_prefixcmp("a/b", "a/b") -> true?
path_prefixcmp("a/b", "a/b/") -> false?
Part of the reason that I implemented string_list_longest_prefix()
instead of the function that you suggest is that the behavior of the
former is far more obvious.
I think I would advocate that the prefix has to match the front of the
path exactly (including any trailing slashes) and either
strlen(prefix) == 0
or the prefix ended with a '/'
or the prefix and path are identical
or the character in path following the matching part is a '/'
This would allow the "is path its own prefix" policy to be decided by
the caller by either including or omitting a trailing slash on the
prefix argument.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
On 10/01/2012 06:51 AM, Michael Haggerty wrote:
I think I would advocate that the prefix has to match the front of the
path exactly (including any trailing slashes) and either
strlen(prefix) == 0
or the prefix ended with a '/'
or the prefix and path are identical
or the character in path following the matching part is a '/'
This would allow the "is path its own prefix" policy to be decided by
the caller by either including or omitting a trailing slash on the
prefix argument.
Thinking about this more, I don't think it will work. As usual, the
special cases around "/" and "//" make things awkward. I think it is
necessary to have a separate argument to specify whether "path is its
own prefix".
So I am trying to decide how a function path_in_directory() should work,
and would like to get some feedback, especially on the following two points:
1. How should "//" be handled? I don't really have experience with the
peculiar paths that start with "//", so I'm not sure how they should be
handled (or even if the handling needs to be platform-dependent). My
working hypothesis is that the inputs should be normalized by the
caller, so if the caller wants "//" to be treated as equivalent to "/"
then the caller should normalize them *before* calling this function.
Conversely, if the caller passes "//" to the function, that implies that
"//" is distinct from "/" and "//" is considered a proper subdirectory
of "/". See the cases marked with "??????" below.
2. Does there need to be any special related to DOS paths?
/*
* Return true iff path is within dir. The comparison is textual,
* meaning that path and dir should be normalized and either both be
* absolute or both be relative to the same directory. If path and
* dir represent the *same* path, then return true iff allow_equal is
* true. Single trailing slashes on either path or dir are ignored,
* (except for the special case "//"); i.e., "a/b" and "a/b/" are
* treated equivalently, as are "" and "/". Examples (* means "don't
* care"):
*
* - path_in_directory("a/b", "a", *) -> true
* - path_in_directory("a", "a/b", *) -> false
* - path_in_directory("ab", "a", *) -> false
* - path_in_directory("a/b", "a/b", 0) -> false
* (same if either argument is replaced with "a/b/")
* - path_in_directory("a/b", "a/b", 1) -> true
* (same if either argument is replaced with "a/b/")
* - path_in_directory(*, "/", 1) -> true
* - path_in_directory("/", "/", 0) -> false
* - path_in_directory("//", "/", 0) -> true ??????
* - path_in_directory("//", "/", 1) -> true
* - path_in_directory("/", "//", 0) -> false
* - path_in_directory("/", "//", 1) -> false ??????
* - path_in_directory("/a/b", "//", *) -> false
*/
int path_in_directory(const char *path, const char *dir, int allow_equal);
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/