On Tue, Jun 18, 2024 at 04:44:33PM -0700, Junio C Hamano wrote:
> The code that reads .gitattributes files was careless in dealing in
> unusual circumstances.
>
> - It let read errors silently ignored.
>
> - It tried to read ".gitattributes" that is a directory on
> platforms that allowed open(2) to open directories.
>
> To make the latter consistent with what we do for fopen() on
> directories ("git grep" for FREAD_READS_DIRECTORIES for details),
> check if we opened a directory, silently close it and return
> success. Catch all read errors before we close and report as
> needed.
Makes sense, but...
> diff --git a/attr.c b/attr.c
> index 300f994ba6..9ab9cf1551 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -747,6 +747,11 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)
> fclose(fp);
> return NULL;
> }
> + if (S_ISDIR(st.st_mode)) {
> + /* On FREAD_READS_DIRECTORIES platforms */
> + fclose(fp);
> + return NULL;
> + }
I don't know that this is really consistent with callers of fopen(),
since they tend to complain noisily. Usually via warn_on_fopen_errors(),
which we ourselves call above.
I.e., if we were using fopen() here rather than open()+fdopen(), our
call would likewise complain noisily. And we even did so prior to
2ef579e261 (attr: do not respect symlinks for in-tree .gitattributes,
2021-02-16)! We had to switch to open() then to use O_NOFOLLOW.
And I notice that every caller of open_nofollow() does an fdopen()
anyway. So I wonder if the better solution would be to make an
fopen_nofollow() that handles the FREAD_READS_DIRECTORIES stuff itself,
just like fopen() does?
That is also an interesting data point regarding the "is it sane to have
directory .gitattributes" question. Older versions would definitely
complain about it (once per file in the diff even):
$ mkdir .gitattributes
$ git.v2.31.0 show >/dev/null
warning: unable to access '.gitattributes': Is a directory
warning: unable to access '.gitattributes': Is a directory
-Peff
