| Message ID | 5c8ef6bec1c99e0fae7ada903885a8e77f8137f9.1697819838.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] grep: die gracefully when outside repository | expand |
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>
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.
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 &&
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(-)