Re: [PATCH 1/5] Add is_absolute_path(), make_absolute_path() and normalize_path()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Thu, 26 Jul 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

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