Duy Nguyen <pclouds@xxxxxxxxx> writes: > On Wed, Jun 11, 2014 at 1:48 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: >> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes: >> >>> Commit 4ad8332 (t0001: test git init when run via an alias - >>> 2010-11-26) noted breakages when running init via alias. The problem >>> is for alias to be used, $GIT_DIR must be searched, but 'init' and >>> 'clone' are not happy with that. So we start a new process like an >>> external command, with clean environment in this case. Env variables >>> that are set by command line (e.g. "git --git-dir=.. ") are kept. >>> >>> This should also fix autocorrecting a command typo to "init" because >>> it's the same problem: aliases are read, then "init" is unhappy with >>> $GIT_DIR already set up because of that. >>> >>> Reminded-by: David Turner <dturner@xxxxxxxxxxxxxxxx> >>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> >>> --- >>> git.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++---- >> >> This goes far deeper than "Fix t0001", doesn't it? > > I followed the way git-revert creates the subject line, thinking that > this fixes an old commit so do similarly. Probably better rephrase it. Yeah, I didn't realize the whole thing was in dq and by that you meant that you are referring to the commit. >> That does not sound so bad. Even though I wonder if that "save and >> then restore" sequence logically belongs around handle_alias(), you >> would not have sufficient clue to let you cheat by not restoring the >> environment for commands that you happen to know that they do not >> care, so that may be a reasonable optimization. > > The save code definitely belongs to handle_alias(). I'm not so > confident about always restoring at the end of handle_alias(). Oh, I am not saying we should restore unconditionally. I was just reading your patch that does not have the restore at the end of handle_alias and trying to rationalize it myself---that code does not know (yet) if the command to be run wants to get the environment restored. > The > restore procedure is just enough not to propagate wrong info to the > child process. For that purpose, guarding cwd and environm are enough. > If after we return from handle_alias() and we run the builtin command > anyway, that' may not be clean enough (e.g. static variables may be > already initialized..) Very true. The "contamination" by the discovery process has got bad enough over time. >> Is it too brittle a solution to force people to mark problematic >> subcommands with NO_SETUP, though? What kind of change to a >> subcommand that currently does not have to be marked with NO_SETUP >> would make it necessary to mark it with NO_SETUP? > > If I had a clear answer here, I could have undone the setup effects > caused by handle_alias() and not resort to spawning a new process :) > So my answer is mostly trial and error. We have evidence that clone > and init do not work with contaminated environment. So we fix them and > wait for new bugs to show up. OK. -- 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