git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC] require-work-tree wants more than what its name says
@ 2011-05-03 23:33 Junio C Hamano
  2011-05-04  7:38 ` Jeff King
  2011-05-04  8:47 ` Michael J Gruber
  0 siblings, 2 replies; 12+ messages in thread
From: Junio C Hamano @ 2011-05-03 23:33 UTC (permalink / raw)
  To: git

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."
 }

^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-05-05 17:31 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-05-03 23:33 [RFC] require-work-tree wants more than what its name says Junio C Hamano
2011-05-04  7:38 ` Jeff King
2011-05-04 15:42   ` Junio C Hamano
2011-05-04 21:28     ` Jeff King
2011-05-05  2:11       ` Junio C Hamano
2011-05-05  4:23         ` Jeff King
2011-05-05  4:28           ` Jeff King
2011-05-05 11:15         ` Sverre Rabbelier
2011-05-05 17:31           ` Junio C Hamano
2011-05-04  8:47 ` Michael J Gruber
2011-05-04  8:50   ` Jeff King
2011-05-04 15:47     ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).