bug#7756: 24.0.50; enhancements to package.el

[Stefan Kangas]

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

0001-Prompt-to-delete-duplicate-package-directory-on-inst.patch
Description: Text Data