{"thread":{"id":"65731","subject":"[PATCH] read_gitfile_gently(): return non-repo path on error","startedAt":"2026-06-02T06:12:06Z","lastAt":"2026-06-16T15:48:58Z","messageCount":12,"participants":["Jeff King","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"544454","messageId":"20260602061159.GA693928@coredump.intra.peff.net","threadId":"65731","inReplyTo":null,"subject":"[PATCH] read_gitfile_gently(): return non-repo path on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-02T06:11:59Z","receivedAt":"2026-06-02T06:12:06Z","isPatch":true,"body":"This patch fixes a potential segfault when resolving a .git file that\npoints to an invalid path. The bug was introduced by 1dd27bfbfd (setup:\nimprove error diagnosis for invalid .git files, 2026-03-04).\n\nIn setup_git_directory_gently() we call read_gitfile_gently(), which may\nreturn a numeric code to us on error. If die_on_error is set, we then\nfeed that code to read_gitfile_error_die(), which also wants the path to\nthe gitfile and, in the case of ERROR_NOT_A_REPO, the non-repo directory\nthat the gitfile pointed to.\n\nBut we don't have that pointed-to directory available, so we just pass\nNULL. That ends up calling die(\"not a git repository: %s\", NULL). This\nmay crash, though on many systems (like glibc) it will just print\n\"(null)\". So even if we don't crash, we're generating nonsense output.\n\nThe problem comes from 1dd27bfbfd. Before that, when die_on_error was\nset we'd pass NULL to read_gitfile_gently()'s return_error_code\nparameter, which means it would call read_gitfile_error_die() itself.\nAnd it _does_ have that pointed-to directory as a string, and correctly\npasses it.  But since 1dd27bfbfd, we always get the numeric error code\nback from read_gitfile_gently(), and then decide whether to call\nread_gitfile_error_die() in the caller. And since we don't have the\n\"dir\" parameter, we just pass NULL.\n\nUnfortunately the fix is not a simple matter of passing the string to\nthe right function. We have to get it out of read_gitfile_gently() in\nthe first place, which means we have to return it as another\nout-parameter. And because it involves allocating memory, we can't just\ndo so unconditionally; callers need to be ready to free it after\nhandling the error.\n\nI've tried to make the minimally-invasive fix here:\n\n  1. We only copy the string when we hit READ_GITFILE_ERR_NOT_A_REPO,\n     so other error codes don't have to worry about freeing it.\n\n  2. We'll turn read_gitfile_gently() into a wrapper which passes NULL\n     by default, leaving other callers unaffected.\n\nThe result is kind of gross. There's an extra layer of macro\nindirection, and the validity of the string is subtly tied to the\nNOT_A_REPO error. A cleaner solution might be an error struct that\ncouples the code and the output string together, along with a function\nto free the error struct. But then all callers would have to be modified\nto call the free function. Alternatively, we could perhaps put a\nlarge-ish fixed-size buffer in the struct, though that means potential\ntruncation and a larger stack footprint in each caller (even when they\ndon't have see an error).\n\nSo I've left that as possible work for the future, or maybe never. Some\nof this gross-ness was already there. For example, the only other caller\nof read_gitfile_error_die() is in submodule.c, and it also passes NULL\nfor the \"dir\" parameter. But it does so only when the code is not\nNOT_A_REPO! So it is depending on the same subtle connection to avoid\ntriggering the bug.\n\nThere's an existing test in t0002 which triggers this case, but we\ndidn't notice the problem because it checks only that we said \"not a\nrepository\", and not the full string. So if we print \"(null)\" it is\nhappy. It will probably crash on some non-glibc platforms, but nobody\nseems to have reported it yet (the breakage is recent-ish as of v2.54).\nI'm also somewhat surprised that building with ASan/UBSan doesn't catch\nthis, but it doesn't seem to (and I found an open issue with somebody\nasking for it to be implemented in the sanitizers).\n\nWe can beef up the test by checking for the full string, which does\ndemonstrate the bug.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTwo other points of interest.\n\nOne, I'm not sure how useful printing the pointed-to directory is. We\n_could_ just say:\n\n  fatal: gitfile does not point to a valid repository: /path/to/.git\n\nwhich is enough for somebody to investigate themselves. That would\ncertainly make the patch smaller.\n\nAnd two, I ran into this running doc-diff:\n\n  $ ./doc-diff HEAD^ HEAD\n  fatal: not a git repository: (null)\n\nThe correct output (which this patch produces) is:\n\n  fatal: not a git repository: /home/peff/compile/git/.git/worktrees/worktree3\n\nAnd indeed, that path is missing. But why? I feel like I've run into\nthis same problem occasionally over the last year or so, but never\nbefore. Did we get more aggressive about removing worktrees at some\npoint? I haven't been able to reproduce whatever is killing off the\nworktree directory, and by the time I see the error it is long gone.\n\nAnyway, that's not strictly related to this bug, but just how I\nhappened across it.\n\n setup.c            | 19 +++++++++++++++----\n setup.h            |  3 ++-\n t/t0002-gitfile.sh |  2 +-\n 3 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 075bf89fa9..2df6fbf595 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -955,8 +955,14 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n  * will be set to an error code and NULL will be returned. If\n  * return_error_code is NULL the function will die instead (for most\n  * cases).\n+ *\n+ * If the code is READ_GITFILE_ERR_NOT_A_REPO and return_error_dir is\n+ * non-NULL, the directory to which the gitfile points will be returned\n+ * there. The caller is responsible for freeing the resulting string.\n  */\n-const char *read_gitfile_gently(const char *path, int *return_error_code)\n+const char *read_gitfile_gently_with_error_dir(const char *path,\n+\t\t\t\t\t       int *return_error_code,\n+\t\t\t\t\t       char **return_error_dir)\n {\n \tconst int max_file_size = 1 << 20;  /* 1MB */\n \tint error_code = 0;\n@@ -1021,6 +1027,8 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \t}\n \tif (!is_git_directory(dir)) {\n \t\terror_code = READ_GITFILE_ERR_NOT_A_REPO;\n+\t\tif (return_error_dir)\n+\t\t\t*return_error_dir = xstrdup(dir);\n \t\tgoto cleanup_return;\n \t}\n \n@@ -1613,11 +1621,12 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\tint offset = dir->len, error_code = 0;\n \t\tchar *gitdir_path = NULL;\n \t\tchar *gitfile = NULL;\n+\t\tchar *error_dst = NULL;\n \n \t\tif (offset > min_offset)\n \t\t\tstrbuf_addch(dir, '/');\n \t\tstrbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);\n-\t\tgitdirenv = read_gitfile_gently(dir->buf, &error_code);\n+\t\tgitdirenv = read_gitfile_gently_with_error_dir(dir->buf, &error_code, &error_dst);\n \t\tif (!gitdirenv) {\n \t\t\tswitch (error_code) {\n \t\t\tcase READ_GITFILE_ERR_MISSING:\n@@ -1641,9 +1650,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n \t\t\tdefault:\n \t\t\t\tif (die_on_error)\n-\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n-\t\t\t\telse\n+\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, error_dst);\n+\t\t\t\telse {\n+\t\t\t\t\tfree(error_dst);\n \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n+\t\t\t\t}\n \t\t\t}\n \t\t} else {\n \t\t\tgitfile = xstrdup(dir->buf);\ndiff --git a/setup.h b/setup.h\nindex 7878c9d267..65f55d5268 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -39,7 +39,8 @@ int is_nonbare_repository_dir(struct strbuf *path);\n #define READ_GITFILE_ERR_MISSING 9\n #define READ_GITFILE_ERR_IS_A_DIR 10\n void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n-const char *read_gitfile_gently(const char *path, int *return_error_code);\n+const char *read_gitfile_gently_with_error_dir(const char *path, int *return_error_code, char **return_error_dir);\n+#define read_gitfile_gently(path, err) read_gitfile_gently_with_error_dir((path), (err), NULL)\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\n const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\n #define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)\ndiff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh\nindex dfbcdddbcc..6967e12b9f 100755\n--- a/t/t0002-gitfile.sh\n+++ b/t/t0002-gitfile.sh\n@@ -27,7 +27,7 @@ test_expect_success 'bad setup: invalid .git file format' '\n test_expect_success 'bad setup: invalid .git file path' '\n \techo \"gitdir: $REAL.not\" >.git &&\n \ttest_must_fail git rev-parse 2>.err &&\n-\ttest_grep \"not a git repository\" .err\n+\ttest_grep \"not a git repository: $REAL.not\" .err\n '\n \n test_expect_success 'final setup + check rev-parse --git-dir' '\n-- \n2.54.0.682.g2f9b59d445\n"},{"id":"544463","messageId":"xmqq4ijlz8vc.fsf@gitster.g","threadId":"65731","inReplyTo":"20260602061159.GA693928@coredump.intra.peff.net","subject":"Re: [PATCH] read_gitfile_gently(): return non-repo path on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-02T07:42:15Z","receivedAt":"2026-06-02T07:42:17Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> I've tried to make the minimally-invasive fix here:\n>\n>   1. We only copy the string when we hit READ_GITFILE_ERR_NOT_A_REPO,\n>      so other error codes don't have to worry about freeing it.\n>\n>   2. We'll turn read_gitfile_gently() into a wrapper which passes NULL\n>      by default, leaving other callers unaffected.\n\nNice, probably.  I do not know what to feel about the first point,\nthough, as it burdens those who add new callers in the future more.\n\n> The result is kind of gross. There's an extra layer of macro\n> indirection, and the validity of the string is subtly tied to the\n> NOT_A_REPO error. A cleaner solution might be an error struct that\n> couples the code and the output string together, along with a function\n> to free the error struct. But then all callers would have to be modified\n> to call the free function. Alternatively, we could perhaps put a\n> large-ish fixed-size buffer in the struct, though that means potential\n> truncation and a larger stack footprint in each caller (even when they\n> don't have see an error).\n\nNone of thoese are particularly appetizing ;-).\n\n> So I've left that as possible work for the future, or maybe never. Some\n> of this gross-ness was already there. For example, the only other caller\n> of read_gitfile_error_die() is in submodule.c, and it also passes NULL\n> for the \"dir\" parameter. But it does so only when the code is not\n> NOT_A_REPO! So it is depending on the same subtle connection to avoid\n> triggering the bug.\n\nYup.  I can agree with this.\n\n> ---\n> Two other points of interest.\n>\n> One, I'm not sure how useful printing the pointed-to directory is. We\n> _could_ just say:\n>\n>   fatal: gitfile does not point to a valid repository: /path/to/.git\n>\n> which is enough for somebody to investigate themselves. That would\n> certainly make the patch smaller.\n\nThanks.  While reading the main explanation, it was the first thing\nthat came to me.\n\nThe implementation and the test are as expected in patches from you\nand matches the intent explained in the log message exactly.\n\nThanks, will queue.\n"},{"id":"544466","messageId":"20260602080223.GA763528@coredump.intra.peff.net","threadId":"65731","inReplyTo":"xmqq4ijlz8vc.fsf@gitster.g","subject":"Re: [PATCH] read_gitfile_gently(): return non-repo path on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-02T08:02:23Z","receivedAt":"2026-06-02T08:02:25Z","isPatch":true,"body":"On Tue, Jun 02, 2026 at 04:42:15PM +0900, Junio C Hamano wrote:\n\n> > One, I'm not sure how useful printing the pointed-to directory is. We\n> > _could_ just say:\n> >\n> >   fatal: gitfile does not point to a valid repository: /path/to/.git\n> >\n> > which is enough for somebody to investigate themselves. That would\n> > certainly make the patch smaller.\n> \n> Thanks.  While reading the main explanation, it was the first thing\n> that came to me.\n\nHere's what that looks like, for reference. It is nice and simple, if we\nthink the change in error message is acceptable. I hate to change\nuser-facing error messages because of internal code details, but I\nreally do wonder if the existing message is the most useful thing to\nprint in the first place.\n\ndiff --git a/setup.c b/setup.c\nindex 075bf89fa9..ed86671d84 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -920,7 +920,7 @@ int verify_repository_format(const struct repository_format *format,\n \treturn 0;\n }\n \n-void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n+void read_gitfile_error_die(int error_code, const char *path)\n {\n \tswitch (error_code) {\n \tcase READ_GITFILE_ERR_NOT_A_FILE:\n@@ -940,7 +940,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n \tcase READ_GITFILE_ERR_NO_PATH:\n \t\tdie(_(\"no path in gitfile: %s\"), path);\n \tcase READ_GITFILE_ERR_NOT_A_REPO:\n-\t\tdie(_(\"not a git repository: %s\"), dir);\n+\t\tdie(_(\"gitfile does not point to a valid repository: %s\"),\n+\t\t    path);\n \tdefault:\n \t\tBUG(\"unknown error code\");\n \t}\n@@ -1031,7 +1032,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \tif (return_error_code)\n \t\t*return_error_code = error_code;\n \telse if (error_code)\n-\t\tread_gitfile_error_die(error_code, path, dir);\n+\t\tread_gitfile_error_die(error_code, path);\n \n \tfree(buf);\n \treturn error_code ? NULL : path;\n@@ -1641,7 +1642,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n \t\t\tdefault:\n \t\t\t\tif (die_on_error)\n-\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n+\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf);\n \t\t\t\telse\n \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n \t\t\t}\ndiff --git a/setup.h b/setup.h\nindex 7878c9d267..436aaa22c1 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -38,7 +38,7 @@ int is_nonbare_repository_dir(struct strbuf *path);\n #define READ_GITFILE_ERR_TOO_LARGE 8\n #define READ_GITFILE_ERR_MISSING 9\n #define READ_GITFILE_ERR_IS_A_DIR 10\n-void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n+void read_gitfile_error_die(int error_code, const char *path);\n const char *read_gitfile_gently(const char *path, int *return_error_code);\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\n const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\ndiff --git a/submodule.c b/submodule.c\nindex a939ff5072..c36732ca0b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2578,7 +2578,7 @@ void absorb_git_dir_into_superproject(const char *path,\n \n \t\tif (err_code != READ_GITFILE_ERR_NOT_A_REPO)\n \t\t\t/* We don't know what broke here. */\n-\t\t\tread_gitfile_error_die(err_code, path, NULL);\n+\t\t\tread_gitfile_error_die(err_code, path);\n \n \t\t/*\n \t\t* Maybe populated, but no git directory was found?\ndiff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh\nindex dfbcdddbcc..6356e9ec72 100755\n--- a/t/t0002-gitfile.sh\n+++ b/t/t0002-gitfile.sh\n@@ -27,7 +27,7 @@ test_expect_success 'bad setup: invalid .git file format' '\n test_expect_success 'bad setup: invalid .git file path' '\n \techo \"gitdir: $REAL.not\" >.git &&\n \ttest_must_fail git rev-parse 2>.err &&\n-\ttest_grep \"not a git repository\" .err\n+\ttest_grep \"gitfile does not point to a valid repository\" .err\n '\n \n test_expect_success 'final setup + check rev-parse --git-dir' '\n"},{"id":"544471","messageId":"ah6WEtk2pXyViEQA@pks.im","threadId":"65731","inReplyTo":"20260602061159.GA693928@coredump.intra.peff.net","subject":"Re: [PATCH] read_gitfile_gently(): return non-repo path on error","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-02T08:36:34Z","receivedAt":"2026-06-02T08:36:39Z","isPatch":true,"body":"On Tue, Jun 02, 2026 at 02:11:59AM -0400, Jeff King wrote:\n[snip]\n> Two other points of interest.\n> \n> One, I'm not sure how useful printing the pointed-to directory is. We\n> _could_ just say:\n> \n>   fatal: gitfile does not point to a valid repository: /path/to/.git\n> \n> which is enough for somebody to investigate themselves. That would\n> certainly make the patch smaller.\n\nI have to agree that the patch is somewhat gross, and I myself don't\nreally see much of an issue to move to an error message like the above\nif it ends up simplifying the logic.\n\n> And two, I ran into this running doc-diff:\n> \n>   $ ./doc-diff HEAD^ HEAD\n>   fatal: not a git repository: (null)\n> \n> The correct output (which this patch produces) is:\n> \n>   fatal: not a git repository: /home/peff/compile/git/.git/worktrees/worktree3\n> \n> And indeed, that path is missing. But why? I feel like I've run into\n> this same problem occasionally over the last year or so, but never\n> before. Did we get more aggressive about removing worktrees at some\n> point? I haven't been able to reproduce whatever is killing off the\n> worktree directory, and by the time I see the error it is long gone.\n\nBoth git-gc(1) and git-maintenance(1) prune orphaned worktrees that are\nolder than three months by default, which can be configured via\n\"gc.worktreePruneExpire\". That logic has changed in 4dda60c9df (Merge\nbranch 'ps/maintenance-missing-tasks', 2025-05-15), which would kind of\nmatch your timeline.\n\nBut rereading that patch series I cannot really see how it could result\nin more aggressive pruning of worktrees. We used `git worktree prune\n--expire <expiry>` before that series, and we still use that logic now.\n\nHum.\n\n> diff --git a/setup.c b/setup.c\n> index 075bf89fa9..2df6fbf595 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1641,9 +1650,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n>  \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n>  \t\t\tdefault:\n>  \t\t\t\tif (die_on_error)\n> -\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n> -\t\t\t\telse\n> +\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, error_dst);\n> +\t\t\t\telse {\n> +\t\t\t\t\tfree(error_dst);\n>  \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n> +\t\t\t\t}\n\nThe `if` branch should also gain some curly braces here.\n\nPatrick\n"},{"id":"544667","messageId":"20260604062720.GA3195904@coredump.intra.peff.net","threadId":"65731","inReplyTo":"ah6WEtk2pXyViEQA@pks.im","subject":"Re: [PATCH] read_gitfile_gently(): return non-repo path on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-04T06:27:20Z","receivedAt":"2026-06-04T06:27:22Z","isPatch":true,"body":"On Tue, Jun 02, 2026 at 10:36:34AM +0200, Patrick Steinhardt wrote:\n\n> > The correct output (which this patch produces) is:\n> > \n> >   fatal: not a git repository: /home/peff/compile/git/.git/worktrees/worktree3\n> > \n> > And indeed, that path is missing. But why? I feel like I've run into\n> > this same problem occasionally over the last year or so, but never\n> > before. Did we get more aggressive about removing worktrees at some\n> > point? I haven't been able to reproduce whatever is killing off the\n> > worktree directory, and by the time I see the error it is long gone.\n> \n> Both git-gc(1) and git-maintenance(1) prune orphaned worktrees that are\n> older than three months by default, which can be configured via\n> \"gc.worktreePruneExpire\". That logic has changed in 4dda60c9df (Merge\n> branch 'ps/maintenance-missing-tasks', 2025-05-15), which would kind of\n> match your timeline.\n> \n> But rereading that patch series I cannot really see how it could result\n> in more aggressive pruning of worktrees. We used `git worktree prune\n> --expire <expiry>` before that series, and we still use that logic now.\n\nYeah, but this .git/worktrees/ directory shouldn't be pruned _at all_.\nThe worktree itself is still there (which is why I'm getting the error).\nSo perhaps there's a bug in checking that things are still there, or\nperhaps something is corrupting .git/worktrees/*/gitdir.\n\nAnother option is \"I moved my git checkout and the worktree prune\ncouldn't find the directory as an absolute path\", but I'm sure I didn't\ndo that.\n\nAn even more exotic option is that I run Git's test suite a lot, and\nvery occasionally bugs in the test suite cause the script to escape the\ntrash directory. And some scripts do run \"rm -r .git/worktrees\". I find\nit pretty unlikely for that to be the culprit though.\n\nOh well. I don't have any good leads, so I guess I'll see if it happens\nagain. But maybe now if somebody else sees it we can commiserate. :)\n\n-Peff\n"},{"id":"544675","messageId":"aiEkgBZJjmntRdNt@pks.im","threadId":"65731","inReplyTo":"20260604062720.GA3195904@coredump.intra.peff.net","subject":"Re: [PATCH] read_gitfile_gently(): return non-repo path on error","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-04T07:08:48Z","receivedAt":"2026-06-04T07:08:57Z","isPatch":true,"body":"On Thu, Jun 04, 2026 at 02:27:20AM -0400, Jeff King wrote:\n> On Tue, Jun 02, 2026 at 10:36:34AM +0200, Patrick Steinhardt wrote:\n> \n> > > The correct output (which this patch produces) is:\n> > > \n> > >   fatal: not a git repository: /home/peff/compile/git/.git/worktrees/worktree3\n> > > \n> > > And indeed, that path is missing. But why? I feel like I've run into\n> > > this same problem occasionally over the last year or so, but never\n> > > before. Did we get more aggressive about removing worktrees at some\n> > > point? I haven't been able to reproduce whatever is killing off the\n> > > worktree directory, and by the time I see the error it is long gone.\n> > \n> > Both git-gc(1) and git-maintenance(1) prune orphaned worktrees that are\n> > older than three months by default, which can be configured via\n> > \"gc.worktreePruneExpire\". That logic has changed in 4dda60c9df (Merge\n> > branch 'ps/maintenance-missing-tasks', 2025-05-15), which would kind of\n> > match your timeline.\n> > \n> > But rereading that patch series I cannot really see how it could result\n> > in more aggressive pruning of worktrees. We used `git worktree prune\n> > --expire <expiry>` before that series, and we still use that logic now.\n> \n> Yeah, but this .git/worktrees/ directory shouldn't be pruned _at all_.\n> The worktree itself is still there (which is why I'm getting the error).\n> So perhaps there's a bug in checking that things are still there, or\n> perhaps something is corrupting .git/worktrees/*/gitdir.\n\nOh, that sounds somewhat scary.\n\n> Another option is \"I moved my git checkout and the worktree prune\n> couldn't find the directory as an absolute path\", but I'm sure I didn't\n> do that.\n> \n> An even more exotic option is that I run Git's test suite a lot, and\n> very occasionally bugs in the test suite cause the script to escape the\n> trash directory. And some scripts do run \"rm -r .git/worktrees\". I find\n> it pretty unlikely for that to be the culprit though.\n> \n> Oh well. I don't have any good leads, so I guess I'll see if it happens\n> again. But maybe now if somebody else sees it we can commiserate. :)\n\nI'll certainly be on the watchout.\n\nPatrick\n"},{"id":"545139","messageId":"xmqqeciezh0w.fsf@gitster.g","threadId":"65731","inReplyTo":"ah6WEtk2pXyViEQA@pks.im","subject":"Re: [PATCH] read_gitfile_gently(): return non-repo path on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-10T13:01:03Z","receivedAt":"2026-06-10T13:01:06Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Tue, Jun 02, 2026 at 02:11:59AM -0400, Jeff King wrote:\n> [snip]\n>> Two other points of interest.\n>> \n>> One, I'm not sure how useful printing the pointed-to directory is. We\n>> _could_ just say:\n>> \n>>   fatal: gitfile does not point to a valid repository: /path/to/.git\n>> \n>> which is enough for somebody to investigate themselves. That would\n>> certainly make the patch smaller.\n>\n> I have to agree that the patch is somewhat gross, and I myself don't\n> really see much of an issue to move to an error message like the above\n> if it ends up simplifying the logic.\n\nSo we are in agreement among three of us that simplifying the code\nto lose error message with dubious value would be a good way\nforward.\n\nPeff, can we have a formal [v2] then?\n\n>> diff --git a/setup.c b/setup.c\n>> index 075bf89fa9..2df6fbf595 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -1641,9 +1650,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n>>  \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n>>  \t\t\tdefault:\n>>  \t\t\t\tif (die_on_error)\n>> -\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n>> -\t\t\t\telse\n>> +\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, error_dst);\n>> +\t\t\t\telse {\n>> +\t\t\t\t\tfree(error_dst);\n>>  \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n>> +\t\t\t\t}\n>\n> The `if` branch should also gain some curly braces here.\n\nTrue.\n\nThanks.\n"},{"id":"545650","messageId":"20260616111919.GC687438@coredump.intra.peff.net","threadId":"65731","inReplyTo":"xmqqeciezh0w.fsf@gitster.g","subject":"[PATCH v2] read_gitfile(): simplify NOT_A_REPO error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-16T11:19:19Z","receivedAt":"2026-06-16T11:19:21Z","isPatch":true,"body":"On Wed, Jun 10, 2026 at 06:01:03AM -0700, Junio C Hamano wrote:\n\n> > I have to agree that the patch is somewhat gross, and I myself don't\n> > really see much of an issue to move to an error message like the above\n> > if it ends up simplifying the logic.\n> \n> So we are in agreement among three of us that simplifying the code\n> to lose error message with dubious value would be a good way\n> forward.\n> \n> Peff, can we have a formal [v2] then?\n\nHere it is.\n\n-- >8 --\nSubject: read_gitfile(): simplify NOT_A_REPO error message\n\nIf a .git file is well-formed but points to a directory that is not\nitself a valid repository, then we say:\n\n  fatal: not a git repository: <pointed-to-repo>\n\nwithout mentioning the .git file that pointed us there in the first\nplace. Doing so could better help the user understand the source of the\nproblem.\n\nIn theory the most helpful thing we could do is mention both paths,\nlike:\n\n  gitfile '<gitfile>' points to invalid repository: <pointed-to-repo>\n\nBut there's another catch: when we generate the error, we don't always\nknow the pointed-to repository! This leads to a potential segfault.\n\nThe message comes from read_gitfile_error_die(). Originally we only\ncalled that function from inside read_gitfile_gently(), passing in both\nthe gitfile path and the pointed-to path. But that changed in 1dd27bfbfd\n(setup: improve error diagnosis for invalid .git files, 2026-03-04).\nSince then, the caller in setup_git_directory_gently(), even if it wants\nto die on error, always passes in the \"return_error_code\" flag, asking\nthe function to instead return a numeric error code. And then it calls\nread_gitfile_error_die() itself, passing NULL for the pointed-to path.\n\nIf we get the READ_GITFILE_ERR_NOT_A_REPO code, we form a message using\nthat NULL pointer, and either segfault or get garbage like \"not a git\nrepository: (null)\", depending on the platform.\n\nWe could fix this by having the function pass out both the numeric error\ncode and the pointed-to path. But that creates a new headache: we have\nto allocate that string on the heap and pass ownership back to the\ncaller. So now every caller has to be aware of it (and either free the\nresult, or signal that they are not interested by using an extra\nparameter).\n\nInstead, let's just drop the pointed-to path from the error message\nentirely, and mention only the gitfile. This fixes the NULL dereference\nwithout introducing any more complexity. The user-facing error message\nis not as detailed as it could be, but is better than the original.\nSince it mentions the gitfile, a user investigating the situation can\nlook there to find the pointed-to path (whereas you could not go the\nother way from the original message).\n\nThere's an existing test in t0002 which triggers this case, but we\ndidn't notice the problem because it checks only that we said \"not a\nrepository\", and not the full string. So if we print \"(null)\" it is\nhappy. It will probably crash on some non-glibc platforms, but nobody\nseems to have reported it yet (the breakage is recent-ish as of v2.54).\nI'm also somewhat surprised that building with ASan/UBSan doesn't catch\nthis, but it doesn't seem to (and I found an open issue with somebody\nasking for NULL printf checks to be implemented in the sanitizers).\n\nWe'll tweak the test to match the new error, but there's no need to beef\nit up further, since we're not showing the pointed-to path at all.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n setup.c            | 9 +++++----\n setup.h            | 2 +-\n submodule.c        | 2 +-\n t/t0002-gitfile.sh | 2 +-\n 4 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex b4652651df..b1d9249d91 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -917,7 +917,7 @@ int verify_repository_format(const struct repository_format *format,\n \treturn 0;\n }\n \n-void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n+void read_gitfile_error_die(int error_code, const char *path)\n {\n \tswitch (error_code) {\n \tcase READ_GITFILE_ERR_NOT_A_FILE:\n@@ -937,7 +937,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n \tcase READ_GITFILE_ERR_NO_PATH:\n \t\tdie(_(\"no path in gitfile: %s\"), path);\n \tcase READ_GITFILE_ERR_NOT_A_REPO:\n-\t\tdie(_(\"not a git repository: %s\"), dir);\n+\t\tdie(_(\"gitfile does not point to a valid repository: %s\"),\n+\t\t    path);\n \tdefault:\n \t\tBUG(\"unknown error code\");\n \t}\n@@ -1028,7 +1029,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \tif (return_error_code)\n \t\t*return_error_code = error_code;\n \telse if (error_code)\n-\t\tread_gitfile_error_die(error_code, path, dir);\n+\t\tread_gitfile_error_die(error_code, path);\n \n \tfree(buf);\n \treturn error_code ? NULL : path;\n@@ -1629,7 +1630,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n \t\t\tdefault:\n \t\t\t\tif (die_on_error)\n-\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n+\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf);\n \t\t\t\telse\n \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n \t\t\t}\ndiff --git a/setup.h b/setup.h\nindex 705d1d6ff7..df8c93687a 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -38,7 +38,7 @@ int is_nonbare_repository_dir(struct strbuf *path);\n #define READ_GITFILE_ERR_TOO_LARGE 8\n #define READ_GITFILE_ERR_MISSING 9\n #define READ_GITFILE_ERR_IS_A_DIR 10\n-void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n+void read_gitfile_error_die(int error_code, const char *path);\n const char *read_gitfile_gently(const char *path, int *return_error_code);\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\n const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\ndiff --git a/submodule.c b/submodule.c\nindex fd91201a92..93d0361072 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2579,7 +2579,7 @@ void absorb_git_dir_into_superproject(const char *path,\n \n \t\tif (err_code != READ_GITFILE_ERR_NOT_A_REPO)\n \t\t\t/* We don't know what broke here. */\n-\t\t\tread_gitfile_error_die(err_code, path, NULL);\n+\t\t\tread_gitfile_error_die(err_code, path);\n \n \t\t/*\n \t\t* Maybe populated, but no git directory was found?\ndiff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh\nindex dfbcdddbcc..6356e9ec72 100755\n--- a/t/t0002-gitfile.sh\n+++ b/t/t0002-gitfile.sh\n@@ -27,7 +27,7 @@ test_expect_success 'bad setup: invalid .git file format' '\n test_expect_success 'bad setup: invalid .git file path' '\n \techo \"gitdir: $REAL.not\" >.git &&\n \ttest_must_fail git rev-parse 2>.err &&\n-\ttest_grep \"not a git repository\" .err\n+\ttest_grep \"gitfile does not point to a valid repository\" .err\n '\n \n test_expect_success 'final setup + check rev-parse --git-dir' '\n-- \n2.55.0.rc0.342.g45e27e83e2\n\n"},{"id":"545651","messageId":"20260616123516.GA2301231@coredump.intra.peff.net","threadId":"65731","inReplyTo":"20260616111919.GC687438@coredump.intra.peff.net","subject":"[PATCH v3] read_gitfile(): simplify NOT_A_REPO error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-16T12:35:16Z","receivedAt":"2026-06-16T12:35:18Z","isPatch":true,"body":"On Tue, Jun 16, 2026 at 07:19:20AM -0400, Jeff King wrote:\n\n> Here it is.\n> \n> -- >8 --\n> Subject: read_gitfile(): simplify NOT_A_REPO error message\n\n<sigh> This triggered a failure in CI after passing tests locally.\nTurned out to be a race in t7450.\n\nHere's an update, with range-diff.\n\n1:  daf7f99511 ! 1:  67d42141e9 read_gitfile(): simplify NOT_A_REPO error message\n    @@ Commit message\n         We'll tweak the test to match the new error, but there's no need to beef\n         it up further, since we're not showing the pointed-to path at all.\n     \n    +    We also racily trigger this in t7450. During parallel cloning we might\n    +    see one of several errors, including this one. And so we must update\n    +    that message, too (you can otherwise find the failure pretty quickly by\n    +    running t7450 with --stress).\n    +\n         Signed-off-by: Jeff King <peff@peff.net>\n     \n      ## setup.c ##\n    @@ t/t0002-gitfile.sh: test_expect_success 'bad setup: invalid .git file format' '\n      '\n      \n      test_expect_success 'final setup + check rev-parse --git-dir' '\n    +\n    + ## t/t7450-bad-git-dotfiles.sh ##\n    +@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'git dirs of sibling submodules must not be nested' '\n    + test_expect_success 'submodule git dir nesting detection must work with parallel cloning' '\n    + \ttest_must_fail git clone --recurse-submodules --jobs=2 nested clone_parallel 2>err &&\n    + \tcat err &&\n    +-\tgrep -E \"(already exists|is inside git dir|not a git repository)\" err &&\n    ++\tgrep -E \"(already exists|is inside git dir|does not point to a valid repository)\" err &&\n    + \t{\n    + \t\ttest_path_is_missing .git/modules/hippo/HEAD ||\n    + \t\ttest_path_is_missing .git/modules/hippo/hooks/HEAD\n\n-- >8 --\nSubject: [PATCH] read_gitfile(): simplify NOT_A_REPO error message\n\nIf a .git file is well-formed but points to a directory that is not\nitself a valid repository, then we say:\n\n  fatal: not a git repository: <pointed-to-repo>\n\nwithout mentioning the .git file that pointed us there in the first\nplace. Doing so could better help the user understand the source of the\nproblem.\n\nIn theory the most helpful thing we could do is mention both paths,\nlike:\n\n  gitfile '<gitfile>' points to invalid repository: <pointed-to-repo>\n\nBut there's another catch: when we generate the error, we don't always\nknow the pointed-to repository! This leads to a potential segfault.\n\nThe message comes from read_gitfile_error_die(). Originally we only\ncalled that function from inside read_gitfile_gently(), passing in both\nthe gitfile path and the pointed-to path. But that changed in 1dd27bfbfd\n(setup: improve error diagnosis for invalid .git files, 2026-03-04).\nSince then, the caller in setup_git_directory_gently(), even if it wants\nto die on error, always passes in the \"return_error_code\" flag, asking\nthe function to instead return a numeric error code. And then it calls\nread_gitfile_error_die() itself, passing NULL for the pointed-to path.\n\nIf we get the READ_GITFILE_ERR_NOT_A_REPO code, we form a message using\nthat NULL pointer, and either segfault or get garbage like \"not a git\nrepository: (null)\", depending on the platform.\n\nWe could fix this by having the function pass out both the numeric error\ncode and the pointed-to path. But that creates a new headache: we have\nto allocate that string on the heap and pass ownership back to the\ncaller. So now every caller has to be aware of it (and either free the\nresult, or signal that they are not interested by using an extra\nparameter).\n\nInstead, let's just drop the pointed-to path from the error message\nentirely, and mention only the gitfile. This fixes the NULL dereference\nwithout introducing any more complexity. The user-facing error message\nis not as detailed as it could be, but is better than the original.\nSince it mentions the gitfile, a user investigating the situation can\nlook there to find the pointed-to path (whereas you could not go the\nother way from the original message).\n\nThere's an existing test in t0002 which triggers this case, but we\ndidn't notice the problem because it checks only that we said \"not a\nrepository\", and not the full string. So if we print \"(null)\" it is\nhappy. It will probably crash on some non-glibc platforms, but nobody\nseems to have reported it yet (the breakage is recent-ish as of v2.54).\nI'm also somewhat surprised that building with ASan/UBSan doesn't catch\nthis, but it doesn't seem to (and I found an open issue with somebody\nasking for NULL printf checks to be implemented in the sanitizers).\n\nWe'll tweak the test to match the new error, but there's no need to beef\nit up further, since we're not showing the pointed-to path at all.\n\nWe also racily trigger this in t7450. During parallel cloning we might\nsee one of several errors, including this one. And so we must update\nthat message, too (you can otherwise find the failure pretty quickly by\nrunning t7450 with --stress).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n setup.c                     | 9 +++++----\n setup.h                     | 2 +-\n submodule.c                 | 2 +-\n t/t0002-gitfile.sh          | 2 +-\n t/t7450-bad-git-dotfiles.sh | 2 +-\n 5 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex b4652651df..b1d9249d91 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -917,7 +917,7 @@ int verify_repository_format(const struct repository_format *format,\n \treturn 0;\n }\n \n-void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n+void read_gitfile_error_die(int error_code, const char *path)\n {\n \tswitch (error_code) {\n \tcase READ_GITFILE_ERR_NOT_A_FILE:\n@@ -937,7 +937,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n \tcase READ_GITFILE_ERR_NO_PATH:\n \t\tdie(_(\"no path in gitfile: %s\"), path);\n \tcase READ_GITFILE_ERR_NOT_A_REPO:\n-\t\tdie(_(\"not a git repository: %s\"), dir);\n+\t\tdie(_(\"gitfile does not point to a valid repository: %s\"),\n+\t\t    path);\n \tdefault:\n \t\tBUG(\"unknown error code\");\n \t}\n@@ -1028,7 +1029,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \tif (return_error_code)\n \t\t*return_error_code = error_code;\n \telse if (error_code)\n-\t\tread_gitfile_error_die(error_code, path, dir);\n+\t\tread_gitfile_error_die(error_code, path);\n \n \tfree(buf);\n \treturn error_code ? NULL : path;\n@@ -1629,7 +1630,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n \t\t\tdefault:\n \t\t\t\tif (die_on_error)\n-\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf, NULL);\n+\t\t\t\t\tread_gitfile_error_die(error_code, dir->buf);\n \t\t\t\telse\n \t\t\t\t\treturn GIT_DIR_INVALID_GITFILE;\n \t\t\t}\ndiff --git a/setup.h b/setup.h\nindex 705d1d6ff7..df8c93687a 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -38,7 +38,7 @@ int is_nonbare_repository_dir(struct strbuf *path);\n #define READ_GITFILE_ERR_TOO_LARGE 8\n #define READ_GITFILE_ERR_MISSING 9\n #define READ_GITFILE_ERR_IS_A_DIR 10\n-void read_gitfile_error_die(int error_code, const char *path, const char *dir);\n+void read_gitfile_error_die(int error_code, const char *path);\n const char *read_gitfile_gently(const char *path, int *return_error_code);\n #define read_gitfile(path) read_gitfile_gently((path), NULL)\n const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);\ndiff --git a/submodule.c b/submodule.c\nindex fd91201a92..93d0361072 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2579,7 +2579,7 @@ void absorb_git_dir_into_superproject(const char *path,\n \n \t\tif (err_code != READ_GITFILE_ERR_NOT_A_REPO)\n \t\t\t/* We don't know what broke here. */\n-\t\t\tread_gitfile_error_die(err_code, path, NULL);\n+\t\t\tread_gitfile_error_die(err_code, path);\n \n \t\t/*\n \t\t* Maybe populated, but no git directory was found?\ndiff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh\nindex dfbcdddbcc..6356e9ec72 100755\n--- a/t/t0002-gitfile.sh\n+++ b/t/t0002-gitfile.sh\n@@ -27,7 +27,7 @@ test_expect_success 'bad setup: invalid .git file format' '\n test_expect_success 'bad setup: invalid .git file path' '\n \techo \"gitdir: $REAL.not\" >.git &&\n \ttest_must_fail git rev-parse 2>.err &&\n-\ttest_grep \"not a git repository\" .err\n+\ttest_grep \"gitfile does not point to a valid repository\" .err\n '\n \n test_expect_success 'final setup + check rev-parse --git-dir' '\ndiff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh\nindex 8cc86522b2..69a17a9d13 100755\n--- a/t/t7450-bad-git-dotfiles.sh\n+++ b/t/t7450-bad-git-dotfiles.sh\n@@ -350,7 +350,7 @@ test_expect_success 'git dirs of sibling submodules must not be nested' '\n test_expect_success 'submodule git dir nesting detection must work with parallel cloning' '\n \ttest_must_fail git clone --recurse-submodules --jobs=2 nested clone_parallel 2>err &&\n \tcat err &&\n-\tgrep -E \"(already exists|is inside git dir|not a git repository)\" err &&\n+\tgrep -E \"(already exists|is inside git dir|does not point to a valid repository)\" err &&\n \t{\n \t\ttest_path_is_missing .git/modules/hippo/HEAD ||\n \t\ttest_path_is_missing .git/modules/hippo/hooks/HEAD\n-- \n2.55.0.rc0.346.g4c7eff6ddc\n\n"},{"id":"545656","messageId":"xmqq7bnya7gh.fsf@gitster.g","threadId":"65731","inReplyTo":"20260616123516.GA2301231@coredump.intra.peff.net","subject":"Re: [PATCH v3] read_gitfile(): simplify NOT_A_REPO error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-16T14:25:02Z","receivedAt":"2026-06-16T14:25:07Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jun 16, 2026 at 07:19:20AM -0400, Jeff King wrote:\n>\n>> Here it is.\n\nThanks.\n\n>     +@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'git dirs of sibling submodules must not be nested' '\n>     + test_expect_success 'submodule git dir nesting detection must work with parallel cloning' '\n>     + \ttest_must_fail git clone --recurse-submodules --jobs=2 nested clone_parallel 2>err &&\n>     + \tcat err &&\n>     +-\tgrep -E \"(already exists|is inside git dir|not a git repository)\" err &&\n>     ++\tgrep -E \"(already exists|is inside git dir|does not point to a valid repository)\" err &&\n\nA few things.\n\n * Will we be happy to see only one of these possibilities, or do we\n   expect to see these once for each kind?\n\n * a recently started in-flight topic tries to catch bare \"grep\" and\n   fails until you write test_grep X-<.\n\n> We also racily trigger this in t7450. During parallel cloning we might\n> see one of several errors, including this one. And so we must update\n> that message, too (you can otherwise find the failure pretty quickly by\n> running t7450 with --stress).\n\n"},{"id":"545657","messageId":"20260616144554.GA2305974@coredump.intra.peff.net","threadId":"65731","inReplyTo":"xmqq7bnya7gh.fsf@gitster.g","subject":"Re: [PATCH v3] read_gitfile(): simplify NOT_A_REPO error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-16T14:45:54Z","receivedAt":"2026-06-16T14:45:56Z","isPatch":true,"body":"On Tue, Jun 16, 2026 at 07:25:02AM -0700, Junio C Hamano wrote:\n\n> >     +@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'git dirs of sibling submodules must not be nested' '\n> >     + test_expect_success 'submodule git dir nesting detection must work with parallel cloning' '\n> >     + \ttest_must_fail git clone --recurse-submodules --jobs=2 nested clone_parallel 2>err &&\n> >     + \tcat err &&\n> >     +-\tgrep -E \"(already exists|is inside git dir|not a git repository)\" err &&\n> >     ++\tgrep -E \"(already exists|is inside git dir|does not point to a valid repository)\" err &&\n> \n> A few things.\n> \n>  * Will we be happy to see only one of these possibilities, or do we\n>    expect to see these once for each kind?\n\nI imagine it is only one. This all comes from 9cf8547320 (clone: prevent\nclashing git dirs when cloning submodule in parallel, 2024-01-28), and\nit is expecting the nested path to cause a failure. Which failure I\nguess depends on the racy ordering. If we create the inner one first,\nthen we probably get \"already exists\", and if the outer one, then \"is\ninside git dir\". I don't know exactly what sequence yields the\nNOT_A_REPO message.\n\nBut none of that is changing in this patch, just what the user-visible\ntext is for the NOT_A_REPO case.\n\nI did briefly wonder if we might see \"not a git repository\" from a\n_different_ code path, and need to catch it along with the new message.\nBut running successfully with --stress implies that we never see the old\none anymore.\n\n>  * a recently started in-flight topic tries to catch bare \"grep\" and\n>    fails until you write test_grep X-<.\n\nYeah. This will create a merge conflict for you, but hopefully the\nresolution should be obvious. I don't think it makes sense to fix here,\nas it's orthogonal to the purpose of the patch.\n\n-Peff\n"},{"id":"545670","messageId":"xmqqjyry4hax.fsf@gitster.g","threadId":"65731","inReplyTo":"20260616144554.GA2305974@coredump.intra.peff.net","subject":"Re: [PATCH v3] read_gitfile(): simplify NOT_A_REPO error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-16T15:48:54Z","receivedAt":"2026-06-16T15:48:58Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jun 16, 2026 at 07:25:02AM -0700, Junio C Hamano wrote:\n>\n>> >     +@@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'git dirs of sibling submodules must not be nested' '\n>> >     + test_expect_success 'submodule git dir nesting detection must work with parallel cloning' '\n>> >     + \ttest_must_fail git clone --recurse-submodules --jobs=2 nested clone_parallel 2>err &&\n>> >     + \tcat err &&\n>> >     +-\tgrep -E \"(already exists|is inside git dir|not a git repository)\" err &&\n>> >     ++\tgrep -E \"(already exists|is inside git dir|does not point to a valid repository)\" err &&\n>> \n>> A few things.\n>> \n>>  * Will we be happy to see only one of these possibilities, or do we\n>>    expect to see these once for each kind?\n>\n> I imagine it is only one. This all comes from 9cf8547320 (clone: prevent\n> clashing git dirs when cloning submodule in parallel, 2024-01-28), and\n> it is expecting the nested path to cause a failure. Which failure I\n> guess depends on the racy ordering. If we create the inner one first,\n> then we probably get \"already exists\", and if the outer one, then \"is\n> inside git dir\". I don't know exactly what sequence yields the\n> NOT_A_REPO message.\n>\n> But none of that is changing in this patch, just what the user-visible\n> text is for the NOT_A_REPO case.\n>\n> I did briefly wonder if we might see \"not a git repository\" from a\n> _different_ code path, and need to catch it along with the new message.\n> But running successfully with --stress implies that we never see the old\n> one anymore.\n\nI see.  Thanks.\n\n>\n>>  * a recently started in-flight topic tries to catch bare \"grep\" and\n>>    fails until you write test_grep X-<.\n>\n> Yeah. This will create a merge conflict for you, but hopefully the\n> resolution should be obvious. I don't think it makes sense to fix here,\n> as it's orthogonal to the purpose of the patch.\n\nYup.  I agree that, given that others in the same script will be\nupdated by that other topic to conflict with this change, it would\nnot make sense to do the same changes here.\n\nThanks.\n"}]}