* Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT
@ 2023-10-14 15:42 ks1322 ks1322
2023-10-14 18:12 ` Kristoffer Haugsbakk
0 siblings, 1 reply; 15+ messages in thread
From: ks1322 ks1322 @ 2023-10-14 15:42 UTC (permalink / raw
To: git
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
`git grep --no-index 123 /dev/stdin` outside of git repository
What did you expect to happen? (Expected behavior)
Ability to grep input from stdin
What happened instead? (Actual behavior)
`git grep` crashed with SIGABRT
$ git grep --no-index 123 /dev/stdin
BUG: environment.c:215: git environment hasn't been setup
Aborted (core dumped)
What's different between what you expected and what actually happened?
Crash, no grep result
Anything else you want to add:
Plain grep can do that, so I suppose `git grep` could also:
$ echo -e "123\n456\n789" | grep 456 /dev/stdin
456
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.41.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Oct 6
19:02:35 UTC 2023 x86_64
compiler info: gnuc: 13.1
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
not run from a git repository - no hooks to show
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT
2023-10-14 15:42 Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT ks1322 ks1322
@ 2023-10-14 18:12 ` Kristoffer Haugsbakk
2023-10-14 19:37 ` Kristoffer Haugsbakk
0 siblings, 1 reply; 15+ messages in thread
From: Kristoffer Haugsbakk @ 2023-10-14 18:12 UTC (permalink / raw
To: ks1322 ks1322; +Cc: git
Hi ks1322
On Sat, Oct 14, 2023, at 17:42, ks1322 ks1322 wrote:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
> `git grep --no-index 123 /dev/stdin` outside of git repository
>
> What did you expect to happen? (Expected behavior)
> Ability to grep input from stdin
>
> What happened instead? (Actual behavior)
> `git grep` crashed with SIGABRT
>
> $ git grep --no-index 123 /dev/stdin
> BUG: environment.c:215: git environment hasn't been setup
> Aborted (core dumped)
>
> What's different between what you expected and what actually happened?
> Crash, no grep result
It looks like `setup.c:verify_filename` fails to deny paths that are not
transitive children (or whatever the term) of the directory that git(1) is
running in:
$ git -C ~/IdeaProjects/ grep --no-index dfddf ~
BUG: environment.c:213: git environment hasn't been setup
Aborted (core dumped)
So `builtin/grep.c` goes past that check, into
`pathspec.c:init_pathspec_item` and dies at line 472 since that function
assumes that we are in a Git repository.
--
Kristoffer Haugsbakk
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT
2023-10-14 18:12 ` Kristoffer Haugsbakk
@ 2023-10-14 19:37 ` Kristoffer Haugsbakk
2023-10-14 21:02 ` [PATCH] grep: die gracefully when outside repository Kristoffer Haugsbakk
0 siblings, 1 reply; 15+ messages in thread
From: Kristoffer Haugsbakk @ 2023-10-14 19:37 UTC (permalink / raw
To: Kristoffer Haugsbakk; +Cc: git, ks1322 ks1322
On Sat, Oct 14, 2023, at 20:12, Kristoffer Haugsbakk wrote:
> It looks like `setup.c:verify_filename` fails to deny paths that are not
> transitive children (or whatever the term) of the directory that git(1) is
> running in:
No, that's not it. I don't think that's the responsibility of that
function. Compare to a successful run, that is when run in a Git
repository:
$ git grep --no-index dfddf /home
fatal: /home: '/home' is outside repository at '/home/kristoffer/programming/git'
And that error happens at `pathspec.c:473`.
So going down into that file is not wrong.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] grep: die gracefully when outside repository
2023-10-14 19:37 ` Kristoffer Haugsbakk
@ 2023-10-14 21:02 ` Kristoffer Haugsbakk
2023-10-15 3:26 ` Jeff King
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Kristoffer Haugsbakk @ 2023-10-14 21:02 UTC (permalink / raw
To: code; +Cc: git, ks1322
Die gracefully when `git grep --no-index` is run outside of a Git
repository and the path is outside the directory tree.
If you are not in a Git repository and say:
git grep --no-index search ..
You trigger a `BUG`:
BUG: environment.c:213: git environment hasn't been setup
Aborted (core dumped)
Because `..` is a valid path which is treated as a pathspec. Then
`pathspec` figures out that it is not in the current directory tree. The
`BUG` is triggered when `pathspec` tries to advice the user about the path
to the (non-existing) repository.
Reported-by: ks1322 ks1322 <ks1322@gmail.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
pathspec.c | 3 +++
t/t7810-grep.sh | 13 +++++++++++++
2 files changed, 16 insertions(+)
diff --git a/pathspec.c b/pathspec.c
index 3a3a5724c44..e115832f17a 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -468,6 +468,9 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
&prefixlen, copyfrom);
if (!match) {
const char *hint_path = get_git_work_tree();
+ if (!have_git_dir())
+ die(_("'%s' is outside the directory tree"),
+ copyfrom);
if (!hint_path)
hint_path = get_git_dir();
die(_("%s: '%s' is outside repository at '%s'"), elt,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 39d6d713ecb..b976f81a166 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
)
'
+test_expect_success 'outside of git repository with pathspec outside the directory tree' '
+ test_when_finished rm -fr non &&
+ rm -fr non &&
+ mkdir -p non/git/sub &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search .. 2>error &&
+ grep "is outside the directory tree" error
+ )
+'
+
test_expect_success 'inside git repository but with --no-index' '
rm -fr is &&
mkdir -p is/git/sub &&
base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40
--
2.42.0.2.g879ad04204
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] grep: die gracefully when outside repository
2023-10-14 21:02 ` [PATCH] grep: die gracefully when outside repository Kristoffer Haugsbakk
@ 2023-10-15 3:26 ` Jeff King
2023-10-15 8:00 ` Kristoffer Haugsbakk
2023-10-15 17:57 ` Junio C Hamano
2023-10-17 16:42 ` Junio C Hamano
` (3 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Jeff King @ 2023-10-15 3:26 UTC (permalink / raw
To: Kristoffer Haugsbakk; +Cc: git, ks1322
On Sat, Oct 14, 2023 at 11:02:38PM +0200, Kristoffer Haugsbakk wrote:
> Die gracefully when `git grep --no-index` is run outside of a Git
> repository and the path is outside the directory tree.
>
> If you are not in a Git repository and say:
>
> git grep --no-index search ..
>
> You trigger a `BUG`:
>
> BUG: environment.c:213: git environment hasn't been setup
> Aborted (core dumped)
>
> Because `..` is a valid path which is treated as a pathspec. Then
> `pathspec` figures out that it is not in the current directory tree. The
> `BUG` is triggered when `pathspec` tries to advice the user about the path
> to the (non-existing) repository.
Is it even reasonable for "grep --no-index" to care about leaving the
tree in the first place? That is, is there a reason we should not allow:
git grep --no-index foo ../bar
? And if we do want to care, there is a weirdness here that even with
your patch, we check to see if the file exists:
$ git grep --no-index foo ../does-exist
fatal: '../does-exist' is outside the directory tree
$ git grep --no-index foo ../does-not-exist
fatal: ../does-not-exist: no such path in the working tree.
If we want to avoid leaving the current directory, then I think we need
to be checking much sooner (but again, I would argue that it is not
worth caring about in no-index mode).
I do think your patch does not make anything worse (and indeed makes the
error output much better). So I do not mind it in the meantime. But I
have a feeling that we'd end up reverting it as part of the fix for the
larger issue.
-Peff
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] grep: die gracefully when outside repository
2023-10-15 3:26 ` Jeff King
@ 2023-10-15 8:00 ` Kristoffer Haugsbakk
2023-10-15 17:57 ` Junio C Hamano
1 sibling, 0 replies; 15+ messages in thread
From: Kristoffer Haugsbakk @ 2023-10-15 8:00 UTC (permalink / raw
To: Jeff King; +Cc: git, ks1322 ks1322
On Sun, Oct 15, 2023, at 05:26, Jeff King wrote:
> Is it even reasonable for "grep --no-index" to care about leaving the
> tree in the first place? That is, is there a reason we should not allow:
>
> git grep --no-index foo ../bar
>
> ?
On second thought yeah, it doesn't make sense. We are outside of any
repository already so what does it matter where the file is relative to
the current working directory?
It seems that `pathspec.c:init_pathspec_item` should let you through in
this case.
--
Kristoffer Haugsbakk
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] grep: die gracefully when outside repository
2023-10-15 3:26 ` Jeff King
2023-10-15 8:00 ` Kristoffer Haugsbakk
@ 2023-10-15 17:57 ` Junio C Hamano
1 sibling, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2023-10-15 17:57 UTC (permalink / raw
To: Jeff King; +Cc: Kristoffer Haugsbakk, git, ks1322
Jeff King <peff@peff.net> writes:
> Is it even reasonable for "grep --no-index" to care about leaving the
> tree in the first place? That is, is there a reason we should not allow:
>
> git grep --no-index foo ../bar
A huge difference between the bare "grep" and "git grep" is that we
know the scope of the project tree, so it goes recursive by default.
Should the above command line recursively go below ../bar? Would we
allow "/" to be given?
I actually do not think these "we are allowing Git tools to be used
on random garbage" is a good idea to begin with X-<. If we invented
something nice for our variant in "git grep" and wish we can use it
outside the repository, contributing the feature to implementations
of "grep" would have been the right way to move forward, instead of
contaminating the codebase with things that are not related to Git.
Whoever did 59332d13 (Resurrect "git grep --no-index", 2010-02-06)
should be punished X-<.
Anyway.
2e48fcdb (grep docs: document --no-index option, 2010-02-25) seems
to have wanted to explicitly limit the search within the "current
directory", and I am fine to keep the search space limited by the
cwd. On the other hand, of course, the users can shoot themselves
in the foot with "grep -r foo /", so letting them use "git grep" the
same way is perhaps OK. Especially if it simplifies the code if we
lift the limitation, that is a very tempting thing to do.
> If we want to avoid leaving the current directory, then I think we need
> to be checking much sooner (but again, I would argue that it is not
> worth caring about in no-index mode).
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] grep: die gracefully when outside repository
2023-10-14 21:02 ` [PATCH] grep: die gracefully when outside repository Kristoffer Haugsbakk
2023-10-15 3:26 ` Jeff King
@ 2023-10-17 16:42 ` Junio C Hamano
2023-10-17 19:51 ` Kristoffer Haugsbakk
2023-10-17 20:39 ` Junio C Hamano
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2023-10-17 16:42 UTC (permalink / raw
To: Kristoffer Haugsbakk; +Cc: git, ks1322
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> diff --git a/pathspec.c b/pathspec.c
> index 3a3a5724c44..e115832f17a 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -468,6 +468,9 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
> &prefixlen, copyfrom);
> if (!match) {
> const char *hint_path = get_git_work_tree();
> + if (!have_git_dir())
> + die(_("'%s' is outside the directory tree"),
> + copyfrom);
> if (!hint_path)
> hint_path = get_git_dir();
> die(_("%s: '%s' is outside repository at '%s'"), elt,
It is curious that the original has two sources of hint_path (i.e.,
get_git_dir() is used as a fallback for get_git_work_tree()). Are
we certain that the check is at the right place? If we do not have
a repository, then both would fail by returning NULL, so it should
not matter if we add the new check before we check either or both,
or even after we checked both before dying.
I wonder if
const char *hint_path = get_git_work_tree();
if (!hint_path)
hint_path = get_git_dir();
if (hint_path)
die(_("%s: '%s' is outside repository at '%s'"),
elt, copyfrom, absolute_path(hint_path));
else
die(_("%s: '%s' is outside the directory tree"),
elt, copyfrom);
makes the intent of the code clearer. We want to hint the location
of the repository by computing hint_path, and if we can compute it,
we use it in the error message, but otherwise we don't add hint. And
we apply that conditional whether we have repository or not---what we
care about is the NULL-ness of the hint string we computed.
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 39d6d713ecb..b976f81a166 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
> )
> '
>
> +test_expect_success 'outside of git repository with pathspec outside the directory tree' '
> + test_when_finished rm -fr non &&
> + rm -fr non &&
> + mkdir -p non/git/sub &&
> + (
> + GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non/git &&
> + test_expect_code 128 git grep --no-index search .. 2>error &&
> + grep "is outside the directory tree" error
Excellent. This is a very good use of the GIT_CEILING_DIRECTORIES
facility.
> + )
> +'
> +
> test_expect_success 'inside git repository but with --no-index' '
> rm -fr is &&
> mkdir -p is/git/sub &&
>
> base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] grep: die gracefully when outside repository
2023-10-17 16:42 ` Junio C Hamano
@ 2023-10-17 19:51 ` Kristoffer Haugsbakk
2023-10-17 20:25 ` Junio C Hamano
0 siblings, 1 reply; 15+ messages in thread
From: Kristoffer Haugsbakk @ 2023-10-17 19:51 UTC (permalink / raw
To: gitster; +Cc: Kristoffer Haugsbakk, git, ks1322, peff
On Tue, Oct 17, 2023, at 18:42, Junio C Hamano wrote:
> It is curious that the original has two sources of hint_path (i.e.,
> get_git_dir() is used as a fallback for get_git_work_tree()). Are
> we certain that the check is at the right place? If we do not have
> a repository, then both would fail by returning NULL, so it should
> not matter if we add the new check before we check either or both,
> or even after we checked both before dying.
>
> I wonder if
>
> const char *hint_path = get_git_work_tree();
>
> if (!hint_path)
> hint_path = get_git_dir();
> if (hint_path)
> die(_("%s: '%s' is outside repository at '%s'"),
> elt, copyfrom, absolute_path(hint_path));
> else
> die(_("%s: '%s' is outside the directory tree"),
> elt, copyfrom);
>
> makes the intent of the code clearer.
That doesn't work since `get_git_dir()` triggers `BUG` instead of
returning `NULL`.
The `hint_path` declaration has to be at the start because of style
rules. But we can initialize it after.
I can also have a second look at the test since I am using `grep` to
test the failure output and not the translation string variant.
-- >8 --
Subject: [PATCH] fixup! grep: die gracefully when outside repository
---
pathspec.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/pathspec.c b/pathspec.c
index e115832f17a..0c1061fad11 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -467,10 +467,11 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
match = prefix_path_gently(prefix, prefixlen,
&prefixlen, copyfrom);
if (!match) {
- const char *hint_path = get_git_work_tree();
+ const char *hint_path;
if (!have_git_dir())
die(_("'%s' is outside the directory tree"),
copyfrom);
+ hint_path = get_git_work_tree();
if (!hint_path)
hint_path = get_git_dir();
die(_("%s: '%s' is outside repository at '%s'"), elt,
--
2.42.0.2.g879ad04204
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] grep: die gracefully when outside repository
2023-10-17 19:51 ` Kristoffer Haugsbakk
@ 2023-10-17 20:25 ` Junio C Hamano
0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2023-10-17 20:25 UTC (permalink / raw
To: Kristoffer Haugsbakk; +Cc: git, ks1322, peff
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> On Tue, Oct 17, 2023, at 18:42, Junio C Hamano wrote:
>> It is curious that the original has two sources of hint_path (i.e.,
>> get_git_dir() is used as a fallback for get_git_work_tree()). Are
>> we certain that the check is at the right place? If we do not have
>> a repository, then both would fail by returning NULL, so it should
>> not matter if we add the new check before we check either or both,
>> or even after we checked both before dying.
>>
>> I wonder if
>>
>> const char *hint_path = get_git_work_tree();
>>
>> if (!hint_path)
>> hint_path = get_git_dir();
>> if (hint_path)
>> die(_("%s: '%s' is outside repository at '%s'"),
>> elt, copyfrom, absolute_path(hint_path));
>> else
>> die(_("%s: '%s' is outside the directory tree"),
>> elt, copyfrom);
>>
>> makes the intent of the code clearer.
>
> That doesn't work since `get_git_dir()` triggers `BUG` instead of
> returning `NULL`.
Ah, interesting.
> The `hint_path` declaration has to be at the start because of style
> rules. But we can initialize it after.
Yes, what you have below (but please leave a blank line between the
last line of decl and the first line of statement for readablility)
looks very readable and sensible.
> I can also have a second look at the test since I am using `grep` to
> test the failure output and not the translation string variant.
That is not necessary, as we no longer run under phoney i18n that
required us to use test_i18ngrep. It is OK to assume that the tests
are run under "C" locale.
Thanks.
> -- >8 --
> Subject: [PATCH] fixup! grep: die gracefully when outside repository
>
> ---
> pathspec.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index e115832f17a..0c1061fad11 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -467,10 +467,11 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
> match = prefix_path_gently(prefix, prefixlen,
> &prefixlen, copyfrom);
> if (!match) {
> - const char *hint_path = get_git_work_tree();
> + const char *hint_path;
> if (!have_git_dir())
> die(_("'%s' is outside the directory tree"),
> copyfrom);
> + hint_path = get_git_work_tree();
> if (!hint_path)
> hint_path = get_git_dir();
> die(_("%s: '%s' is outside repository at '%s'"), elt,
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] grep: die gracefully when outside repository
2023-10-14 21:02 ` [PATCH] grep: die gracefully when outside repository Kristoffer Haugsbakk
2023-10-15 3:26 ` Jeff King
2023-10-17 16:42 ` Junio C Hamano
@ 2023-10-17 20:39 ` Junio C Hamano
2023-10-20 16:40 ` [PATCH v2] " Kristoffer Haugsbakk
2023-10-20 18:04 ` [PATCH v3] " Kristoffer Haugsbakk
4 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2023-10-17 20:39 UTC (permalink / raw
To: Kristoffer Haugsbakk; +Cc: git, ks1322
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 39d6d713ecb..b976f81a166 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
> )
> '
>
> +test_expect_success 'outside of git repository with pathspec outside the directory tree' '
> + test_when_finished rm -fr non &&
> + rm -fr non &&
> + mkdir -p non/git/sub &&
> + (
> + GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non/git &&
> + test_expect_code 128 git grep --no-index search .. 2>error &&
> + grep "is outside the directory tree" error
> + )
> +'
> +
So you create non/git/sub, go to non/git (so there is sub/ directory),
and try running "..".
If you had a directory non/tig next to non/git and used ../tig
instead of .. as the path given to "git grep", it would also
correctly fail. Searching in a non-existing path, ../non, dies in a
different error, with an error message that is not technically
wrong, but it probably can be improved. It has been a while since I
looked at the pathspec matching code, but if we are lucky, it might
be just the matter of swapping the order of checking (in other
words, check "is it outside" first and then "does it exist" next, or
something like that)?
t/t7810-grep.sh | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git c/t/t7810-grep.sh w/t/t7810-grep.sh
index b976f81a16..84838c0fe1 100755
--- c/t/t7810-grep.sh
+++ w/t/t7810-grep.sh
@@ -1234,16 +1234,30 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
)
'
-test_expect_success 'outside of git repository with pathspec outside the directory tree' '
+test_expect_success 'no repository with path outside $cwd' '
test_when_finished rm -fr non &&
rm -fr non &&
- mkdir -p non/git/sub &&
+ mkdir -p non/git/sub non/tig &&
(
GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
export GIT_CEILING_DIRECTORIES &&
cd non/git &&
test_expect_code 128 git grep --no-index search .. 2>error &&
grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../tig 2>error &&
+ grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../non 2>error &&
+ grep "no such path in the working tree" error
)
'
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2] grep: die gracefully when outside repository
2023-10-14 21:02 ` [PATCH] grep: die gracefully when outside repository Kristoffer Haugsbakk
` (2 preceding siblings ...)
2023-10-17 20:39 ` Junio C Hamano
@ 2023-10-20 16:40 ` Kristoffer Haugsbakk
2023-10-20 17:05 ` Eric Sunshine
2023-10-20 18:04 ` [PATCH v3] " Kristoffer Haugsbakk
4 siblings, 1 reply; 15+ messages in thread
From: Kristoffer Haugsbakk @ 2023-10-20 16:40 UTC (permalink / raw
To: code; +Cc: gitster, ks1322, git
Die gracefully when `git grep --no-index` is run outside of a Git
repository and the path is outside the directory tree.
If you are not in a Git repository and say:
git grep --no-index search ..
You trigger a `BUG`:
BUG: environment.c:213: git environment hasn't been setup
Aborted (core dumped)
Because `..` is a valid path which is treated as a pathspec. Then
`pathspec` figures out that it is not in the current directory tree. The
`BUG` is triggered when `pathspec` tries to advice the user about how the
path is not in the current (non-existing) repository.
Reported-by: ks1322 ks1322 <ks1322@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
- Initialize `hint_path` after we know that we are in a Git repository
- Apply Junio's suggestion for the test: https://lore.kernel.org/git/xmqqzg0hf0g8.fsf@gitster.g/
pathspec.c | 7 ++++++-
t/t7810-grep.sh | 27 +++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/pathspec.c b/pathspec.c
index 3a3a5724c44..264b4929a55 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -467,7 +467,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
match = prefix_path_gently(prefix, prefixlen,
&prefixlen, copyfrom);
if (!match) {
- const char *hint_path = get_git_work_tree();
+ const char *hint_path;
+
+ if (!have_git_dir())
+ die(_("'%s' is outside the directory tree"),
+ copyfrom);
+ hint_path = get_git_work_tree();
if (!hint_path)
hint_path = get_git_dir();
die(_("%s: '%s' is outside repository at '%s'"), elt,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 39d6d713ecb..84838c0fe1b 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1234,6 +1234,33 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
)
'
+test_expect_success 'no repository with path outside $cwd' '
+ test_when_finished rm -fr non &&
+ rm -fr non &&
+ mkdir -p non/git/sub non/tig &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search .. 2>error &&
+ grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../tig 2>error &&
+ grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../non 2>error &&
+ grep "no such path in the working tree" error
+ )
+'
+
test_expect_success 'inside git repository but with --no-index' '
rm -fr is &&
mkdir -p is/git/sub &&
--
2.42.0.2.g879ad04204
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] grep: die gracefully when outside repository
2023-10-20 16:40 ` [PATCH v2] " Kristoffer Haugsbakk
@ 2023-10-20 17:05 ` Eric Sunshine
2023-10-20 18:06 ` Junio C Hamano
0 siblings, 1 reply; 15+ messages in thread
From: Eric Sunshine @ 2023-10-20 17:05 UTC (permalink / raw
To: Kristoffer Haugsbakk; +Cc: gitster, ks1322, git
On Fri, Oct 20, 2023 at 12:40 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> Die gracefully when `git grep --no-index` is run outside of a Git
> repository and the path is outside the directory tree.
>
> If you are not in a Git repository and say:
>
> git grep --no-index search ..
>
> You trigger a `BUG`:
>
> BUG: environment.c:213: git environment hasn't been setup
> Aborted (core dumped)
>
> Because `..` is a valid path which is treated as a pathspec. Then
> `pathspec` figures out that it is not in the current directory tree. The
> `BUG` is triggered when `pathspec` tries to advice the user about how the
> path is not in the current (non-existing) repository.
s/advice/advise/
(probably not worth a reroll)
> Reported-by: ks1322 ks1322 <ks1322@gmail.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] grep: die gracefully when outside repository
2023-10-14 21:02 ` [PATCH] grep: die gracefully when outside repository Kristoffer Haugsbakk
` (3 preceding siblings ...)
2023-10-20 16:40 ` [PATCH v2] " Kristoffer Haugsbakk
@ 2023-10-20 18:04 ` Kristoffer Haugsbakk
4 siblings, 0 replies; 15+ messages in thread
From: Kristoffer Haugsbakk @ 2023-10-20 18:04 UTC (permalink / raw
To: code; +Cc: gitster, sunshine, ks1322, git
Die gracefully when `git grep --no-index` is run outside of a Git
repository and the path is outside the directory tree.
If you are not in a Git repository and say:
git grep --no-index search ..
You trigger a `BUG`:
BUG: environment.c:213: git environment hasn't been setup
Aborted (core dumped)
Because `..` is a valid path which is treated as a pathspec. Then
`pathspec` figures out that it is not in the current directory tree. The
`BUG` is triggered when `pathspec` tries to advise the user about how the
path is not in the current (non-existing) repository.
Reported-by: ks1322 ks1322 <ks1322@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
- Initialize `hint_path` after we know that we are in a Git repository
- Apply Junio's suggestion for the test: https://lore.kernel.org/git/xmqqzg0hf0g8.fsf@gitster.g/
v3:
- commit message: correct to “advise” (“advice” is a noun)
pathspec.c | 7 ++++++-
t/t7810-grep.sh | 27 +++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/pathspec.c b/pathspec.c
index 3a3a5724c44..264b4929a55 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -467,7 +467,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
match = prefix_path_gently(prefix, prefixlen,
&prefixlen, copyfrom);
if (!match) {
- const char *hint_path = get_git_work_tree();
+ const char *hint_path;
+
+ if (!have_git_dir())
+ die(_("'%s' is outside the directory tree"),
+ copyfrom);
+ hint_path = get_git_work_tree();
if (!hint_path)
hint_path = get_git_dir();
die(_("%s: '%s' is outside repository at '%s'"), elt,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 39d6d713ecb..84838c0fe1b 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1234,6 +1234,33 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
)
'
+test_expect_success 'no repository with path outside $cwd' '
+ test_when_finished rm -fr non &&
+ rm -fr non &&
+ mkdir -p non/git/sub non/tig &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search .. 2>error &&
+ grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../tig 2>error &&
+ grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../non 2>error &&
+ grep "no such path in the working tree" error
+ )
+'
+
test_expect_success 'inside git repository but with --no-index' '
rm -fr is &&
mkdir -p is/git/sub &&
--
2.42.0.2.g879ad04204
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] grep: die gracefully when outside repository
2023-10-20 17:05 ` Eric Sunshine
@ 2023-10-20 18:06 ` Junio C Hamano
0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2023-10-20 18:06 UTC (permalink / raw
To: Eric Sunshine; +Cc: Kristoffer Haugsbakk, ks1322, git
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Fri, Oct 20, 2023 at 12:40 PM Kristoffer Haugsbakk
> <code@khaugsbakk.name> wrote:
>> Die gracefully when `git grep --no-index` is run outside of a Git
>> repository and the path is outside the directory tree.
>>
>> If you are not in a Git repository and say:
>>
>> git grep --no-index search ..
>>
>> You trigger a `BUG`:
>>
>> BUG: environment.c:213: git environment hasn't been setup
>> Aborted (core dumped)
>>
>> Because `..` is a valid path which is treated as a pathspec. Then
>> `pathspec` figures out that it is not in the current directory tree. The
>> `BUG` is triggered when `pathspec` tries to advice the user about how the
>> path is not in the current (non-existing) repository.
>
> s/advice/advise/
>
> (probably not worth a reroll)
The only remaining niggle I have is that the effect of this change
would be much wider than just "grep", but in "git shortlog" output
it may appear that this is specific to it, making later developers'
life a bit harder when they are hunting for the cause of a behaviour
change that is outside "grep", but still caused by this patch.
But I think I am worried too much in this particular case. Once
this codepath is entered, the code will die no matter what, and we
are merely making it die a bit more nicely.
There still is the "we say different things depending on the path
outside the hierarchy exists or not" raised by Peff remaining, but
for now, let's declare a victory and merge it to 'next'.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-10-20 18:06 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-14 15:42 Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT ks1322 ks1322
2023-10-14 18:12 ` Kristoffer Haugsbakk
2023-10-14 19:37 ` Kristoffer Haugsbakk
2023-10-14 21:02 ` [PATCH] grep: die gracefully when outside repository Kristoffer Haugsbakk
2023-10-15 3:26 ` Jeff King
2023-10-15 8:00 ` Kristoffer Haugsbakk
2023-10-15 17:57 ` Junio C Hamano
2023-10-17 16:42 ` Junio C Hamano
2023-10-17 19:51 ` Kristoffer Haugsbakk
2023-10-17 20:25 ` Junio C Hamano
2023-10-17 20:39 ` Junio C Hamano
2023-10-20 16:40 ` [PATCH v2] " Kristoffer Haugsbakk
2023-10-20 17:05 ` Eric Sunshine
2023-10-20 18:06 ` Junio C Hamano
2023-10-20 18:04 ` [PATCH v3] " Kristoffer Haugsbakk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).