From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.devel Subject: Re: [ELPA] New package: dired-duplicates Date: Wed, 01 Nov 2023 11:32:32 +0000 Message-ID: <87zfzx1zfz.fsf@posteo.net> References: <875y2nugm8.fsf@posteo.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="24646"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Harald Judt Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Wed Nov 01 12:33:17 2023 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qy9TI-0006AA-O7 for ged-emacs-devel@m.gmane-mx.org; Wed, 01 Nov 2023 12:33:16 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qy9Si-0001NT-LZ; Wed, 01 Nov 2023 07:32:40 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qy9Sg-0001Ho-QX for emacs-devel@gnu.org; Wed, 01 Nov 2023 07:32:38 -0400 Original-Received: from mout02.posteo.de ([185.67.36.66]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qy9Se-00005y-3K for emacs-devel@gnu.org; Wed, 01 Nov 2023 07:32:38 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id ACAA7240103 for ; Wed, 1 Nov 2023 12:32:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1698838353; bh=eqfZS3awiPe3pdSRE/FCscd1PsGMJcFXeSWvCaQf8Pk=; h=From:To:Cc:Subject:Autocrypt:Date:Message-ID:MIME-Version: Content-Transfer-Encoding:From; b=dD5nTDS8wqTfLhB7qdf0xFf+U8UgV3jDqIKPZAa0MHTNMmPVqkx9YOG1R1SQm+tTv QHl9WJvjOGw+QULoMXJrS3qG18Gy5Hu7m0JBghn5OJB8G4/fbBuVWXSrsQHELgCW5H ywqQ9jhIQQkgjk2MGSrhFhoLzS5AWF8ailTP/K7TVsD7E9k3Jv53A3OeeHiZ+B278K UUdET9B/qR/XTKsOKD1Jte3xG4bMNBlAFUn6RygY8dnAwGv3Es0gKk/Y5cQplXEZFL AV3EJP75FRDgUjpZdqlI3rCndsgcAITKpx3lm4ToGvvE4U0ekLkvwQek+wyGMv8UlW QKyVGLkBsWgbQ== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4SL4cK1FpNz9rxK; Wed, 1 Nov 2023 12:32:33 +0100 (CET) In-Reply-To: (Harald Judt's message of "Tue, 31 Oct 2023 22:05:12 +0100") Autocrypt: addr=philipk@posteo.net; keydata= mDMEZBBQQhYJKwYBBAHaRw8BAQdAHJuofBrfqFh12uQu0Yi7mrl525F28eTmwUDflFNmdui0QlBo aWxpcCBLYWx1ZGVyY2ljIChnZW5lcmF0ZWQgYnkgYXV0b2NyeXB0LmVsKSA8cGhpbGlwa0Bwb3N0 ZW8ubmV0PoiWBBMWCAA+FiEEDg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwMFCQHhM4AFCwkI BwIGFQoJCAsCBBYCAwECHgECF4AACgkQ8xYDWXahwulikAEA77hloUiSrXgFkUVJhlKBpLCHUjA0 mWZ9j9w5d08+jVwBAK6c4iGP7j+/PhbkxaEKa4V3MzIl7zJkcNNjHCXmvFcEuDgEZBBQQhIKKwYB BAGXVQEFAQEHQI5NLiLRjZy3OfSt1dhCmFyn+fN/QKELUYQetiaoe+MMAwEIB4h+BBgWCAAmFiEE Dg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwwFCQHhM4AACgkQ8xYDWXahwukm+wEA8cml4JpK NeAu65rg+auKrPOP6TP/4YWRCTIvuYDm0joBALw98AMz7/qMHvSCeU/hw9PL6u6R2EScxtpKnWof z4oM Received-SPF: pass client-ip=185.67.36.66; envelope-from=philipk@posteo.net; helo=mout02.posteo.de X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H5=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:312050 Archived-At: You forgot to CC me in the response, I would have almost missed your respon= se. Harald Judt 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, ot= herwise nil." >> The executable used is defined by >> `dired-duplicates-checksum-exec'." >> (let* ((default-directory (file-name-directory (expand-file-name fil= e))) >> - (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 =3D (dired-duplicates-checks= um-file f) >> do (setf (gethash checksum checksum-table) >> (append (gethash checksum checks= um-table) (list f))))) >> (cl-loop for same-files being the hash-value in checksum-ta= ble 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-attribu= tes (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 re= sults group." >> for len in lengths >> do >> (forward-line len) >> - ;; (forward-line len) >> (let ((inhibit-read-only t)) >> (beginning-of-line) >> (unless (=3D (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 c= alls >> (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 directo= ries))) >> + (let* ((directories (if (listp directories) directories (list directo= ries))) ;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 writes: > Harald Judt 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-lin= es." >> > - :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 =E2=80=98defcustom=E2=80=99 does not specify any =E2=80=98:grou= p=E2=80=99, the last group > defined with =E2=80=98defgroup=E2=80=99 in the same file will be use= d. This way, > most =E2=80=98defcustom=E2=80=99 do not need an explicit =E2=80=98:g= roup=E2=80=99. > > 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 writes: > On 2023-10-31 13:21, Philip Kaludercic wrote: > > [...] > >> @@ -225,7 +220,7 @@ This is the same as `dired-do-flagged-delete', but c= alls >> (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 writes: >> From: Philip Kaludercic >> Cc: emacs-devel@gnu.org >> Date: Tue, 31 Oct 2023 12:21:51 +0000 >>=20 >> 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?