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

'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!"

On Thu, Oct 13, 2011 at 7:38 PM, Kirill Likhodedov <kirill.likhodedov@jetbrains.com> wrote:

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 :)

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

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(-)

Jeff King <peff@peff.net> writes:

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.

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

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

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

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

Jeff King <peff@peff.net> writes:

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.

Jeff King <peff@peff.net> writes:

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.