[RFC,v2] prefix_path: show gitdir when arg is outside repo
diff mbox series

Message ID 20200215010013.190528-1-emilyshaffer@google.com
State New
Headers show
Series
  • [RFC,v2] prefix_path: show gitdir when arg is outside repo
Related show

Commit Message

Emily Shaffer Feb. 15, 2020, 1 a.m. UTC
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(-)

Comments

brian m. carlson Feb. 15, 2020, 2:56 a.m. UTC | #1
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.
Jonathan Nieder Feb. 28, 2020, 7:58 p.m. UTC | #2
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;
>  }
>
Emily Shaffer March 3, 2020, 2:19 a.m. UTC | #3
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
diff mbox series

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