From: Karthik Nayak Date: Tue, 17 Feb 2026 18:56:03 GMT Subject: Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory Message-ID: In-Reply-To: Tian Yuchen writes: > 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! > My indication wasn't separate the tests out into an individual commit. But rather to highlight that this one commit is doing multiple things and it would be nice to split it out and each commit could tackle an individual problem with tests included. >> 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* > My understanding was Junio was suggesting that what you're doing is the inverse of what is expected. In short (on top of your patch), something like: diff --git a/setup.c b/setup.c index c3dd6a4197..7edf921564 100644 --- a/setup.c +++ b/setup.c @@ -898,10 +898,14 @@ 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_FAILED: - case READ_GITFILE_ERR_NOT_A_FILE: + case READ_GITFILE_ERR_STAT_ENOENT: + case READ_GITFILE_ERR_IS_A_DIR: /* non-fatal; follow return path */ break; + case READ_GITFILE_ERR_STAT_FAILED: + die(_("stat failed to run correctly")) + case READ_GITFILE_ERR_NOT_A_FILE: + die(_("unsupported file type")) case READ_GITFILE_ERR_OPEN_FAILED: die_errno(_("error opening '%s'"), path); case READ_GITFILE_ERR_TOO_LARGE: >> 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