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.
>
>> t/t0001-init.sh | 4 ++--
>> 2 files changed, 50 insertions(+), 6 deletions(-)
>>
>> diff --git a/git.c b/git.c
>> index 7780572..d1e49da 100644
>> --- a/git.c
>> +++ b/git.c
>> @@ -484,6 +521,10 @@ static void handle_builtin(int argc, const char **argv)
>> struct cmd_struct *p = commands+i;
>> if (strcmp(p->cmd, cmd))
>> continue;
>> + if (saved_environment && (p->option & NO_SETUP)) {
>> + restore_env();
>> + break;
>> + }
>> exit(run_builtin(p, argc, argv));
>> }
>> }
>> @@ -539,7 +580,10 @@ static int run_argv(int *argcp, const char ***argv)
>> * of overriding "git log" with "git show" by having
>> * alias.log = show
>> */
>> - if (done_alias || !handle_alias(argcp, argv))
>> + if (done_alias)
>> + break;
>> + save_env();
>> + if (!handle_alias(argcp, argv))
>> break;
>> done_alias = 1;
>> }
>
> So the save-env kicks in only after we tried the builtins and the
> externals and didn't find any, and before expanding the alias (which
> has to look at the config, which means we need to do discovery and
> may contaminate the environment and the globals), and then when we
> retry with the expanded alias, we restore when we know the command
> will misbehave if we didn't do so?
>
> 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(). 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..)
> 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.
--
Duy
--
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