[Bug] git pull doesn't recognize --work-tree parameter

[Bug] git pull doesn't recognize --work-tree parameter

[Kirill Likhodedov]

'git pull' doesn't work from outside the working tree even if '--work-tree' is specified:

# git version
git version 1.7.6
# git --git-dir=/Users/loki/sandbox/git/child/.git --work-tree=/Users/loki/sandbox/git/child pull
fatal: /opt/local/libexec/git-core/git-pull cannot be used without a working tree.

Note that  'git fetch' and 'git merge origin/master' work fine, so 'git pull' should be easy to fix :)

# git --git-dir=/Users/loki/sandbox/git/child/.git --work-tree=/Users/loki/sandbox/git/child merge origin/master
Already up-to-date.


----------------------------------
Kirill Likhodedov
JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"

Re: [Bug] git pull doesn't recognize --work-tree parameter

[Nguyen Thai Ngoc Duy]

On Thu, Oct 13, 2011 at 7:38 PM, Kirill Likhodedov
<kirill.likhodedov@jetbrains.com> wrote:
>
> 'git pull' doesn't work from outside the working tree even if '--work-tree' is specified:
>
> # git version
> git version 1.7.6
> # git --git-dir=/Users/loki/sandbox/git/child/.git --work-tree=/Users/loki/sandbox/git/child pull
> fatal: /opt/local/libexec/git-core/git-pull cannot be used without a working tree.
>
> Note that  'git fetch' and 'git merge origin/master' work fine, so 'git pull' should be easy to fix :)

Not exactly. git-pull is a shell script and may have problems that a
builtin command like git-fetch never has. An "easy" way is to just
build git-pull in. I have such a (stalled) WIP since May. Thanks for
reminding me to finish my work :)
-- 
Duy

Re: [Bug] git pull doesn't recognize --work-tree parameter

[Jeff King]

On Thu, Oct 13, 2011 at 12:38:53PM +0400, Kirill Likhodedov wrote:

> 'git pull' doesn't work from outside the working tree even if '--work-tree' is specified:
> 
> # git version
> git version 1.7.6
> # git --git-dir=/Users/loki/sandbox/git/child/.git --work-tree=/Users/loki/sandbox/git/child pull
> fatal: /opt/local/libexec/git-core/git-pull cannot be used without a working tree.
> 
> Note that  'git fetch' and 'git merge origin/master' work fine, so 'git pull' should be easy to fix :)
> 
> # git --git-dir=/Users/loki/sandbox/git/child/.git --work-tree=/Users/loki/sandbox/git/child merge origin/master
> Already up-to-date.

This is a known issue, and the fix is a one-liner, but it needed
somebody to look through the pull script to make sure it wasn't
introducing any new bugs. I just did this; the patch below should fix
your problem.

-- >8 --
Subject: [PATCH] pull,rebase: handle GIT_WORK_TREE better

You can't currently run git-pull or git-rebase from outside
of the work tree, even with GIT_WORK_TREE set, due to an
overeager require_work_tree function. Commit e2eb527
documents this problem and provides the infrastructure for a
fix, but left it to later commits to audit and update
individual scripts.

Changing these scripts to use require_work_tree_exists is
easy to verify. We immediately call cd_to_toplevel, anyway.
Therefore no matter which function we use, the state
afterwards is one of:

  1. We have a work tree, and we are at the top level.

  2. We don't have a work tree, and we have died.

The only catch is that we must also make sure no code that
ran before the cd_to_toplevel assumed that we were already
in the working tree.

In this case, we will only have included shell libraries and
called set_reflog_action, neither of which care about the
current working directory at all.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the low-hanging, obviously correct fruit. git-am and
git-stash also immediately cd_to_toplevel, but they look at
$PWD or `git rev-parse --show-prefix` beforehand, which
means those uses have to be audited separately.

 git-pull.sh   |    2 +-
 git-rebase.sh |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 63da37b..902fc4a 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -11,7 +11,7 @@ OPTIONS_SPEC=
 . git-sh-setup
 . git-sh-i18n
 set_reflog_action "pull${1+ $*}"
-require_work_tree
+require_work_tree_exists
 cd_to_toplevel
 
 
diff --git a/git-rebase.sh b/git-rebase.sh
index 6759702..00ca7b9 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -63,7 +63,7 @@ skip!              skip current patch and continue
 "
 . git-sh-setup
 set_reflog_action rebase
-require_work_tree
+require_work_tree_exists
 cd_to_toplevel
 
 LF='
-- 
1.7.6.4.37.g43b58b

Re: [Bug] git pull doesn't recognize --work-tree parameter

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Changing these scripts to use require_work_tree_exists is
> easy to verify. We immediately call cd_to_toplevel, anyway.
> Therefore no matter which function we use, the state
> afterwards is one of:
>
>   1. We have a work tree, and we are at the top level.
>
>   2. We don't have a work tree, and we have died.
>
> The only catch is that we must also make sure no code that
> ran before the cd_to_toplevel assumed that we were already
> in the working tree.
>
> In this case, we will only have included shell libraries and
> called set_reflog_action, neither of which care about the
> current working directory at all.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is the low-hanging, obviously correct fruit.

I am not absolutely sure about "obviously correct", given that you assume
that cd_to_toplevel does what its name makes you think it does.  I've been
wondering if we want to do give a bit more sanity to "cd_to_toplevel" and
"rev-parse --show-toplevel".

    $ pwd
    /srv/project/git/git.git
    $ (cd Documentation/howto && git rev-parse --show-toplevel); echo $?
    /srv/project/git/git.git
    0

So far so good, however:

    $ (cd .git/refs/heads && git rev-parse --show-toplevel); echo $?
    0

I do not think this is quite right.

We would probably want to add "rev-parse --show-work-tree", but we would
need to audit the users of cd_to_toplevel before starting to use it.  I
wouldn't be surprised if there is a script that creates a temporary work
tree in .git/some/where and runs the scripted Porcelains without setting
GIT_WORK_TREE, relying on the historical behaviour of cd_to_toplevel that
does not really go to the top level.

Re: [Bug] git pull doesn't recognize --work-tree parameter

[Jeff King]

On Thu, Oct 13, 2011 at 11:01:13AM -0700, Junio C Hamano wrote:

> I am not absolutely sure about "obviously correct", given that you assume
> that cd_to_toplevel does what its name makes you think it does.  I've been
> wondering if we want to do give a bit more sanity to "cd_to_toplevel" and
> "rev-parse --show-toplevel".
> 
>     $ pwd
>     /srv/project/git/git.git
>     $ (cd Documentation/howto && git rev-parse --show-toplevel); echo $?
>     /srv/project/git/git.git
>     0
> 
> So far so good, however:
> 
>     $ (cd .git/refs/heads && git rev-parse --show-toplevel); echo $?
>     0
> 
> I do not think this is quite right.

Ugh. You are right. I for some reason assumed that cd_to_toplevel would,
of all things, cd to the toplevel.  I think the right solution is to
introduce a "cd_to_work_tree_toplevel" (or similarly named) command that
always moves to the root of the work tree.

And then convert the two scripts in my patch to use it (along with the
change to require_work_tree_exists).  That would make my prior analysis
hold, then, as the annoying do-nothing behavior of "cd_to_toplevel" only
kicks in when we are outside the work tree (i.e., it could not have
happened before in those scripts, because the existing require_work_tree
call would cause us to die).

> We would probably want to add "rev-parse --show-work-tree", but we would
> need to audit the users of cd_to_toplevel before starting to use it.  I
> wouldn't be surprised if there is a script that creates a temporary work
> tree in .git/some/where and runs the scripted Porcelains without setting
> GIT_WORK_TREE, relying on the historical behaviour of cd_to_toplevel that
> does not really go to the top level.

Right. I suspect the proposed behavior for cd_to_toplevel is what they
all would want eventually, but some scripts may need minor tweaks. I
think we should follow the same path as require_work_tree_exists, and
introduce the new function, use it where we know it's safe, and then
eventually get rid of the old one.

The real trick is coming up with a good name, because cd_to_toplevel is
already taken. :)

-Peff

Re: [Bug] git pull doesn't recognize --work-tree parameter

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> And then convert the two scripts in my patch to use it (along with the
> change to require_work_tree_exists).  That would make my prior analysis
> hold, then, as the annoying do-nothing behavior of "cd_to_toplevel" only
> kicks in when we are outside the work tree (i.e., it could not have
> happened before in those scripts, because the existing require_work_tree
> call would cause us to die).
> ...
> Right. I suspect the proposed behavior for cd_to_toplevel is what they
> all would want eventually, but some scripts may need minor tweaks. I
> think we should follow the same path as require_work_tree_exists, and
> introduce the new function, use it where we know it's safe, and then
> eventually get rid of the old one.
>
> The real trick is coming up with a good name, because cd_to_toplevel is
> already taken. :)

It is not as simple as that I am afraid. We could introduce cd_to_top with
the new semantics and use it in pull and rebase, but a case that would
break is for a script (let's call that hypothetical operation "git svn
dcommit", even though I do not know if dcommit uses the real working tree
or a temporary one) that prepares a temporary working tree inside .git/svn/
and run "git rebase" there without setting GIT_WORKING_TREE to point at
the temporary directory.

With cd_to_toplevel, such a "rebase" would work and "git svn dcommit" can
take that result and do whatever it wants to the real working tree after
it finishes. When we start using cd_to_top in the updated "rebase", such a
script suddenly breaks, as we would start touching the real working tree.

So I do not think it makes much sense to add cd_to_top with updated
semantics while keeping cd_to_toplevel.

What we could do is to update cd_to_toplevel so that it would notice and
warn when the results between the historical incorrect behaviour and the
updated behaviour would be different. The warning can first read "You are
running 'rebase' somewhere in $GIT_DIR without setting $GIT_WORK_TREE; we
historically used the directory you started 'rebase' as the top level of
the working tree, and this version continues to do so, but it will change
to work on the real working tree associated with your $GIT_DIR in future
versions of git. Update your script to correctly set $GIT_WORK_TREE", and
then we transition to start using the new semantics while rewording the
warning message, and then later remove the warning altogether.

Re: [Bug] git pull doesn't recognize --work-tree parameter

[Jeff King]

On Thu, Oct 13, 2011 at 12:06:46PM -0700, Junio C Hamano wrote:

> It is not as simple as that I am afraid. We could introduce cd_to_top with
> the new semantics and use it in pull and rebase, but a case that would
> break is for a script (let's call that hypothetical operation "git svn
> dcommit", even though I do not know if dcommit uses the real working tree
> or a temporary one) that prepares a temporary working tree inside .git/svn/
> and run "git rebase" there without setting GIT_WORKING_TREE to point at
> the temporary directory.

I didn't think that could happen now, because you would not be in the
working tree, and therefore require_work_tree would fail. E.g., with
current git I get:

  $ mkdir .git/tmp
  $ cd .git/tmp
  $ git rebase
  fatal: fatal: /home/peff/local/git/private/libexec/git-core/git-rebase
  cannot be used without a working tree.

So that case is already broken. The only change this would make is that
what used to fail would not actually take them to the top-level of the
working tree[1].

-Peff

[1] Actually, I am not sure it would do that. If we are in $GIT_DIR, do
we necessarily know where the working tree is? I guess in a non-bare
repo, we assume it is $GIT_DIR/..?

Re: [Bug] git pull doesn't recognize --work-tree parameter

[Jeff King]

On Thu, Oct 13, 2011 at 03:14:57PM -0400, Jeff King wrote:

> On Thu, Oct 13, 2011 at 12:06:46PM -0700, Junio C Hamano wrote:
> 
> > It is not as simple as that I am afraid. We could introduce cd_to_top with
> > the new semantics and use it in pull and rebase, but a case that would
> > break is for a script (let's call that hypothetical operation "git svn
> > dcommit", even though I do not know if dcommit uses the real working tree
> > or a temporary one) that prepares a temporary working tree inside .git/svn/
> > and run "git rebase" there without setting GIT_WORKING_TREE to point at
> > the temporary directory.
> 
> I didn't think that could happen now, because you would not be in the
> working tree, and therefore require_work_tree would fail. E.g., with
> current git I get:
> 
>   $ mkdir .git/tmp
>   $ cd .git/tmp
>   $ git rebase
>   fatal: fatal: /home/peff/local/git/private/libexec/git-core/git-rebase
>   cannot be used without a working tree.
> 
> So that case is already broken. The only change this would make is that
> what used to fail would not actually take them to the top-level of the
> working tree[1].

Ugh. It does work if you do:

  mkdir .git/tmp
  cd .git/tmp
  GIT_DIR=$PWD/.. git rebase

What a god-awful mess our initialization rules are.

-Peff

Re: [Bug] git pull doesn't recognize --work-tree parameter

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I didn't think that could happen now, because you would not be in the
> working tree, and therefore require_work_tree would fail.

Ok, then I was worried about a non-existing problem, which is good.

Then we can introduce cd_to_top that is more sensible than cd_to_toplevel
and perhaps add some warning to the latter about deprecation and
migration.

Thanks.

Re: [Bug] git pull doesn't recognize --work-tree parameter

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>   $ mkdir .git/tmp
>>   $ cd .git/tmp
>>   $ git rebase
>>   fatal: fatal: /home/peff/local/git/private/libexec/git-core/git-rebase
>>   cannot be used without a working tree.
>> 
>> So that case is already broken. The only change this would make is that
>> what used to fail would not actually take them to the top-level of the
>> working tree[1].
>
> Ugh. It does work if you do:
>
>   mkdir .git/tmp
>   cd .git/tmp
>   GIT_DIR=$PWD/.. git rebase

Actually, this one is OK. Presense of GIT_DIR combined with the lack of
GIT_WORK_TREE means that .git/tmp must be the top of the working tree, so
it is the script's responsibility to populate the directory with what
matches to $GIT_DIR/index between "mkdir" and "rebase" in the above
sequence.

So I suspect we won't have to worry about this case either.