From: Junio C Hamano <gitster@pobox•com>
To: Duy Nguyen <pclouds@gmail•com>
Cc: Git Mailing List <git@vger•kernel.org>,
Jonathan Niedier <jrnieder@gmail•com>,
David Turner <dturner@twopensource•com>
Subject: Re: [PATCH] Fix "t0001: test git init when run via an alias"
Date: Thu, 12 Jun 2014 10:11:33 -0700 [thread overview]
Message-ID: <xmqqr42u6oje.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CACsJy8Dh9miVOGM0JLWXs6kzzEnum1ggnc2qm0gKaCwX+9iZYQ@mail.gmail.com> (Duy Nguyen's message of "Wed, 11 Jun 2014 17:57:50 +0700")
Duy Nguyen <pclouds@gmail•com> writes:
> 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.
>> 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.
next prev parent reply other threads:[~2014-06-12 17:11 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-05 3:49 Git autocorrect bug David Turner
2014-06-05 6:06 ` Øystein Walle
2014-06-05 6:10 ` Øystein Walle
2014-06-05 6:29 ` Duy Nguyen
2014-06-05 8:28 ` David Turner
2014-06-06 11:09 ` Duy Nguyen
2014-06-08 9:37 ` [PATCH] Fix "t0001: test git init when run via an alias" Nguyễn Thái Ngọc Duy
2014-06-10 18:48 ` Junio C Hamano
2014-06-11 10:57 ` Duy Nguyen
2014-06-12 17:11 ` Junio C Hamano [this message]
2014-06-10 18:53 ` Junio C Hamano
2014-06-10 19:09 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqr42u6oje.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=dturner@twopensource$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jrnieder@gmail$(echo .)com \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox