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

From: Junio C Hamano <gitster@pobox.com>
Date: May 3, 2011, 23:33 UTC
Message-ID: <7vhb9bgy0a.fsf@alter.siamese.dyndns.org>

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?

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index aa16b83..0b25f12 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -141,6 +141,13 @@ cd_to_toplevel () {
 }
 
 require_work_tree () {
+	if test "z$(git rev-parse --is-bare-repository)" != zfalse
+	then
+		die "fatal: $0 cannot be used without a working tree."
+	fi
+}
+
+require_to_be_in_work_tree () {
 	test "$(git rev-parse --is-inside-work-tree 2>/dev/null)" = true ||
 	die "fatal: $0 cannot be used without a working tree."
 }