From: Tian Yuchen Date: Tue, 17 Feb 2026 15:30:25 GMT Subject: Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory Message-ID: In-Reply-To: Hi Karthik, Thanks for the review! > Small nit, it would have been a bit nicer to separate these out into > individual commits with tests added per commit. Since this is a security fix involving logic changes, I kept the tests and code together to ensure the commit is self-contained. I hope keeping them together is acceptable here! > Wouldn't something like 't0009-git-dir-validation.sh' be a better name? Indeed a much better name. Will rename it in the next reroll. > I understand the exclusion here (they are non-fatal flows), but wouldn't > it more make sense to add these two exclusions within > `read_gitfile_error_die()` which already has two such exclusions? By > separating this out, it gets really confusing. I actually implemented exactly that in previous patches (handling these exclusions inside 'read_gitfile_error_die'), but Junio pointed out that: >> diff --git a/setup.c b/setup.c >> index 3a6a048620..8681a8a9d1 100644 >> --- a/setup.c >> +++ b/setup.c >> @@ -911,6 +911,10 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir) >> die(_("no path in gitfile: %s"), path); >> case READ_GITFILE_ERR_NOT_A_REPO: >> die(_("not a git repository: %s"), dir); >> + case READ_GITFILE_ERR_STAT_ENOENT: >> + die(_("Not a git repository: %s"), path); >> + case READ_GITFILE_ERR_IS_A_DIR: >> + die(_("Not a git file (is a directory): %s"), path); > > Hmph, isn't this backwards? > > We used to treat STAT_FAILED as OK without dying in this function, > because we conflated "there is nothing there, so you should go one > level up and try again" happy case with all other stat(2) failure, > and that is why we introduced STAT_ENOENT here. ENOENT is the > *only* case among what used to be STAT_FAILED that we do *not* want > to die in this function. The same thing with NOT_A_FILE vs > IS_A_DIR. We used to treat the former as OK but the only case we > wanted to treat as OK was IS_A_DIR and all other cases, like FIFO, > we wanted to complain, no? In other word, ENOENT and IS_A_DIR cases are *VALID* states during the discovery process, not *ERRORS* that need to be suppressed in a "die" function. Therefore, we moved the decision-making logic up to the caller. This allows 'setup_git_directory_gently_1' to decide: ENOENT -> Continue search IS_A_DIR -> Check dir NOT_A_FILE -> Die Other -> Call 'read_gitfile_error_die()' *REAL ERROR* > Okay so we unconditionally read the error into errorcode, quick question > that comes to mind: Wouldn't this break the previous flow for when > `die_on_error = 1`? Where `read_gitfile_error_die()` would've been > called? It does change the flow, but intentionally, by passing &error_code (making it non-NULL), we prevent 'read_gitfile_gently' from automatically dying. We must do it because if it encounters a "garbage file", we now want to capture that error code and verify it in the caller. But more importantly, if it encounters ENOENT (which is now a distinct error code), we definitely do not want it to die, nor do we want to treat it as a fatal error. It does look a bit verbose, but it makes the state transitions explicit in 'setup_git_directory_gently_1'. I believe we are on the right track! Thanks again for the feedback. Regards, Yuchen