Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)

[Junio C Hamano]

Duy Nguyen  writes:

> ... But let me sleep on it (and everybody please
> give ideas if you have any). Meanwhile, maybe reverting d95138e should
> be done any way for now. Broken aliases are not as bad as broken
> hooks.

OK, when/if you decide that our first step should be a revert of
d95138e please send in a patch to do so with a brief write-up of a
follow-up plan.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)

[Duy Nguyen]

On Mon, Nov 30, 2015 at 9:16 PM, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>> I was wrong, GIT_WORK_TREE support was added in git-clone many years
>> ago in 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06). So
>> my change accidentally triggers an (undocumented) feature. We could
>> add a hack to ignore GIT_WORK_TREE if GIT_DIR is set too, but I don't
>> think people will like it. I don't really like reverting d95138e
>> (setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR -
>> 2015-06-26) because another bug reappears.
>
>> So I'm out of options..
>
> Perhaps d95138e can be reverted and then the bug it tried to fix can
> be fixed in a different way somehow?
>
> (I am not quite up to speed yet, so the above may turn out to be
> infeasible--take it with a large grain of salt please).

That would mean we do not set $GIT_DIR too early. While it sounds
good, it could be just another trap, and could be a lot of
reorganizing setup code. I'm more tempted to revert 20ccef4, with
deprecation warning for some releases, and a new git-clone option for
the same functionality. But let me sleep on it (and everybody please
give ideas if you have any). Meanwhile, maybe reverting d95138e should
be done any way for now. Broken aliases are not as bad as broken
hooks.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)

[Duy Nguyen]

On Wed, Nov 25, 2015 at 9:13 PM, Duy Nguyen  wrote:
> On Tue, Nov 24, 2015 at 6:57 PM, Stefan Beller  wrote:
>> +to Nguyễn Thái Ngọc Duy 
>>
>> On Mon, Nov 23, 2015 at 6:22 PM, Anthony Sottile  wrote:
>>> * Short description of the problem *
>>>
>>> It seems GIT_WORK_DIR is now exported invariantly when calling git
>>> hooks such as pre-commit.  If these hooks involve cloning repositories
>>> they will not fail due to this exported environment variable.  This
>>> was not the case in prior versions (such as v2.5.0).
>
> I'm getting good at fixing one bug and adding ten more. I don't think
> the cited commit is the problem. It just exposes another bug. I did
>
>> ~/w/git $ GIT_WORK_TREE=abc ./git clone .git /tmp/def
>
> and what I got was really surprising, /tmp/def contains the git
> repository while the true worktree is in "abc". It does not make
> sense, at least from the first sight, unless it inherits this from
> git-init, where we do(?)  want GIT_WORK_TREE to specify a separate
> worktree. No time to dig to the bottom yet..

I was wrong, GIT_WORK_TREE support was added in git-clone many years
ago in 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06). So
my change accidentally triggers an (undocumented) feature. We could
add a hack to ignore GIT_WORK_TREE if GIT_DIR is set too, but I don't
think people will like it. I don't really like reverting d95138e
(setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR -
2015-06-26) because another bug reappears. So I'm out of options..
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)

[Junio C Hamano]

Duy Nguyen  writes:

> I was wrong, GIT_WORK_TREE support was added in git-clone many years
> ago in 20ccef4 (make git-clone GIT_WORK_TREE aware - 2007-07-06). So
> my change accidentally triggers an (undocumented) feature. We could
> add a hack to ignore GIT_WORK_TREE if GIT_DIR is set too, but I don't
> think people will like it. I don't really like reverting d95138e
> (setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR -
> 2015-06-26) because another bug reappears.

> So I'm out of options..

Perhaps d95138e can be reverted and then the bug it tried to fix can
be fixed in a different way somehow?

(I am not quite up to speed yet, so the above may turn out to be
infeasible--take it with a large grain of salt please).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)

[Duy Nguyen]

On Tue, Nov 24, 2015 at 6:57 PM, Stefan Beller  wrote:
> +to Nguyễn Thái Ngọc Duy 
>
> On Mon, Nov 23, 2015 at 6:22 PM, Anthony Sottile  wrote:
>> * Short description of the problem *
>>
>> It seems GIT_WORK_DIR is now exported invariantly when calling git
>> hooks such as pre-commit.  If these hooks involve cloning repositories
>> they will not fail due to this exported environment variable.  This
>> was not the case in prior versions (such as v2.5.0).

I'm getting good at fixing one bug and adding ten more. I don't think
the cited commit is the problem. It just exposes another bug. I did

> ~/w/git $ GIT_WORK_TREE=abc ./git clone .git /tmp/def

and what I got was really surprising, /tmp/def contains the git
repository while the true worktree is in "abc". It does not make
sense, at least from the first sight, unless it inherits this from
git-init, where we do(?)  want GIT_WORK_TREE to specify a separate
worktree. No time to dig to the bottom yet..
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git clone fails during pre-commit hook due to GIT_WORK_TREE=. (regression 2.5 -> 2.6)

[Stefan Beller]

+to Nguyễn Thái Ngọc Duy 

On Mon, Nov 23, 2015 at 6:22 PM, Anthony Sottile  wrote:
> * Short description of the problem *
>
> It seems GIT_WORK_DIR is now exported invariantly when calling git
> hooks such as pre-commit.  If these hooks involve cloning repositories
> they will not fail due to this exported environment variable.  This
> was not the case in prior versions (such as v2.5.0).
>
> * Simple reproduction *
>
> ```
> $ cat test.sh
> #!/usr/bin/env bash
> set -ex
>
> rm -rf test
>
> # Exit non {0, 1} to abort git bisect
> make -j 8 > /dev/null || exit 2
>
> # Put our new git on the path
> PATH="$(pwd):$PATH"
>
> git init test
>
> pushd test
> mkdir -p .git/hooks
> echo 'git clone git://github.com/asottile/css-explore css-explore' >
> .git/hooks/pre-commit
> chmod 755 .git/hooks/pre-commit
>
> git commit -m foo --allow-empty || exit 1
> ```
>
> * Under 2.6.3 *
>
> ```
> $ ./test.sh
>
> ...
>
> + git init test
> warning: templates not found /home/anthony/share/git-core/templates
> Initialized empty Git repository in /home/anthony/workspace/git/test/.git/
> + pushd test
> ~/workspace/git/test ~/workspace/git
> + mkdir -p .git/hooks
> + echo 'git clone git://github.com/asottile/css-explore css-explore'
> + chmod 755 .git/hooks/pre-commit
> + git commit -m foo --allow-empty
> fatal: working tree '.' already exists.
> + exit 1
> ```
>
> * Under 2.5 *
>
> ```
> $ ./test.sh
>
> ...
>
> + git init test
> warning: templates not found /home/anthony/share/git-core/templates
> Initialized empty Git repository in /home/anthony/workspace/git/test/.git/
> + pushd test
> ~/workspace/git/test ~/workspace/git
> + mkdir -p .git/hooks
> + echo 'git clone git://github.com/asottile/css-explore css-explore'
> + chmod 755 .git/hooks/pre-commit
> + git commit -m foo --allow-empty
> Cloning into 'css-explore'...
> warning: templates not found /home/anthony/share/git-core/templates
> remote: Counting objects: 214, done.
> remote: Total 214 (delta 0), reused 0 (delta 0), pack-reused 214
> Receiving objects: 100% (214/214), 25.89 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (129/129), done.
> Checking connectivity... done.
> [master (root-commit) 5eb999d] foo
> ```
>
>
> * Bisect *
>
> ```
> $ git bisect good v2.5.0
> $ git bisect bad origin/master
> $ git bisect run ./test.sh
>
> ...
>
> d95138e695d99d32dcad528a2a7974f434c51e79 is the first bad commit
> commit d95138e695d99d32dcad528a2a7974f434c51e79
> Author: Nguyễn Thái Ngọc Duy 
> Date:   Fri Jun 26 17:37:35 2015 +0700
>
> setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR
>
> In the test case, we run setup_git_dir_gently() the first time to read
> $GIT_DIR/config so that we can resolve aliases. We'll enter
> setup_discovered_git_dir() and may or may not call set_git_dir() near
> the end of the function, depending on whether the detected git dir is
> ".git" or not. This set_git_dir() will set env var $GIT_DIR.
>
> For normal repo, git dir detected via setup_discovered_git_dir() will be
> ".git", and set_git_dir() is not called. If .git file is used however,
> the git dir can't be ".git" and set_git_dir() is called and $GIT_DIR
> set. This is the key of this problem.
>
> If we expand an alias (or autocorrect command names), then
> setup_git_dir_gently() is run the second time. If $GIT_DIR is not set in
> the first run, we run the same setup_discovered_git_dir() as before.
> Nothing to see. If it is, however, we'll enter setup_explicit_git_dir()
> this time.
>
> This is where the "fun" is.  If $GIT_WORK_TREE is not set but
> $GIT_DIR is, you are supposed to be at the root level of the
> worktree.  But if you are in a subdir "foo/bar" (real worktree's top
> is "foo"), this rule bites you: your detected worktree is now
> "foo/bar", even though the first run correctly detected worktree as
> "foo". You get "internal error: work tree has already been set" as a
> result.
>
> Bottom line is, when $GIT_DIR is set, $GIT_WORK_TREE should be set too
> unless there's no work tree. But setting $GIT_WORK_TREE inside
> set_git_dir() may backfire. We don't know at that point if work tree is
> already configured by the caller. So set it when work tree is
> detected. It does not harm if $GIT_WORK_TREE is set while $GIT_DIR is
> not.
>
> Reported-by: Bjørnar Snoksrud 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> Signed-off-by: Junio C Hamano 
>
> :100644 100644 9daa0ba4a36ced9f63541203e7bcc2ab9e1eae56
> 36fbba57fc83afd36d99bf5d4f3a1fc3feefba09 Menvironment.c
> :04 04 1d7c4bf77e0fd49ca315271993cb69a8b055c3aa
> 145d85895cb6cb0810597e1854a7721ccfc8f457 Mt
> bisect run success
> ```
>
> Causing me a few headaches in
> https://github.com/pre-commit/pre-commit/issues/300
> I'm working around it in