bug#7756: 24.0.50; enhancements to package.el

[Stefan Kangas <stefan@marxist.se>]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> @@ -338,9 +338,14 @@
>>       (pkg-file (expand-file-name
>>              (concat (package-strip-version package) "-pkg")
>>              pkg-dir)))
>> -    (when (and (file-directory-p pkg-dir)
>> -           (file-exists-p (concat pkg-file ".el")))
>> -      (load pkg-file nil t))))
>> +    ;; When one is creating a package and testing it out, it is easy
>> +    ;; to forget to add the -pkg.el file.  When that happens, it would
>> +    ;; be useful to provide feedback to the user rather than silently
>> +    ;; failing.  That is what (error ...) below is for.
>> +    (when (file-directory-p pkg-dir)
>> +      (if (file-exists-p (concat pkg-file ".el"))
>> +          (load pkg-file nil t)
>> +        (error "'%s' file is missing!")))))
>
> I agree with the intention, but I wonder: why do we pass the `noerror'
> parameter to `load' in the first place?

This code seems to have changed substantially since this patch was
proposed in 2010.  Having looked at it now, I think naively raising an
error here would risk breaking things.  I therefore suggest to do
nothing here for now.

>> @@ -569,8 +574,16 @@
>>  (defun package-unpack (name version)
>>    (let ((pkg-dir (expand-file-name (concat (symbol-name name) "-" version)
>>                     package-user-dir)))
>> +    ;; Delete the package directory if it exists already.  This may
>> +    ;; not be useful to the end users.  However this is extremely
>> +    ;; important for package developers as they experiment with which
>> +    ;; files to include in a package.  If a file is removed from one
>> +    ;; iteration to the next, then the presence of the unwanted elisp
>> +    ;; file could cause problems by polluting the generated autoload
>> +    ;; file.
>> +    (if (file-directory-p pkg-dir)
>> +        (delete-directory pkg-dir 'recursive))
>
> Here, I also agree with the intention, but I'm a little uneasy blindly
> removing a whole subdirectory without warning: I'd add a confirmation prompt.

I have attached a patch which does the same but adds a confirmation
prompt.  I've also added a test case for this particular case.  WDYT?

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Prompt-to-delete-duplicate-package-directory-on-inst.patch --]
[-- Type: text/x-patch, Size: 3400 bytes --]

From b1962898dac4c89c58db066f598b656ba42f8807 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 15 Sep 2019 01:49:37 +0200
Subject: [PATCH] Prompt to delete duplicate package directory on install

* lisp/emacs-lisp/package.el (package-unpack): Prompt to delete
package directory if it already exists.  (Bug#7756)

* test/lisp/emacs-lisp/package-tests.el
(package-test-install-multi-file/pkg-dir-already-exists): New test
case for installation when package directory already exists.
(with-package-test): Support above test case.
---
 lisp/emacs-lisp/package.el            |  6 +++++-
 test/lisp/emacs-lisp/package-tests.el | 12 +++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index ef0c5171de..2ac07bc055 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -888,7 +888,11 @@ package-unpack
                (if (> (length file-list) 1) 'tar 'single))))
       ('tar
        (make-directory package-user-dir t)
-       ;; FIXME: should we delete PKG-DIR if it exists?
+       (when (and (file-directory-p pkg-dir)
+                  (yes-or-no-p
+                   (format "Package directory already exists: `%s'.  Delete it?"
+                           (directory-file-name pkg-dir))))
+         (delete-directory pkg-dir 'recursive))
        (let* ((default-directory (file-name-as-directory package-user-dir)))
          (package-untar-buffer dirname)))
       ('single
diff --git a/test/lisp/emacs-lisp/package-tests.el b/test/lisp/emacs-lisp/package-tests.el
index c757bccf67..ffe04e8fe7 100644
--- a/test/lisp/emacs-lisp/package-tests.el
+++ b/test/lisp/emacs-lisp/package-tests.el
@@ -109,7 +109,8 @@ with-package-test
                                            install
                                            location
                                            update-news
-                                           upload-base)
+                                           upload-base
+                                           create-directory)
                                 &rest body)
   "Set up temporary locations and variables for testing."
   (declare (indent 1) (debug (([&rest form]) body)))
@@ -138,6 +139,9 @@ with-package-test
            ,(if basedir `(cd ,basedir))
            (unless (file-directory-p package-user-dir)
              (mkdir package-user-dir))
+           ,(if create-directory
+                `(make-directory (expand-file-name ,create-directory
+                                                   package-test-user-dir) t))
            (cl-letf (((symbol-function 'yes-or-no-p) (lambda (&rest r) t))
                      ((symbol-function 'y-or-n-p)    (lambda (&rest r) t)))
              ,@(when install
@@ -344,6 +348,12 @@ package-test-install-multifile
           (goto-char (point-min))
           (should (re-search-forward re nil t)))))))
 
+(ert-deftest package-test-install-multi-file/pkg-dir-already-exists ()
+  "Install multi-file package over already existing directory."
+  (with-package-test (:basedir "package-resources" :install '(multi-file)
+                      :create-directory "multi-file-0.2.3")
+    (should (package-installed-p 'multi-file))))
+
 (ert-deftest package-test-update-listing ()
   "Ensure installed package status is updated."
   (with-package-test ()
-- 
2.20.1