Johannes Schindelin [off-list ref] writes:
Agree. Maybe a comment above the function, like
/*
* The function normalize_path() converts ".." and "." names in
* the given path so that "foo/../bar/./" will come out as "bar".
*
* Note: normalize_path() does not follow symlinks, so if "foo" is
* a symlink in the example above, the result will not work as
* expected.
*/
Hmm?
That comment only states the obvious and does not give a clue to
the callers when it should not be used, I am afraid. For
example, paths taken out of index or recursively reading trees
are Ok because there will not be ".." and "." in them. Making a
path given by the user relative to the cwd by prepending what is
returned by setup_git_directory() may or may not be safe,
depending on how setup_git_directory() does things (I think the
original one is safe; I am reasonably sure with the current one
when GIT_WORK_TREE is not in use; I do not know when that
environment variable is there with the current code with or
without your patch series).
Hi,
On Thu, 26 Jul 2007, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
Agree. Maybe a comment above the function, like
/*
* The function normalize_path() converts ".." and "." names in
* the given path so that "foo/../bar/./" will come out as "bar".
*
* Note: normalize_path() does not follow symlinks, so if "foo" is
* a symlink in the example above, the result will not work as
* expected.
*/
Hmm?
That comment only states the obvious and does not give a clue to
the callers when it should not be used, I am afraid.
I am afraid, that it was unobvious enough to yours truly to forget about
that when writing the patch.
For example, paths taken out of index or recursively reading trees are
Ok because there will not be ".." and "." in them. Making a path given
by the user relative to the cwd by prepending what is returned by
setup_git_directory() may or may not be safe, depending on how
setup_git_directory() does things (I think the original one is safe; I
am reasonably sure with the current one when GIT_WORK_TREE is not in
use; I do not know when that environment variable is there with the
current code with or without your patch series).
I am afraid that already GIT_DIR can contain symlinks, and is not checked
by setup_git_env().
So I think some concrete comment is needed in _addition_:
get_git_dir() is not safe, and therefore git_path(), too.
Hmm.
Maybe the easiest way _is_ to getcwd(); chdir() getcwd(); chdir(back);
Ugly.
Ciao,
Dscho