Re: [PATCH] Fix "t0001: test git init when run via an alias"

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 12, 2014, 17:11 UTC
Message-ID: <xmqqr42u6oje.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACsJy8Dh9miVOGM0JLWXs6kzzEnum1ggnc2qm0gKaCwX+9iZYQ@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 23 quoted lines

> On Wed, Jun 11, 2014 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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@twopensource.com>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>>  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.

Show 8 quoted lines

>> 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.

Show 6 quoted lines

> 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.

Show 10 quoted lines

>> 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.