[RFC] require-work-tree wants more than what its name says

Somebody tried "git pull" from a random place completely outside the work tree, while exporting GIT_DIR and GIT_WORK_TREE that are set to correct places, e.g.

	GIT_WORK_TREE=$HOME/git.git
        GIT_DIR=$GIT_WORK_TREE/.git
        export GIT_WORK_TREE GIT_DIR
        cd /tmp
        git pull

At the beginning of git-pull, we check "require-work-tree" and then "cd-to-toplevel". I _think_ the original intention when I wrote the command was "we MUST have a work tree, our $(cwd) might not be at the top-level directory of it", and no stronger than that. That check is a very sensible thing to do before doing cd-to-toplevel. We check that the place we would want to go exists, and then go there.

But the implementation of require_work_tree we have today is quite different. I don't have energy to dig the history, but currently it says:

 	test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||
 	die "fatal: $0 cannot be used without a working tree."

Which is completely bogus. Even though we may happen to be just outside of it right now, we may have a working tree that we can cd_to_toplevel back to.

I recall there was a discussion sometime last year about this topic, and vaguely recall somebody proposed to swap the order of cd-to-toplevel and require-work-tree. While I agree that would sweep the issue under the rug, I think the right solution would be to apply the attached patch; and then audit all the callers that call "require_work_tree" to see if any of them meant "No, it is not Ok just to have working tree somewhere! I want you to be IN that working tree when you call me", and convert them to call the new require_to_be_in_work_tree instead.

Thoughts?

On Tue, May 03, 2011 at 04:33:41PM -0700, Junio C Hamano wrote:

Yeah, I ran into this just recently when converting merge-one-file to use git-sh-setup. I was surprised that require_work_tree also required one to be inside it. My solution was to call cd_to_toplevel first, as you noted.

Your proposed semantics for require_work_tree make much more sense to me, but I worry about compatibility. We can audit our in-tree scripts, but git-sh-setup is part of the recommended API for external scripts, no? This change might break those scripts, so we would need to do the usual deprecation thing. I'm also concerned that the breakage might be pretty severe. As in, not just "script doesn't work" but "script silently produces incorrect results" or "script deletes data".

For example, the merge-one-file bug I fixed recently was silently producing bogus merges because of a confusion over whether it was in the workdir. Something like "git clean" would be even worse.

-Peff

Jeff King <peff@peff.net> writes:

I am Ok with renaming the thing to "require_work_tree_exists", and all the worry will go away.

On Wed, May 04, 2011 at 08:42:58AM -0700, Junio C Hamano wrote:

Yeah, that seems like a fine solution to me.

-Peff

Jeff King <peff@peff.net> writes:

Ok, then here is a reroll.

-- >8 -- Somebody tried "git pull" from a random place completely outside the work tree, while exporting GIT_DIR and GIT_WORK_TREE that are set to correct places, e.g.

	GIT_WORK_TREE=$HOME/git.git
        GIT_DIR=$GIT_WORK_TREE/.git
        export GIT_WORK_TREE GIT_DIR
        cd /tmp
        git pull

At the beginning of git-pull, we check "require-work-tree" and then "cd-to-toplevel". I _think_ the original intention when I wrote the command was "we MUST have a work tree, our $(cwd) might not be at the top-level directory of it", and no stronger than that. That check is a very sensible thing to do before doing cd-to-toplevel. We check that the place we would want to go exists, and then go there.

But the implementation of require_work_tree we have today is quite different. I don't have energy to dig the history, but currently it says:

 	test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||
 	die "fatal: $0 cannot be used without a working tree."

Which is completely bogus. Even though we may happen to be just outside of it right now, we may have a working tree that we can cd_to_toplevel back to.

Add a function "require_work_tree_exists" that implements the check this function originally intended (this is so that third-party scripts that rely on the current behaviour do not have to get broken), and update all the in-tree users to use it.

Some comments:

 - The call in git-rebase--interactive.sh can actually be removed, I
   think, as it is always called from git-rebase.sh that has done
   cd_to_toplevel already.

 - I am not sure if git-submodule.sh correctly works with the "as long as
   there is a working tree somewhere, it is OK" semantics.  It may have
   to stay "require_work_tree" to ensure that the $cwd is within the
   working tree.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Documentation/git-sh-setup.txt |   11 ++++++++---
 git-am.sh                      |    2 +-
 git-bisect.sh                  |    2 +-
 git-mergetool.sh               |    2 +-
 git-pull.sh                    |    2 +-
 git-rebase--interactive.sh     |    2 +-
 git-rebase.sh                  |    2 +-
 git-sh-setup.sh                |    7 +++++++
 git-stash.sh                   |    2 +-
 git-submodule.sh               |    2 +-
 10 files changed, 23 insertions(+), 11 deletions(-)

On Wed, May 04, 2011 at 07:11:18PM -0700, Junio C Hamano wrote:

Looks sane. A few comments:

AFAICT, it has been that way since 7ae3df8 (Use new semantics of is_bare/inside_git_dir/inside_work_tree, 2007-06-03), which is around the time GIT_WORK_TREE came into existence.

So I think it was a matter of being overly strict then, either accidentally, or because it matched the prior semantics (which is that there either was _no_ work tree, or you were in it).

Overall, I think this is a good thing to do. And for the existing working cases (i.e., when you are in the repo) it should be a no-op. It may be worth doing some basic sanity tests to make sure that the scripts properly work when GIT_DIR and GIT_WORK_TREE point elsewhere from the current working directory. But I confess I am not excited about doing the work to write those tests.

-Peff

On Thu, May 05, 2011 at 12:23:19AM -0400, Jeff King wrote:

Sorry, this should have been the more specific: "i.e., when are you are in the _working tree_".

-Peff

Heya,

On Thu, May 5, 2011 at 04:11, Junio C Hamano <gitster@pobox.com> wrote:

So do we need more tests for git-submodule to find out, or is this a hint to the submodule people to chime in? If so, should they be cc-ed?

Sverre Rabbelier <srabbelier@gmail.com> writes:

I could queue a version without any conversion of in-tree users, so that stakeholders can verify and convert their use of require-work-tree to the new saner alternative one by one. Actually I tend to like that better.

I am not convinced myself if it is a sane use case to run "git pull" from a totally random place and let fetch and merge magically happen somewhere completely unrelated to your current working directory only because you have GIT_DIR and GIT_WORK_TREE set to begin with. After getting into a habit of relying on these environment variables so much that you do not even think about their existence, you will reach a point where you have no idea where to go offhand when a merge conflict actually happens and you have to go there to fix things up.

Junio C Hamano venit, vidit, dixit 04.05.2011 01:33:

I may have been that one that year or another...

Same thoughts as Jeff. git-sh-setup is explicitly meant to be used by scripters, and the users in git.git serve as an example how to use it. If that usage changes it requires a long migration plan.

The only thing I can imagine doing right now is changing require_work_tree() to actually cd to toplevel when possible, so that (like before) on success we're really within. But that changes cwd, of course. In summary, a require_work_tree() now can have three assumptions when it returns with success:

- we have a worktree
- we are within worktree
- cwd has not changed

I'd rather break the last one than the second one, but breaking any may be a problem, depending on the caller.

Michael

On Wed, May 04, 2011 at 10:47:29AM +0200, Michael J Gruber wrote:

Check out some of the older scripts in contrib/examples. Several of them require_work_tree, but do not cd_to_toplevel immediately; instead, they do it much later for some specific bits.

I didn't go through and analyze what would happen in each case if we did the cd_to_toplevel first. I suspect some of it would be pretty mild breakage (like "git commit -F foo" not finding "foo"). But it would be breakage nonetheless.

-Peff

Jeff King <peff@peff.net> writes:

Another datapoint I found was "clean".

It used to say "you have to be inside work tree" in the scripted version. Imagine what builtin/clean.c does today? It has NEED_WORK_TREE bit in the command table in git.c and that bit means "We need to have a work tree that we can later chdir to if we wanted to".