| Message ID | 20240618234436.4107855-3-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | .git{ignore,attributes} directories? | expand |
On Tue, Jun 18, 2024 at 7:46 PM Junio C Hamano <gitster@pobox.com> wrote: > The code that reads .gitattributes files was careless in dealing in > unusual circumstances. > > - It let read errors silently ignored. ECANTPARSE: perhaps? s/errors/& be/ > - 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. > > Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> ECANTPARSE: perhaps? s/errors/& be/
You're right. Thanks.
On Tue, Jun 18, 2024 at 9:18 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > ECANTPARSE: perhaps? s/errors/& be/ > > You're right. Thanks. The same problem occurs in the commit message of patch [3/4].
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
Jeff King <peff@peff.net> writes: > 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. Argh, you're right. I thought the guard in warn_on_fopen_errors() would catch the artificial error code we give when we found that we opened a directory (by mistake), but the error we use is EISDIR, so it will be shown. OK, let's scrap the whole thing. It does not look like it is solving any real-world problems. Thanks.
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; + } if (st.st_size >= ATTR_MAX_FILE_SIZE) { warning(_("ignoring overly large gitattributes file '%s'"), path); fclose(fp); @@ -760,7 +765,10 @@ static struct attr_stack *read_attr_from_file(const char *path, unsigned flags) handle_attr_line(res, buf.buf, path, ++lineno, flags); } - fclose(fp); + if (ferror(fp)) + warning_errno(_("failed while reading gitattributes file '%s'"), path); + if (fclose(fp)) + warning_errno(_("cannot fclose gitattributes file '%s'"), path); strbuf_release(&buf); return res; } diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 66ccb5889d..783c20146d 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -603,6 +603,15 @@ test_expect_success EXPENSIVE 'large attributes blob ignored' ' test_cmp expect err ' +test_expect_success '.gitattribute directory behaves as if it does not exist' ' + test_when_finished "rm -fr dir" && + mkdir -p dir/.gitattributes && + >dir/ectory && + git -C dir check-attr --all ectory >out 2>err && + test_grep ! "failed while reading" err && + test_grep ! "cannot fclose" err +' + test_expect_success 'builtin object mode attributes work (dir and regular paths)' ' >normal && attr_check_object_mode normal 100644 &&
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. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- attr.c | 10 +++++++++- t/t0003-attributes.sh | 9 +++++++++ 2 files changed, 18 insertions(+), 1 deletion(-)