Subject: Re: [PATCH v6 4/7] pack-bitmap.c: don't ignore ENOENT silently

From: Junio C Hamano <[email-masked]>
Date: Mon, 11 Jul 2022 14:22:15 -0700

Teng Long <[email-masked]> 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.