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

Jeff King [off-list ref] writes:

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

quoted

Jeff King [off-list ref] writes:

quoted

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

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

Yeah, that seems like a fine solution to me.

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 <redacted>
---

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

diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt
index 3da2413..1f02c4b 100644
--- a/Documentation/git-sh-setup.txt
+++ b/Documentation/git-sh-setup.txt

@@ -58,9 +58,14 @@ cd_to_toplevel::
 	runs chdir to the toplevel of the working tree.
 
 require_work_tree::
-	checks if the repository is a bare repository, and dies
-	if so.  Used by scripts that require working tree
-	(e.g. `checkout`).
+	checks if the current directory is within the working tree
+	of the repository, and otherwise dies.
+
+require_work_tree_exists::
+	checks if the working tree associated with the repository
+	exists, and otherwise dies.  Often done before calling
+	cd_to_toplevel, which is impossible to do if there is no
+	working tree.
 
 get_author_ident_from_commit::
 	outputs code for use with eval to set the GIT_AUTHOR_NAME,

diff --git a/git-am.sh b/git-am.sh
index 6cdd591..430bbd5 100755
--- a/git-am.sh
+++ b/git-am.sh

@@ -39,7 +39,7 @@ rebasing*       (internal use for git-rebase)"
 . git-sh-setup
 prefix=$(git rev-parse --show-prefix)
 set_reflog_action am
-require_work_tree
+require_work_tree_exists
 cd_to_toplevel
 
 git var GIT_COMMITTER_IDENT >/dev/null ||

diff --git a/git-bisect.sh b/git-bisect.sh
index c21e33c..f46dda2 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -28,7 +28,7 @@ Please use "git help bisect" to get the full man page.'
 
 OPTIONS_SPEC=
 . git-sh-setup
-require_work_tree
+require_work_tree_exists
 
 _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
 _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 2f8dc44..8a74d00 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh

@@ -14,7 +14,7 @@ OPTIONS_SPEC=
 TOOL_MODE=merge
 . git-sh-setup
 . git-mergetool--lib
-require_work_tree
+require_work_tree_exists
 
 # Returns true if the mode reflects a symlink
 is_symlink () {

diff --git a/git-pull.sh b/git-pull.sh
index eb87f49..7500b58 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -10,7 +10,7 @@ SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 . git-sh-setup
 set_reflog_action "pull $*"
-require_work_tree
+require_work_tree_exists
 cd_to_toplevel
 
 

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5873ba4..051a4ad 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -34,7 +34,7 @@ autosquash         move commits that begin with squash!/fixup! under -i
 "
 
 . git-sh-setup
-require_work_tree
+require_work_tree_exists
 
 DOTEST="$GIT_DIR/rebase-merge"
 

diff --git a/git-rebase.sh b/git-rebase.sh
index cbb0ea9..60a405d 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -31,7 +31,7 @@ SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 . git-sh-setup
 set_reflog_action rebase
-require_work_tree
+require_work_tree_exists
 cd_to_toplevel
 
 LF='

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index aa16b83..94e26ed 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh

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

diff --git a/git-stash.sh b/git-stash.sh
index 7561b37..ce633f3 100755
--- a/git-stash.sh
+++ b/git-stash.sh

@@ -13,7 +13,7 @@ USAGE="list [<options>]
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 . git-sh-setup
-require_work_tree
+require_work_tree_exists
 cd_to_toplevel
 
 TMP="$GIT_DIR/.git-stash.$$"

diff --git a/git-submodule.sh b/git-submodule.sh
index 8b90589..f2a541e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -15,7 +15,7 @@ USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <r
 OPTIONS_SPEC=
 . git-sh-setup
 . git-parse-remote
-require_work_tree
+require_work_tree_exists
 
 command=
 branch=

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

quoted

quoted

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

Yeah, that seems like a fine solution to me.

Ok, then here is a reroll.

Looks sane. A few comments:

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.

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

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.

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:

quoted

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.

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

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 [off-list ref] wrote:

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

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?

-- 
Cheers,

Sverre Rabbelier