Re: [PATCH v2 03/10] scalar-diagnose: add directory to archiver more gently

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2022, 21:09 UTC
Message-ID: <xmqqsfmb3ed9.fsf@gitster.g>
In-Reply-To: <220804.8635ebu55j.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 6 quoted lines

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

Hmph, the receiving end that inspects the archive will know that a directory did not get archived, but they cannot tell if that is because it did not exist, or because it was unreadable, and the trouble the user may be having can well be the result of having the directory unreadable. So from that point of view, in addition to these warnings and errors, it would be helpful to record the errors we encounter while we generate the diagnostic archive in the archive itself for inspection.

But the warning on the local side has merit to warn the user of an unusual situation. "I am puzzled why the hook I thought I wrote did not trigger, but the diag tool says I do not have .git/hooks at all" is a welcome side effect, even though it may not be the primary effect we are aiming to gain by having these warning messages.

Show 14 quoted lines

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

Funny. I find it much clearer if we can use our own message without having to rely on whatever strerror(error) gives us. We know better than the C library why we got ENOENT and be more readable. They say "No such file or directory" because from ENOENT alone they cannot tell you if it was a file or a directory that was missing, but we have a better context like "we were trying to create an archive" and "we tried opendir, expecting that the thing is a directory".

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

As I said above, I do not think it is an advantage in this case that strerror() is translated, as the point of having a separate message is because we can be more to-the-point, and we do not need to use the strerror() result in there.

Show 10 quoted lines

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

And an added clarity that we can use a separate message is something I think is worth having, compared to the cost of having an extra message over the more generic one for any errno.