{"thread":{"id":"60366","subject":"Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT","startedAt":"2023-10-14T15:42:22Z","lastAt":"2023-10-20T18:06:09Z","messageCount":15,"participants":["ks1322 ks1322","Kristoffer Haugsbakk","Jeff King","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"483246","messageId":"CAKFQ_Q_P4HvCMHsg4=6ycb8r44qprhRCGSmLQf7B3_-zy28_oQ@mail.gmail.com","threadId":"60366","inReplyTo":null,"subject":"Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT","fromName":"ks1322 ks1322","fromEmail":"ks1322@gmail.com","sentAt":"2023-10-14T15:42:09Z","receivedAt":"2023-10-14T15:42:22Z","isPatch":false,"sender":{"key":"ks1322@gmail.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n`git grep --no-index 123 /dev/stdin` outside of git repository\n\nWhat did you expect to happen? (Expected behavior)\nAbility to grep input from stdin\n\nWhat happened instead? (Actual behavior)\n`git grep` crashed with SIGABRT\n\n$ git grep --no-index 123 /dev/stdin\nBUG: environment.c:215: git environment hasn't been setup\nAborted (core dumped)\n\nWhat's different between what you expected and what actually happened?\nCrash, no grep result\n\nAnything else you want to add:\nPlain grep can do that, so I suppose `git grep` could also:\n\n$ echo -e \"123\\n456\\n789\" | grep 456 /dev/stdin\n456\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.41.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 6.5.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Oct 6\n19:02:35 UTC 2023 x86_64\ncompiler info: gnuc: 13.1\nlibc info: glibc: 2.37\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n"},{"id":"483257","messageId":"2d8eca81-0415-43bf-b3c4-f1163713422b@app.fastmail.com","threadId":"60366","inReplyTo":"CAKFQ_Q_P4HvCMHsg4=6ycb8r44qprhRCGSmLQf7B3_-zy28_oQ@mail.gmail.com","subject":"Re: Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T18:12:04Z","receivedAt":"2023-10-14T18:14:38Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi ks1322 \n\nOn Sat, Oct 14, 2023, at 17:42, ks1322 ks1322 wrote:\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> `git grep --no-index 123 /dev/stdin` outside of git repository\n>\n> What did you expect to happen? (Expected behavior)\n> Ability to grep input from stdin\n>\n> What happened instead? (Actual behavior)\n> `git grep` crashed with SIGABRT\n>\n> $ git grep --no-index 123 /dev/stdin\n> BUG: environment.c:215: git environment hasn't been setup\n> Aborted (core dumped)\n>\n> What's different between what you expected and what actually happened?\n> Crash, no grep result\n\nIt looks like `setup.c:verify_filename` fails to deny paths that are not\ntransitive children (or whatever the term) of the directory that git(1) is\nrunning in:\n\n $ git -C ~/IdeaProjects/ grep --no-index dfddf ~\n BUG: environment.c:213: git environment hasn't been setup\n Aborted (core dumped)\n\nSo `builtin/grep.c` goes past that check, into\n`pathspec.c:init_pathspec_item` and dies at line 472 since that function\nassumes that we are in a Git repository.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"483258","messageId":"6bb48aac-460c-4d7f-9057-40c3df0c807d@app.fastmail.com","threadId":"60366","inReplyTo":"2d8eca81-0415-43bf-b3c4-f1163713422b@app.fastmail.com","subject":"Re: Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T19:37:08Z","receivedAt":"2023-10-14T19:37:47Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Sat, Oct 14, 2023, at 20:12, Kristoffer Haugsbakk wrote:\n> It looks like `setup.c:verify_filename` fails to deny paths that are not\n> transitive children (or whatever the term) of the directory that git(1) is\n> running in:\n\nNo, that's not it. I don't think that's the responsibility of that\nfunction. Compare to a successful run, that is when run in a Git\nrepository:\n\n $ git grep --no-index dfddf /home\n fatal: /home: '/home' is outside repository at '/home/kristoffer/programming/git'\n\nAnd that error happens at `pathspec.c:473`.\n\nSo going down into that file is not wrong.\n"},{"id":"483261","messageId":"087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name","threadId":"60366","inReplyTo":"6bb48aac-460c-4d7f-9057-40c3df0c807d@app.fastmail.com","subject":"[PATCH] grep: die gracefully when outside repository","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T21:02:38Z","receivedAt":"2023-10-14T21:05:00Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Die gracefully when `git grep --no-index` is run outside of a Git\nrepository and the path is outside the directory tree.\n\nIf you are not in a Git repository and say:\n\n git grep --no-index search ..\n\nYou trigger a `BUG`:\n\n BUG: environment.c:213: git environment hasn't been setup\n Aborted (core dumped)\n\nBecause `..` is a valid path which is treated as a pathspec. Then\n`pathspec` figures out that it is not in the current directory tree. The\n`BUG` is triggered when `pathspec` tries to advice the user about the path\nto the (non-existing) repository.\n\nReported-by: ks1322 ks1322 \nSigned-off-by: Kristoffer Haugsbakk \n---\n pathspec.c | 3 +++\n t/t7810-grep.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 3a3a5724c44..e115832f17a 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -468,6 +468,9 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\t\t\t\t &prefixlen, copyfrom);\n \t\tif (!match) {\n \t\t\tconst char *hint_path = get_git_work_tree();\n+\t\t\tif (!have_git_dir())\n+\t\t\t\tdie(_(\"'%s' is outside the directory tree\"),\n+\t\t\t\t copyfrom);\n \t\t\tif (!hint_path)\n \t\t\t\thint_path = get_git_dir();\n \t\t\tdie(_(\"%s: '%s' is outside repository at '%s'\"), elt,\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 39d6d713ecb..b976f81a166 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '\n \t)\n '\n \n+test_expect_success 'outside of git repository with pathspec outside the directory tree' '\n+\ttest_when_finished rm -fr non &&\n+\trm -fr non &&\n+\tmkdir -p non/git/sub &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\ttest_expect_code 128 git grep --no-index search .. 2>error &&\n+\t\tgrep \"is outside the directory tree\" error\n+\t)\n+'\n+\n test_expect_success 'inside git repository but with --no-index' '\n \trm -fr is &&\n \tmkdir -p is/git/sub &&\n\nbase-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40\n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483277","messageId":"20231015032636.GC554702@coredump.intra.peff.net","threadId":"60366","inReplyTo":"087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name","subject":"Re: [PATCH] grep: die gracefully when outside repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-15T03:26:36Z","receivedAt":"2023-10-15T03:26:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 14, 2023 at 11:02:38PM +0200, Kristoffer Haugsbakk wrote:\n\n> Die gracefully when `git grep --no-index` is run outside of a Git\n> repository and the path is outside the directory tree.\n> \n> If you are not in a Git repository and say:\n> \n> git grep --no-index search ..\n> \n> You trigger a `BUG`:\n> \n> BUG: environment.c:213: git environment hasn't been setup\n> Aborted (core dumped)\n> \n> Because `..` is a valid path which is treated as a pathspec. Then\n> `pathspec` figures out that it is not in the current directory tree. The\n> `BUG` is triggered when `pathspec` tries to advice the user about the path\n> to the (non-existing) repository.\n\nIs it even reasonable for \"grep --no-index\" to care about leaving the\ntree in the first place? That is, is there a reason we should not allow:\n\n git grep --no-index foo ../bar\n\n? And if we do want to care, there is a weirdness here that even with\nyour patch, we check to see if the file exists:\n\n $ git grep --no-index foo ../does-exist\n fatal: '../does-exist' is outside the directory tree\n\n $ git grep --no-index foo ../does-not-exist\n fatal: ../does-not-exist: no such path in the working tree.\n\nIf we want to avoid leaving the current directory, then I think we need\nto be checking much sooner (but again, I would argue that it is not\nworth caring about in no-index mode).\n\nI do think your patch does not make anything worse (and indeed makes the\nerror output much better). So I do not mind it in the meantime. But I\nhave a feeling that we'd end up reverting it as part of the fix for the\nlarger issue.\n\n-Peff\n"},{"id":"483284","messageId":"b953efce-da38-4dbc-9032-985481f3d721@app.fastmail.com","threadId":"60366","inReplyTo":"20231015032636.GC554702@coredump.intra.peff.net","subject":"Re: [PATCH] grep: die gracefully when outside repository","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-15T08:00:20Z","receivedAt":"2023-10-15T08:00:46Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Sun, Oct 15, 2023, at 05:26, Jeff King wrote:\n> Is it even reasonable for \"grep --no-index\" to care about leaving the\n> tree in the first place? That is, is there a reason we should not allow:\n>\n> git grep --no-index foo ../bar\n>\n> ?\n\nOn second thought yeah, it doesn't make sense. We are outside of any\nrepository already so what does it matter where the file is relative to\nthe current working directory?\n\nIt seems that `pathspec.c:init_pathspec_item` should let you through in\nthis case.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"483292","messageId":"xmqq7cnnpy3z.fsf@gitster.g","threadId":"60366","inReplyTo":"20231015032636.GC554702@coredump.intra.peff.net","subject":"Re: [PATCH] grep: die gracefully when outside repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-15T17:57:36Z","receivedAt":"2023-10-15T17:57:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Is it even reasonable for \"grep --no-index\" to care about leaving the\n> tree in the first place? That is, is there a reason we should not allow:\n>\n> git grep --no-index foo ../bar\n\nA huge difference between the bare \"grep\" and \"git grep\" is that we\nknow the scope of the project tree, so it goes recursive by default.\nShould the above command line recursively go below ../bar? Would we\nallow \"/\" to be given?\n\nI actually do not think these \"we are allowing Git tools to be used\non random garbage\" is a good idea to begin with X-<. If we invented\nsomething nice for our variant in \"git grep\" and wish we can use it\noutside the repository, contributing the feature to implementations\nof \"grep\" would have been the right way to move forward, instead of\ncontaminating the codebase with things that are not related to Git.\nWhoever did 59332d13 (Resurrect \"git grep --no-index\", 2010-02-06)\nshould be punished X-<.\n\nAnyway.\n\n2e48fcdb (grep docs: document --no-index option, 2010-02-25) seems\nto have wanted to explicitly limit the search within the \"current\ndirectory\", and I am fine to keep the search space limited by the\ncwd. On the other hand, of course, the users can shoot themselves\nin the foot with \"grep -r foo /\", so letting them use \"git grep\" the\nsame way is perhaps OK. Especially if it simplifies the code if we\nlift the limitation, that is a very tempting thing to do.\n\n> If we want to avoid leaving the current directory, then I think we need\n> to be checking much sooner (but again, I would argue that it is not\n> worth caring about in no-index mode).\n"},{"id":"483352","messageId":"xmqqmswhjj48.fsf@gitster.g","threadId":"60366","inReplyTo":"087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name","subject":"Re: [PATCH] grep: die gracefully when outside repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-17T16:42:31Z","receivedAt":"2023-10-17T16:42:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk writes:\n\n> diff --git a/pathspec.c b/pathspec.c\n> index 3a3a5724c44..e115832f17a 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -468,6 +468,9 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n> \t\t\t\t\t &prefixlen, copyfrom);\n> \t\tif (!match) {\n> \t\t\tconst char *hint_path = get_git_work_tree();\n> +\t\t\tif (!have_git_dir())\n> +\t\t\t\tdie(_(\"'%s' is outside the directory tree\"),\n> +\t\t\t\t copyfrom);\n> \t\t\tif (!hint_path)\n> \t\t\t\thint_path = get_git_dir();\n> \t\t\tdie(_(\"%s: '%s' is outside repository at '%s'\"), elt,\n\nIt is curious that the original has two sources of hint_path (i.e.,\nget_git_dir() is used as a fallback for get_git_work_tree()). Are\nwe certain that the check is at the right place? If we do not have\na repository, then both would fail by returning NULL, so it should\nnot matter if we add the new check before we check either or both,\nor even after we checked both before dying.\n\nI wonder if\n\n\tconst char *hint_path = get_git_work_tree();\n\n\tif (!hint_path)\n\t hint_path = get_git_dir();\n\tif (hint_path)\n\t\tdie(_(\"%s: '%s' is outside repository at '%s'\"),\n\t\t elt, copyfrom, absolute_path(hint_path));\n\telse\n\t\tdie(_(\"%s: '%s' is outside the directory tree\"),\n\t\t elt, copyfrom);\n\nmakes the intent of the code clearer. We want to hint the location\nof the repository by computing hint_path, and if we can compute it,\nwe use it in the error message, but otherwise we don't add hint. And\nwe apply that conditional whether we have repository or not---what we\ncare about is the NULL-ness of the hint string we computed.\n\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index 39d6d713ecb..b976f81a166 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '\n> \t)\n> '\n> \n> +test_expect_success 'outside of git repository with pathspec outside the directory tree' '\n> +\ttest_when_finished rm -fr non &&\n> +\trm -fr non &&\n> +\tmkdir -p non/git/sub &&\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd non/git &&\n> +\t\ttest_expect_code 128 git grep --no-index search .. 2>error &&\n> +\t\tgrep \"is outside the directory tree\" error\n\nExcellent. This is a very good use of the GIT_CEILING_DIRECTORIES\nfacility.\n\n> +\t)\n> +'\n> +\n> test_expect_success 'inside git repository but with --no-index' '\n> \trm -fr is &&\n> \tmkdir -p is/git/sub &&\n>\n> base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40\n"},{"id":"483358","messageId":"f8a2abc0f610912af3eb56536ed217b8f90db2f9.1697571664.git.code@khaugsbakk.name","threadId":"60366","inReplyTo":"xmqqmswhjj48.fsf@gitster.g","subject":"Re: [PATCH] grep: die gracefully when outside repository","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-17T19:51:08Z","receivedAt":"2023-10-17T19:51:29Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Oct 17, 2023, at 18:42, Junio C Hamano wrote:\n> It is curious that the original has two sources of hint_path (i.e.,\n> get_git_dir() is used as a fallback for get_git_work_tree()). Are\n> we certain that the check is at the right place? If we do not have\n> a repository, then both would fail by returning NULL, so it should\n> not matter if we add the new check before we check either or both,\n> or even after we checked both before dying.\n>\n> I wonder if\n>\n> \tconst char *hint_path = get_git_work_tree();\n>\n> \tif (!hint_path)\n> \t hint_path = get_git_dir();\n> \tif (hint_path)\n> \t\tdie(_(\"%s: '%s' is outside repository at '%s'\"),\n> \t\t elt, copyfrom, absolute_path(hint_path));\n> \telse\n> \t\tdie(_(\"%s: '%s' is outside the directory tree\"),\n> \t\t elt, copyfrom);\n>\n> makes the intent of the code clearer.\n\nThat doesn't work since `get_git_dir()` triggers `BUG` instead of\nreturning `NULL`.\n\nThe `hint_path` declaration has to be at the start because of style\nrules. But we can initialize it after.\n\nI can also have a second look at the test since I am using `grep` to\ntest the failure output and not the translation string variant.\n\n-- >8 --\nSubject: [PATCH] fixup! grep: die gracefully when outside repository\n\n---\n pathspec.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex e115832f17a..0c1061fad11 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -467,10 +467,11 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\tmatch = prefix_path_gently(prefix, prefixlen,\n \t\t\t\t\t &prefixlen, copyfrom);\n \t\tif (!match) {\n-\t\t\tconst char *hint_path = get_git_work_tree();\n+\t\t\tconst char *hint_path;\n \t\t\tif (!have_git_dir())\n \t\t\t\tdie(_(\"'%s' is outside the directory tree\"),\n \t\t\t\t copyfrom);\n+\t\t\thint_path = get_git_work_tree();\n \t\t\tif (!hint_path)\n \t\t\t\thint_path = get_git_dir();\n \t\t\tdie(_(\"%s: '%s' is outside repository at '%s'\"), elt,\n-- \n2.42.0.2.g879ad04204\n"},{"id":"483364","messageId":"xmqqcyxdgfn2.fsf@gitster.g","threadId":"60366","inReplyTo":"f8a2abc0f610912af3eb56536ed217b8f90db2f9.1697571664.git.code@khaugsbakk.name","subject":"Re: [PATCH] grep: die gracefully when outside repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-17T20:25:53Z","receivedAt":"2023-10-17T20:26:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk writes:\n\n> On Tue, Oct 17, 2023, at 18:42, Junio C Hamano wrote:\n>> It is curious that the original has two sources of hint_path (i.e.,\n>> get_git_dir() is used as a fallback for get_git_work_tree()). Are\n>> we certain that the check is at the right place? If we do not have\n>> a repository, then both would fail by returning NULL, so it should\n>> not matter if we add the new check before we check either or both,\n>> or even after we checked both before dying.\n>>\n>> I wonder if\n>>\n>> \tconst char *hint_path = get_git_work_tree();\n>>\n>> \tif (!hint_path)\n>> \t hint_path = get_git_dir();\n>> \tif (hint_path)\n>> \t\tdie(_(\"%s: '%s' is outside repository at '%s'\"),\n>> \t\t elt, copyfrom, absolute_path(hint_path));\n>> \telse\n>> \t\tdie(_(\"%s: '%s' is outside the directory tree\"),\n>> \t\t elt, copyfrom);\n>>\n>> makes the intent of the code clearer.\n>\n> That doesn't work since `get_git_dir()` triggers `BUG` instead of\n> returning `NULL`.\n\nAh, interesting.\n\n> The `hint_path` declaration has to be at the start because of style\n> rules. But we can initialize it after.\n\nYes, what you have below (but please leave a blank line between the\nlast line of decl and the first line of statement for readablility)\nlooks very readable and sensible.\n\n> I can also have a second look at the test since I am using `grep` to\n> test the failure output and not the translation string variant.\n\nThat is not necessary, as we no longer run under phoney i18n that\nrequired us to use test_i18ngrep. It is OK to assume that the tests\nare run under \"C\" locale.\n\nThanks.\n\n> -- >8 --\n> Subject: [PATCH] fixup! grep: die gracefully when outside repository\n>\n> ---\n> pathspec.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index e115832f17a..0c1061fad11 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -467,10 +467,11 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n> \t\tmatch = prefix_path_gently(prefix, prefixlen,\n> \t\t\t\t\t &prefixlen, copyfrom);\n> \t\tif (!match) {\n> -\t\t\tconst char *hint_path = get_git_work_tree();\n> +\t\t\tconst char *hint_path;\n> \t\t\tif (!have_git_dir())\n> \t\t\t\tdie(_(\"'%s' is outside the directory tree\"),\n> \t\t\t\t copyfrom);\n> +\t\t\thint_path = get_git_work_tree();\n> \t\t\tif (!hint_path)\n> \t\t\t\thint_path = get_git_dir();\n> \t\t\tdie(_(\"%s: '%s' is outside repository at '%s'\"), elt,\n"},{"id":"483365","messageId":"xmqqzg0hf0g8.fsf@gitster.g","threadId":"60366","inReplyTo":"087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name","subject":"Re: [PATCH] grep: die gracefully when outside repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-17T20:39:19Z","receivedAt":"2023-10-17T20:39:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristoffer Haugsbakk writes:\n\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index 39d6d713ecb..b976f81a166 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -1234,6 +1234,19 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '\n> \t)\n> '\n> \n> +test_expect_success 'outside of git repository with pathspec outside the directory tree' '\n> +\ttest_when_finished rm -fr non &&\n> +\trm -fr non &&\n> +\tmkdir -p non/git/sub &&\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd non/git &&\n> +\t\ttest_expect_code 128 git grep --no-index search .. 2>error &&\n> +\t\tgrep \"is outside the directory tree\" error\n> +\t)\n> +'\n> +\n\nSo you create non/git/sub, go to non/git (so there is sub/ directory),\nand try running \"..\".\n\nIf you had a directory non/tig next to non/git and used ../tig\ninstead of .. as the path given to \"git grep\", it would also\ncorrectly fail. Searching in a non-existing path, ../non, dies in a\ndifferent error, with an error message that is not technically\nwrong, but it probably can be improved. It has been a while since I\nlooked at the pathspec matching code, but if we are lucky, it might\nbe just the matter of swapping the order of checking (in other\nwords, check \"is it outside\" first and then \"does it exist\" next, or\nsomething like that)?\n\n t/t7810-grep.sh | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git c/t/t7810-grep.sh w/t/t7810-grep.sh\nindex b976f81a16..84838c0fe1 100755\n--- c/t/t7810-grep.sh\n+++ w/t/t7810-grep.sh\n@@ -1234,16 +1234,30 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '\n \t)\n '\n \n-test_expect_success 'outside of git repository with pathspec outside the directory tree' '\n+test_expect_success 'no repository with path outside $cwd' '\n \ttest_when_finished rm -fr non &&\n \trm -fr non &&\n-\tmkdir -p non/git/sub &&\n+\tmkdir -p non/git/sub non/tig &&\n \t(\n \t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n \t\texport GIT_CEILING_DIRECTORIES &&\n \t\tcd non/git &&\n \t\ttest_expect_code 128 git grep --no-index search .. 2>error &&\n \t\tgrep \"is outside the directory tree\" error\n+\t) &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\ttest_expect_code 128 git grep --no-index search ../tig 2>error &&\n+\t\tgrep \"is outside the directory tree\" error\n+\t) &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\ttest_expect_code 128 git grep --no-index search ../non 2>error &&\n+\t\tgrep \"no such path in the working tree\" error\n \t)\n '\n \n"},{"id":"483567","messageId":"5c8ef6bec1c99e0fae7ada903885a8e77f8137f9.1697819838.git.code@khaugsbakk.name","threadId":"60366","inReplyTo":"087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name","subject":"[PATCH v2] grep: die gracefully when outside repository","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-20T16:40:07Z","receivedAt":"2023-10-20T16:40:36Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Die gracefully when `git grep --no-index` is run outside of a Git\nrepository and the path is outside the directory tree.\n\nIf you are not in a Git repository and say:\n\n git grep --no-index search ..\n\nYou trigger a `BUG`:\n\n BUG: environment.c:213: git environment hasn't been setup\n Aborted (core dumped)\n\nBecause `..` is a valid path which is treated as a pathspec. Then\n`pathspec` figures out that it is not in the current directory tree. The\n`BUG` is triggered when `pathspec` tries to advice the user about how the\npath is not in the current (non-existing) repository.\n\nReported-by: ks1322 ks1322 \nHelped-by: Junio C Hamano \nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v2:\n - Initialize `hint_path` after we know that we are in a Git repository\n - Apply Junio's suggestion for the test: https://lore.kernel.org/git/xmqqzg0hf0g8.fsf@gitster.g/\n\n pathspec.c | 7 ++++++-\n t/t7810-grep.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 3a3a5724c44..264b4929a55 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -467,7 +467,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\tmatch = prefix_path_gently(prefix, prefixlen,\n \t\t\t\t\t &prefixlen, copyfrom);\n \t\tif (!match) {\n-\t\t\tconst char *hint_path = get_git_work_tree();\n+\t\t\tconst char *hint_path;\n+\n+\t\t\tif (!have_git_dir())\n+\t\t\t\tdie(_(\"'%s' is outside the directory tree\"),\n+\t\t\t\t copyfrom);\n+\t\t\thint_path = get_git_work_tree();\n \t\t\tif (!hint_path)\n \t\t\t\thint_path = get_git_dir();\n \t\t\tdie(_(\"%s: '%s' is outside repository at '%s'\"), elt,\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 39d6d713ecb..84838c0fe1b 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1234,6 +1234,33 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '\n \t)\n '\n \n+test_expect_success 'no repository with path outside $cwd' '\n+\ttest_when_finished rm -fr non &&\n+\trm -fr non &&\n+\tmkdir -p non/git/sub non/tig &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\ttest_expect_code 128 git grep --no-index search .. 2>error &&\n+\t\tgrep \"is outside the directory tree\" error\n+\t) &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\ttest_expect_code 128 git grep --no-index search ../tig 2>error &&\n+\t\tgrep \"is outside the directory tree\" error\n+\t) &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\ttest_expect_code 128 git grep --no-index search ../non 2>error &&\n+\t\tgrep \"no such path in the working tree\" error\n+\t)\n+'\n+\n test_expect_success 'inside git repository but with --no-index' '\n \trm -fr is &&\n \tmkdir -p is/git/sub &&\n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483573","messageId":"CAPig+cTBYw9=Wo=TR8MD5xX9hgurnfR2Xzc_wHSYnL1R00=xpw@mail.gmail.com","threadId":"60366","inReplyTo":"5c8ef6bec1c99e0fae7ada903885a8e77f8137f9.1697819838.git.code@khaugsbakk.name","subject":"Re: [PATCH v2] grep: die gracefully when outside repository","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-10-20T17:05:03Z","receivedAt":"2023-10-20T17:05:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 20, 2023 at 12:40 PM Kristoffer Haugsbakk\n wrote:\n> Die gracefully when `git grep --no-index` is run outside of a Git\n> repository and the path is outside the directory tree.\n>\n> If you are not in a Git repository and say:\n>\n> git grep --no-index search ..\n>\n> You trigger a `BUG`:\n>\n> BUG: environment.c:213: git environment hasn't been setup\n> Aborted (core dumped)\n>\n> Because `..` is a valid path which is treated as a pathspec. Then\n> `pathspec` figures out that it is not in the current directory tree. The\n> `BUG` is triggered when `pathspec` tries to advice the user about how the\n> path is not in the current (non-existing) repository.\n\ns/advice/advise/\n\n(probably not worth a reroll)\n\n> Reported-by: ks1322 ks1322 \n> Helped-by: Junio C Hamano \n> Signed-off-by: Kristoffer Haugsbakk \n"},{"id":"483582","messageId":"82b6a48b8f98036e5c394ae39f96ef5f3f6a641e.1697824787.git.code@khaugsbakk.name","threadId":"60366","inReplyTo":"087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name","subject":"[PATCH v3] grep: die gracefully when outside repository","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-20T18:04:04Z","receivedAt":"2023-10-20T18:04:21Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Die gracefully when `git grep --no-index` is run outside of a Git\nrepository and the path is outside the directory tree.\n\nIf you are not in a Git repository and say:\n\n git grep --no-index search ..\n\nYou trigger a `BUG`:\n\n BUG: environment.c:213: git environment hasn't been setup\n Aborted (core dumped)\n\nBecause `..` is a valid path which is treated as a pathspec. Then\n`pathspec` figures out that it is not in the current directory tree. The\n`BUG` is triggered when `pathspec` tries to advise the user about how the\npath is not in the current (non-existing) repository.\n\nReported-by: ks1322 ks1322 \nHelped-by: Junio C Hamano \nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v2:\n - Initialize `hint_path` after we know that we are in a Git repository\n - Apply Junio's suggestion for the test: https://lore.kernel.org/git/xmqqzg0hf0g8.fsf@gitster.g/\n v3:\n - commit message: correct to “advise” (“advice” is a noun)\n\n pathspec.c | 7 ++++++-\n t/t7810-grep.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 3a3a5724c44..264b4929a55 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -467,7 +467,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\tmatch = prefix_path_gently(prefix, prefixlen,\n \t\t\t\t\t &prefixlen, copyfrom);\n \t\tif (!match) {\n-\t\t\tconst char *hint_path = get_git_work_tree();\n+\t\t\tconst char *hint_path;\n+\n+\t\t\tif (!have_git_dir())\n+\t\t\t\tdie(_(\"'%s' is outside the directory tree\"),\n+\t\t\t\t copyfrom);\n+\t\t\thint_path = get_git_work_tree();\n \t\t\tif (!hint_path)\n \t\t\t\thint_path = get_git_dir();\n \t\t\tdie(_(\"%s: '%s' is outside repository at '%s'\"), elt,\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 39d6d713ecb..84838c0fe1b 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1234,6 +1234,33 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '\n \t)\n '\n \n+test_expect_success 'no repository with path outside $cwd' '\n+\ttest_when_finished rm -fr non &&\n+\trm -fr non &&\n+\tmkdir -p non/git/sub non/tig &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\ttest_expect_code 128 git grep --no-index search .. 2>error &&\n+\t\tgrep \"is outside the directory tree\" error\n+\t) &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\ttest_expect_code 128 git grep --no-index search ../tig 2>error &&\n+\t\tgrep \"is outside the directory tree\" error\n+\t) &&\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$(pwd)/non\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd non/git &&\n+\t\ttest_expect_code 128 git grep --no-index search ../non 2>error &&\n+\t\tgrep \"no such path in the working tree\" error\n+\t)\n+'\n+\n test_expect_success 'inside git repository but with --no-index' '\n \trm -fr is &&\n \tmkdir -p is/git/sub &&\n-- \n2.42.0.2.g879ad04204\n\n"},{"id":"483583","messageId":"xmqqcyx9qid0.fsf@gitster.g","threadId":"60366","inReplyTo":"CAPig+cTBYw9=Wo=TR8MD5xX9hgurnfR2Xzc_wHSYnL1R00=xpw@mail.gmail.com","subject":"Re: [PATCH v2] grep: die gracefully when outside repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-20T18:06:03Z","receivedAt":"2023-10-20T18:06:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Fri, Oct 20, 2023 at 12:40 PM Kristoffer Haugsbakk\n> wrote:\n>> Die gracefully when `git grep --no-index` is run outside of a Git\n>> repository and the path is outside the directory tree.\n>>\n>> If you are not in a Git repository and say:\n>>\n>> git grep --no-index search ..\n>>\n>> You trigger a `BUG`:\n>>\n>> BUG: environment.c:213: git environment hasn't been setup\n>> Aborted (core dumped)\n>>\n>> Because `..` is a valid path which is treated as a pathspec. Then\n>> `pathspec` figures out that it is not in the current directory tree. The\n>> `BUG` is triggered when `pathspec` tries to advice the user about how the\n>> path is not in the current (non-existing) repository.\n>\n> s/advice/advise/\n>\n> (probably not worth a reroll)\n\nThe only remaining niggle I have is that the effect of this change\nwould be much wider than just \"grep\", but in \"git shortlog\" output\nit may appear that this is specific to it, making later developers'\nlife a bit harder when they are hunting for the cause of a behaviour\nchange that is outside \"grep\", but still caused by this patch.\n\nBut I think I am worried too much in this particular case. Once\nthis codepath is entered, the code will die no matter what, and we\nare merely making it die a bit more nicely.\n\nThere still is the \"we say different things depending on the path\noutside the hierarchy exists or not\" raised by Peff remaining, but\nfor now, let's declare a victory and merge it to 'next'.\n\nThanks.\n\n\n"}]}