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

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

[emilyshaffer]

From: Emily Shaffer <emilyshaffer@google.com>

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 <emilyshaffer@google.com>
---
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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]

On 2020-02-14 at 23:29:33, emilyshaffer@google.com wrote:
> From: Emily Shaffer <emilyshaffer@google.com>
> 
> 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 <emilyshaffer@google.com>
> ---
> 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.

> 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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Emily Shaffer]

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:
> > From: Emily Shaffer <emilyshaffer@google.com>
> > 
> > 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 <emilyshaffer@google.com>
> > ---
> > 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.
> 
> > 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

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

[Emily Shaffer]

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 <emilyshaffer@google.com>
---
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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

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 <emilyshaffer@google.com>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Jonathan Nieder]

Hi,

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 <emilyshaffer@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()));

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

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

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

[Emily Shaffer]

On Fri, Feb 28, 2020 at 11:58:05AM -0800, Jonathan Nieder wrote:
> Hi,
> 
> 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 <emilyshaffer@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()));
> 
> 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

[PATCH] prefix_path: show gitdir if worktree unavailable

[Emily Shaffer]

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 <emilyshaffer@google.com>
---
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

Re: [PATCH] prefix_path: show gitdir if worktree unavailable

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> 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 <emilyshaffer@google.com>
> ---
> 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:

Thanks.  We should protect the fix with a test, no?

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