Skip to content

Commit 54a441b

Browse files
peffgitster
authored andcommitted
read_gitfile(): simplify NOT_A_REPO error message
If a .git file is well-formed but points to a directory that is not itself a valid repository, then we say: fatal: not a git repository: <pointed-to-repo> without mentioning the .git file that pointed us there in the first place. Doing so could better help the user understand the source of the problem. In theory the most helpful thing we could do is mention both paths, like: gitfile '<gitfile>' points to invalid repository: <pointed-to-repo> But there's another catch: when we generate the error, we don't always know the pointed-to repository! This leads to a potential segfault. The message comes from read_gitfile_error_die(). Originally we only called that function from inside read_gitfile_gently(), passing in both the gitfile path and the pointed-to path. But that changed in 1dd27bf (setup: improve error diagnosis for invalid .git files, 2026-03-04). Since then, the caller in setup_git_directory_gently(), even if it wants to die on error, always passes in the "return_error_code" flag, asking the function to instead return a numeric error code. And then it calls read_gitfile_error_die() itself, passing NULL for the pointed-to path. If we get the READ_GITFILE_ERR_NOT_A_REPO code, we form a message using that NULL pointer, and either segfault or get garbage like "not a git repository: (null)", depending on the platform. We could fix this by having the function pass out both the numeric error code and the pointed-to path. But that creates a new headache: we have to allocate that string on the heap and pass ownership back to the caller. So now every caller has to be aware of it (and either free the result, or signal that they are not interested by using an extra parameter). Instead, let's just drop the pointed-to path from the error message entirely, and mention only the gitfile. This fixes the NULL dereference without introducing any more complexity. The user-facing error message is not as detailed as it could be, but is better than the original. Since it mentions the gitfile, a user investigating the situation can look there to find the pointed-to path (whereas you could not go the other way from the original message). There's an existing test in t0002 which triggers this case, but we didn't notice the problem because it checks only that we said "not a repository", and not the full string. So if we print "(null)" it is happy. It will probably crash on some non-glibc platforms, but nobody seems to have reported it yet (the breakage is recent-ish as of v2.54). I'm also somewhat surprised that building with ASan/UBSan doesn't catch this, but it doesn't seem to (and I found an open issue with somebody asking for NULL printf checks to be implemented in the sanitizers). We'll tweak the test to match the new error, but there's no need to beef it up further, since we're not showing the pointed-to path at all. We also racily trigger this in t7450. During parallel cloning we might see one of several errors, including this one. And so we must update that message, too (you can otherwise find the failure pretty quickly by running t7450 with --stress). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 94f0577 commit 54a441b

5 files changed

Lines changed: 9 additions & 8 deletions

File tree

setup.c

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -917,7 +917,7 @@ int verify_repository_format(const struct repository_format *format,
917917
return 0;
918918
}
919919

920-
void read_gitfile_error_die(int error_code, const char *path, const char *dir)
920+
void read_gitfile_error_die(int error_code, const char *path)
921921
{
922922
switch (error_code) {
923923
case READ_GITFILE_ERR_NOT_A_FILE:
@@ -937,7 +937,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
937937
case READ_GITFILE_ERR_NO_PATH:
938938
die(_("no path in gitfile: %s"), path);
939939
case READ_GITFILE_ERR_NOT_A_REPO:
940-
die(_("not a git repository: %s"), dir);
940+
die(_("gitfile does not point to a valid repository: %s"),
941+
path);
941942
default:
942943
BUG("unknown error code");
943944
}
@@ -1028,7 +1029,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
10281029
if (return_error_code)
10291030
*return_error_code = error_code;
10301031
else if (error_code)
1031-
read_gitfile_error_die(error_code, path, dir);
1032+
read_gitfile_error_die(error_code, path);
10321033

10331034
free(buf);
10341035
return error_code ? NULL : path;
@@ -1633,7 +1634,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
16331634
return GIT_DIR_INVALID_GITFILE;
16341635
default:
16351636
if (die_on_error)
1636-
read_gitfile_error_die(error_code, dir->buf, NULL);
1637+
read_gitfile_error_die(error_code, dir->buf);
16371638
else
16381639
return GIT_DIR_INVALID_GITFILE;
16391640
}

setup.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ int is_nonbare_repository_dir(struct strbuf *path);
3838
#define READ_GITFILE_ERR_TOO_LARGE 8
3939
#define READ_GITFILE_ERR_MISSING 9
4040
#define READ_GITFILE_ERR_IS_A_DIR 10
41-
void read_gitfile_error_die(int error_code, const char *path, const char *dir);
41+
void read_gitfile_error_die(int error_code, const char *path);
4242
const char *read_gitfile_gently(const char *path, int *return_error_code);
4343
#define read_gitfile(path) read_gitfile_gently((path), NULL)
4444
const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);

submodule.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2578,7 +2578,7 @@ void absorb_git_dir_into_superproject(const char *path,
25782578

25792579
if (err_code != READ_GITFILE_ERR_NOT_A_REPO)
25802580
/* We don't know what broke here. */
2581-
read_gitfile_error_die(err_code, path, NULL);
2581+
read_gitfile_error_die(err_code, path);
25822582

25832583
/*
25842584
* Maybe populated, but no git directory was found?

t/t0002-gitfile.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ test_expect_success 'bad setup: invalid .git file format' '
2727
test_expect_success 'bad setup: invalid .git file path' '
2828
echo "gitdir: $REAL.not" >.git &&
2929
test_must_fail git rev-parse 2>.err &&
30-
test_grep "not a git repository" .err
30+
test_grep "gitfile does not point to a valid repository" .err
3131
'
3232

3333
test_expect_success 'final setup + check rev-parse --git-dir' '

t/t7450-bad-git-dotfiles.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ test_expect_success 'git dirs of sibling submodules must not be nested' '
348348
test_expect_success 'submodule git dir nesting detection must work with parallel cloning' '
349349
test_must_fail git clone --recurse-submodules --jobs=2 nested clone_parallel 2>err &&
350350
cat err &&
351-
grep -E "(already exists|is inside git dir|not a git repository)" err &&
351+
grep -E "(already exists|is inside git dir|does not point to a valid repository)" err &&
352352
{
353353
test_path_is_missing .git/modules/hippo/HEAD ||
354354
test_path_is_missing .git/modules/hippo/hooks/HEAD

0 commit comments

Comments
 (0)