| Message ID | 009cc49a18f2846c24256102e07437894ac16908.1657540174.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace2: dump scope when print "interesting" config | expand |
On Mon, Jul 11 2022, Teng Long wrote: > When finished call git_open(), instead of ignoring ENOENT silently > and return error_errno(_("cannot stat...")), it's better to check > the ENOENT before then output the warning. Makes sense, but... > - if (fd < 0) > + if (fd < 0) { > + if (errno != ENOENT) > + warning("'%s' cannot open '%s'", strerror(errno), bitmap_name); This should just be warning_errno("cannot open '%s'", bitmap_name), unless I'm missing something... > + free(bitmap_name); > return -1; > + } > + free(bitmap_name); > > if (fstat(fd, &st)) { > close(fd); > @@ -376,10 +379,14 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git > > bitmap_name = pack_bitmap_filename(packfile); > fd = git_open(bitmap_name); > - free(bitmap_name); > > - if (fd < 0) > + if (fd < 0) { > + if (errno != ENOENT) > + warning("'%s' cannot open '%s'", strerror(errno), bitmap_name); ...ditto. > + free(bitmap_name); > return -1; > + } > + free(bitmap_name); > > if (fstat(fd, &st)) { > close(fd);
Teng Long <dyroneteng@gmail.com> writes: > When finished call git_open(), instead of ignoring ENOENT silently > and return error_errno(_("cannot stat...")), it's better to check > the ENOENT before then output the warning. Both the title and the body describes a complete opposite of what the patch does, doesn't it? pack-bitmap.c: do not ignore error when opening a bitmap file Calls to git_open() to open the pack bitmap file and multi-pack bitmap file do not report any error when they fail. These files are optional and it is not an error if open failed due to ENOENT, but we shouldn't be ignoring other kinds of errors. or something? > + if (fd < 0) { > + if (errno != ENOENT) > + warning("'%s' cannot open '%s'", strerror(errno), bitmap_name); > + free(bitmap_name); Showing the errno is good, but I do not think it should be enclosed in single quotes. One thing you should consider when writing a "this is an optional file and not finding it is perfectly fine" logic like this one is that you may or may not want to ignore ENOTDIR. ENOTDIR is what you get when you say open("a/b") and "a" exists as something other than a directory. If you have a path with a slash in bitmap_name, and if in a sane and healthy repository the leading path should always exist (i.e. the file "a/b" may be missing, but the directory "a/" should be there), then getting ENOTDIR is a noteworthy event. There may be cases where ENOTDIR can justifiably and should be ignored.
On Mon, 11 Jul 2022 16:38:01 +0200, Ævar Arnfjörð Bjarmason wrote: > Makes sense, but... > ... > This should just be warning_errno("cannot open '%s'", bitmap_name), > unless I'm missing something... > ... > ...ditto. > You are right, I didn't notice warning_errno() before, will improve. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Both the title and the body describes a complete opposite of what > the patch does, doesn't it? > > pack-bitmap.c: do not ignore error when opening a bitmap file > > Calls to git_open() to open the pack bitmap file and > multi-pack bitmap file do not report any error when they > fail. These files are optional and it is not an error if > open failed due to ENOENT, but we shouldn't be ignoring > other kinds of errors. > > or something? Yes, will apply, thanks for this detailed assistent. > > + if (fd < 0) { > > + if (errno != ENOENT) > > + warning("'%s' cannot open '%s'", strerror(errno), bitmap_name); > > + free(bitmap_name); > > Showing the errno is good, but I do not think it should be enclosed > in single quotes. Yes, it should not be enclosed around and by Ævar Arnfjörð Bjarmason's advice the "warning" should be replaced by "warning_errno", I agree with that, so, I think at the same time it solves this problem. > One thing you should consider when writing a "this is an optional > file and not finding it is perfectly fine" logic like this one is > that you may or may not want to ignore ENOTDIR. $ errno ENOTDIR ENOTDIR 20 Not a directory >ENOTDIR is what you > get when you say open("a/b") and "a" exists as something other than > a directory. If you have a path with a slash in bitmap_name, and if > in a sane and healthy repository the leading path should always > exist (i.e. the file "a/b" may be missing, but the directory "a/" > should be there), then getting ENOTDIR is a noteworthy event. Make sense, and I tried to move "pack" away and touch a new file named "pack", then executed "git fsck", it will show the result about ENOTDIR: $ git fsck Checking object directories: 100% (256/256), done. error: unable to open object pack directory: /Users/tenglong.tl/Downloads/dyrone/.git/objects/pack: Not a directory So, this is how it works in git I think, caller should also print the "errno" when deal error and warning, append errno string as the last part and the first part is to describe what's wrong happened in git. > There may be cases where ENOTDIR can justifiably and should be ignored. Yes. If I understand it correctly, I think it's the current scene. For example, if this happens in case like "./pack" is a non-directory, then we print warning which contains ENOTDIR description under current change. Thanks.
diff --git a/pack-bitmap.c b/pack-bitmap.c index 7d8cc063fc..319eb721d8 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -317,10 +317,13 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, char *bitmap_name = midx_bitmap_filename(midx); int fd = git_open(bitmap_name); - free(bitmap_name); - - if (fd < 0) + if (fd < 0) { + if (errno != ENOENT) + warning("'%s' cannot open '%s'", strerror(errno), bitmap_name); + free(bitmap_name); return -1; + } + free(bitmap_name); if (fstat(fd, &st)) { close(fd); @@ -376,10 +379,14 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git bitmap_name = pack_bitmap_filename(packfile); fd = git_open(bitmap_name); - free(bitmap_name); - if (fd < 0) + if (fd < 0) { + if (errno != ENOENT) + warning("'%s' cannot open '%s'", strerror(errno), bitmap_name); + free(bitmap_name); return -1; + } + free(bitmap_name); if (fstat(fd, &st)) { close(fd);
When finished call git_open(), instead of ignoring ENOENT silently and return error_errno(_("cannot stat...")), it's better to check the ENOENT before then output the warning. Signed-off-by: Teng Long <dyroneteng@gmail.com> --- pack-bitmap.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)