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

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

Karthik Nayak <karthik.188@gmail.com> writes:

>> @@ -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);
>>
>
> 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.

Absolutely.  The point of this change, IIUC, is that these two
existing exclusions were too broad.  stat() can fail for many
reasons, but because we did not differenciate ENOENT (which we *are*
happy to see and do not want to consider an error) from all other
error cases (which we may have been better off if we diagnosed them
as error), we pretended both ENOENT and all other stat() failures
were happy case and "case ERR_STAT_FAILED:" covered both.  To fix
this, the patch splits stat() failures into two, ERR_STAT_ENOENT is
the happy case we should have been returning without dying from
read_gitfile_error_die(), and ERR_STAT_FAILED is the rest that we
should have been dying there but in order to return from there
without dying when we got ENOENT, we were not dying there.  Now we
have a separate ERR_STAT_ENOENT, read_gitfile_error_die() can (and
should) die when we see ERR_STAT_FAILED, and it can (and should)
return to us when we see ERR_STAT_ENOENT as a happy case.  The story
is exactly the same between ERR_NOT_A_FILE (which had been non-error
only because we wanted to treat a directory as OK, but we can make
it an error) and ERR_IS_A DIR (which is new, and is an OK case).

The above exception on the caller's side you quoted is a complete
opposite from that line of reasoning, and that is why it is
confusing.

If there are other callers of read_gitfile_error_die() and different
semantics, such a "now we die on every possible errors" may also be
a valid position to take, *but* then it does not make sense unless
this patch makes read_gitfile_error_die() to die on ERR_STAT_FAILED
and ERR_NOT_A_FILE.