GNU bug report logs - #55149
29.0.50; Commit f30625943e broke magit/with-editor

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55149 in the body.
You can then email your comments to 55149 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <thorn <at> fastmail.fm>
To: bug-gnu-emacs <at> gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Wed, 27 Apr 2022 14:48:58 +0200

Hi Stefan, your commit f30625943e on master somehow broke the magit
and/or with-editor packages by somehow breaking emacsclient, see:

  https://github.com/magit/with-editor/issues/111
  https://github.com/magit/magit/issues/4669

With stock-emacs when doing "emacsclient foo.txt", it just prints
"Waiting for Emacs..." but finishes immediately.  The existing emacs
doesn't have a foo.txt buffer thereafter.  "emacsclient -c foo.txt"
results in a new frame which has a foo.txt buffer but it is not
displayed in a window...


In GNU Emacs 29.0.50 (build 23, x86_64-pc-linux-gnu, GTK+ Version 3.24.33, cairo version 1.17.6)
 of 2022-04-27 built on thinkpad-t440p
Repository revision: bc9be5449e1127bc1b05a6cad8471c6eba52c8e9
Repository branch: HEAD
System Description: Arch Linux

Configured using:
 'configure --with-pgtk --with-native-compilation --with-xinput2'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY
PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS WEBP XIM GTK3 ZLIB

Important settings:
  value of $LC_MONETARY: de_DE.utf8
  value of $LC_NUMERIC: de_DE.utf8
  value of $LC_TIME: de_DE.utf8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: mu4e:main

Minor modes in effect:
  editorconfig-mode: t
  global-aggressive-indent-mode: t
  pdf-occur-global-minor-mode: t
  diredfl-global-mode: t
  mu4e-search-minor-mode: t
  mu4e-update-minor-mode: t
  mu4e-context-minor-mode: t
  which-key-mode: t
  highlight-parentheses-mode: t
  global-corfu-mode: t
  corfu-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  vertico-mode: t
  marginalia-mode: t
  minibuffer-depth-indicate-mode: t
  electric-pair-mode: t
  recentf-mode: t
  pixel-scroll-precision-mode: t
  pixel-scroll-mode: t
  override-global-mode: t
  save-place-mode: t
  savehist-mode: t
  shell-dirtrack-mode: t
  puni-global-mode: t
  puni-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  overwrite-mode: overwrite-mode-binary

Load-path shadows:
~/Repos/el/mu/build/mu4e/mu4e hides ~/Repos/el/mu/mu4e/mu4e
~/Repos/el/mu/build/mu4e/mu4e-main hides ~/Repos/el/mu/mu4e/mu4e-main
~/Repos/el/mu/build/mu4e/mu4e-view hides ~/Repos/el/mu/mu4e/mu4e-view
~/Repos/el/mu/build/mu4e/mu4e-org hides ~/Repos/el/mu/mu4e/mu4e-org
~/Repos/el/mu/build/mu4e/mu4e-lists hides ~/Repos/el/mu/mu4e/mu4e-lists
~/Repos/el/mu/build/mu4e/mu4e-actions hides ~/Repos/el/mu/mu4e/mu4e-actions
~/Repos/el/mu/build/mu4e/mu4e-helpers hides ~/Repos/el/mu/mu4e/mu4e-helpers
~/Repos/el/mu/build/mu4e/mu4e-search hides ~/Repos/el/mu/mu4e/mu4e-search
~/Repos/el/mu/build/mu4e/mu4e-server hides ~/Repos/el/mu/mu4e/mu4e-server
~/Repos/el/mu/build/mu4e/mu4e-update hides ~/Repos/el/mu/mu4e/mu4e-update
~/Repos/el/mu/build/mu4e/mu4e-context hides ~/Repos/el/mu/mu4e/mu4e-context
~/Repos/el/mu/build/mu4e/mu4e-draft hides ~/Repos/el/mu/mu4e/mu4e-draft
~/Repos/el/mu/build/mu4e/mu4e-bookmarks hides ~/Repos/el/mu/mu4e/mu4e-bookmarks
~/Repos/el/mu/build/mu4e/mu4e-message hides ~/Repos/el/mu/mu4e/mu4e-message
~/Repos/el/mu/build/mu4e/mu4e-compose hides ~/Repos/el/mu/mu4e/mu4e-compose
~/Repos/el/mu/build/mu4e/mu4e-headers hides ~/Repos/el/mu/mu4e/mu4e-headers
~/Repos/el/mu/build/mu4e/mu4e-mark hides ~/Repos/el/mu/mu4e/mu4e-mark
~/Repos/el/mu/build/mu4e/mu4e-contacts hides ~/Repos/el/mu/mu4e/mu4e-contacts
~/Repos/el/mu/build/mu4e/mu4e-icalendar hides ~/Repos/el/mu/mu4e/mu4e-icalendar
~/Repos/el/mu/build/mu4e/mu4e-folders hides ~/Repos/el/mu/mu4e/mu4e-folders
~/Repos/el/mu/build/mu4e/mu4e-speedbar hides ~/Repos/el/mu/mu4e/mu4e-speedbar
~/Repos/el/mu/build/mu4e/mu4e-contrib hides ~/Repos/el/mu/mu4e/mu4e-contrib
~/Repos/el/mu/build/mu4e/mu4e-vars hides ~/Repos/el/mu/mu4e/mu4e-vars
/home/horn/.emacs.d/elpa/transient-20220425.1314/transient hides /home/horn/Repos/el/emacs/lisp/transient

Features:
(shadow editorconfig-core editorconfig-core-handle editorconfig-fnmatch
emacsbug so-long view puni pulse display-fill-column-indicator
auto-package-update finder-inf generic yaml-mode fish-mode cargo
cargo-process rust-utils rust-mode rust-rustfmt rust-playpen
rust-compile rust-cargo web-mode disp-table preview-latex auto-loads
tex-site editorconfig elfeed-show elfeed-search vc-mtn vc-hg vc-bzr
vc-src vc-sccs vc-svn vc-cvs vc-rcs debbugs-browse elfeed-csv elfeed
elfeed-curl elfeed-log elfeed-db elfeed-lib avl-tree generator url-queue
xml-query socks elpher hl-todo aggressive-indent rainbow-mode pdf-occur
tablist tablist-filter semantic/wisent/comp semantic/wisent
semantic/wisent/wisent semantic/util-modes semantic/util semantic
semantic/tag semantic/lex semantic/fw mode-local cedet pdf-isearch
pdf-misc pdf-tools pdf-view magit-bookmark bookmark jka-compr pdf-cache
pdf-info tq pdf-util pdf-macs image-mode exif vc-git vc-dir ewoc
epa-file rdictcc diredfl dired-x mu4e-icalendar gnus-icalendar
org-capture org-refile icalendar diary-lib diary-loaddefs mu4e mu4e-org
mu4e-view org ob ob-tangle ob-ref ob-lob ob-table ob-exp org-macro
org-footnote org-src ob-comint org-pcomplete org-list org-faces
org-entities org-version ob-emacs-lisp ob-core ob-eval org-table
oc-basic bibtex ol org-keys oc org-compat org-macs org-loaddefs
find-func cal-menu calendar cal-loaddefs mu4e-main mu4e-headers
mu4e-lists mu4e-compose mu4e-draft mu4e-actions smtpmail mu4e-search
mu4e-bookmarks mu4e-mark mu4e-message flow-fill mule-util hl-line
mu4e-contacts mu4e-update mu4e-folders mu4e-server mu4e-context
mu4e-vars mu4e-helpers ido mu4e-meta ecomplete auto-dictionary flyspell
ispell tramp-smb which-key highlight-parentheses restclient kind-icon
svg-lib corfu yasnippet forge-list forge-commands forge-semi
forge-bitbucket buck forge-gogs gogs forge-gitea gtea forge-gitlab glab
forge-github ghub-graphql treepy gsexp ghub let-alist forge-notify
forge-revnote forge-pullreq forge-issue forge-topic yaml forge-post
markdown-mode color thingatpt noutline outline forge-repo forge
forge-core forge-db closql emacsql-sqlite emacsql emacsql-compiler
magit-submodule magit-obsolete magit-blame magit-stash magit-reflog
magit-bisect magit-push magit-pull magit-fetch magit-clone magit-remote
magit-commit magit-sequence magit-notes magit-worktree magit-tag
magit-merge magit-branch magit-reset magit-files magit-refs magit-status
magit magit-repos magit-apply magit-wip magit-log which-func imenu
magit-diff smerge-mode diff diff-mode git-commit log-edit pcvs-util
add-log magit-core magit-autorevert autorevert filenotify magit-margin
magit-transient magit-process with-editor server magit-mode magit-git
magit-base magit-section crm dash compat-27 compat-26
visual-filename-abbrev rg vc vc-dispatcher rg-info-hack advice rg-menu
transient comp comp-cstr compat rg-ibuffer rg-result wgrep-rg wgrep
rg-history rg-header ibuf-ext ibuffer ibuffer-loaddefs grep compile
debbugs soap-client url-http url-auth url-gw nsm warnings rng-xsd rng-dt
rng-util xsd-regexp bug-reference vertico edmacro kmacro marginalia
icomplete mb-depth use-package-diminish ace-window avy elec-pair rx
recentf tree-widget pixel-scroll cua-base use-package-bind-key bind-key
saveplace savehist smiley gnus-art mm-uu mml2015 mm-view mml-smime smime
gnutls dig gnus-sum shr pixel-fill kinsoku url-file url-dired svg dom
gnus-group gnus-undo gnus-start gnus-dbus dbus xml gnus-cloud nnimap
nnmail mail-source utf7 netrc nnoo gnus-spec gnus-int gnus-range message
sendmail yank-media rmc puny dired dired-loaddefs rfc822 mml mml-sec epa
derived epg rfc6068 epg-config mm-decode mm-bodies mm-encode mail-parse
rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils mailheader
gnus-win gnus nnheader gnus-util text-property-search mm-util mail-prsvr
mail-utils range doom-themes-ext-org doom-themes-ext-visual-bell
face-remap doom-Iosvkem-theme doom-themes doom-themes-base diminish
cl-extra help-mode use-package-ensure use-package-core tramp
tramp-loaddefs trampver tramp-integration cus-edit pp cus-load wid-edit
files-x tramp-compat shell pcomplete comint ansi-color ring parse-time
iso8601 time-date ls-lisp format-spec easy-mmode info package browse-url
url url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap url-handlers url-parse auth-source cl-seq
eieio eieio-core cl-macs eieio-loaddefs password-cache json map url-vars
seq gv subr-x byte-opt bytecomp byte-compile cconv cl-loaddefs cl-lib
iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/pgtk-win pgtk-win term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice
simple cl-generic cham georgian utf-8-lang misc-lang vietnamese tibetan
thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek romanian
slovak czech european ethiopic indian cyrillic chinese composite
emoji-zwj charscript charprop case-table epa-hook jka-cmpr-hook help
abbrev obarray oclosure cl-preloaded button loaddefs faces cus-face
macroexp files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify dynamic-setting system-font-setting
font-render-setting cairo gtk pgtk lcms2 multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 835148 61455)
 (symbols 48 49796 11)
 (strings 32 245719 8853)
 (string-bytes 1 7800532)
 (vectors 16 110598)
 (vector-slots 8 1761956 85817)
 (floats 8 999 399)
 (intervals 56 3115 390)
 (buffers 992 16))

Message #8 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: No Wayman <iarchivedmywholelife <at> gmail.com>
To: thorn <at> fastmail.fm, 55149 <at> debbugs.gnu.org
Subject: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Wed, 27 Apr 2022 15:41:29 -0400

Just chiming in to confirm that the same commit has broken 
something with emacsclient frames as well.

Message #11 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: No Wayman <iarchivedmywholelife <at> gmail.com>
Cc: thorn <at> fastmail.fm, 55149 <at> debbugs.gnu.org
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Wed, 27 Apr 2022 17:48:27 -0400

No Wayman [2022-04-27 15:41:29] wrote:
> Just chiming in to confirm that the same commit has broken something with
> emacsclient frames as well.

Could you give some details about that?
The `git-commit` bug involves a lot of code, so it would be very helpful
to have a recipe that involves less code.


        Stefan

Message #14 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <thorn <at> fastmail.fm>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: No Wayman <iarchivedmywholelife <at> gmail.com>, 55149 <at> debbugs.gnu.org
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 05:39:57 +0200 (GMT+02:00)

I think he means what I've written in my original report.

```
With stock-emacs when doing "emacsclient foo.txt", it just prints
"Waiting for Emacs..." but finishes immediately.  The existing emacs
doesn't have a foo.txt buffer thereafter.  "emacsclient -c foo.txt"
results in a new frame which has a foo.txt buffer but it is not
displayed in a window...
```

Testing this strange emacsclient behavior requires no external packages at all.

27.04.2022 23:48:32 Stefan Monnier <monnier <at> iro.umontreal.ca>:

> No Wayman [2022-04-27 15:41:29] wrote:
>> Just chiming in to confirm that the same commit has broken something with
>> emacsclient frames as well.
> 
> Could you give some details about that?
> The `git-commit` bug involves a lot of code, so it would be very helpful
> to have a recipe that involves less code.
> 
> 
>         Stefan

Message #17 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: No Wayman <iarchivedmywholelife <at> gmail.com>
To: Tassilo Horn <thorn <at> fastmail.fm>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 55149 <at> debbugs.gnu.org
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 00:02:36 -0400

Tassilo Horn <thorn <at> fastmail.fm> writes:

> I think he means what I've written in my original report.

Yes. Consider that a +1 in general from me.
I've rolled everything back and unfortunately I don't have the 
time to dig into this one.
I took a look at with-editor and it does advise several functions, 
including `server-switch-buffer'. Not sure if that is where the 
problem lies, just a hunch.

Message #20 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <thorn <at> fastmail.fm>
To: "M. Ian Graham" <hello <at> miangraham.com>
Cc: "55149 <at> debbugs.gnu.org" <55149 <at> debbugs.gnu.org>,
 "monnier <at> iro.umontreal.ca" <monnier <at> iro.umontreal.ca>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 06:47:50 +0200

"M. Ian Graham" <hello <at> miangraham.com> writes:

>> With stock-emacs when doing "emacsclient foo.txt", it just prints
>> "Waiting for Emacs..." but finishes immediately.
>
> Hi, quick clarification on this: As far as I can tell from my own
> observations, all emacsclient breakage here is a downstream
> side-effect of magit/with-editor's advice calls somehow getting messed
> up.

Oh, you are right.  But I was able to extract (at least one of) the
problematic with-editor advice(s) which cause the breakage.

Save the below as bug-recipe.el:

--8<---------------cut here---------------start------------->8---
(advice-add 'server-visit-files :after
            #'server-visit-files--with-editor-file-name-history-exclude)

(defun server-visit-files--with-editor-file-name-history-exclude
    (files _proc &optional _nowait)
  (pcase-dolist (`(,file . ,_) files)
    (when (cl-find-if (lambda (regexp)
                        (string-match-p regexp file))
                      '("don't care"))
      (setq file-name-history (delete file file-name-history)))))

(server-start)
--8<---------------cut here---------------end--------------->8---

Then start emacs as "emacs -Q -l bug-recipe.el" and you'll be able to
perform my "stock emacs" recipe, i.e.,

  emacsclient foo.txt => finishes immediately
  emacsclient -c foo.txt => doesn't display the foo.txt buffer

Bye,
Tassilo

Message #23 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <thorn <at> fastmail.fm>
To: "M. Ian Graham" <hello <at> miangraham.com>
Cc: "55149 <at> debbugs.gnu.org" <55149 <at> debbugs.gnu.org>,
 "monnier <at> iro.umontreal.ca" <monnier <at> iro.umontreal.ca>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 08:28:37 +0200

"M. Ian Graham" <hello <at> miangraham.com> writes:

>> Oh, you are right. But I was able to extract (at least one of) the
>> problematic with-editor advice(s) which cause the breakage.
>
>> Save the below as bug-recipe.el:
>
> Nice find. Confirmed on my end that this breaks emacsclient commands
> after f30625943e but not before. Had to add a require 'cl-lib to the
> top, otherwise works perfectly with bare emacs -Q.

Oh, well, the recipe can even be simplyfied to defining just a
do-nothing advice!

--8<---------------cut here---------------start------------->8---
(advice-add 'server-visit-files :after
            #'server-visit-files--with-editor-file-name-history-exclude)

(defun server-visit-files--with-editor-file-name-history-exclude
    (files _proc &optional _nowait)
  nil)

(server-start)
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

Message #26 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: "M. Ian Graham" <hello <at> miangraham.com>
To: "55149 <at> debbugs.gnu.org" <55149 <at> debbugs.gnu.org>
Subject: (No Subject)
Date: Thu, 28 Apr 2022 02:06:02 +0000

[Message part 1 (text/plain, inline)]

Quick clarification on server/emacsclient involvement: As far as I can tell, any emacsclient breakage here is a downstream side effect of magit/with-editor's advice calls somehow getting messed up.

Using bc9be5449e1127bc1b05a6cad8471c6eba52c8e9 and prior I cannot reproduce any related problems.
Using f30625943edefbd88ebf84acbc254ed88db27beb and later causes the magit-commit command to break with a GitError ("problem with the editor" where the editor is emacsclient) before displaying its message editor window.

In the same emacs instance, only after first reproducing the magit-commit error (and presumably leaving a stalled client connection in place), later emacsclient usage fails via the CLI.
Given a new session using a fresh server-start and using basic emacsclient commands, I don't observe any problems.

I'm not yet sure what a minimal breaking advice test looks like. Will shout back if I find something.

-Ian

[Message part 2 (text/html, inline)]

Message #29 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: "M. Ian Graham" <hello <at> miangraham.com>
To: "55149 <at> debbugs.gnu.org" <55149 <at> debbugs.gnu.org>
Cc: "thorn <at> fastmail.fm" <thorn <at> fastmail.fm>,
 "monnier <at> iro.umontreal.ca" <monnier <at> iro.umontreal.ca>
Subject: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 04:06:49 +0000

[Message part 1 (text/plain, inline)]

> With stock-emacs when doing "emacsclient foo.txt", it just prints "Waiting for Emacs..." but finishes immediately.

Hi, quick clarification on this: As far as I can tell from my own observations, all emacsclient breakage here is a downstream side-effect of magit/with-editor's advice calls somehow getting messed up.

Observed breakage:

Using bc9be5449e1127bc1b05a6cad8471c6eba52c8e9 and prior I cannot reproduce any related problems. Magit works properly.
Using f30625943edefbd88ebf84acbc254ed88db27beb and later causes the magit-commit command to break with a GitError ("problem with the editor", where the editor is emacsclient) before displaying its message editor window. Given the commit contents I assume the advice-add usage in with-editor is being affected somehow.

emacsclient issues:

In the same emacs instance as above, only after first reproducing the magit-commit error (and presumably leaving a stalled client connection in place), later emacsclient usage fails via the CLI.
Given a new session using a fresh server-start and using basic emacsclient commands, I don't observe any problems.

If I'm misunderstanding anyone and you're able to repro manual emacsclient breakage with a fresh server, please follow up.

I'm not yet sure what a minimal breaking advice test looks like. Trying simple advice-add calls that look similar to with-editor's hasn't turned up any problems yet. I'll shout back if I find something.
-Ian

[Message part 2 (text/html, inline)]

Message #32 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: "M. Ian Graham" <hello <at> miangraham.com>
To: Tassilo Horn <thorn <at> fastmail.fm>
Cc: "55149 <at> debbugs.gnu.org" <55149 <at> debbugs.gnu.org>,
 "monnier <at> iro.umontreal.ca" <monnier <at> iro.umontreal.ca>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 06:12:33 +0000

> Oh, you are right. But I was able to extract (at least one of) the
> problematic with-editor advice(s) which cause the breakage.

> Save the below as bug-recipe.el:

Nice find. Confirmed on my end that this breaks emacsclient commands after f30625943e but not before. Had to add a require 'cl-lib to the top, otherwise works perfectly with bare emacs -Q.

-Ian

Message #35 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
 text editors" <bug-gnu-emacs <at> gnu.org>
Cc: thorn <at> fastmail.fm, No Wayman <iarchivedmywholelife <at> gmail.com>,
 55149 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 09:58:45 +0300

> Could you give some details about that?
> The `git-commit` bug involves a lot of code, so it would be very helpful
> to have a recipe that involves less code.

I have the same problem, and distilled it to the shortest test case:

```
(defun testa ()
  1)

(advice-add 'testa :after
            (lambda (&rest _) 2))

(defun testb ()
  (interactive)
  (message "! %S" (testa)))
```

In 28 it returns 1.
In 29 it returns 2.

The regression is that the return value of :after now overrides
the return value of the original function.

Message #41 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <thorn <at> fastmail.fm>
To: Juri Linkov <juri <at> linkov.net>
Cc: "Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of
 text editors" <bug-gnu-emacs <at> gnu.org>,
 No Wayman <iarchivedmywholelife <at> gmail.com>, 55149 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 09:12:27 +0200

Juri Linkov <juri <at> linkov.net> writes:

> I have the same problem, and distilled it to the shortest test case:
>
> ```
> (defun testa ()
>   1)
>
> (advice-add 'testa :after
>             (lambda (&rest _) 2))
>
> (defun testb ()
>   (interactive)
>   (message "! %S" (testa)))
> ```
>
> In 28 it returns 1.
> In 29 it returns 2.
>
> The regression is that the return value of :after now overrides
> the return value of the original function.

Oh, indeed.  But isn't that exactly as documented for add-function?

 :after (lambda (&rest r) (apply OLDFUN r) (apply FUNCTION r))

Of course, we'd rather want

 :after (lambda (&rest r) (prog1 (apply OLDFUN r) (apply FUNCTION r)))

Bye,
Tassilo

Message #47 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Tassilo Horn <thorn <at> fastmail.fm>
Cc: "Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of
 text editors" <bug-gnu-emacs <at> gnu.org>,
 No Wayman <iarchivedmywholelife <at> gmail.com>, 55149 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 10:22:24 +0300

>> The regression is that the return value of :after now overrides
>> the return value of the original function.
>
> Oh, indeed.  But isn't that exactly as documented for add-function?
>
>  :after (lambda (&rest r) (apply OLDFUN r) (apply FUNCTION r))

Indeed, it seems documentation differed from the actual implementation.

> Of course, we'd rather want
>
>  :after (lambda (&rest r) (prog1 (apply OLDFUN r) (apply FUNCTION r)))

Yep, this looks like what it's natural to expect from such advice type.
Or maybe a new type could be added to modify the return value?

Message #53 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <thorn <at> fastmail.fm>
To: Juri Linkov <juri <at> linkov.net>
Cc: "Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of
 text editors" <bug-gnu-emacs <at> gnu.org>,
 No Wayman <iarchivedmywholelife <at> gmail.com>, 55149 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 09:31:22 +0200

Juri Linkov <juri <at> linkov.net> writes:

>>> The regression is that the return value of :after now overrides
>>> the return value of the original function.
>>
>> Oh, indeed.  But isn't that exactly as documented for add-function?
>>
>>  :after (lambda (&rest r) (apply OLDFUN r) (apply FUNCTION r))
>
> Indeed, it seems documentation differed from the actual implementation.
>
>> Of course, we'd rather want
>>
>>  :after (lambda (&rest r) (prog1 (apply OLDFUN r) (apply FUNCTION r)))
>
> Yep, this looks like what it's natural to expect from such advice type.
> Or maybe a new type could be added to modify the return value?

There is :filter-return to do exactly that.

Bye,
Tassilo

Message #59 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: "M. Ian Graham" <hello <at> miangraham.com>
To: Tassilo Horn <thorn <at> fastmail.fm>
Cc: "55149 <at> debbugs.gnu.org" <55149 <at> debbugs.gnu.org>,
 "monnier <at> iro.umontreal.ca" <monnier <at> iro.umontreal.ca>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 06:56:19 +0000

> Oh, well, the recipe can even be simplyfied to defining just a
> do-nothing advice!

Ah ha. Return value shenanigans. Here's a no-server test case.

--- begin elisp ---
(advice-add 'original :after
            #'original--advice)

(defun original () "correct: original return value passes through")
(defun original--advice () "broken: advice function's return value is used")

(message (original))
--- end elisp ---

Output before f30625943e:
$ emacs -Q --script bug-recipe.el
correct: original return value passes through

Output after f30625943e:
$ emacs -Q --script bug-recipe.el
broken: advice function's return value is used

-Ian

Message #64 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: dick <dick.r.chiang <at> gmail.com>
Cc: 55149 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 55161 <at> debbugs.gnu.org
Subject: Re: bug#55161: 29.0.50; [PATCH] oclosure transcription error redux
Date: Thu, 28 Apr 2022 12:23:04 +0200

dick <dick.r.chiang <at> gmail.com> writes:

> -   (:after (apply cdr r) (apply car r))
> +   (:after (prog1 (apply cdr r) (apply car r)))

I guess this is the same as bug#55149, so I've merged the reports.

Apparently, returning the value of the :after function is what's
documented to happen (and is what the trunk does now), but it didn't
use to do that, so it's breaking advice that relies on the old
behaviour, if I understand 55149 correctly.

Stefan?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #69 received at 55149-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Juri Linkov <juri <at> linkov.net>
Cc: thorn <at> fastmail.fm, No Wayman <iarchivedmywholelife <at> gmail.com>,
 55149-done <at> debbugs.gnu.org
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 09:39:16 -0400

> I have the same problem, and distilled it to the shortest test case:
>
> ```
> (defun testa ()
>   1)
>
> (advice-add 'testa :after
>             (lambda (&rest _) 2))
>
> (defun testb ()
>   (interactive)
>   (message "! %S" (testa)))
> ```

Thanks, should be fixed now,


        Stefan

Message #82 received at 55149-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 55161-done <at> debbugs.gnu.org, dick <dick.r.chiang <at> gmail.com>,
 55149-done <at> debbugs.gnu.org
Subject: Re: bug#55161: 29.0.50; [PATCH] oclosure transcription error redux
Date: Thu, 28 Apr 2022 09:42:08 -0400

Lars Ingebrigtsen [2022-04-28 12:23:04] wrote:
> dick <dick.r.chiang <at> gmail.com> writes:
>
>> -   (:after (apply cdr r) (apply car r))
>> +   (:after (prog1 (apply cdr r) (apply car r)))
>
> I guess this is the same as bug#55149, so I've merged the reports.
>
> Apparently, returning the value of the :after function is what's
> documented to happen (and is what the trunk does now), but it didn't
> use to do that, so it's breaking advice that relies on the old
> behaviour, if I understand 55149 correctly.
> Stefan?

Yes, I implemented the new code from the doc (since the old code was
written in the form of an "undecipherable" bytecode string), but the doc
was wrong.  It's now fixed.


        Stefan

Message #85 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rudolf Schlatte <rudi <at> constantly.at>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#55149: bug#55161: 29.0.50;
 [PATCH] oclosure transcription error redux
Date: Thu, 28 Apr 2022 15:50:14 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> dick <dick.r.chiang <at> gmail.com> writes:
>
>> -   (:after (apply cdr r) (apply car r))
>> +   (:after (prog1 (apply cdr r) (apply car r)))
>
> I guess this is the same as bug#55149, so I've merged the reports.
>
> Apparently, returning the value of the :after function is what's
> documented to happen (and is what the trunk does now), but it didn't
> use to do that, so it's breaking advice that relies on the old
> behaviour, if I understand 55149 correctly.

This behavior is a bit surprising, since Common Lisp specifies that the
return values of :after methods are ignored (“All the after methods are
called in most-specific-last order. Their values are ignored.”
http://www.lispworks.com/documentation/HyperSpec/Body/07_ffb.htm).  To
override the return value of a method, :around methods are used.

I don't want to argue that CL method combinations and elisp advice are
the same thing, or that Emacs Lisp has to necessarily follow CL
semantics, but it would be good to have the possibility of inserting a
bit of code that should "just run" after a function, without overriding
its return value.

Message #88 received at 55149-done <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <thorn <at> fastmail.fm>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 55149-done <at> debbugs.gnu.org, No Wayman <iarchivedmywholelife <at> gmail.com>,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 16:14:56 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> I have the same problem, and distilled it to the shortest test case:
>>
>> ```
>> (defun testa ()
>>   1)
>>
>> (advice-add 'testa :after
>>             (lambda (&rest _) 2))
>>
>> (defun testb ()
>>   (interactive)
>>   (message "! %S" (testa)))
>> ```
>
> Thanks, should be fixed now,

It is, thanks.  Shouldn't you also adapt the add-function docstring?
Oh, that's adapted automatically!  Interesting approach.  And even more
interesting that the info docs were correct since 2014. :-)

Bye,
Tassilo

Message #91 received at 55149-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Tassilo Horn <thorn <at> fastmail.fm>
Cc: 55149-done <at> debbugs.gnu.org, No Wayman <iarchivedmywholelife <at> gmail.com>,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 10:28:25 -0400

> It is, thanks.  Shouldn't you also adapt the add-function docstring?
> Oh, that's adapted automatically!  Interesting approach.

:-)
Actually, it was done to avoid copy&paste between the docstrings of
`add-function` and `advice-add`, but it's nice that it also avoids
duplication between the code and the doc and makes sure the doc is in
sync with the code.


        Stefan

Message #94 received at 55149-done <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <thorn <at> fastmail.fm>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 55149-done <at> debbugs.gnu.org, No Wayman <iarchivedmywholelife <at> gmail.com>,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 16:39:11 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> It is, thanks.  Shouldn't you also adapt the add-function docstring?
>> Oh, that's adapted automatically!  Interesting approach.
>
> :-)
> Actually, it was done to avoid copy&paste between the docstrings of
> `add-function` and `advice-add`, but it's nice that it also avoids
> duplication between the code and the doc and makes sure the doc is in
> sync with the code.

Indeed, and I think until recently advice-add didn't document HOW but
just referred to add-function which was a bit annoying because usually
one uses advice-add so does C-h f on that and then needs to TAB to
add-function to get the docs on that.

Would you mind adding this magic also to define-advice so that the HOW
is described there as well?

Bye,
Tassilo

Message #97 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: Jonas Bernoulli <jonas <at> bernoul.li>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 55161-done <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 55149-done <at> debbugs.gnu.org, 55149 <at> debbugs.gnu.org,
 dick <dick.r.chiang <at> gmail.com>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 17:26:47 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> Lars Ingebrigtsen [2022-04-28 12:23:04] wrote:
>> dick <dick.r.chiang <at> gmail.com> writes:
>>
>>> -   (:after (apply cdr r) (apply car r))
>>> +   (:after (prog1 (apply cdr r) (apply car r)))
>>
>> I guess this is the same as bug#55149, so I've merged the reports.
>>
>> Apparently, returning the value of the :after function is what's
>> documented to happen (and is what the trunk does now), but it didn't
>> use to do that, so it's breaking advice that relies on the old
>> behaviour, if I understand 55149 correctly.
>> Stefan?
>
> Yes, I implemented the new code from the doc (since the old code was
> written in the form of an "undecipherable" bytecode string), but the doc
> was wrong.  It's now fixed.

At least add-function's docstring did contain the (prog1 ...) until
92e49944a39ce6372a80430f65913c4c8b531677 started to generate the
docstring.

-`:after'	(lambda (&rest r) (prog1 (apply OLDFUN r) (apply FUNCTION r)))
- ...
+<<>>

So we are not restoring an old bug and changing the docstring to
describe the restored behavior instead of the intended but not
actually implemented behavior.

We unintentionally briefly changed the behavior *as well as* the
documentation and have now restored both to what we had before.

Message #103 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jonas Bernoulli <jonas <at> bernoul.li>
Cc: 55161-done <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 55149-done <at> debbugs.gnu.org, 55149 <at> debbugs.gnu.org,
 dick <dick.r.chiang <at> gmail.com>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 12:32:33 -0400

> -`:after'	(lambda (&rest r) (prog1 (apply OLDFUN r) (apply FUNCTION r)))

Hmm... so it was really just a plain mess up on my part!


        Stefan

Message #109 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: Jonas Bernoulli <jonas <at> bernoul.li>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 55161-done <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 55149-done <at> debbugs.gnu.org, 55149 <at> debbugs.gnu.org,
 dick <dick.r.chiang <at> gmail.com>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Fri, 29 Apr 2022 00:49:52 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> -`:after'	(lambda (&rest r) (prog1 (apply OLDFUN r) (apply FUNCTION r)))
>
> Hmm... so it was really just a plain mess up on my part!
>
>
>         Stefan

That's not the part I was trying to focus on, just that the
documentation always was correct (and that my use of :after
didn't just work by accident).  ;D

     Jonas

Message #115 received at 55149 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jonas Bernoulli <jonas <at> bernoul.li>
Cc: 55161-done <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>,
 55149-done <at> debbugs.gnu.org, 55149 <at> debbugs.gnu.org,
 dick <dick.r.chiang <at> gmail.com>
Subject: Re: bug#55149: 29.0.50; Commit f30625943e broke magit/with-editor
Date: Thu, 28 Apr 2022 19:44:12 -0400

> That's not the part I was trying to focus on, just that the
> documentation always was correct (and that my use of :after
> didn't just work by accident).  ;D

The intended semantics was very much the one you relied on, yes.
And apparently, the doc reflected that :-)


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.