bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[sbaugh]

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

Tags: patch


Previously we just assumed that the car of an element of
custom-current-group-alist was a filename.  But actually it can be nil
if a custom group was defined by just evaling Lisp.

* lisp/emacs-lisp/package.el (package-report-bug): Don't fail when a
custom group was defined by eval.

In GNU Emacs 29.0.92 (build 13, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2023-07-04 built on earth
Repository revision: 5ef6e74202e2bdae5999913bff527a4a16d9176e
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: NixOS 23.05 (Stoat)

Configured using:
 'configure --cache-file=config.cache --with-x-toolkit=lucid
 --with-tree-sitter --with-xinput2 CC=gcc PKG_CONFIG=pkg-config
 PKG_CONFIG_PATH=/nix/store/s3r15m8wbl4wqk4khqlf41ikryhjm1bi-file-5.44-dev/lib/pkgconfig:/nix/store/f9jbn419h46c78z1pi49yn9a8742b0ql-gnutls-3.8.0-dev/lib/pkgconfig:/nix/store/knq0pv08wm4dins7m4jh0n7cv7pjvdjr-nettle-3.9.1-dev/lib/pkgconfig:/nix/store/dy8p07vrrhdgpnl45xz9c0k0didbikdh-gmp-with-cxx-6.2.1-dev/lib/pkgconfig:/nix/store/6hkdabzyqhyq5ypq4c9b2cibr1d1zg1s-harfbuzz-7.3.0-dev/lib/pkgconfig:/nix/store/hyns944pqgblw4icskylvlpm5krmfvcr-graphite2-1.3.14-dev/lib/pkgconfig:/nix/store/08cdp9vgvy023ysfa2y01gzsm2jv6phx-jansson-2.14/lib/pkgconfig:/nix/store/nqlbk40lh7igs61l77dwgdkn8dc2akcm-libxml2-2.10.4-dev/lib/pkgconfig:/nix/store/b3axl73v3yvqqx7g47njqb5wzxvm280p-zlib-1.2.13-dev/lib/pkgconfig:/nix/store/3f2rc4inlcxmq11718qmz94v2rpybw70-ncurses-6.4-dev/lib/pkgconfig:/nix/store/bxy745kyb1fwhpfkiaaz3wgvpkpvwcpq-dbus-1.14.8-dev/lib/pkgconfig:/nix/store/9714v7c4cgpm4yqcyqk6n9xw9iq3a1bs-expat-2.5.0-dev/lib/pkgconfig:/nix/store/zzi7pcadidqh798yddxv6pwdbwpkikma-libselinux-3.3-dev/lib/pkgconfig:/nix/store/w14j7y5nl14vy4ikcivss35jmrqq3fxj-libotf-0.9.16-dev/lib/pkgconfig:/nix/store/arhk7hsch4scyv6m24fw03yq6wq5wbbx-m17n-lib-1.8.2/lib/pkgconfig:/nix/store/1jbbrny8xcjb68lb5m30cvxycfkyhvsv-sqlite-3.42.0-dev/lib/pkgconfig:/nix/store/5vx779yqkxaysv48gicwlgv0ippbrhc4-systemd-253.5-dev/lib/pkgconfig:/nix/store/5vx779yqkxaysv48gicwlgv0ippbrhc4-systemd-253.5-dev/share/pkgconfig:/nix/store/djifahvk3qp06ssqxv6gy1ixdnnypr9s-tree-sitter-0.20.8/lib/pkgconfig:/nix/store/74aasy1d2r5y27zn68cs1rxwy1llzn05-libwebp-1.3.0/lib/pkgconfig:/nix/store/8sk7bp89iwb4gw96fq6xakb6lcy2x52n-Xaw3d-1.6.3/lib/pkgconfig:/nix/store/ppvb3ha8148am3ajnzxnm6i3ri38c01n-libXmu-1.1.3-dev/lib/pkgconfig:/nix/store/jyxf8cjbj3nzh00x48nfram79i63chdi-libX11-1.8.6-dev/lib/pkgconfig:/nix/store/zk9v0nr5zdfi1ybkhcfifmxsng7hfl23-xorgproto-2021.5/share/pkgconfig:/nix/store/3q1k18v8aa6mxs538bha4ry0mp3m321l-libxcb-1.14-dev/lib/pkgconfig:/nix/store/hcscz68zvfk1skyb25wrnha959f6hhrc-libXt-1.2.1-dev/lib/pkgconfig:/nix/store/kl55wj6qc3v481jsgvzm5w2csnhm84zf-libSM-1.2.3-dev/lib/pkgconfig:/nix/store/s3f67kvsn55rxp2rc98xv0hkq364yci1-libICE-1.0.10-dev/lib/pkgconfig:/nix/store/rsw4ri8025jgln8vpsrmg82bzgbcw3zr-cairo-1.16.0-dev/lib/pkgconfig:/nix/store/jir0rqbcy0d9qr9kf5cwf2yphql4ykyw-fontconfig-2.14.2-dev/lib/pkgconfig:/nix/store/n2g3xblaz1k4civv1z6hhm1nsmp3m17p-freetype-2.13.0-dev/lib/pkgconfig:/nix/store/isbmyzm2shmp0wsjr4cy45v2i58h2zvw-bzip2-1.0.8-dev/lib/pkgconfig:/nix/store/bl2qwy78jr2sqm260imgxmd5dzhjqvag-brotli-1.0.9-dev/lib/pkgconfig:/nix/store/z96jh9ag5b3565lwwb5chjb9bfp5i2qv-libpng-apng-1.6.39-dev/lib/pkgconfig:/nix/store/jjd4z18grhky6lh8n463v648nnf5628b-pixman-0.42.2/lib/pkgconfig:/nix/store/qd14wrazwcspjv3q65vgh35pl7b8nifq-libXext-1.3.4-dev/lib/pkgconfig:/nix/store/gj8i21xx87ip9b971j2d1m0rmrzyhbir-libXau-1.0.9-dev/lib/pkgconfig:/nix/store/4gpinwwdqhi927xkrfpr1hvdd56baxgk-libXrender-0.9.10-dev/lib/pkgconfig:/nix/store/d1jbygs6hcn6dysk706i9zf07yd18wmr-xcb-util-0.4.1-dev/lib/pkgconfig:/nix/store/hdc4ika0mb1cv0cf6dchwxbr004rc50i-glib-2.76.3-dev/lib/pkgconfig:/nix/store/wxyh848a6xcqy2v8727vcwspri53pqwi-libffi-3.4.4-dev/lib/pkgconfig:/nix/store/42jx72681qzliic0xsjhvx24cil2gapk-libGL-1.6.0-dev/lib/pkgconfig:/nix/store/b9lmdkxpvgkj6zc956fvhshzisqpi767-libglvnd-1.6.0-dev/lib/pkgconfig:/nix/store/gff29sbhg1gcw969mpm5rb693kj5v18w-libXaw-1.0.14-dev/lib/pkgconfig:/nix/store/776xijk8rsb1b4c0dsxwq0k82bvm7mm9-libXpm-3.5.15-dev/lib/pkgconfig:/nix/store/qizdmm43xi65mdngal8bpbpqcdc8290d-libjpeg-turbo-2.1.5.1-dev/lib/pkgconfig:/nix/store/db7ix62fx4nvr9j1fjdvnznl2npff4pr-librsvg-2.55.1-dev/lib/pkgconfig:/nix/store/q0hg0951w1dv9y40m9ggln8phwil6lxc-gdk-pixbuf-2.42.10-dev/lib/pkgconfig:/nix/store/34rr5nvgljsc4bi3mxjxg8abmjr1f7hn-libtiff-4.5.0-dev/lib/pkgconfig:/nix/store/zwkr4kjcjs213pw9mhzi46bzlw6qwxzq-libdeflate-1.18/lib/pkgconfig:/nix/store/6na552yzwml88j8g5vqf5h9ir3vw8myi-xz-5.4.3-dev/lib/pkgconfig'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-report-bug-don-t-fail-on-custom-groups-defin.patch --]
[-- Type: text/patch, Size: 1175 bytes --]

From 663f36f52c7c8e66e54d4e93ce037428bf1b3dd0 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Sun, 9 Jul 2023 12:59:50 -0400
Subject: [PATCH] package-report-bug: don't fail on custom groups defined by
 eval

Previously we just assumed that the car of an element of
custom-current-group-alist was a filename.  But actually it can be nil
if a custom group was defined by just evaling Lisp.

* lisp/emacs-lisp/package.el (package-report-bug): Don't fail when a
custom group was defined by eval.
---
 lisp/emacs-lisp/package.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 3e6acd9b388..f67e99e04b5 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -4642,6 +4642,7 @@ package-report-bug
                    (boundp (car ent))
                    (not (eq (custom--standard-value (car ent))
                             (default-toplevel-value (car ent))))
+                   (car group)
                    (file-in-directory-p (car group) (package-desc-dir desc)))
           (push (car ent) vars))))
     (dlet ((reporter-prompt-for-summary-p t))
-- 
2.41.0

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Philip Kaludercic]

sbaugh@catern.com writes:

Thanks,

> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sun, 9 Jul 2023 12:59:50 -0400
> Subject: [PATCH] package-report-bug: don't fail on custom groups defined by
>  eval
>
> Previously we just assumed that the car of an element of
> custom-current-group-alist was a filename.  But actually it can be nil
> if a custom group was defined by just evaling Lisp.

Where is this behaviour documented?  I couldn't reproduce it with a
simple experiment.

> * lisp/emacs-lisp/package.el (package-report-bug): Don't fail when a
> custom group was defined by eval.
> ---
>  lisp/emacs-lisp/package.el | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 3e6acd9b388..f67e99e04b5 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -4642,6 +4642,7 @@ package-report-bug
>                     (boundp (car ent))
>                     (not (eq (custom--standard-value (car ent))
>                              (default-toplevel-value (car ent))))
> +                   (car group)

If you are checking for (car group), when do this in the loop instead
of wrapping the entire `dolist'?

>                     (file-in-directory-p (car group) (package-desc-dir desc)))
>            (push (car ent) vars))))
>      (dlet ((reporter-prompt-for-summary-p t))

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[sbaugh]

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

Philip Kaludercic <philipk@posteo.net> writes:

> sbaugh@catern.com writes:
>
> Thanks,
>
>> From: Spencer Baugh <sbaugh@catern.com>
>> Date: Sun, 9 Jul 2023 12:59:50 -0400
>> Subject: [PATCH] package-report-bug: don't fail on custom groups defined by
>>  eval
>>
>> Previously we just assumed that the car of an element of
>> custom-current-group-alist was a filename.  But actually it can be nil
>> if a custom group was defined by just evaling Lisp.
>
> Where is this behaviour documented?  I couldn't reproduce it with a
> simple experiment.

To reproduce:
M-: (defgroup mygroup nil "my group") RET

I don't think it's documented anywhere?  custom-current-group-alist is
not documented so this wouldn't be either.

>> * lisp/emacs-lisp/package.el (package-report-bug): Don't fail when a
>> custom group was defined by eval.
>> ---
>>  lisp/emacs-lisp/package.el | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
>> index 3e6acd9b388..f67e99e04b5 100644
>> --- a/lisp/emacs-lisp/package.el
>> +++ b/lisp/emacs-lisp/package.el
>> @@ -4642,6 +4642,7 @@ package-report-bug
>>                     (boundp (car ent))
>>                     (not (eq (custom--standard-value (car ent))
>>                              (default-toplevel-value (car ent))))
>> +                   (car group)
>
> If you are checking for (car group), when do this in the loop instead
> of wrapping the entire `dolist'?

Good point.  I was just copying the last condition in the and.  Lifted
them both out of the loop:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Support-transforming-the-dirname-used-by-uniquify.patch --]
[-- Type: text/x-patch, Size: 5803 bytes --]

From 2873d4c482acfd1ced749d593fd512c408e0a578 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Sun, 9 Jul 2023 22:21:03 -0400
Subject: [PATCH] Support transforming the dirname used by uniquify

By transforming the dirname, we can add additional information to use
during uniquifying.  A basic one: uniquifying buffer names based on
the project name.

* lisp/progmodes/project.el (project-uniquify-dirname-transform): Add.
* lisp/uniquify.el (uniquify-dirname-transform-default)
(uniquify-dirname-transform): Add. (bug#62621)
(uniquify-rationalize-file-buffer-names, uniquify-buffer-file-name):
Use uniquify-dirname-transform.
* test/lisp/uniquify-tests.el (uniquify-home,
uniquify-project-transform): Add tests.
---
 lisp/progmodes/project.el   | 12 ++++++++++++
 lisp/uniquify.el            | 26 +++++++++++++++++++++-----
 test/lisp/uniquify-tests.el | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index d482cc24d70..78f9fb410c1 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1835,5 +1835,17 @@ project-switch-project
     (let ((project-current-directory-override dir))
       (call-interactively command))))
 
+;;;###autoload
+(defun project-uniquify-dirname-transform (dirname)
+  "Include `project-name' in DIRNAME if in a project."
+  (if-let (proj (project-current nil dirname))
+      (let ((root (project-root proj)))
+        (expand-file-name
+         (file-name-concat
+          (file-name-directory root)
+          (project-name proj)
+          (file-relative-name dirname root))))
+    dirname))
+
 (provide 'project)
 ;;; project.el ends here
diff --git a/lisp/uniquify.el b/lisp/uniquify.el
index d1ca455b673..b2f28a10d51 100644
--- a/lisp/uniquify.el
+++ b/lisp/uniquify.el
@@ -168,6 +168,20 @@ uniquify-list-buffers-directory-modes
 That means that when `buffer-file-name' is set to nil, `list-buffers-directory'
 contains the name of the directory which the buffer is visiting.")
 
+(defun uniquify-dirname-transform-default (dirname)
+  "Simply return DIRNAME unchanged."
+  dirname)
+
+(defcustom uniquify-dirname-transform #'uniquify-dirname-transform-default
+  "A function to transform the dirname used to uniquify a buffer.
+
+It takes a single argument: the directory of the buffer.  It
+should return a string filename (which does not need to actually
+exist in the filesystem) to use for uniquifying the buffer name."
+  :type '(choice (function-item uniquify-dirname-transform-default)
+                 function)
+  :group 'uniquify)
+
 ;;; Utilities
 
 ;; uniquify-fix-list data structure
@@ -209,7 +223,8 @@ uniquify-rationalize-file-buffer-names
   ;; this buffer.
   (with-current-buffer newbuf (setq uniquify-managed nil))
   (when dirname
-    (setq dirname (expand-file-name (directory-file-name dirname)))
+    (setq dirname (funcall uniquify-dirname-transform
+                           (expand-file-name (directory-file-name dirname))))
     (let ((fix-list (list (uniquify-make-item base dirname newbuf
                                               nil)))
 	  items)
@@ -268,10 +283,11 @@ uniquify-buffer-file-name
 	       (if (memq major-mode uniquify-list-buffers-directory-modes)
 		   list-buffers-directory))))
       (when filename
-	(directory-file-name
-	 (file-name-directory
-	  (expand-file-name
-	   (directory-file-name filename))))))))
+	 (funcall uniquify-dirname-transform
+	          (directory-file-name
+	          (file-name-directory
+	           (expand-file-name
+	            (directory-file-name filename)))))))))
 
 (defun uniquify-rerationalize-w/o-cb (fix-list)
   "Re-rationalize the buffers in FIX-LIST, but ignoring `current-buffer'."
diff --git a/test/lisp/uniquify-tests.el b/test/lisp/uniquify-tests.el
index abd61fa3504..e533c4b644c 100644
--- a/test/lisp/uniquify-tests.el
+++ b/test/lisp/uniquify-tests.el
@@ -88,6 +88,21 @@ uniquify-dirs
                        '("a/dir/" "b/dir/")))
         (mapc #'kill-buffer bufs)))))
 
+(ert-deftest uniquify-home ()
+  "uniquify works, albeit confusingly, in the presence of directories named \"~\""
+  (let (bufs)
+    (save-excursion
+      (push (find-file-noselect "~") bufs)
+      (push (find-file-noselect "./~") bufs)
+      (should (equal (mapcar #'buffer-name bufs)
+                     '("~<test>" "~<>")))
+      (push (find-file-noselect "~/foo") bufs)
+      (push (find-file-noselect "./~/foo") bufs)
+      (should (equal (mapcar #'buffer-name bufs)
+                     '("foo<~>" "foo</nonexistent>" "~<test>" "~<>")))
+      (while bufs
+        (kill-buffer (pop bufs))))))
+
 (ert-deftest uniquify-rename-to-dir ()
   "Giving a buffer a name which matches a directory doesn't rename the buffer"
   (let ((uniquify-buffer-name-style 'forward)
@@ -125,5 +140,23 @@ uniquify-space-prefix
     (should (equal (buffer-name) "| foo"))
     (kill-buffer)))
 
+(require 'project)
+(ert-deftest uniquify-project-transform ()
+  "`project-uniquify-dirname-transform' works"
+  (let ((uniquify-dirname-transform #'project-uniquify-dirname-transform)
+        (project-vc-name "foo1/bar")
+        bufs)
+    (save-excursion
+      (should (file-exists-p "../README"))
+      (push (find-file-noselect "../README") bufs)
+      (push (find-file-noselect "other/README") bufs)
+      (should (equal (mapcar #'buffer-name bufs)
+                     '("README<other>" "README<bar>")))
+      (push (find-file-noselect "foo2/bar/README") bufs)
+      (should (equal (mapcar #'buffer-name bufs)
+                     '("README<foo2/bar>" "README<other>" "README<foo1/bar>")))
+      (while bufs
+        (kill-buffer (pop bufs))))))
+
 (provide 'uniquify-tests)
 ;;; uniquify-tests.el ends here
-- 
2.41.0

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Philip Kaludercic]

sbaugh@catern.com writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> sbaugh@catern.com writes:
>>
>> Thanks,
>>
>>> From: Spencer Baugh <sbaugh@catern.com>
>>> Date: Sun, 9 Jul 2023 12:59:50 -0400
>>> Subject: [PATCH] package-report-bug: don't fail on custom groups defined by
>>>  eval
>>>
>>> Previously we just assumed that the car of an element of
>>> custom-current-group-alist was a filename.  But actually it can be nil
>>> if a custom group was defined by just evaling Lisp.
>>
>> Where is this behaviour documented?  I couldn't reproduce it with a
>> simple experiment.
>
> To reproduce:
> M-: (defgroup mygroup nil "my group") RET

OK, I misunderstood your point, I assumed we were talking about
`defcustom' declarations that were evaluated outside the context of a
file.

> I don't think it's documented anywhere?  custom-current-group-alist is
> not documented so this wouldn't be either.
>
>>> * lisp/emacs-lisp/package.el (package-report-bug): Don't fail when a
>>> custom group was defined by eval.
>>> ---
>>>  lisp/emacs-lisp/package.el | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
>>> index 3e6acd9b388..f67e99e04b5 100644
>>> --- a/lisp/emacs-lisp/package.el
>>> +++ b/lisp/emacs-lisp/package.el
>>> @@ -4642,6 +4642,7 @@ package-report-bug
>>>                     (boundp (car ent))
>>>                     (not (eq (custom--standard-value (car ent))
>>>                              (default-toplevel-value (car ent))))
>>> +                   (car group)
>>
>> If you are checking for (car group), when do this in the loop instead
>> of wrapping the entire `dolist'?
>
> Good point.  I was just copying the last condition in the and.  Lifted
> them both out of the loop:
>
> From 2873d4c482acfd1ced749d593fd512c408e0a578 Mon Sep 17 00:00:00 2001
> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sun, 9 Jul 2023 22:21:03 -0400
> Subject: [PATCH] Support transforming the dirname used by uniquify

Wrong patch?

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Spencer Baugh]

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

Philip Kaludercic <philipk@posteo.net> writes:

> sbaugh@catern.com writes:
>> Good point.  I was just copying the last condition in the and.  Lifted
>> them both out of the loop:
>>
>> From 2873d4c482acfd1ced749d593fd512c408e0a578 Mon Sep 17 00:00:00 2001
>> From: Spencer Baugh <sbaugh@catern.com>
>> Date: Sun, 9 Jul 2023 22:21:03 -0400
>> Subject: [PATCH] Support transforming the dirname used by uniquify
>
> Wrong patch?
Oops! Here's the right one:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-package-report-bug-don-t-fail-on-custom-groups-defin.patch --]
[-- Type: text/x-patch, Size: 1901 bytes --]

From fb31777bcc1935de351eaa35c1ed679c66d24a92 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@catern.com>
Date: Sun, 9 Jul 2023 12:59:50 -0400
Subject: [PATCH] package-report-bug: don't fail on custom groups defined by
 eval

Previously we just assumed that the car of an element of
custom-current-group-alist was a filename.  But actually it can be nil
if a custom group was defined by just evaling Lisp.

* lisp/emacs-lisp/package.el (package-report-bug): Don't fail when a
custom group was defined by eval.
---
 lisp/emacs-lisp/package.el | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 3e6acd9b388..58ca19f7fe2 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -4637,13 +4637,14 @@ package-report-bug
         vars)
     (dolist-with-progress-reporter (group custom-current-group-alist)
         "Scanning for modified user options..."
-      (dolist (ent (get (cdr group) 'custom-group))
-        (when (and (custom-variable-p (car ent))
-                   (boundp (car ent))
-                   (not (eq (custom--standard-value (car ent))
-                            (default-toplevel-value (car ent))))
-                   (file-in-directory-p (car group) (package-desc-dir desc)))
-          (push (car ent) vars))))
+      (when (and (car group)
+                 (file-in-directory-p (car group) (package-desc-dir desc)))
+        (dolist (ent (get (cdr group) 'custom-group))
+          (when (and (custom-variable-p (car ent))
+                     (boundp (car ent))
+                     (not (eq (custom--standard-value (car ent))
+                              (default-toplevel-value (car ent)))))
+            (push (car ent) vars)))))
     (dlet ((reporter-prompt-for-summary-p t))
       (reporter-submit-bug-report maint name vars))))
 
-- 
2.39.3

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Philip Kaludercic]

Spencer Baugh <sbaugh@janestreet.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> sbaugh@catern.com writes:
>>> Good point.  I was just copying the last condition in the and.  Lifted
>>> them both out of the loop:
>>>
>>> From 2873d4c482acfd1ced749d593fd512c408e0a578 Mon Sep 17 00:00:00 2001
>>> From: Spencer Baugh <sbaugh@catern.com>
>>> Date: Sun, 9 Jul 2023 22:21:03 -0400
>>> Subject: [PATCH] Support transforming the dirname used by uniquify
>>
>> Wrong patch?
> Oops! Here's the right one:
>
> From fb31777bcc1935de351eaa35c1ed679c66d24a92 Mon Sep 17 00:00:00 2001
> From: Spencer Baugh <sbaugh@catern.com>
> Date: Sun, 9 Jul 2023 12:59:50 -0400
> Subject: [PATCH] package-report-bug: don't fail on custom groups defined by
>  eval
>
> Previously we just assumed that the car of an element of
> custom-current-group-alist was a filename.  But actually it can be nil
> if a custom group was defined by just evaling Lisp.
>
> * lisp/emacs-lisp/package.el (package-report-bug): Don't fail when a
> custom group was defined by eval.
> ---
>  lisp/emacs-lisp/package.el | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 3e6acd9b388..58ca19f7fe2 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -4637,13 +4637,14 @@ package-report-bug
>          vars)
>      (dolist-with-progress-reporter (group custom-current-group-alist)
>          "Scanning for modified user options..."
> -      (dolist (ent (get (cdr group) 'custom-group))
> -        (when (and (custom-variable-p (car ent))
> -                   (boundp (car ent))
> -                   (not (eq (custom--standard-value (car ent))
> -                            (default-toplevel-value (car ent))))
> -                   (file-in-directory-p (car group) (package-desc-dir desc)))
> -          (push (car ent) vars))))
> +      (when (and (car group)
> +                 (file-in-directory-p (car group) (package-desc-dir desc)))
> +        (dolist (ent (get (cdr group) 'custom-group))
> +          (when (and (custom-variable-p (car ent))
> +                     (boundp (car ent))
> +                     (not (eq (custom--standard-value (car ent))
> +                              (default-toplevel-value (car ent)))))
> +            (push (car ent) vars)))))
>      (dlet ((reporter-prompt-for-summary-p t))
>        (reporter-submit-bug-report maint name vars))))

LGTM.

Eli, would it be OK to push this to emacs-29, as this is a bug in the
existing code?  Most of the changes are indentation/whitespace changes
anyway.

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: sbaugh@catern.com,  64543@debbugs.gnu.org, "'Eli Zaretskii'" <eliz@gnu.org>
> Date: Fri, 14 Jul 2023 19:44:15 +0000
> 
> Eli, would it be OK to push this to emacs-29, as this is a bug in the
> existing code?  Most of the changes are indentation/whitespace changes
> anyway.

I don't understand the cases in which this bug happens, and so cannot
make the decision.  The entire discussion of this bug never described
such a case, AFAICT.  (Also, the commit log message could use some
loving care: what does it mean when it says "Previously"?)

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: sbaugh@catern.com,  64543@debbugs.gnu.org, "'Eli Zaretskii'" <eliz@gnu.org>
>> Date: Fri, 14 Jul 2023 19:44:15 +0000
>> 
>> Eli, would it be OK to push this to emacs-29, as this is a bug in the
>> existing code?  Most of the changes are indentation/whitespace changes
>> anyway.
>
> I don't understand the cases in which this bug happens, and so cannot
> make the decision.  

Spencer gave this example:

  >> Previously we just assumed that the car of an element of
  >> custom-current-group-alist was a filename.  But actually it can be nil
  >> if a custom group was defined by just evaling Lisp.
  >
  > Where is this behaviour documented?  I couldn't reproduce it with a
  > simple experiment.

  To reproduce:
  M-: (defgroup mygroup nil "my group") RET

The patch would ensure that if groups like these are defined (which
might happen by mistake), then `package-report-bug' will remain robust
and not fail due to a unrelated issue.

>                     The entire discussion of this bug never described
> such a case, AFAICT.  (Also, the commit log message could use some
> loving care: what does it mean when it says "Previously"?)

How about

--8<---------------cut here---------------start------------->8---
* lisp/emacs-lisp/package.el (package-report-bug): Do not assume that
every entry in 'custom-current-group-alist' has a non-nil entry for a
filename.

It is possible for a group to not be associated with any file, e.g. when
a 'defgroup' form is evaluated using 'eval-expression'.  (bug#64543)
--8<---------------cut here---------------end--------------->8---

A related discussion might be if `custom-declare-group' could be
improved, by not just consulting `load-file-name', but using
`macroexp-file-name'.

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: sbaugh@janestreet.com,  sbaugh@catern.com,  64543@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 07:40:01 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Eli, would it be OK to push this to emacs-29, as this is a bug in the
> >> existing code?  Most of the changes are indentation/whitespace changes
> >> anyway.
> >
> > I don't understand the cases in which this bug happens, and so cannot
> > make the decision.  
> 
> Spencer gave this example:
> 
>   >> Previously we just assumed that the car of an element of
>   >> custom-current-group-alist was a filename.  But actually it can be nil
>   >> if a custom group was defined by just evaling Lisp.
>   >
>   > Where is this behaviour documented?  I couldn't reproduce it with a
>   > simple experiment.
> 
>   To reproduce:
>   M-: (defgroup mygroup nil "my group") RET
> 
> The patch would ensure that if groups like these are defined (which
> might happen by mistake), then `package-report-bug' will remain robust
> and not fail due to a unrelated issue.

Is this case important enough to make this change so late in the
pretest?  Spencer, how did you bump into this situation in Real Life?

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Eli Zaretskii]

> Cc: 64543@debbugs.gnu.org
> From: sbaugh@catern.com
> Date: Wed, 12 Jul 2023 19:26:13 +0000 (UTC)
> 
> +(defun uniquify-dirname-transform-default (dirname)
> +  "Simply return DIRNAME unchanged."
> +  dirname)

If you don't explain the need for its existence in the doc string,
this function looks weirdly redundant.

> +(defcustom uniquify-dirname-transform #'uniquify-dirname-transform-default
> +  "A function to transform the dirname used to uniquify a buffer.

Come to think about it, why not use 'identity as the default value?
Then the above weird function could be deleted.

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Eli Zaretskii]

> Date: Sat, 15 Jul 2023 09:12:48 +0000 (UTC)
> From: Spencer Baugh <sbaugh@catern.com>
> Cc: Philip Kaludercic <philipk@posteo.net>, sbaugh@janestreet.com,
> 	64543@debbugs.gnu.org
> 
> On Jul 15, 2023 04:57, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>  >   >> Previously we just assumed that the car of an element of 
>  >   >> custom-current-group-alist was a filename.  But actually it can be nil 
>  >   >> if a custom group was defined by just evaling Lisp. 
>  >   > 
>  >   > Where is this behaviour documented?  I couldn't reproduce it with a 
>  >   > simple experiment. 
>  > 
>  >   To reproduce: 
>  >   M-: (defgroup mygroup nil "my group") RET 
>  > 
>  > The patch would ensure that if groups like these are defined (which 
>  > might happen by mistake), then `package-report-bug' will remain robust 
>  > and not fail due to a unrelated issue. 
> 
>  Is this case important enough to make this change so late in the 
>  pretest?  Spencer, how did you bump into this situation in Real Life?
> 
> I evaled a buffer containing a defgroup with C-c C-e (or maybe just a region?)

Why did you do that?

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 15 Jul 2023 09:12:48 +0000 (UTC)
>> From: Spencer Baugh <sbaugh@catern.com>
>> Cc: Philip Kaludercic <philipk@posteo.net>, sbaugh@janestreet.com,
>> 	64543@debbugs.gnu.org
>> 
>> On Jul 15, 2023 04:57, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>  >   >> Previously we just assumed that the car of an element of 
>>  >   >> custom-current-group-alist was a filename.  But actually it can be nil 
>>  >   >> if a custom group was defined by just evaling Lisp. 
>>  >   > 
>>  >   > Where is this behaviour documented?  I couldn't reproduce it with a 
>>  >   > simple experiment. 
>>  > 
>>  >   To reproduce: 
>>  >   M-: (defgroup mygroup nil "my group") RET 
>>  > 
>>  > The patch would ensure that if groups like these are defined (which 
>>  > might happen by mistake), then `package-report-bug' will remain robust 
>>  > and not fail due to a unrelated issue. 
>> 
>>  Is this case important enough to make this change so late in the 
>>  pretest?  Spencer, how did you bump into this situation in Real Life?
>> 
>> I evaled a buffer containing a defgroup with C-c C-e (or maybe just a region?)
>
> Why did you do that?

Evaluating an entire buffer with C-c C-e is not unreasonable, but it
will not bind `load-file-name', which `load-file' would have done, but
there is no binding for that.

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Spencer Baugh <sbaugh@catern.com>,  sbaugh@janestreet.com,
>   64543@debbugs.gnu.org
> Date: Sat, 15 Jul 2023 10:05:13 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I evaled a buffer containing a defgroup with C-c C-e (or maybe just a region?)
> >
> > Why did you do that?
> 
> Evaluating an entire buffer with C-c C-e is not unreasonable, but it
> will not bind `load-file-name', which `load-file' would have done, but
> there is no binding for that.

<Shrug> it sounds strange to me.

But okay, please install on emacs-29 with the improved log message,
and thanks.

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Spencer Baugh <sbaugh@catern.com>,  sbaugh@janestreet.com,
>>   64543@debbugs.gnu.org
>> Date: Sat, 15 Jul 2023 10:05:13 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> I evaled a buffer containing a defgroup with C-c C-e (or maybe
>> > just a region?)
>> >
>> > Why did you do that?
>> 
>> Evaluating an entire buffer with C-c C-e is not unreasonable, but it
>> will not bind `load-file-name', which `load-file' would have done, but
>> there is no binding for that.
>
> <Shrug> it sounds strange to me.
>
> But okay, please install on emacs-29 with the improved log message,
> and thanks.

Done.

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[sbaugh]

Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Date: Sat, 15 Jul 2023 09:12:48 +0000 (UTC)
>>> From: Spencer Baugh <sbaugh@catern.com>
>>> Cc: Philip Kaludercic <philipk@posteo.net>, sbaugh@janestreet.com,
>>> 	64543@debbugs.gnu.org
>>> 
>>> On Jul 15, 2023 04:57, Eli Zaretskii <eliz@gnu.org> wrote:
>>> 
>>>  >   >> Previously we just assumed that the car of an element of 
>>>  >   >> custom-current-group-alist was a filename.  But actually it
>>>  >   >> can be nil
>>>  >   >> if a custom group was defined by just evaling Lisp. 
>>>  >   > 
>>>  >   > Where is this behaviour documented?  I couldn't reproduce it with a 
>>>  >   > simple experiment. 
>>>  > 
>>>  >   To reproduce: 
>>>  >   M-: (defgroup mygroup nil "my group") RET 
>>>  > 
>>>  > The patch would ensure that if groups like these are defined (which 
>>>  > might happen by mistake), then `package-report-bug' will remain robust 
>>>  > and not fail due to a unrelated issue. 
>>> 
>>>  Is this case important enough to make this change so late in the 
>>>  pretest?  Spencer, how did you bump into this situation in Real Life?
>>> 
>>> I evaled a buffer containing a defgroup with C-c C-e (or maybe just
>>> a region?)
>>
>> Why did you do that?
>
> Evaluating an entire buffer with C-c C-e is not unreasonable, but it
> will not bind `load-file-name', which `load-file' would have done, but
> there is no binding for that.

Maybe we should add a binding for load-file?  Or maybe C-c C-e should
bind load-file-name if the region is not active?

Btw, just curious: how do others test modifications they make to Lisp
files?  I usually make a couple changes to several functions or
variables and then reloading the whole file is easier than going defun
by defun.

bug#64543: [PATCH] package-report-bug: don't fail on custom groups defined by eval

[Philip Kaludercic]

sbaugh@catern.com writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Date: Sat, 15 Jul 2023 09:12:48 +0000 (UTC)
>>>> From: Spencer Baugh <sbaugh@catern.com>
>>>> Cc: Philip Kaludercic <philipk@posteo.net>, sbaugh@janestreet.com,
>>>> 	64543@debbugs.gnu.org
>>>> 
>>>> On Jul 15, 2023 04:57, Eli Zaretskii <eliz@gnu.org> wrote:
>>>> 
>>>>  >   >> Previously we just assumed that the car of an element of 
>>>>  >   >> custom-current-group-alist was a filename.  But actually it
>>>>  >   >> can be nil
>>>>  >   >> if a custom group was defined by just evaling Lisp. 
>>>>  >   > 
>>>>  >   > Where is this behaviour documented?  I couldn't reproduce it with a 
>>>>  >   > simple experiment. 
>>>>  > 
>>>>  >   To reproduce: 
>>>>  >   M-: (defgroup mygroup nil "my group") RET 
>>>>  > 
>>>>  > The patch would ensure that if groups like these are defined (which 
>>>>  > might happen by mistake), then `package-report-bug' will remain robust 
>>>>  > and not fail due to a unrelated issue. 
>>>> 
>>>>  Is this case important enough to make this change so late in the 
>>>>  pretest?  Spencer, how did you bump into this situation in Real Life?
>>>> 
>>>> I evaled a buffer containing a defgroup with C-c C-e (or maybe just
>>>> a region?)
>>>
>>> Why did you do that?
>>
>> Evaluating an entire buffer with C-c C-e is not unreasonable, but it
>> will not bind `load-file-name', which `load-file' would have done, but
>> there is no binding for that.
>
> Maybe we should add a binding for load-file?  Or maybe C-c C-e should
> bind load-file-name if the region is not active?

I am not familiar enough with the intention between distinguishing
evaluation and loading to comment on this, or where the distinction
/should/ matter.

> Btw, just curious: how do others test modifications they make to Lisp
> files?  I usually make a couple changes to several functions or
> variables and then reloading the whole file is easier than going defun
> by defun.

I either eval the defuns right after editing them using C-M-x or I'd
infrequently use M-x load-file.