[RFC PATCH] prefix_path: show gitdir when arg is outside repo

From: Emily Shaffer <redacted>

When developing a script, it can be painful to understand why Git thinks
something is outside the current repo, if the current repo isn't what
the user thinks it is. Since this can be tricky to diagnose, especially
in cases like submodules or nested worktrees, let's give the user a hint
about which repository is offended about that path.

Signed-off-by: Emily Shaffer <redacted>
---
This one comes from a user feature request. This user is running some
Git client commands on a build machine somewhere and finding it hard to
reason about the cause of the "outside repo" error.

I see two arguments:

For:
 - A user checking their own `pwd` might still not come to the same
   conclusion Git does about the current repo, if their filesystem is in
   some weird state
 - This warning is intended for human eyes (die(), stderr) so it's reasonable
   to give some info to make the human's life easier

Against:
 - It's chatty, especially given the absolute directory. This may be a
   pretty common mistake ('git add' with thumbfingers?) so it could be
   chatty, frequently - not great.
   (Sidebar: Just including the relative directory is really not very
   useful - since you're still left thinking, "relative to where?")

I also dug around a little to see whether I could consolidate the
pathspec.c logic, which is nearly identical to setup.c, into another
helper in setup.c (a la prefix_path_1()). But since the die() message is
somewhat different, and init_pathspec_item() is the _only_ place which
uses prefix_path_gently() in this way, it wasn't feasible. (The other
caller of prefix_path_gently() is immediately below prefix_path(), and
doesn't die at all, print errors, or capture out-params from
prefix_path_gently().)

 - Emily

 pathspec.c | 3 ++-
 setup.c    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 128f27fcb7..5d661df5cf 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -439,7 +439,8 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		match = prefix_path_gently(prefix, prefixlen,
 					   &prefixlen, copyfrom);
 		if (!match)
-			die(_("%s: '%s' is outside repository"), elt, copyfrom);
+			die(_("%s: '%s' is outside repository at '%s'"), elt,
+			    copyfrom, absolute_path(get_git_dir()));
 	}
 
 	item->match = match;

diff --git a/setup.c b/setup.c
index 12228c0d9c..48cc2320cb 100644
--- a/setup.c
+++ b/setup.c

@@ -121,7 +121,8 @@ char *prefix_path(const char *prefix, int len, const char *path)
 {
 	char *r = prefix_path_gently(prefix, len, NULL, path);
 	if (!r)
-		die(_("'%s' is outside repository"), path);
+		die(_("'%s' is outside repository at '%s'"), path,
+		    absolute_path(get_git_dir()));
 	return r;
 }
 

-- 
2.25.0.265.gbab2e86ba0-goog

On 2020-02-14 at 23:29:33, emilyshaffer@google.com wrote:

From: Emily Shaffer <redacted>

When developing a script, it can be painful to understand why Git thinks
something is outside the current repo, if the current repo isn't what
the user thinks it is. Since this can be tricky to diagnose, especially
in cases like submodules or nested worktrees, let's give the user a hint
about which repository is offended about that path.

Signed-off-by: Emily Shaffer <redacted>
---
This one comes from a user feature request. This user is running some
Git client commands on a build machine somewhere and finding it hard to
reason about the cause of the "outside repo" error.

I see two arguments:

For:
 - A user checking their own `pwd` might still not come to the same
   conclusion Git does about the current repo, if their filesystem is in
   some weird state
 - This warning is intended for human eyes (die(), stderr) so it's reasonable
   to give some info to make the human's life easier

Against:
 - It's chatty, especially given the absolute directory. This may be a
   pretty common mistake ('git add' with thumbfingers?) so it could be
   chatty, frequently - not great.
   (Sidebar: Just including the relative directory is really not very
   useful - since you're still left thinking, "relative to where?")

I'm very much in favor of this patch.  I recently ran into a similar
problem with Git LFS with path canonicalization and having both paths in
the error message made it immediately obvious what the problem was.

Do we want the top level directory in these two spots instead of the git
directory?  I suspect that might be more helpful, since it looks like
we're dealing with working tree files.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

On Sat, Feb 15, 2020 at 12:02:30AM +0000, brian m. carlson wrote:

On 2020-02-14 at 23:29:33, emilyshaffer@google.com wrote:

quoted

From: Emily Shaffer <redacted>

When developing a script, it can be painful to understand why Git thinks
something is outside the current repo, if the current repo isn't what
the user thinks it is. Since this can be tricky to diagnose, especially
in cases like submodules or nested worktrees, let's give the user a hint
about which repository is offended about that path.

Signed-off-by: Emily Shaffer <redacted>
---
This one comes from a user feature request. This user is running some
Git client commands on a build machine somewhere and finding it hard to
reason about the cause of the "outside repo" error.

I see two arguments:

For:
 - A user checking their own `pwd` might still not come to the same
   conclusion Git does about the current repo, if their filesystem is in
   some weird state
 - This warning is intended for human eyes (die(), stderr) so it's reasonable
   to give some info to make the human's life easier

Against:
 - It's chatty, especially given the absolute directory. This may be a
   pretty common mistake ('git add' with thumbfingers?) so it could be
   chatty, frequently - not great.
   (Sidebar: Just including the relative directory is really not very
   useful - since you're still left thinking, "relative to where?")

I'm very much in favor of this patch.  I recently ran into a similar
problem with Git LFS with path canonicalization and having both paths in
the error message made it immediately obvious what the problem was.

quoted

diff --git a/pathspec.c b/pathspec.c
index 128f27fcb7..5d661df5cf 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -439,7 +439,8 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		match = prefix_path_gently(prefix, prefixlen,
 					   &prefixlen, copyfrom);
 		if (!match)
-			die(_("%s: '%s' is outside repository"), elt, copyfrom);
+			die(_("%s: '%s' is outside repository at '%s'"), elt,
+			    copyfrom, absolute_path(get_git_dir()));

Do we want the top level directory in these two spots instead of the git
directory?  I suspect that might be more helpful, since it looks like
we're dealing with working tree files.

I had considered it, and thought .git directory was less ambiguous,
primarily for the first bullet in the "For" list above - "for some
reason, the .git dir I see isn't the one that Git is coming up with".
The .git dir does have a pointer to the worktree ('gitdir' in the
worktree case and 'config' in the submodule case). Conversely, in both
the submodule and worktree cases the worktree contains a ".git" file
with the path to the gitdir.

I also tried the following:

  $ git clone --separate-git-dir=explicit-gitdir https://github.com/git/git
  explicit-worktree
  $ cd explicit-gitdir
  $ grep -Rn "explicit-worktree"
  <no results>
  $ cd ../explicit-worktree
  $ cat .git
  <absolute path to explicit-gitdir>

So it looks like in the clone with --separate-git-dir case, there's no
way to identify the worktree from the gitdir.

All this to say, on second thought, I think you're right. From the
worktree's top level, the gitdir is consistently findable. From gitdir,
the worktree is not consistently findable.

I'll send a reroll.

 - Emily

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

When developing a script, it can be painful to understand why Git thinks
something is outside the current repo, if the current repo isn't what
the user thinks it is. Since this can be tricky to diagnose, especially
in cases like submodules or nested worktrees, let's give the user a hint
about which repository is offended about that path.

Signed-off-by: Emily Shaffer <redacted>
---
As suggested by brian, print the worktree instead. See
https://lore.kernel.org/git/20200215004606.GM190927@google.com.

 pathspec.c | 3 ++-
 setup.c    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 128f27fcb7..166d255642 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -439,7 +439,8 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		match = prefix_path_gently(prefix, prefixlen,
 					   &prefixlen, copyfrom);
 		if (!match)
-			die(_("%s: '%s' is outside repository"), elt, copyfrom);
+			die(_("%s: '%s' is outside repository at '%s'"), elt,
+			    copyfrom, absolute_path(get_git_work_tree()));
 	}
 
 	item->match = match;

diff --git a/setup.c b/setup.c
index 12228c0d9c..4ea7a0b081 100644
--- a/setup.c
+++ b/setup.c

@@ -121,7 +121,8 @@ char *prefix_path(const char *prefix, int len, const char *path)
 {
 	char *r = prefix_path_gently(prefix, len, NULL, path);
 	if (!r)
-		die(_("'%s' is outside repository"), path);
+		die(_("'%s' is outside repository at '%s'"), path,
+		    absolute_path(get_git_work_tree()));
 	return r;
 }
 

-- 
2.25.0.265.gbab2e86ba0-goog

On 2020-02-15 at 01:00:13, Emily Shaffer wrote:

When developing a script, it can be painful to understand why Git thinks
something is outside the current repo, if the current repo isn't what
the user thinks it is. Since this can be tricky to diagnose, especially
in cases like submodules or nested worktrees, let's give the user a hint
about which repository is offended about that path.

Signed-off-by: Emily Shaffer <redacted>

This looks good to me.  Thanks for this improvement.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

Hi,

Emily Shaffer wrote:

This is producing segfaults when run by magit.  Reproduction recipe:

	cd .git
	git ls-files ..

Expected result:

	fatal: ..: '..' is outside repository

Actual result:

	Segmentation fault

Does this need an extra case to handle when there is no work tree?

Thanks,
Jonathan

On Fri, Feb 28, 2020 at 11:58:05AM -0800, Jonathan Nieder wrote:

Hi,

Emily Shaffer wrote:

quoted

When developing a script, it can be painful to understand why Git thinks
something is outside the current repo, if the current repo isn't what
the user thinks it is. Since this can be tricky to diagnose, especially
in cases like submodules or nested worktrees, let's give the user a hint
about which repository is offended about that path.

Signed-off-by: Emily Shaffer <redacted>
---
 pathspec.c | 3 ++-
 setup.c    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 128f27fcb7..166d255642 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -439,7 +439,8 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 		match = prefix_path_gently(prefix, prefixlen,
 					   &prefixlen, copyfrom);
 		if (!match)
-			die(_("%s: '%s' is outside repository"), elt, copyfrom);
+			die(_("%s: '%s' is outside repository at '%s'"), elt,
+			    copyfrom, absolute_path(get_git_work_tree()));

This is producing segfaults when run by magit.  Reproduction recipe:

	cd .git
	git ls-files ..

Expected result:

	fatal: ..: '..' is outside repository

Actual result:

	Segmentation fault

Does this need an extra case to handle when there is no work tree?

Ah, I see it. You're right.

In that case I'll fall back on the git dir.

Will hopefully have a patch along today. Thanks for reporting it.

 - Emily

If there is no worktree at present, we can still hint the user about
Git's current directory by showing them the absolute path to the Git
directory. Even though the Git directory doesn't make it as easy to
locate the worktree in question, it can still help a user figure out
what's going on while developing a script.

This fixes a segmentation fault introduced in e0020b2f829.

Signed-off-by: Emily Shaffer <redacted>
---
This patch is built on top of es/outside-repo-errmsg-hints. I think it
doesn't need to be, since that change is in master, though.

Sounds like there's a segfault in the wild:
https://lore.kernel.org/git/20200228195805.GA190372@google.com

I figured since the error doesn't specify "this is the worktree" or
"this is the gitdir" to the user, it didn't need more prettying when
falling back to gitdir in this patch.

It seems to me that if we are checking whether something is inside or
outside of the repository, we definitely will get a good result from
get_git_dir() - if there's no Git dir, then there's no repository to be
in or out of.

CI run here: https://github.com/gitgitgadget/git/pull/572/checks

 - Emily

 pathspec.c | 8 ++++++--
 setup.c    | 8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 166d255642..8243e06eab 100644
--- a/pathspec.c
+++ b/pathspec.c

@@ -438,9 +438,13 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 	} else {
 		match = prefix_path_gently(prefix, prefixlen,
 					   &prefixlen, copyfrom);
-		if (!match)
+		if (!match) {
+			const char *hint_path = get_git_work_tree();
+			if (!hint_path)
+				hint_path = get_git_dir();
 			die(_("%s: '%s' is outside repository at '%s'"), elt,
-			    copyfrom, absolute_path(get_git_work_tree()));
+			    copyfrom, absolute_path(hint_path));
+		}
 	}
 
 	item->match = match;

diff --git a/setup.c b/setup.c
index 17814a080b..f4897287f7 100644
--- a/setup.c
+++ b/setup.c

@@ -120,9 +120,13 @@ char *prefix_path_gently(const char *prefix, int len,
 char *prefix_path(const char *prefix, int len, const char *path)
 {
 	char *r = prefix_path_gently(prefix, len, NULL, path);
-	if (!r)
+	if (!r) {
+		const char *hint_path = get_git_work_tree();
+		if (!hint_path)
+			hint_path = get_git_dir();
 		die(_("'%s' is outside repository at '%s'"), path,
-		    absolute_path(get_git_work_tree()));
+		    absolute_path(hint_path));
+	}
 	return r;
 }
 

-- 
2.25.0.265.gbab2e86ba0-goog