On Thu, Aug 04 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>>> + dir = opendir(at_root ? "." : path);
>>> + if (!dir) {
>>> + if (errno == ENOENT) {
>>
>> Per [1]
>
> "Per [1]" somehow sounds more like a reference to an authoritative
> source, at least to me. Every time you use it, I have to see what
> it refers to, and after realizing that you used it as a replacement
> of "I said it already in [1]" again, it leaves a funny feeling.
"Per" in the sense of "Per what I noted in [1]".
>> I think this is incorrect or overly strict. Let's not spew
>> warnings if the user "rm -rf .git/hooks" or whatever.
>
> The above is doing the right thing even in that situation, doesn't
> it? If there is no ".git/hooks" that is fine. We get ENOENT, give
> a warning to indicate that we found an unusual situation, and return
> without failing. If we got something other than ENOENT, we fail with
> error_errno(), because opendir() failed for a reason other than "No
> such file or directory".
I'm mainly noting that the point of this step is to produce an archive
for the consumption of the remote end.
Therefore it seems to me like it would me much more useful to note these
"oddities" in some log that we're about to zip up, rather than issue a
warning().
The "per [1]" was a reference to the (paraphrasing) "maybe that's not
needed at all", but you seemed to think so. But for the purposes of the
discussion here let's assume we keep it.
>> You already have an errno, so using *_errno() will add the standard
>> information about what the issue is.
>
> Reading the code aloud, slowly, may help. When errno says ENOENT,
> we know opendir() failed because of "No such file or directory",
> so "path" was missing. So let's say 'not archiving a missing directory'".
>
> ENOENT or "No such file or directory" is an implementation detail
> that does not help the end user.
>
> The other side, i.e. when the errno is *not* ENOENT, already uses
> error_errno().
>
> So, I am puzzled.
I'm pointing out that we don't need to include that part in the message,
because warning_errno() will already give us that for free. I.e.:
warning: could not archive directory '<some dir>': No such file or directory
v.s.:
warning: could not archive missing directory '<some dir>'
The advantages of doing so being:
* It's clear (at least to the keen eye) that it's using the "errno
format", so you know it's not just saying "could not for <whatever
reason>", it specifically got ENOENT.
* The i18n for the strerror() comes from the C library, which will be
translated already, whereas a new git.pot message won't be (but we'll
hopefully bridge the gap eventually).
* This way we we can share the message, whatever the errno happens to
be, so we could e.g.:
errno = ENOENT;
warning_errno(_("could not archive directory '%s'"), "<some dir>");
errno = ENOMEM;
error_errno(_("could not archive directory '%s'"), "<some dir>");
Whereas putting the reason for why we couldn't (which just duplicates
the errno) in the message forces the messages & i18n to diverge.
