bug#75065: Upon archive download failure print the original error

[Philip Kaludercic]

Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:

> On Thu, 2024-12-26 at 18:13 +0000, Philip Kaludercic wrote:
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
>> > > Date: Tue, 24 Dec 2024 18:25:30 +0300
>> > > 
>> > > I was recently helping out a new Emacs user with package
>> > > installation¹,
>> > > and I found an interesting thing: if you put to `package-
>> > > archives` a
>> > > URL without `https` prefix, download will fail. Long story short,
>> > > the
>> > > reason turns out that `package-archives` also supports local
>> > > paths,
>> > > which the URL being considered as. However, Emacs never prints a
>> > > message about that, even though such message exists in the code.
>> > > Instead it just says that download failed, leaving a user
>> > > wondering
>> > > why.
>> > > 
>> > > That happens because (package--download-and-read-archives)
>> > > ignores the
>> > > exception message, and always just prints generic failure
>> > > message.
>> > > 
>> > > This code fixes this, so now the actual failure message will be
>> > > correctly shown.
>> > > 
>> > > 1:
>> > > https://emacs.stackexchange.com/questions/82828/is-installing-deadgrep-fron-melpa-still-possible/82829#82829
>> > > 
>> > > 
>> > > From fb4685238726a79599f6388318916d2962da93ae Mon Sep 17 00:00:00
>> > > 2001
>> > > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
>> > > Date: Tue, 24 Dec 2024 18:16:41 +0300
>> > > Subject: [PATCH] Upon archive download failure print the original
>> > > error
>> > > 
>> > > * lisp/emacs-lisp/package.el (package--download-and-read-
>> > > archives):
>> > > upon catching exception, use the exception message as part of the
>> > > error to provide more context about the failure.
>> > > ---
>> > >  lisp/emacs-lisp/package.el | 6 +++---
>> > >  1 file changed, 3 insertions(+), 3 deletions(-)
>> > > 
>> > > diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-
>> > > lisp/package.el
>> > > index 5f785071ea3..cb81efc71f0 100644
>> > > --- a/lisp/emacs-lisp/package.el
>> > > +++ b/lisp/emacs-lisp/package.el
>> > > @@ -1829,10 +1829,10 @@ Populate `package-archive-contents' with
>> > > the result.
>> > >  If optional argument ASYNC is non-nil, perform the downloads
>> > >  asynchronously."
>> > >    (dolist (archive package-archives)
>> > > -    (condition-case-unless-debug nil
>> > > +    (condition-case-unless-debug err
>> > >          (package--download-one-archive archive "archive-
>> > > contents" async)
>> > > -      (error (message "Failed to download `%s' archive."
>> > > -               (car archive))))))
>> > > +      (error (message "Failed to download `%s' archive. Error:
>> > > %S"
>> > > +               (car archive) (cdr err))))))
>> > >  
>> > >  (defvar package-refresh-contents-hook (list #'package--download-
>> > > and-read-archives)
>> > >    "List of functions to call to refresh the package archive.
>> > > -- 
>> > > 2.47.1
>> > 
>> > Thanks.
>> > 
>> > Stefan and Philip, is this okay to install?
>> 
>> It seems harmless, I am just uncertain if we should prefer %S or %s
>> to
>> format the error message.
>
> The reason I used %S is that 2x lines below there's similar
> construction with (error …) and it's using %S, so the usage here goes
> in line with the other code.
>
> That said, I have no preference, I can change it to %s, even in both
> locations if you want.

In that case I think %S is preferable, and the patch is fine with me.