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

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

[Junio C Hamano]

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

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

[Jeff King]

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

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

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.

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

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

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 04.05.2011 01:33:
> 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 may have been that one that year or another...

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

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

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > ... 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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, May 04, 2011 at 08:42:58AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > ... 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 <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(-)

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=

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Sverre Rabbelier]

Heya,

On Thu, May 5, 2011 at 04:11, Junio C Hamano <gitster@pobox.com> 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

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

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

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

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.