[ELPA] New package: dired-duplicates

[ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 1206 bytes --]

Hi,

I have developed the following package and assigned copyright for it to the 
FSF and would now like to publish it to GNU ELPA (it has previously been 
published to MELPA):

URL: https://codeberg.org/hjudt/dired-duplicates

Description:
This package helps to find duplicate files on local and remote filesystems.
It is similar to the fdupes command-line utility but written in Emacs Lisp
and should also work on every remote filesystem that TRAMP supports and
where executable commands can be called remotely.  The only external
requirement is a checksum program like md5 or sha256sum that generates a
hash value from the contents of a file used for comparison, because Emacs
cannot do that in a performance-efficient way.

dired-duplicates works by first searching files of the same size, then
invoking the calculation of the checksum for these files, and presents the
grouped results in a Dired buffer that the user can work with similarly to
a regular Dired buffer.
----

Could someone help me with the next steps?

Regards,
Harald


-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic]

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

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

> Hi,
>
> I have developed the following package and assigned copyright for it
> to the FSF and would now like to publish it to GNU ELPA (it has
> previously been published to MELPA):
>
> URL: https://codeberg.org/hjudt/dired-duplicates
>
> Description:
> This package helps to find duplicate files on local and remote filesystems.
> It is similar to the fdupes command-line utility but written in Emacs Lisp
> and should also work on every remote filesystem that TRAMP supports and
> where executable commands can be called remotely.  The only external
> requirement is a checksum program like md5 or sha256sum that generates a
> hash value from the contents of a file used for comparison, because Emacs
> cannot do that in a performance-efficient way.
>
> dired-duplicates works by first searching files of the same size, then
> invoking the calculation of the checksum for these files, and presents the
> grouped results in a Dired buffer that the user can work with similarly to
> a regular Dired buffer.
> ----
>
> Could someone help me with the next steps?

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:


[-- Attachment #2: Type: text/plain, Size: 6202 bytes --]

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
   :tag "Separate results"
   :type 'boolean)
 
@@ -64,7 +63,6 @@
 
 The checksums will be used for comparison of files of the same
 size."
-  :group 'dired-duplicates
   :tag "Checksum executable"
   :type 'string)
 
@@ -73,10 +71,9 @@ size."
   "The comparison function used for sorting grouped results.
 
 The sorting can be in ascending (<) or descending (>) order."
-  :group 'dired-duplicates
   :tag "Ascending or descending file size sort order"
-  :type '(choice (const :tag "Ascending" :value <)
-                 (const :tag "Descending" :value >)))
+  :type '(choice (const :tag "Ascending" <)
+                 (const :tag "Descending" >)))
 
 (defcustom dired-duplicates-file-filter-functions
   nil
@@ -84,14 +81,12 @@ The sorting can be in ascending (<) or descending (>) order."
 
 A filter function must accept as its single argument the file and
 return boolean t if the file matches a criteria, otherwise nil."
-  :group 'dired-duplicates
   :tag "File filter functions"
   :type 'hook)
 
 (defcustom dired-duplicates-search-directories-recursively
   t
   "Search directories recursively."
-  :group 'dired-duplicates
   :tag "Search directories recursively"
   :type 'boolean)
 
@@ -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)))))
 
 (defun dired-duplicates--apply-file-filter-functions (files)
   "Apply file filter functions to FILES, returning the resulting list."
-  (if (and dired-duplicates-file-filter-functions files)
-      (dolist (filter-func dired-duplicates-file-filter-functions files)
-        (setf files (cl-delete-if-not filter-func files)))
-    files))
+  (dolist (filter-func dired-duplicates-file-filter-functions files)
+    (setf files (cl-delete-if-not filter-func files))))
 
 (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<)))
@@ -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))
@@ -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'
          (truncated-dirs (truncate-string-to-width (string-join directories ", ") 40 0 nil t)))
     (message "Finding duplicate files in %s..." truncated-dirs)
     (if-let ((default-directory "/")
@@ -257,7 +252,7 @@ The results will be shown in a Dired buffer."
           (message "Finding duplicate files in %s completed." truncated-dirs)
           (dired (cons "/" (flatten-list results)))
           (set-keymap-parent dired-duplicates-map (current-local-map))
-          (setf (current-local-map) dired-duplicates-map)
+	  (use-local-map dired-duplicates-map)
           (setq-local dired-duplicates-directories directories)
           (setq-local revert-buffer-function 'dired-duplicates-dired-revert)
           (dired-duplicates--post-process-dired-buffer results))

[-- Attachment #3: Type: text/plain, Size: 44 bytes --]


> Regards,
> Harald

-- 
Philip Kaludercic

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 5246 bytes --]

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?

> @@ -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 ;-)

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

[...]

Thanks for the review and your suggestions.

Regards,
Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Michael Heerdegen]

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.

Michael.

RE: [External] : Re: [ELPA] New package: dired-duplicates

[Drew Adams]

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

All true.
But "not need" doesn't imply "shouldn't have".

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

FWIW, I disagree, stylistically; I think it's worse
style.

1. I disagree that it's important either way, and
that removing "redundant" :group's should ever be
suggested as a convention.

2. It's clearer, less error-prone, and more
practical to use :group explicitly, even when
"redundant", because moving defcustoms around (e.g.
reordering) doesn't risk implicit changes to the
wrong group.

Yes, this is a personal choice - and IMO it should
be.  I don't see a reason for suggesting any
particular convention for users wrt including
:group when it's "redundant".

A convention for code that's to be included in
Emacs itself is a different matter.  I'm talking
about a suggestion that there's something bad or
wrong with users including :group redundantly.

(I realize that here it is about code submitted
to be included in Emacs itself.  I also think
removing "redundant" :group's shouldn't be a
requirement for Emacs itself.  But that's not up
to me.)

Re: [External] : Re: [ELPA] New package: dired-duplicates

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> Date: Wed, 1 Nov 2023 15:16:13 +0000
> 
> >      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’.
> 
> All true.
> But "not need" doesn't imply "shouldn't have".

We decided that it means "shouldn't have".

> > 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.
> 
> FWIW, I disagree, stylistically; I think it's worse
> style.

You are entitled to your opinions, but it is futile to reiterate them
here, since the project as a whole has decided on a certain policy,
and we ask you to please respect that and stop fighting it here, lest
someone is mistaken to think that it is a matter of personal
preferences, when Emacs source code is discussed.

RE: [External] : Re: [ELPA] New package: dired-duplicates

[Drew Adams]

> > > 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’.
> >
> > All true.
> > But "not need" doesn't imply "shouldn't have".
> 
> We decided that it means "shouldn't have".

I wasn't aware that such a rigid decision had been made.

However, I don't see it in the Elisp manual.  Where is
it specified?

Instead, in the Elisp manual, node `Group Definitions':

  you specify the group’s members by using the
  ‘:group’ keyword when defining those members.

Sounds a bit like one explicitly specifies :group
for each member.  Nothing about specifying it only
for the first member of a set of consecutive member
definitions.  Ambiguous, at best, I guess, if it's
intended to say where and when you "should" specify
:group.

Perhaps you should change the text quoted by Michael,
to make clear that you don't really mean (what most
people understand by) "do not need", but instead you
really mean "shouldn't have"?

  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’.
                   ^^^^^^^^^^^
                 shouldn't have

As for an explicit statement of the convention, I
find nothing at all about :group in the sections
of the manual (such as `Coding Conventions'), that
state the Elisp coding conventions.

Is this a doc bug?  Is there somewhere else where
what you say was decided is declared?

> > > 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.
> >
> > FWIW, I disagree, stylistically; I think it's worse
> > style.
> 
> You are entitled to your opinions, but it is futile to reiterate them
> here, since the project as a whole has decided on a certain policy,
> and we ask you to please respect that and stop fighting it here, lest
> someone is mistaken to think that it is a matter of personal
> preferences, when Emacs source code is discussed.

I made clear that I was talking about use of :group by
users in their code, not for code that's to be included
in Emacs.  Very clear.  No fighting, though you appear
to be trying.

But I now have the question about where your edict about
:group is proclaimed for code that's to be included in
Emacs.  Where is that, besides in your slap-down reply
to my msg?

Re: [External] : Re: [ELPA] New package: dired-duplicates

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "michael_heerdegen@web.de" <michael_heerdegen@web.de>,
>         "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Wed, 1 Nov 2023 16:28:40 +0000
> 
> > > > 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’.
> > >
> > > All true.
> > > But "not need" doesn't imply "shouldn't have".
> > 
> > We decided that it means "shouldn't have".
> 
> I wasn't aware that such a rigid decision had been made.

We make a point of saying that in every patch review.

> However, I don't see it in the Elisp manual.

The ELisp manual documents the language, not our policy.  But even it
talks about that, in the part that was quoted.

> Perhaps you should change the text quoted by Michael,
> to make clear that you don't really mean (what most
> people understand by) "do not need", but instead you
> really mean "shouldn't have"?

I don't see a need for such a change.

Re: [External] : Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic]

Drew Adams <drew.adams@oracle.com> writes:

> Yes, this is a personal choice - and IMO it should
> be.  I don't see a reason for suggesting any
> particular convention for users wrt including
> :group when it's "redundant".

One reason for suggesting it is that many people don't know that it is
optional.  Most of the suggestions I make at least are of a "fun fact"
or "neat but not necessary" type.

Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic]

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?

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 9635 bytes --]

Hi,

On 2023-11-01 12:32, Philip Kaludercic wrote:

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

I am very sorry for this.

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

Thanks for both of you giving an explanation.


[...]

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

I want to keep Emacs-27 as target unless the packages grows that much that it 
warrants including compat or bumping the Emacs version.

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

No, I have used package-lint already (it has great integration into flymake 
btw) so this did not happen.

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

To be honest, I have looked at how some existing package had implemented 
customization and did it the same way. I have no problems with removing the 
:group declarations if the defgroup is the default.


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

Yes, whether space or not, it doesn't matter, at least in Emacs-29.1. But that 
aside, bug reference mode wants to take me to a page on my project 
(https://codeberg.org/bug/issues/57565) which does not exist because it is an 
*Emacs* bug that is referenced in the comment, not a bug in dired-duplicates.

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

I have not tried to calculate SHA256 using Emacs's own primitives, but not 
because I did not want to. The reasoning behind my decision is the following:

dired-duplicates does support local files and *remote* files - via TRAMP. For 
local files using Emacs's own primitives could be OK performance-wise, I have 
not tested this. But if one starts to use them for fetching files via TRAMP 
which means over network, then things could slow down to a crawl pretty fast 
(it even takes quite a while collecting size stats over the network). So it is 
not Emacs's SHA256 calculation that would be a problem here, but instead slow 
transport over network. If the host does the calculation, this should be a lot 
faster (though that of course depends on whether the files are local files on 
the host of course and not some mounted network resource). I think it is 
reasonable to assume that one would *not* want to download large video files 
over the network to check if they are duplicates.

The second thing is customizability of course, and probably necessary because 
of my reasoning explained in the previous paragraph. The remote host might 
have md5sum installed, but not sha256sum. So I wanted to provide a way for the 
user to configure the checksum command.

I do not object against implementing a fallback as Eli suggested; I simply 
have not implemented it because I did not know about Emacs's primitives, and 
at that same time my reasoning about remote files started and so that is the 
way it is now.

Regards,
Harald


-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic]

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

> Hi,
>
> On 2023-11-01 12:32, Philip Kaludercic wrote:
>
>> You forgot to CC me in the response, I would have almost missed your response.
>
> I am very sorry for this.

No worries, happens to everyone :)

>> 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.
>
> Yes, whether space or not, it doesn't matter, at least in
> Emacs-29.1. But that aside, bug reference mode wants to take me to a
> page on my project (https://codeberg.org/bug/issues/57565) which does
> not exist because it is an *Emacs* bug that is referenced in the
> comment, not a bug in dired-duplicates.

I am afraid I don't understand.  Did you modify `bug-reference-url-format'?

>> 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?
>
> I have not tried to calculate SHA256 using Emacs's own primitives, but
> not because I did not want to. The reasoning behind my decision is the
> following:
>
> dired-duplicates does support local files and *remote* files - via
> TRAMP. For local files using Emacs's own primitives could be OK
> performance-wise, I have not tested this. But if one starts to use
> them for fetching files via TRAMP which means over network, then
> things could slow down to a crawl pretty fast (it even takes quite a
> while collecting size stats over the network). So it is not Emacs's
> SHA256 calculation that would be a problem here, but instead slow
> transport over network. If the host does the calculation, this should
> be a lot faster (though that of course depends on whether the files
> are local files on the host of course and not some mounted network
> resource). I think it is reasonable to assume that one would *not*
> want to download large video files over the network to check if they
> are duplicates.
>
> The second thing is customizability of course, and probably necessary
> because of my reasoning explained in the previous paragraph. The
> remote host might have md5sum installed, but not sha256sum. So I
> wanted to provide a way for the user to configure the checksum
> command.
>
> I do not object against implementing a fallback as Eli suggested; I
> simply have not implemented it because I did not know about Emacs's
> primitives, and at that same time my reasoning about remote files
> started and so that is the way it is now.
>
> Regards,
> Harald

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

> Hi,
>
> On 2023-11-01 20:24, Eli Zaretskii wrote:
>>> From: Philip Kaludercic <philipk@posteo.net>
>>> Cc: Visuwesh <visuweshm@gmail.com>,  h.judt@gmx.at,  emacs-devel@gnu.org
>>> Date: Wed, 01 Nov 2023 17:57:40 +0000
>>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>> I'd like to see the numbers which led to the conclusion that
>>>> performance was prohibitive.
>>>>
>>>> And even if the performance is indeed much worse, it could be a
>>>> fallback in case the program is not available -- which would IMO be
>>>> much better than simply failing to provide the functionality in that
>>>> case.
>>>
>>> This is a cheap test on a 1.4GB ISO I had lying around:
>> Thanks.  But this is just a single large file, not a frequent use
>> case
>> for this package.
>> 
>>> --8<---------------cut here---------------start------------->8---
>>> (benchmark-run 1
>>>    (with-temp-buffer
>>>      (insert-file-contents-literally "~/Downloads/haiku-r1beta4-x86_64-anyboot.iso")
>>>      (secure-hash 'sha512 (current-buffer))))
>>> ;; (44.389091035 1 1.5836082630000021)
>>>
>>> (benchmark-run 1
>>>    (with-temp-buffer
>>>      (call-process "sha512sum" nil t nil (expand-file-name "~/Downloads/haiku-r1beta4-x86_64-anyboot.iso"))
>>>      (goto-char (point-min))
>>>      (and (looking-at (rx bos (+ alnum)))
>>> 	 (match-string 0))))
>>> ;; (5.155846791 0 0.0)
>>> --8<---------------cut here---------------end--------------->8---
>> And this is not the package doing its job, this is just a single
>> task
>> the package does when looking for duplicates.  The numbers when
>> running the package with call-process replaced by secure-hash will
>> probably be different.
>
> Thanks for the numbers. What troubles me a bit more than the speed is
> which effects inserting such a large file into a buffer has
> memory-wise?

It appears to depend on the size of the file.  I haven't determined the
exact tipping point where loading a file into memory becomes more
expensive than spawning a process, but it seems to be relatively low.

Another advantage of using a subprocess, is that the files can be
processed in parallel, assuming you start the processes asynchronously. 

> Regards,
> Harald

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

> Hi Philip,
>
> Another more organisational question regarding your review: If I apply
> your patch and create commits from it in the project, should I then
> use your name and email address as author of the commit (I guess I can
> assume you also assigned copyright to the FSF ;-))?

You can attribute it to me, but I don't really care much either way.  I
didn't really send you a patch to apply, but just the output of C-x v =
after leaving a few comments in the file.

> Harald

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 1485 bytes --]

On 2023-11-01 22:29, Philip Kaludercic wrote:

[...]

> Harald Judt <h.judt@gmx.at> writes:
> 
>> Hi Philip,
>>
>> Another more organisational question regarding your review: If I apply
>> your patch and create commits from it in the project, should I then
>> use your name and email address as author of the commit (I guess I can
>> assume you also assigned copyright to the FSF ;-))?
> 
> You can attribute it to me, but I don't really care much either way.  I
> didn't really send you a patch to apply, but just the output of C-x v =
> after leaving a few comments in the file.
> 
>> Harald

Thank you, I have attributed the improvement for error handling of the 
checksum program to you, while simply pushing the rest.

I will try to implement the checksumming using Emacs's primitives in one of 
the next versions, but first have to think about what's the best way to 
integrate it from a usability perspective (and whether it should be a fallback 
or just another togglable option like setting checksum exec var to nil).

If anyone has other suggestions how to improve functionality of this package, 
I'd be glad to hear.

Now, I think I have addressed all points from your feedback, how may I go on 
pushing a release? Simply increase the version number to 0.2 as you said in my 
codeberg repo?

Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic]

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

> Now, I think I have addressed all points from your feedback, how may I
> go on pushing a release? Simply increase the version number to 0.2 as
> you said in my codeberg repo?

In principle yes, but we haven't added the package yet.  Were there any
outstanding issues that were not resolved?

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 873 bytes --]

Am 03.11.23 um 09:19 schrieb Philip Kaludercic:
> Harald Judt <h.judt@gmx.at> writes:
> 
>> Now, I think I have addressed all points from your feedback, how may I
>> go on pushing a release? Simply increase the version number to 0.2 as
>> you said in my codeberg repo?
> 
> In principle yes, but we haven't added the package yet.  Were there any
> outstanding issues that were not resolved?

I do not think there are any outstanding issues left, I have fixed the things 
discussed here with the last 7 commits 
(https://codeberg.org/hjudt/dired-duplicates/commits/branch/main). Of course, 
there is Eli's proposal to use Emacs's primitives as a fallback, but I intend 
to implement this in a future version.

Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic]

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

> Am 03.11.23 um 09:19 schrieb Philip Kaludercic:
>> Harald Judt <h.judt@gmx.at> writes:
>> 
>>> Now, I think I have addressed all points from your feedback, how may I
>>> go on pushing a release? Simply increase the version number to 0.2 as
>>> you said in my codeberg repo?
>> In principle yes, but we haven't added the package yet.  Were there
>> any
>> outstanding issues that were not resolved?
>
> I do not think there are any outstanding issues left, I have fixed the
> things discussed here with the last 7 commits
> (https://codeberg.org/hjudt/dired-duplicates/commits/branch/main). Of
> course, there is Eli's proposal to use Emacs's primitives as a
> fallback, but I intend to implement this in a future version.

OK, I have added the package to GNU ELPA.  It should appear within the
next day (you'll get an email, because your address is listed as the
maintainer).

> Harald

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 3854 bytes --]

Hi,

On 2023-11-04 at 16:27, Philip Kaludercic wrote:
> Harald Judt <h.judt@gmx.at> writes:
> 
>> Am 03.11.23 um 09:19 schrieb Philip Kaludercic:
>>> Harald Judt <h.judt@gmx.at> writes:
>>>
>>>> Now, I think I have addressed all points from your feedback, how may I
>>>> go on pushing a release? Simply increase the version number to 0.2 as
>>>> you said in my codeberg repo?
>>> In principle yes, but we haven't added the package yet.  Were there
>>> any
>>> outstanding issues that were not resolved?
>>
>> I do not think there are any outstanding issues left, I have fixed the
>> things discussed here with the last 7 commits
>> (https://codeberg.org/hjudt/dired-duplicates/commits/branch/main). Of
>> course, there is Eli's proposal to use Emacs's primitives as a
>> fallback, but I intend to implement this in a future version.
> 
> OK, I have added the package to GNU ELPA.  It should appear within the
> next day (you'll get an email, because your address is listed as the
> maintainer).
> 
>> Harald

Unfortunately, I got a mail that it failed to build and tried to follow the 
instructions locally but it also failed for me (likely due to some other issue 
here because it is missing a shared lib in a sandboxed environment?):

--------------------------------------------------------------------------------
make build/dired-duplicates
emacs --batch -l /mnt/scratch/work/elpa/admin/elpa-admin.el     \
          -f elpaa-batch-make-one-package dired-duplicates
======== Building tarball 
archive-devel/dired-duplicates-0.1.0.20231105.3955.tar...
Build error for archive-devel/dired-duplicates-0.1.0.20231105.3955.tar: (error 
"Error-indicating exit code in elpaa--call-sandboxed:
emacs: error while loading shared libraries: libgccjit.so.0: cannot open 
shared object file: No such file or directory
")
######## Build of package 
archive-devel/dired-duplicates-0.1.0.20231105.3955.tar FAILED!!
======== Building tarball archive/dired-duplicates-0.1.tar...
Build error for archive/dired-duplicates-0.1.tar: (error "Can’t find main file 
/mnt/scratch/work/elpa/packages/dired-duplicates/dired-duplicates.el file in 
/mnt/scratch/work/elpa/packages/dired-duplicates/")
######## Build of package archive/dired-duplicates-0.1.tar FAILED!!
--------------------------------------------------------------------------------


The error I got by mail from ELPA is the following:


--------------------------------------------------------------------------------
The build scripts failed to build the tarball
for version 0.1 of the package dired-duplicates.
You can consult the latest error output in the file
"dired-duplicates-build-failure.txt" in the GNU ELPA archive web site.

You can also try and reproduce the error locally as follows:

     git clone --single-branch https://git.sv.gnu.org/git/emacs/elpa.git
     cd elpa
     make                            # Setup the infrastructure
     make packages/dired-duplicates  # Create a worktree of the package
     make build/dired-duplicates     # Build the tarballs into archive(-devel)/

## The current error output was the following:

======== Building tarball archive/dired-duplicates-0.1.tar...
Build error for archive/dired-duplicates-0.1.tar: (error "Can't find main file 
home/elpa/elpa/packages/dired-duplicates/dired-duplicates.el file in 
/home/elpa/elpa/packages/dired-duplicates")
######## Build of package archive/dired-duplicates-0.1.tar FAILED!!
--------------------------------------------------------------------------------

When I look into the archive/dired-duplicates-build-failure.txt file the paths 
mentioned there seem correct.


Regards,
Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic]

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

> Hi,
>
> On 2023-11-04 at 16:27, Philip Kaludercic wrote:
>> Harald Judt <h.judt@gmx.at> writes:
>> 
>>> Am 03.11.23 um 09:19 schrieb Philip Kaludercic:
>>>> Harald Judt <h.judt@gmx.at> writes:
>>>>
>>>>> Now, I think I have addressed all points from your feedback, how may I
>>>>> go on pushing a release? Simply increase the version number to 0.2 as
>>>>> you said in my codeberg repo?
>>>> In principle yes, but we haven't added the package yet.  Were there
>>>> any
>>>> outstanding issues that were not resolved?
>>>
>>> I do not think there are any outstanding issues left, I have fixed the
>>> things discussed here with the last 7 commits
>>> (https://codeberg.org/hjudt/dired-duplicates/commits/branch/main). Of
>>> course, there is Eli's proposal to use Emacs's primitives as a
>>> fallback, but I intend to implement this in a future version.
>> OK, I have added the package to GNU ELPA.  It should appear within
>> the
>> next day (you'll get an email, because your address is listed as the
>> maintainer).
>> 
>>> Harald
>
> Unfortunately, I got a mail that it failed to build and tried to
> follow the instructions locally but it also failed for me (likely due
> to some other issue here because it is missing a shared lib in a
> sandboxed environment?):
>
> --------------------------------------------------------------------------------
> make build/dired-duplicates
> emacs --batch -l /mnt/scratch/work/elpa/admin/elpa-admin.el     \
>          -f elpaa-batch-make-one-package dired-duplicates
> ======== Building tarball
> archive-devel/dired-duplicates-0.1.0.20231105.3955.tar...
> Build error for
> archive-devel/dired-duplicates-0.1.0.20231105.3955.tar: (error
> "Error-indicating exit code in elpaa--call-sandboxed:
> emacs: error while loading shared libraries: libgccjit.so.0: cannot
> open shared object file: No such file or directory
> ")
> ######## Build of package
>          archive-devel/dired-duplicates-0.1.0.20231105.3955.tar
>         FAILED!!
> ======== Building tarball archive/dired-duplicates-0.1.tar...
> Build error for archive/dired-duplicates-0.1.tar: (error "Can’t find
> main file
> /mnt/scratch/work/elpa/packages/dired-duplicates/dired-duplicates.el
> file in /mnt/scratch/work/elpa/packages/dired-duplicates/")
> ######## Build of package archive/dired-duplicates-0.1.tar FAILED!!
> --------------------------------------------------------------------------------
>
>
> The error I got by mail from ELPA is the following:
>
>
> --------------------------------------------------------------------------------
> The build scripts failed to build the tarball
> for version 0.1 of the package dired-duplicates.
> You can consult the latest error output in the file
> "dired-duplicates-build-failure.txt" in the GNU ELPA archive web site.
>
> You can also try and reproduce the error locally as follows:
>
>     git clone --single-branch https://git.sv.gnu.org/git/emacs/elpa.git
>     cd elpa
>     make                            # Setup the infrastructure
>     make packages/dired-duplicates  # Create a worktree of the package
>     make build/dired-duplicates     # Build the tarballs into archive(-devel)/
>
> ## The current error output was the following:
>
> ======== Building tarball archive/dired-duplicates-0.1.tar...
> Build error for archive/dired-duplicates-0.1.tar: (error "Can't find
> main file home/elpa/elpa/packages/dired-duplicates/dired-duplicates.el
> file in /home/elpa/elpa/packages/dired-duplicates")
> ######## Build of package archive/dired-duplicates-0.1.tar FAILED!!
> --------------------------------------------------------------------------------
>
> When I look into the archive/dired-duplicates-build-failure.txt file
> the paths mentioned there seem correct.


Sorry for the late response; It appears that the package can now be
built (see https://elpa.gnu.org/packages/dired-duplicates.html), I had
the same issue as you describe on my local machine and now it appears to
be gone.  Perhaps this was resolved by your recent 0.2 commit?

>
>
> Regards,
> Harald

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 2883 bytes --]

Hi,

On 2023-11-10 at 09:37, Philip Kaludercic wrote:

[...]

> Sorry for the late response; It appears that the package can now be
> built (see https://elpa.gnu.org/packages/dired-duplicates.html), I had
> the same issue as you describe on my local machine and now it appears to
> be gone.  Perhaps this was resolved by your recent 0.2 commit?

Thank you. Yes that seems to be so. Everything is fine now.

I am still unable to build it locally using the elpa instructions, but I 
suppose this has something todo with sandboxing and my local configuration. It 
can byte-compile dired-duplicates.el but not create the tarball:

make packages/dired-duplicates
-----------------------------------------------------------------------------
emacs --batch -Q -l admin/elpa-admin.el \
          -f elpaa-batch-pkg-spec-make-dependencies .pkg-descs.mk
Generating description file packages/dired-duplicates/dired-duplicates-pkg.el
Byte compiling packages/dired-duplicates/dired-duplicates.el
emacs --batch -l admin/elpa-admin.el                               \
          -f elpaa-batch-generate-autoloads 
packages/dired-duplicates/dired-duplicates-autoloads.el
   INFO     Scraping files for loaddefs...
   INFO     Scraping files for loaddefs...done
   GEN      dired-duplicates-autoloads.el
-----------------------------------------------------------------------------


make build/dired-duplicates
-----------------------------------------------------------------------------
emacs --batch -l /mnt/scratch/work/elpa/admin/elpa-admin.el     \
          -f elpaa-batch-make-one-package dired-duplicates
======== Building tarball 
archive-devel/dired-duplicates-0.2.0.20231109.135341.tar...
Build error for archive-devel/dired-duplicates-0.2.0.20231109.135341.tar: 
(error "Error-indicating exit code in elpaa--call-sandboxed:
emacs: error while loading shared libraries: libgccjit.so.0: cannot open 
shared object file: No such file or directory
")
######## Build of package 
archive-devel/dired-duplicates-0.2.0.20231109.135341.tar FAILED!!
======== Building tarball archive/dired-duplicates-0.2.tar...
Build error for archive/dired-duplicates-0.2.tar: (error "Error-indicating 
exit code in elpaa--call-sandboxed:
emacs: error while loading shared libraries: libgccjit.so.0: cannot open 
shared object file: No such file or directory
")
######## Build of package archive/dired-duplicates-0.2.tar FAILED!!
-----------------------------------------------------------------------------

The libgccjit.so resides in a directory that is a subdir of /usr which is 
listed in the sandbox ro-binds 
(/usr/lib/gcc/x86_64-pc-linux-gnu/13/libgccjit.so.0), so I wonder what's the 
problem.

Harald


-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic]

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

> Hi,
>
> On 2023-11-10 at 09:37, Philip Kaludercic wrote:
>
> [...]
>
>> Sorry for the late response; It appears that the package can now be
>> built (see https://elpa.gnu.org/packages/dired-duplicates.html), I had
>> the same issue as you describe on my local machine and now it appears to
>> be gone.  Perhaps this was resolved by your recent 0.2 commit?
>
> Thank you. Yes that seems to be so. Everything is fine now.
>
> I am still unable to build it locally using the elpa instructions, but
> I suppose this has something todo with sandboxing and my local
> configuration. It can byte-compile dired-duplicates.el but not create
> the tarball:
>
> make packages/dired-duplicates
> -----------------------------------------------------------------------------
> emacs --batch -Q -l admin/elpa-admin.el \
>          -f elpaa-batch-pkg-spec-make-dependencies .pkg-descs.mk
> Generating description file packages/dired-duplicates/dired-duplicates-pkg.el
> Byte compiling packages/dired-duplicates/dired-duplicates.el
> emacs --batch -l admin/elpa-admin.el                               \
>          -f elpaa-batch-generate-autoloads
>           packages/dired-duplicates/dired-duplicates-autoloads.el
>   INFO     Scraping files for loaddefs...
>   INFO     Scraping files for loaddefs...done
>   GEN      dired-duplicates-autoloads.el
> -----------------------------------------------------------------------------
>
>
> make build/dired-duplicates
> -----------------------------------------------------------------------------
> emacs --batch -l /mnt/scratch/work/elpa/admin/elpa-admin.el     \
>          -f elpaa-batch-make-one-package dired-duplicates
> ======== Building tarball
> archive-devel/dired-duplicates-0.2.0.20231109.135341.tar...
> Build error for
> archive-devel/dired-duplicates-0.2.0.20231109.135341.tar: (error
> "Error-indicating exit code in elpaa--call-sandboxed:
> emacs: error while loading shared libraries: libgccjit.so.0: cannot
> open shared object file: No such file or directory
> ")
> ######## Build of package
>          archive-devel/dired-duplicates-0.2.0.20231109.135341.tar
>         FAILED!!
> ======== Building tarball archive/dired-duplicates-0.2.tar...
> Build error for archive/dired-duplicates-0.2.tar: (error
> "Error-indicating exit code in elpaa--call-sandboxed:
> emacs: error while loading shared libraries: libgccjit.so.0: cannot
> open shared object file: No such file or directory
> ")
> ######## Build of package archive/dired-duplicates-0.2.tar FAILED!!
> -----------------------------------------------------------------------------
>
> The libgccjit.so resides in a directory that is a subdir of /usr which
> is listed in the sandbox ro-binds
> (/usr/lib/gcc/x86_64-pc-linux-gnu/13/libgccjit.so.0), so I wonder
> what's the problem.

I am not sure either... what version of bwrap do you have installed?  If
you manually edit the `elpaa--sandbox' in elpa-admin.el to nil, does
anything change?

> Harald

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 698 bytes --]

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?

Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Stefan Kangas]

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

>>   (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?

See `bug-reference-mode'.

Re: [ELPA] New package: dired-duplicates

[Eli Zaretskii]

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

Re: [ELPA] New package: dired-duplicates

[Visuwesh]

[புதன் நவம்பர் 01, 2023] Eli Zaretskii wrote:

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

From OP,

    The only external requirement is a checksum program like md5 or
    sha256sum that generates a hash value from the contents of a file used
    for comparison, because Emacs cannot do that in a performance-efficient
    way.

So I guess the answer is no.

Re: [ELPA] New package: dired-duplicates

[Eli Zaretskii]

> From: Visuwesh <visuweshm@gmail.com>
> Cc: Philip Kaludercic <philipk@posteo.net>,  h.judt@gmx.at,
>   emacs-devel@gnu.org
> Date: Wed, 01 Nov 2023 19:23:32 +0530
> 
> [புதன் நவம்பர் 01, 2023] Eli Zaretskii wrote:
> 
> >> From: Philip Kaludercic <philipk@posteo.net>
> >> Cc: emacs-devel@gnu.org
> >> Date: Tue, 31 Oct 2023 12:21:51 +0000
> >> 
> > 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.
> 
> >From OP,
> 
>     The only external requirement is a checksum program like md5 or
>     sha256sum that generates a hash value from the contents of a file used
>     for comparison, because Emacs cannot do that in a performance-efficient
>     way.
> 
> So I guess the answer is no.

I'd like to see the numbers which led to the conclusion that
performance was prohibitive.

And even if the performance is indeed much worse, it could be a
fallback in case the program is not available -- which would IMO be
much better than simply failing to provide the functionality in that
case.

Re: [ELPA] New package: dired-duplicates

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Visuwesh <visuweshm@gmail.com>
>> Cc: Philip Kaludercic <philipk@posteo.net>,  h.judt@gmx.at,
>>   emacs-devel@gnu.org
>> Date: Wed, 01 Nov 2023 19:23:32 +0530
>> 
>> [புதன் நவம்பர் 01, 2023] Eli Zaretskii wrote:
>> 
>> >> From: Philip Kaludercic <philipk@posteo.net>
>> >> Cc: emacs-devel@gnu.org
>> >> Date: Tue, 31 Oct 2023 12:21:51 +0000
>> >> 
>> > 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.
>> 
>> >From OP,
>> 
>>     The only external requirement is a checksum program like md5 or
>>     sha256sum that generates a hash value from the contents of a file used
>>     for comparison, because Emacs cannot do that in a performance-efficient
>>     way.
>> 
>> So I guess the answer is no.
>
> I'd like to see the numbers which led to the conclusion that
> performance was prohibitive.
>
> And even if the performance is indeed much worse, it could be a
> fallback in case the program is not available -- which would IMO be
> much better than simply failing to provide the functionality in that
> case.

This is a cheap test on a 1.4GB ISO I had lying around:

--8<---------------cut here---------------start------------->8---
(benchmark-run 1
  (with-temp-buffer
    (insert-file-contents-literally "~/Downloads/haiku-r1beta4-x86_64-anyboot.iso")
    (secure-hash 'sha512 (current-buffer))))
;; (44.389091035 1 1.5836082630000021)

(benchmark-run 1
  (with-temp-buffer
    (call-process "sha512sum" nil t nil (expand-file-name "~/Downloads/haiku-r1beta4-x86_64-anyboot.iso"))
    (goto-char (point-min))
    (and (looking-at (rx bos (+ alnum)))
	 (match-string 0))))
;; (5.155846791 0 0.0)
--8<---------------cut here---------------end--------------->8---

Re: [ELPA] New package: dired-duplicates

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Visuwesh <visuweshm@gmail.com>,  h.judt@gmx.at,  emacs-devel@gnu.org
> Date: Wed, 01 Nov 2023 17:57:40 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'd like to see the numbers which led to the conclusion that
> > performance was prohibitive.
> >
> > And even if the performance is indeed much worse, it could be a
> > fallback in case the program is not available -- which would IMO be
> > much better than simply failing to provide the functionality in that
> > case.
> 
> This is a cheap test on a 1.4GB ISO I had lying around:

Thanks.  But this is just a single large file, not a frequent use case
for this package.

> --8<---------------cut here---------------start------------->8---
> (benchmark-run 1
>   (with-temp-buffer
>     (insert-file-contents-literally "~/Downloads/haiku-r1beta4-x86_64-anyboot.iso")
>     (secure-hash 'sha512 (current-buffer))))
> ;; (44.389091035 1 1.5836082630000021)
> 
> (benchmark-run 1
>   (with-temp-buffer
>     (call-process "sha512sum" nil t nil (expand-file-name "~/Downloads/haiku-r1beta4-x86_64-anyboot.iso"))
>     (goto-char (point-min))
>     (and (looking-at (rx bos (+ alnum)))
> 	 (match-string 0))))
> ;; (5.155846791 0 0.0)
> --8<---------------cut here---------------end--------------->8---

And this is not the package doing its job, this is just a single task
the package does when looking for duplicates.  The numbers when
running the package with call-process replaced by secure-hash will
probably be different.

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 1976 bytes --]

Hi,

On 2023-11-01 20:24, Eli Zaretskii wrote:
>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Visuwesh <visuweshm@gmail.com>,  h.judt@gmx.at,  emacs-devel@gnu.org
>> Date: Wed, 01 Nov 2023 17:57:40 +0000
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>> I'd like to see the numbers which led to the conclusion that
>>> performance was prohibitive.
>>>
>>> And even if the performance is indeed much worse, it could be a
>>> fallback in case the program is not available -- which would IMO be
>>> much better than simply failing to provide the functionality in that
>>> case.
>>
>> This is a cheap test on a 1.4GB ISO I had lying around:
> 
> Thanks.  But this is just a single large file, not a frequent use case
> for this package.
> 
>> --8<---------------cut here---------------start------------->8---
>> (benchmark-run 1
>>    (with-temp-buffer
>>      (insert-file-contents-literally "~/Downloads/haiku-r1beta4-x86_64-anyboot.iso")
>>      (secure-hash 'sha512 (current-buffer))))
>> ;; (44.389091035 1 1.5836082630000021)
>>
>> (benchmark-run 1
>>    (with-temp-buffer
>>      (call-process "sha512sum" nil t nil (expand-file-name "~/Downloads/haiku-r1beta4-x86_64-anyboot.iso"))
>>      (goto-char (point-min))
>>      (and (looking-at (rx bos (+ alnum)))
>> 	 (match-string 0))))
>> ;; (5.155846791 0 0.0)
>> --8<---------------cut here---------------end--------------->8---
> 
> And this is not the package doing its job, this is just a single task
> the package does when looking for duplicates.  The numbers when
> running the package with call-process replaced by secure-hash will
> probably be different.

Thanks for the numbers. What troubles me a bit more than the speed is which 
effects inserting such a large file into a buffer has memory-wise?

Regards,
Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Eli Zaretskii]

> Date: Wed, 1 Nov 2023 21:09:48 +0100
> Cc: visuweshm@gmail.com, emacs-devel@gnu.org
> From: Harald Judt <h.judt@gmx.at>
> 
> Thanks for the numbers. What troubles me a bit more than the speed is which 
> effects inserting such a large file into a buffer has memory-wise?

It is IMO okay to fail when sha256sum is not available and a file is
larger than the available VM, so Emacs runs out of memory, if this is
the situation that worries you.  But these cases should be relatively
rare, and so there's no reason to fail to have this feature in the
much more frequent case that the files are not as large as the
available VM.

If the file can be read by Emacs, even if it's large, then killing the
buffer after computing the hash should not have any adverse effects on
memory usage of that Emacs session.

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 1557 bytes --]

Hi Eli,

Am 02.11.23 um 06:55 schrieb Eli Zaretskii:
>> Date: Wed, 1 Nov 2023 21:09:48 +0100
>> Cc: visuweshm@gmail.com, emacs-devel@gnu.org
>> From: Harald Judt <h.judt@gmx.at>
>>
>> Thanks for the numbers. What troubles me a bit more than the speed is which
>> effects inserting such a large file into a buffer has memory-wise?
> 
> It is IMO okay to fail when sha256sum is not available and a file is
> larger than the available VM, so Emacs runs out of memory, if this is
> the situation that worries you.  But these cases should be relatively
> rare, and so there's no reason to fail to have this feature in the
> much more frequent case that the files are not as large as the
> available VM.
> 
> If the file can be read by Emacs, even if it's large, then killing the
> buffer after computing the hash should not have any adverse effects on
> memory usage of that Emacs session.

I have started to implement the fallback to internal functions, here are my 
results - it does even have size-limiting to avoid getting Emacs killed, which 
I managed to do trying with a big 4 GiB ISO file:

https://codeberg.org/hjudt/dired-duplicates/compare/main...fallback-to-internal-checksumming

Eli, is that how you imagined it? I would be glad if someone could give it a 
quick review.

BTW: My project does not build in ELPA (I have sent a mail about this earlier).

Best regards,
Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Eli Zaretskii]

> Date: Wed, 8 Nov 2023 21:29:46 +0100
> Cc: philipk@posteo.net, visuweshm@gmail.com, emacs-devel@gnu.org
> From: Harald Judt <h.judt@gmx.at>
> 
> > It is IMO okay to fail when sha256sum is not available and a file is
> > larger than the available VM, so Emacs runs out of memory, if this is
> > the situation that worries you.  But these cases should be relatively
> > rare, and so there's no reason to fail to have this feature in the
> > much more frequent case that the files are not as large as the
> > available VM.
> > 
> > If the file can be read by Emacs, even if it's large, then killing the
> > buffer after computing the hash should not have any adverse effects on
> > memory usage of that Emacs session.
> 
> I have started to implement the fallback to internal functions, here are my 
> results - it does even have size-limiting to avoid getting Emacs killed, which 
> I managed to do trying with a big 4 GiB ISO file:
> 
> https://codeberg.org/hjudt/dired-duplicates/compare/main...fallback-to-internal-checksumming
> 
> Eli, is that how you imagined it? I would be glad if someone could give it a 
> quick review.

Yes, that was the idea I had, thanks.

The size limitation should have its default value dependent on whether
the build is a 32-bit (which we still support) or 64-bit.  You can
look at how we compute treesit-max-buffer-size, to figure out how to
express the conditions for the default value.

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 2063 bytes --]

Am 09.11.23 um 06:52 schrieb Eli Zaretskii:
>> Date: Wed, 8 Nov 2023 21:29:46 +0100
>> Cc: philipk@posteo.net, visuweshm@gmail.com, emacs-devel@gnu.org
>> From: Harald Judt <h.judt@gmx.at>
>>
>>> It is IMO okay to fail when sha256sum is not available and a file is
>>> larger than the available VM, so Emacs runs out of memory, if this is
>>> the situation that worries you.  But these cases should be relatively
>>> rare, and so there's no reason to fail to have this feature in the
>>> much more frequent case that the files are not as large as the
>>> available VM.
>>>
>>> If the file can be read by Emacs, even if it's large, then killing the
>>> buffer after computing the hash should not have any adverse effects on
>>> memory usage of that Emacs session.
>>
>> I have started to implement the fallback to internal functions, here are my
>> results - it does even have size-limiting to avoid getting Emacs killed, which
>> I managed to do trying with a big 4 GiB ISO file:
>>
>> https://codeberg.org/hjudt/dired-duplicates/compare/main...fallback-to-internal-checksumming
>>
>> Eli, is that how you imagined it? I would be glad if someone could give it a
>> quick review.
> 
> Yes, that was the idea I had, thanks.
> 
> The size limitation should have its default value dependent on whether
> the build is a 32-bit (which we still support) or 64-bit.  You can
> look at how we compute treesit-max-buffer-size, to figure out how to
> express the conditions for the default value.

Yes, but I wonder, why do this? There can be 32-bit systems as well as 64-bit 
systems that can have only 2GiB RAM, both might fail when trying to open a 
file that has e.g. 1536MiB. Then, there might be both types of systems that 
have 8gb of RAM that can open such files with no problems?

Maybe it would be possible to make it dependent on the amount of RAM available 
on the system?

Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[tomas]

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

On Thu, Nov 09, 2023 at 09:00:11AM +0100, Harald Judt wrote:

[...]

> Yes, but I wonder, why do this? There can be 32-bit systems as well as
> 64-bit systems that can have only 2GiB RAM, both might fail when trying to
> open a file that has e.g. 1536MiB. Then, there might be both types of
> systems that have 8gb of RAM that can open such files with no problems?

Eli's remark was about Emacs's integer size (which is used to address
spots in buffers). On 32 bit *builds* it isn't wide enough for big
buffers. On 64 bit builds it is.

It's about Emacs's build, not the OS around it. Think "address space",
just one or two onion layers further.

Cheers
-- 
t

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [ELPA] New package: dired-duplicates

[Eli Zaretskii]

> Date: Thu, 9 Nov 2023 09:38:50 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> From:  <tomas@tuxteam.de>
> 
> On Thu, Nov 09, 2023 at 09:00:11AM +0100, Harald Judt wrote:
> 
> [...]
> 
> > Yes, but I wonder, why do this? There can be 32-bit systems as well as
> > 64-bit systems that can have only 2GiB RAM, both might fail when trying to
> > open a file that has e.g. 1536MiB. Then, there might be both types of
> > systems that have 8gb of RAM that can open such files with no problems?
> 
> Eli's remark was about Emacs's integer size (which is used to address
> spots in buffers). On 32 bit *builds* it isn't wide enough for big
> buffers. On 64 bit builds it is.
> 
> It's about Emacs's build, not the OS around it. Think "address space",
> just one or two onion layers further.

Actually, it's about both.

Re: [ELPA] New package: dired-duplicates

[Eli Zaretskii]

> Date: Thu, 9 Nov 2023 09:00:11 +0100
> Cc: emacs-devel@gnu.org
> From: Harald Judt <h.judt@gmx.at>
> 
> > The size limitation should have its default value dependent on whether
> > the build is a 32-bit (which we still support) or 64-bit.  You can
> > look at how we compute treesit-max-buffer-size, to figure out how to
> > express the conditions for the default value.
> 
> Yes, but I wonder, why do this? There can be 32-bit systems as well as 64-bit 
> systems that can have only 2GiB RAM, both might fail when trying to open a 
> file that has e.g. 1536MiB. Then, there might be both types of systems that 
> have 8gb of RAM that can open such files with no problems?

If you are saying that we can do with a single value, I'm okay with
that, provided that this value will be accepted by users.  32-bit
systems cannot have buffers larger than 2 GiB, and a reasonable limit
would be something like 500 MiB, I think.  This could be too low for
users of 64-bit systems, but if it's okay, it's fine with me.  Your
proposed default, 1 GiB, is too large for a typical 32-bit system,
IMO.

> Maybe it would be possible to make it dependent on the amount of RAM available 
> on the system?

Ideally, yes, but in practice knowing how much is available is not
that easy on a modern OS, so I don't think it's worth the hassle,
especially in fallback code.

Re: [ELPA] New package: dired-duplicates

[tomas]

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

On Thu, Nov 09, 2023 at 10:43:32AM +0200, Eli Zaretskii wrote:
> > Date: Thu, 9 Nov 2023 09:00:11 +0100
> > Cc: emacs-devel@gnu.org
> > From: Harald Judt <h.judt@gmx.at>

[...]

> > Maybe it would be possible to make it dependent on the amount of RAM available 
> > on the system?
> 
> Ideally, yes, but in practice knowing how much is available is not
> that easy on a modern OS, so I don't think it's worth the hassle,
> especially in fallback code.

Don't even try :-)

No, seriously. There's swap. Depending on tech, using it might be
viable (NVME) or not that much (spinning rust). The sysadmin might
plug in another 2TB of RAM on request [1] (or the cloud orchestration
system might allot to you another 16G chunk).

Modern OSes usually overcommit (if you say "malloc" they say "there
you go" and fault-in page by page whenever you access them first).

Cheers

[1] The Linux kernel can pull this kind of tricks; your hardware
   might, if you have paid the price for that
   https://www.kernel.org/doc/html/v5.0/admin-guide/mm/memory-hotplug.html
-- 
t

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 1796 bytes --]

On 2023-11-09 at 09:43, Eli Zaretskii wrote:>> Date: Thu, 9 Nov 2023 09:00:11 
+0100
>> Cc: emacs-devel@gnu.org
>> From: Harald Judt <h.judt@gmx.at>
>>
>>> The size limitation should have its default value dependent on whether
>>> the build is a 32-bit (which we still support) or 64-bit.  You can
>>> look at how we compute treesit-max-buffer-size, to figure out how to
>>> express the conditions for the default value.
>>
>> Yes, but I wonder, why do this? There can be 32-bit systems as well as 64-bit
>> systems that can have only 2GiB RAM, both might fail when trying to open a
>> file that has e.g. 1536MiB. Then, there might be both types of systems that
>> have 8gb of RAM that can open such files with no problems?
> 
> If you are saying that we can do with a single value, I'm okay with
> that, provided that this value will be accepted by users.  32-bit
> systems cannot have buffers larger than 2 GiB, and a reasonable limit
> would be something like 500 MiB, I think.  This could be too low for
> users of 64-bit systems, but if it's okay, it's fine with me.  Your
> proposed default, 1 GiB, is too large for a typical 32-bit system,
> IMO.
> 
>> Maybe it would be possible to make it dependent on the amount of RAM available
>> on the system?
> 
> Ideally, yes, but in practice knowing how much is available is not
> that easy on a modern OS, so I don't think it's worth the hassle,
> especially in fallback code.

I agree, 500M for 32-bit and 1G for 64-bit are good defaults for the 
dired-duplicates use-case, I guess most typical systems will be able to handle 
it. I will implement it this way.

Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [ELPA] New package: dired-duplicates

[Harald Judt]

[-- Attachment #1.1: Type: text/plain, Size: 3379 bytes --]

On 2023-11-09 at 10:18, Harald Judt wrote:
> On 2023-11-09 at 09:43, Eli Zaretskii wrote:>> Date: Thu, 9 Nov 2023 09:00:11 
> +0100
>>> Cc: emacs-devel@gnu.org
>>> From: Harald Judt <h.judt@gmx.at>
>>>
>>>> The size limitation should have its default value dependent on whether
>>>> the build is a 32-bit (which we still support) or 64-bit.  You can
>>>> look at how we compute treesit-max-buffer-size, to figure out how to
>>>> express the conditions for the default value.
>>>
>>> Yes, but I wonder, why do this? There can be 32-bit systems as well as 64-bit
>>> systems that can have only 2GiB RAM, both might fail when trying to open a
>>> file that has e.g. 1536MiB. Then, there might be both types of systems that
>>> have 8gb of RAM that can open such files with no problems?
>>
>> If you are saying that we can do with a single value, I'm okay with
>> that, provided that this value will be accepted by users.  32-bit
>> systems cannot have buffers larger than 2 GiB, and a reasonable limit
>> would be something like 500 MiB, I think.  This could be too low for
>> users of 64-bit systems, but if it's okay, it's fine with me.  Your
>> proposed default, 1 GiB, is too large for a typical 32-bit system,
>> IMO.
>>
>>> Maybe it would be possible to make it dependent on the amount of RAM available
>>> on the system?
>>
>> Ideally, yes, but in practice knowing how much is available is not
>> that easy on a modern OS, so I don't think it's worth the hassle,
>> especially in fallback code.
> 
> I agree, 500M for 32-bit and 1G for 64-bit are good defaults for the 
> dired-duplicates use-case, I guess most typical systems will be able to handle 
> it. I will implement it this way.
> 
> Harald

Ok, I have pushed these changes and bumped the version to 0.2 and I hope that 
this time the package builds correctly on ELPA (the last time it failed though 
it loads and compiles on my machine). Where would I find that file 
dired-duplicates-build-failure.txt in the GNU ELPA archive web site?

The output from the previous run was:
-------------------------------------------------------------------------------
The build scripts failed to build the tarball
for version 0.1 of the package dired-duplicates.
You can consult the latest error output in the file
"dired-duplicates-build-failure.txt" in the GNU ELPA archive web site.

You can also try and reproduce the error locally as follows:

     git clone --single-branch https://git.sv.gnu.org/git/emacs/elpa.git
     cd elpa
     make                            # Setup the infrastructure
     make packages/dired-duplicates  # Create a worktree of the package
     make build/dired-duplicates     # Build the tarballs into archive(-devel)/

## The current error output was the following:

======== Building tarball archive/dired-duplicates-0.1.tar...
Build error for archive/dired-duplicates-0.1.tar: (error "Can't find main file 
home/elpa/elpa/packages/dired-duplicates/dired-duplicates.el file in 
/home/elpa/elpa/packages/dired-duplicates")
######## Build of package archive/dired-duplicates-0.1.tar FAILED!!
---------------------------------------------------------------------------------


Harald

-- 
`Experience is the best teacher.'

PGP Key ID: 4FFFAB21B8580ABD
Fingerprint: E073 6DD8 FF40 9CF2 0665 11D4 4FFF AB21 B858 0ABD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]