Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic <philipk@posteo.net>]

You forgot to CC me in the response, I would have almost missed your response.

Harald Judt <h.judt@gmx.at> writes:

> Hi,
>
>
> On 2023-10-31 13:21, Philip Kaludercic wrote:
>
> [...]
>
>> Sure, I can take care of adding the package to the archive, you'll only
>> have to bump the "Version" header when you want the package to be
>> released (and in the future as well, the commit that touches the
>> "Version" header triggers a new release).
>> In the meantime, here is some quick and superficial feedback on the
>> code:
>
> Great news! Thank you for your feedback, I'll try to incorporate it
> and push the changes to the package git repository soon. Though I have
> a few questions understanding some of this:
>
>> diff --git a/dired-duplicates.el b/dired-duplicates.el
>> index d62af02..4af37a3 100644
>> --- a/dired-duplicates.el
>> +++ b/dired-duplicates.el
>> @@ -54,7 +54,6 @@
>>   (defcustom dired-duplicates-separate-results
>>     t
>>     "Boolean value indicating whether to separate results with new-lines."
>> -  :group 'dired-duplicates
>
> [...]
>
> Why should I not use :group for customization? I thought this makes it
> easier to explore customization?

See Michael's response.  Basically what is going on is that the defgroup
macro has a side effect during macro expansion.  Consider something like:

(defvar foo)

(defmacro bar (val)
  (ignore (setq foo val)))

(defmacro baz ()
  foo)

then:

(progn (bar 2) (baz)) expands to (progn nil 2)

>> @@ -103,21 +98,22 @@ return boolean t if the file matches a criteria, otherwise nil."
>>     The executable used is defined by
>> `dired-duplicates-checksum-exec'."
>>     (let* ((default-directory (file-name-directory (expand-file-name file)))
>> -         (exec (executable-find dired-duplicates-checksum-exec t)))
>> +         (exec (executable-find dired-duplicates-checksum-exec t))
>> +	 (file (expand-file-name (file-local-name file))))
>>       (unless exec
>> -      (user-error "Checksum program %s not found in exec-path" exec))
>> -    (car (split-string
>> -          (shell-command-to-string
>> -           (concat exec " \"" (expand-file-name (file-local-name file)) "\""))
>> -          nil
>> -          t))))
>> +      (user-error "Checksum program %s not found in `exec-path'" exec))
>> +    (with-temp-buffer
>> +      (unless (zerop (call-process exec nil t nil file))
>> +	(error "Failed to start checksum program %s" exec))
>> +      (goto-char (point-min))
>> +      (if (looking-at "\\`[[:alnum:]]+")
>> +	  (match-string 0)
>> +	(error "Unexpected output from checksum program %s" exec)))))
>
> Yeah, good idea to improve the error handling.
>
> [...]
>
>>   (defun dired-duplicates--find-and-filter-files (directories)
>>     "Search below DIRECTORIES for duplicate files.
>> @@ -140,14 +136,14 @@ duplicate files as values."
>>                       (append (gethash size same-size-table) (list f)))
>>              finally
>>              (cl-loop for same-size-files being the hash-values in same-size-table
>> -                    if (> (length same-size-files) 1) do
>> +                    if (> (length same-size-files) 1) do ;see length>
>>                       (cl-loop for f in same-size-files
>>                                for checksum = (dired-duplicates-checksum-file f)
>>                                do (setf (gethash checksum checksum-table)
>>                                         (append (gethash checksum checksum-table) (list f)))))
>>              (cl-loop for same-files being the hash-value in checksum-table using (hash-key checksum)
>>                       do
>> -                    (if (> (length same-files) 1)
>> +                    (if (cdr same-files) ;(> (length same-files) 1) in O(1)
>>                           (setf (gethash checksum checksum-table)
>>                                 (cons (file-attribute-size (file-attributes (car same-files)))
>>                                       (sort same-files #'string<)))
>
> Nice to know. Though I find length> more readable, I'll use cdr since
> I also use car ;-)

Note that that would require a hard dependency on Emacs 28 (unless you
use Compat).

>> @@ -180,7 +176,6 @@ Currently, this simply adds a new-line after each results group."
>>                  for len in lengths
>>                  do
>>                  (forward-line len)
>> -               ;; (forward-line len)
>>                  (let ((inhibit-read-only t))
>>                    (beginning-of-line)
>>                    (unless (= (point) (point-max))
>
> Thanks, I wonder how I have not noticed this.
>
>> @@ -225,7 +220,7 @@ This is the same as `dired-do-flagged-delete', but calls
>>     (defvar dired-duplicates-map
>>     (let ((map (make-sparse-keymap)))
>> -    ;; workaround for Emacs bug #57565
>> +    ;; workaround for Emacs bug#57565
>>       (when (< emacs-major-version 29)
>>         (define-key map (kbd "x") 'dired-duplicates--do-flagged-delete)
>>         (define-key map (kbd "D") 'dired-duplicates--do-delete))
>> @@ -248,7 +243,7 @@ The results will be shown in a Dired buffer."
>>                                                  default-directory)))
>>     (unless directories
>>       (user-error "Specify one or more directories to search in"))
>> -  (let* ((directories (if (listp directories) directories (list directories)))
>> +  (let* ((directories (if (listp directories) directories (list directories))) ;see `ensure-list'
>
> `ensure-list' would bump emacs requirements to 28.1 or require compat
> hence I did not use it.

But you have used other definitions that would have also raised the
dependency to at least 28.1?  You can use package-lint to detect these.

> [...]
>
> Thanks for the review and your suggestions.
>
> Regards,
> Harald

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Harald Judt <h.judt@gmx.at> writes:
>
>> > diff --git a/dired-duplicates.el b/dired-duplicates.el
>> > index d62af02..4af37a3 100644
>> > --- a/dired-duplicates.el
>> > +++ b/dired-duplicates.el
>> > @@ -54,7 +54,6 @@
>> >   (defcustom dired-duplicates-separate-results
>> >     t
>> >     "Boolean value indicating whether to separate results with new-lines."
>> > -  :group 'dired-duplicates
>>
>> [...]
>>
>> Why should I not use :group for customization? I thought this makes it
>> easier to explore customization?
>
> It's redundant - see (info "(elisp) Variable Definitions"):
>
>      If a ‘defcustom’ does not specify any ‘:group’, the last group
>      defined with ‘defgroup’ in the same file will be used.  This way,
>      most ‘defcustom’ do not need an explicit ‘:group’.
>
> If :group is specified, it will most of the time mean the defcustom should
> be assigned to some other, not to the default, group.  So it's better
> style to not explicitly specify the default.

One notable exception is if you have two groups defined in one file, and
you want to disambiguate the user options or avoid having to write these
in a specific order.

> Michael.

Harald Judt <h.judt@gmx.at> writes:

> On 2023-10-31 13:21, Philip Kaludercic wrote:
>
> [...]
>
>> @@ -225,7 +220,7 @@ This is the same as `dired-do-flagged-delete', but calls
>>     (defvar dired-duplicates-map
>>     (let ((map (make-sparse-keymap)))
>> -    ;; workaround for Emacs bug #57565
>> +    ;; workaround for Emacs bug#57565
>>       (when (< emacs-major-version 29)
>>         (define-key map (kbd "x") 'dired-duplicates--do-flagged-delete)
>>         (define-key map (kbd "D") 'dired-duplicates--do-delete))
>
> BTW: What's the reasoning behind this suggested change?

I thought that this would be necessary for bug-reference-mode to work,
but apparently it can detect both of these references.

> Harald

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: emacs-devel@gnu.org
>> Date: Tue, 31 Oct 2023 12:21:51 +0000
>> 
>> In the meantime, here is some quick and superficial feedback on the code:
>
> What I would like to ask is whether Harald tried to calculate SHA256
> using Emacs's own primitives, instead of relying on an external
> program, which may or may not be installed.

That is a good point, but I can imagine that one reason for using an
external tool is that if the user has a specific program they wish to
use that is not available as an Emacs primitive?