Re: [PATCH 2/4] attr: notice and report read failure of .gitattributes files

From: Jeff King <peff@peff.net>
Date: Jun 19, 2024, 13:57 UTC
Message-ID: <20240619135754.GB943023@coredump.intra.peff.net>
In-Reply-To: <20240618234436.4107855-3-gitster@pobox.com>

On Tue, Jun 18, 2024 at 04:44:33PM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> 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...

Show 13 quoted lines

> 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