Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

>>  setup.c                       | 42 ++++++++++++++------
>>  setup.h                       |  2 +
>>  submodule.c                   |  2 +-
>>  t/meson.build                 |  1 +
>>  t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
>>  worktree.c                    |  6 ++-
>>  6 files changed, 110 insertions(+), 15 deletions(-)
>>  create mode 100755 t/t0009-git-dir-validation.sh
>
> We'd probably need to treat ENOTDIR the same way as ENOENT to deal
> with cases where we expect a directory "sm1" to be the root of a
> submodule working tree, and we have a modification that removes the
> submodule directory and replace it with a regular file "sm1".  In
> the code path touched by this patch in submodule.c, we would ask "is
> sm1/.git a git directory?" and the stat(2) call on that path in
> read_gitfile_gently() used to say "Ah, a failure, that means we
> cannot positively say that 'sm1/.git' is a git directory or a gitdir
> file."  Now we inspect the error code in an attempt to tell if it is
> a system failure (e.g., a corrupt filesystem), but catching only
> ENOENT is probably a bit too tight.  In the above scenario, asking
> about 'sm1/.git' when 'sm1' is a regular file will not result in
> ENOENT but in ENOTDIR (i.e., "the leading 'sm1' is not a directory so
> it makes no sense to ask about 'sm1/.git'").
>
> Is it always sensible to treat ENOTDIR and ENOENT as two equivalent
> errors for the purpose of read_gitfile_gently()?  I have no clear
> answer offhand myself.  This is part of what we need to think about
> and resolve while addressing the original "NEEDSWORK:" comment.


----- >8 -----
Subject: [PATCH] read_gitfile(): group ENOENT and ENOTDIR into a
 single MISSING error

The code from the previous step does not deal wellwith a case where
we check if "sm/.git" is a good directory after replacing "sm" with
a regular file.  ENOTDIR is returned when we ask about "sm/.git",
not ENOENT, and the code would want to handle both.

I am not convinced if this is a good change, though.  Outside the
submodule caller, the story might be different and we may want to
treat ENOTDIR differently from ENOENT.  I dunno.  That is why this
is not squashed into the patch (yet).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c     | 8 ++++----
 setup.h     | 2 +-
 submodule.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/setup.c b/setup.c
index b79a9233f5..d4afbed3eb 100644
--- a/setup.c
+++ b/setup.c
@@ -895,7 +895,7 @@ int verify_repository_format(const struct repository_format *format,
 void read_gitfile_error_die(int error_code, const char *path, const char *dir)
 {
 	switch (error_code) {
-	case READ_GITFILE_ERR_STAT_ENOENT:
+	case READ_GITFILE_ERR_STAT_MISSING:
 	case READ_GITFILE_ERR_IS_A_DIR:
 		/* non-fatal; follow return path */
 		break;
@@ -943,8 +943,8 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 	static struct strbuf realpath = STRBUF_INIT;
 
 	if (stat(path, &st)) {
-		if (errno == ENOENT)
-			error_code = READ_GITFILE_ERR_STAT_ENOENT;
+		if (errno == ENOENT || errno == ENOTDIR)
+			error_code = READ_GITFILE_ERR_STAT_MISSING;
 		else
 			error_code = READ_GITFILE_ERR_STAT_FAILED;
 		goto cleanup_return;
@@ -1589,7 +1589,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		gitdirenv = read_gitfile_gently(dir->buf, &error_code);
 		if (!gitdirenv) {
 			switch (error_code) {
-			case READ_GITFILE_ERR_STAT_ENOENT:
+			case READ_GITFILE_ERR_STAT_MISSING:
 				/* no .git in this directory, move on */
 				break;
 			case READ_GITFILE_ERR_IS_A_DIR:
diff --git a/setup.h b/setup.h
index c23629cb4f..cc45f962fa 100644
--- a/setup.h
+++ b/setup.h
@@ -36,7 +36,7 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
-#define READ_GITFILE_ERR_STAT_ENOENT 9
+#define READ_GITFILE_ERR_STAT_MISSING 9
 #define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
diff --git a/submodule.c b/submodule.c
index 52a7cf0e43..fc85a7a1d8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2413,7 +2413,7 @@ void absorb_git_dir_into_superproject(const char *path,
 		const struct submodule *sub;
 		struct strbuf sub_gitdir = STRBUF_INIT;
 
-		if (err_code == READ_GITFILE_ERR_STAT_ENOENT) {
+		if (err_code == READ_GITFILE_ERR_STAT_MISSING) {
 			/* unpopulated as expected */
 			strbuf_release(&gitdir);
 			return;
-- 
2.53.0-455-g62fcd67e6e