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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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