On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:
Steffen Prohaska <prohaska@xxxxxx> writes:
On Nov 22, 2007, at 3:34 AM, Junio C Hamano wrote:
Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
Does this not have a fundamental issue? When you call other git
programs
with run_command(), you _need_ GIT_DIR to be set, no?
It is much worse. set_git_dir() does not just setenv() but does
setup_git_env() as well.
What do your comments mean?
My understanding is that set_git_dir() sets the environment and
then calls setup_git_env() to cache all pointers. This call
updates dangling pointer if they have been cached earlier.
Well, I was agreeing with you. "Worse" was about what the
current code does _not_ do.
If there are earlier calls that obtain locations relative to the
earlier definition of GIT_DIR, the locations they obtained are
not just stored in memory that is "dangling" (which was what
your proposed log message described) but they are also
inconsistent with the updated definition of GIT_DIR.
I suspect Johannes mistook set_git_dir() was only local
(i.e. per calling process) matter without noticing that it has
its own setenv() when he made that comment, hence my response to
point out that the current code only calls setenv(), but
set_git_dir() does setup_git_env() too, which should hide the
inconsistency problem.
HOWEVER.
I suspect that if there are even earlier callers than these
early parts in the codepaths (handle_options, enter_repo, and
setup_git_directory_gently), maybe these earlier callers are
doing something wrong. Logically, if you are somewhere very
early in the codepath that you can still change the value of
GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR
and cached locations relative to that directory, no? What are
the problematic callers? What values do they access and why?
I thought about these questions, too. But only very briefly.
I did not analyze the code path that lead to calls of getenv().
I'm not sure if it's really necessary. Calling set_git_dir()
looks more sensible too me than the old code. I believe using
set_git_dir() is the safer choice, and should not do any harm.
So I stopped analyzing too much, and instead proposed to use
set_git_dir().
Interesting, though, is to find out if we have other potentially
dangerous calls to getenv() that are not removed by this patch.
Steffen
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
