Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory

[Tian Yuchen <a3205153416@gmail.com>]

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