Re: [PATCH v3] setup: fail if .git is not a file or directory

[Tian Yuchen <a3205153416@gmail.com>]

diff --git a/setup.c b/setup.c
index 3a6a048620..269aa9faaa 100644
--- a/setup.c
+++ b/setup.c
@@ -939,8 +939,14 @@ const char *read_gitfile_gently(const char *path, 
int *return_error_code)
  	static struct strbuf realpath = STRBUF_INIT;

  	if (stat(path, &st)) {
-		/* NEEDSWORK: discern between ENOENT vs other errors */
-		error_code = READ_GITFILE_ERR_STAT_FAILED;
+		if (errno == ENOENT)
+			error_code = READ_GITFILE_ERR_STAT_ENOENT;
+		else
+			error_code = READ_GITFILE_ERR_STAT_FAILED;
+		goto cleanup_return;
+	}
+	if (S_ISDIR(st.st_mode)) {
+		error_code = READ_GITFILE_ERR_IS_A_DIR;
  		goto cleanup_return;
  	}
  	if (!S_ISREG(st.st_mode)) {
@@ -994,7 +1000,9 @@ const char *read_gitfile_gently(const char *path, 
int *return_error_code)
  cleanup_return:
  	if (return_error_code)
  		*return_error_code = error_code;
-	else if (error_code)
+	else if (error_code &&
+		error_code != READ_GITFILE_ERR_STAT_ENOENT &&
+		error_code != READ_GITFILE_ERR_IS_A_DIR)
  		read_gitfile_error_die(error_code, path, dir);

  	free(buf);
@@ -1576,20 +1584,27 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
  		if (offset > min_offset)
  			strbuf_addch(dir, '/');
  		strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
-		gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
-						NULL : &error_code);
+		gitdirenv = read_gitfile_gently(dir->buf, &error_code);
  		if (!gitdirenv) {
-			if (die_on_error ||
-			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
-				/* NEEDSWORK: fail if .git is not file nor dir */
+			if (error_code == READ_GITFILE_ERR_STAT_ENOENT) {
+				;
+			} else if (error_code == READ_GITFILE_ERR_IS_A_DIR) {
  				if (is_git_directory(dir->buf)) {
  					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
  					gitdir_path = xstrdup(dir->buf);
  				}
-			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
-				return GIT_DIR_INVALID_GITFILE;
-		} else
-			gitfile = xstrdup(dir->buf);
+			} else if (error_code == READ_GITFILE_ERR_NOT_A_FILE) {
+				if (die_on_error)
+					die(_("Invalid %s: not a regular file or directory"), dir->buf);
+				else
+					return GIT_DIR_INVALID_GITFILE;
+			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
+				if (die_on_error)
+					read_gitfile_error_die(error_code, dir->buf, NULL);
+				else
+					return GIT_DIR_INVALID_GITFILE;
+			}
+		}
  		/*
  		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
  		 * to check that directory for a repository.
diff --git a/setup.h b/setup.h
index d55dcc6608..0271cc8f93 100644
--- a/setup.h
+++ b/setup.h
@@ -36,6 +36,9 @@ 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_IS_A_DIR 10
+


Still working on it.

Looks clearer, right? I just wanna confirm whether I'm on the right 
track. Notice that I didn't change the following part: (though you 
mentioned earlier)

 > cleanup_return:
 > 	if (return_error_code)
 > 		*return_error_code = error_code;
 >-	else if (error_code)
 >+	else if (error_code &&
 >+		error_code != READ_GITFILE_ERR_STAT_ENOENT &&
 >+		error_code != READ_GITFILE_ERR_IS_A_DIR)
 > 		read_gitfile_error_die(error_code, path, dir);

My understanding is that we haven't distinguished between “unexpected” 
(ENOENT, IS_A_DIR) and “errors” (NOT_A_FILE, format error) in the code 
using a categorical approach. Instead, we differentiate them solely 
based on whether they are listed in `read_gitfile_error_die()` (if I 
haven't missed anything). Consequently, retaining this code here seems 
to be the easiest and most readable approach to ensure that it doesn't 
return prematurely. However, I believe this check shouldn't reside here. 
That's why I'm replying to you again.

(By the way, I've always been a bit curious about how your name is 
pronounced :/)

Regards,

Yuchen