bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Tassilo Horn]

Since commit 1af91d271e077134e272055407fb8c4312a7579b I get strange
"Invalid version syntax: <package> <version> (must start with a number)"
errors during M-x completion in the minibuffer.  I can't reproduce with
emacs -Q but in the error, <package> is always some (M)ELPA package.

For example, M-x mag TAB gives

  completion--some: Invalid version syntax: ‘Magit 3.0.0’ (must start with a number)

Here is a backtrace:

--8<---------------cut here---------------start------------->8---
Debugger entered--Lisp error: (error "Invalid version syntax: ‘Magit 3.0.0’ (must start ...")
  error("Invalid version syntax: `%s' (must start with a nu..." "Magit 3.0.0")
  version-to-list("Magit 3.0.0")
  #f(compiled-function (sym) #<bytecode -0x190dc04a7f35bb9f>)(magit-dispatch-popup)
  try-completion("mag" [## magit-section-show-headings vc-src-responsible-p Read\ manual tag-end dashboard-ls Set\ Slice debbugs-gnu-log-edit-mode mu4e-kill-update-mail Reduce\ To:\ to\ Cc: mu4e-view-header-field-keymap c-electric-lt-gt :neither yas-x-prompt cc-vars rainbow-colorize-ansi znc magit-xref-insert-button inactive gnus-agent-group-covered-p web-mode-extra-snippets smiley-directory message-expand-name-databases transient:magit-diff-refresh:--irreversible-delete er/looking-back-max tree-widget-button-keymap rust--same-line-p gnus-summary-show-article-from-menu-as-charset-ebcdic-int :where org-switch-to-buffer-other-window nexus th/visual-line-mode-init :mac xref--push-markers vc-bzr-shelve-menu display-even-when-displayed widget-face-sample-face-get time-subsecond gnus-summary-limit-to-marks he-line-search-regexp symbol-before-point-for-complete magit-filename utf7-direct-encoding-chars generic--normalize-comments org-get-org-file check-declare xref-group :org-comment stack-mode Boring\ headers ...] #f(compiled-function (sym) #<bytecode -0x190dc04a7f35bb9f>))
  complete-with-action(nil [## magit-section-show-headings vc-src-responsible-p Read\ manual tag-end dashboard-ls Set\ Slice debbugs-gnu-log-edit-mode mu4e-kill-update-mail Reduce\ To:\ to\ Cc: mu4e-view-header-field-keymap c-electric-lt-gt :neither yas-x-prompt cc-vars rainbow-colorize-ansi znc magit-xref-insert-button inactive gnus-agent-group-covered-p web-mode-extra-snippets smiley-directory message-expand-name-databases transient:magit-diff-refresh:--irreversible-delete er/looking-back-max tree-widget-button-keymap rust--same-line-p gnus-summary-show-article-from-menu-as-charset-ebcdic-int :where org-switch-to-buffer-other-window nexus th/visual-line-mode-init :mac xref--push-markers vc-bzr-shelve-menu display-even-when-displayed widget-face-sample-face-get time-subsecond gnus-summary-limit-to-marks he-line-search-regexp symbol-before-point-for-complete magit-filename utf7-direct-encoding-chars generic--normalize-comments org-get-org-file check-declare xref-group :org-comment stack-mode Boring\ headers ...] "mag" #f(compiled-function (sym) #<bytecode -0x190dc04a7f35bb9f>))
  #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46>("mag" #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>) nil)
  completion-basic-try-completion("mag" #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46> #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>) 3)
  #f(compiled-function (style) #<bytecode 0xaee4d3d0651ffdc>)(basic)
  completion--some(#f(compiled-function (style) #<bytecode 0xaee4d3d0651ffdc>) (basic partial-completion substring initials flex))
  completion--nth-completion(1 "mag" #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46> #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>) 3 (metadata (affixation-function . read-extended-command--affixation) (category . command)))
  completion-try-completion("mag" #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46> #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>) 3 (metadata (affixation-function . read-extended-command--affixation) (category . command)))
  completion--do-completion(5 8)
  completion--in-region-1(5 8)
  #f(compiled-function (start end collection predicate) #<bytecode 0x50e15298b25ed54>)(5 8 #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46> #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>))
  apply(#f(compiled-function (start end collection predicate) #<bytecode 0x50e15298b25ed54>) (5 8 #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46> #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>)))
  #f(compiled-function (funs global args) #<bytecode -0xf7a9686f11d622>)(nil nil (5 8 #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46> #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>)))
  completion--in-region(5 8 #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46> #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>))
  completion-in-region(5 8 #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46> #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>))
  minibuffer-complete()
  funcall-interactively(minibuffer-complete)
  command-execute(minibuffer-complete)
  completing-read-default("M-x " #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_46> #f(compiled-function (sym) #<bytecode 0x4d1b943a9e2c352>) t nil extended-command-history nil nil)
  read-extended-command()
  byte-code("\302\30\11\303 \10E)\207" [execute-extended-command--last-typed current-prefix-arg nil read-extended-command] 3)
  command-execute(execute-extended-command)
--8<---------------cut here---------------end--------------->8---



In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.29, cairo version 1.17.4)
 of 2021-05-13 built on thinkpad-t440p
Repository revision: 43701a84367b76ccc93ad46f89110486988eec10
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101001
System Description: Arch Linux

Configured using:
 'configure --with-modules --with-native-compilation'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSYSTEMD LIBXML2 M17N_FLT MODULES NATIVE_COMP NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XDBE XIM XPM 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:
  global-aggressive-indent-mode: t
  dynamic-completion-mode: t
  which-key-mode: t
  company-posframe-mode: t
  global-company-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  override-global-mode: t
  minibuffer-depth-indicate-mode: t
  recentf-mode: t
  pixel-scroll-mode: t
  save-place-mode: t
  savehist-mode: t
  show-paren-mode: t
  shell-dirtrack-mode: t
  tooltip-mode: t
  global-eldoc-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
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-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-utils hides ~/Repos/el/mu/mu4e/mu4e-utils
~/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-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-view-common hides ~/Repos/el/mu/mu4e/mu4e-view-common
~/Repos/el/mu/build/mu4e/mu4e-view-old hides ~/Repos/el/mu/mu4e/mu4e-view-old
~/Repos/el/mu/build/mu4e/mu4e-view-gnus hides ~/Repos/el/mu/mu4e/mu4e-view-gnus
~/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-icalendar hides ~/Repos/el/mu/mu4e/mu4e-icalendar
~/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-proc hides ~/Repos/el/mu/mu4e/mu4e-proc
~/Repos/el/mu/build/mu4e/mu4e-meta hides ~/Repos/el/mu/mu4e/mu4e-meta
~/Repos/el/mu/build/mu4e/mu4e-vars hides ~/Repos/el/mu/mu4e/mu4e-vars
/home/horn/.emacs.d/elpa/transient-20210427.833/transient hides /home/horn/Repos/el/emacs/lisp/transient

Features:
(shadow hippie-exp emacsbug debbugs-gnu qp sort gnus-cite mail-extr
goto-addr magit-extras cursor-sensor face-remap paredit vc-mtn vc-hg
vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs display-fill-column-indicator
company-oddmuse company-keywords company-etags etags fileloop generator
xref project company-gtags company-dabbrev-code company-dabbrev
company-files company-clang company-capf company-cmake company-semantic
company-template company-bbdb cus-edit pp cus-start cus-load
auto-package-update 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 deadgrep spinner
hl-todo aggressive-indent rainbow-mode vc-git vc-dir ewoc vc
vc-dispatcher epa-file dired-x mu4e-alert time ht s mu4e-icalendar
gnus-icalendar org-capture org-refile icalendar diary-lib diary-loaddefs
mu4e mu4e-org mu4e-main mu4e-view mu4e-view-gnus mu4e-view-common
mu4e-headers mu4e-compose mu4e-context mu4e-draft mu4e-actions ido
rfc2368 mu4e-mark mu4e-proc mu4e-utils doc-view jka-compr image-mode
exif mu4e-lists mu4e-message flow-fill 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 ol org-keys org-compat org-macs org-loaddefs cal-menu
calendar cal-loaddefs mule-util hl-line mu4e-vars mu4e-meta
smtpmail-multi smtpmail sendmail ecomplete completion auto-dictionary
flyspell ispell tramp-smb which-key highlight-parentheses
company-restclient know-your-http-well http-status-codes http-relations
http-methods http-headers restclient company-posframe posframe company
pcase yasnippet compile find-func autoload radix-tree lisp-mnt
finder-inf mm-archive network-stream url-cache 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
gnutls forge-notify forge-revnote forge-pullreq forge-issue forge-topic
forge-post markdown-mode color thingatpt noutline outline forge-repo
forge forge-core forge-db closql emacsql-sqlite advice 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 transient comp comp-cstr magit-git magit-section magit-utils
crm dash visual-filename-abbrev debbugs soap-client url-http url-auth
url-gw nsm warnings rng-xsd rng-dt rng-util xsd-regexp bug-reference
use-package-bind-key bind-key easy-mmode vertico aggressive-completion
icomplete mb-depth use-package-diminish windmove alert log4e
notifications gntp rx tramp-cache tramp-sh recentf tree-widget
pixel-scroll saveplace savehist paren smiley gnus-art mm-uu mml2015
mm-view mml-smime smime dig gnus-sum shr kinsoku 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 rmc
puny dired dired-loaddefs rfc822 mml mml-sec epa derived epg epg-config
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader gnus-win gnus wid-edit nnheader gnus-util rmail
rmail-loaddefs rfc2047 rfc2045 ietf-drums text-property-search mm-util
mail-prsvr mail-utils edmacro kmacro dracula-theme diminish cl-extra
help-mode use-package-ensure use-package-core tramp tramp-loaddefs
trampver tramp-integration files-x tramp-compat shell pcomplete comint
ansi-color ring parse-time iso8601 time-date ls-lisp format-spec 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 subr-x map url-vars seq byte-opt gv bytecomp
byte-compile cconv cl-loaddefs cl-lib iso-transl tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-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 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 charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 720190 82629)
 (symbols 48 47153 5)
 (strings 32 211165 12252)
 (string-bytes 1 6287106)
 (vectors 16 84612)
 (vector-slots 8 1466836 93450)
 (floats 8 635 676)
 (intervals 56 4752 453)
 (buffers 992 30))

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Date: Thu, 13 May 2021 22:45:01 +0200
> Cc: Stefan Kangas <stefan@marxist.se>
> 
> Since commit 1af91d271e077134e272055407fb8c4312a7579b I get strange
> "Invalid version syntax: <package> <version> (must start with a number)"
> errors during M-x completion in the minibuffer.  I can't reproduce with
> emacs -Q but in the error, <package> is always some (M)ELPA package.
> 
> For example, M-x mag TAB gives
> 
>   completion--some: Invalid version syntax: ‘Magit 3.0.0’ (must start with a number)

That commit uses byte-obsolete-info property of a symbol, and expects
its caddr to be a valid version number.  It sounds like in some cases
it is not an Emacs version, but something else.  So, questions:

  . can you find out what kind of value does the byte-obsolete-info
    property is there on Magit functions? and
  . how come these properties were added to something that is not an
    Emacs core code?

In any case, it sounds like blindly trusting the value of that
property is not a good idea, and we should wrap the call to
version-to-list there in condition-case.  Could you try that?

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> For example, M-x mag TAB gives
>> 
>>   completion--some: Invalid version syntax: ‘Magit 3.0.0’ (must start with a number)
>
> That commit uses byte-obsolete-info property of a symbol, and expects
> its caddr to be a valid version number.  It sounds like in some cases
> it is not an Emacs version, but something else.  So, questions:
>
>   . can you find out what kind of value does the byte-obsolete-info
>     property is there on Magit functions? and

It's not just Magit.  I've grepped my ~/.emacs.d/elpa/ and here are some
values:

  (make-obsolete-variable 'magit-refresh-args nil "Magit 3.0.0")
  (make-obsolete-variable backported sym "yasnippet 0.8")
  (make-obsolete-variable 'which-key-key-replacement-alist
                          'which-key-replacement-alist "2016-11-21")

So at least magit and yasnippet use "<package> <version>" syntax here,
and which-key uses a date (which seems fine for version-to-list).

>   . how come these properties were added to something that is not an
>     Emacs core code?

It seems to be added by `make-obsolete', no?  Otherwise, there is no
writing menion of byte-obsolete-info.

> In any case, it sounds like blindly trusting the value of that
> property is not a good idea, and we should wrap the call to
> version-to-list there in condition-case.  Could you try that?

I've tried this

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/simple.el b/lisp/simple.el
index 0255f69e42..573e344fea 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2020,8 +2020,10 @@ read-extended-command
                                     ;; Has a current-name.
                                     (functionp (car obsolete))
                                     ;; when >= emacs-major-version
-                                    (>= (car (version-to-list (caddr obsolete)))
-                                        emacs-major-version))))))
+                                    (condition-case nil
+                                        (>= (car (version-to-list (caddr obsolete)))
+                                            emacs-major-version)
+                                      (t nil)))))))
                     pred)))
              (complete-with-action action obarray string pred))))
--8<---------------cut here---------------end--------------->8---

which does the trick.

Should I write bug reports for Magit and Yasnippets so that they should
only specify the version number in `make-obsolete' /
`make-obsolete-variable'?  Or should emacs support the "<package>
<version>" syntax?

Bye,
Tassilo

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: 48404@debbugs.gnu.org, stefan@marxist.se
> Date: Fri, 14 May 2021 08:47:32 +0200
> 
> > In any case, it sounds like blindly trusting the value of that
> > property is not a good idea, and we should wrap the call to
> > version-to-list there in condition-case.  Could you try that?
> 
> I've tried this
> 
> --8<---------------cut here---------------start------------->8---
> diff --git a/lisp/simple.el b/lisp/simple.el
> index 0255f69e42..573e344fea 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -2020,8 +2020,10 @@ read-extended-command
>                                      ;; Has a current-name.
>                                      (functionp (car obsolete))
>                                      ;; when >= emacs-major-version
> -                                    (>= (car (version-to-list (caddr obsolete)))
> -                                        emacs-major-version))))))
> +                                    (condition-case nil
> +                                        (>= (car (version-to-list (caddr obsolete)))
> +                                            emacs-major-version)
> +                                      (t nil)))))))
>                      pred)))
>               (complete-with-action action obarray string pred))))
> --8<---------------cut here---------------end--------------->8---
> 
> which does the trick.

Thanks, I think this should be installed unless someone comes up with
a better idea (but see below).

> Should I write bug reports for Magit and Yasnippets so that they should
> only specify the version number in `make-obsolete' /
> `make-obsolete-variable'?  Or should emacs support the "<package>
> <version>" syntax?

I don't think we will support the deviant syntax in version-to-list,
no.

But I don't really understand why we compare versions in the snippet
above -- the command is already established as being obsolete, so
we've got to be running an Emacs version where it is indeed obsolete,
and the version text sounds redundant to me.  Stefan?

If we remove the version check, the problem with 3rd-party packages
will be automatically resolved.  Otherwise, I don't think we can solve
this except by ignoring those non-Emacs versions, because there's no
way for us to know whether, e.g., "Magit 3.0.0" is older or newer that
Emacs whose version is emacs-major-version.  IOW, if we insist on
checking the version, AFAIU this feature cannot possibly work for
obsolete commands from 3rd-party packages, they will always be
included in completion lists.

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Eli Zaretskii]

> Date: Fri, 14 May 2021 10:18:40 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 48404@debbugs.gnu.org, stefan@marxist.se
> 
> But I don't really understand why we compare versions in the snippet
> above -- the command is already established as being obsolete, so
> we've got to be running an Emacs version where it is indeed obsolete,
> and the version text sounds redundant to me.  Stefan?
                  ^^^^
Should be "test", sorry.  My fingers sometimes think they know better...

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Lars Ingebrigtsen]

Tassilo Horn <tsdh@gnu.org> writes:

> I've tried this

[...]

> -                                    (>= (car (version-to-list (caddr obsolete)))
> -                                        emacs-major-version))))))
> +                                    (condition-case nil
> +                                        (>= (car (version-to-list (caddr obsolete)))
> +                                            emacs-major-version)
> +                                      (t nil)))))))

Yes, I think this is the right solution.  (Or just `ignore-errors'.)

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

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> But I don't really understand why we compare versions in the snippet
> above -- the command is already established as being obsolete, so
> we've got to be running an Emacs version where it is indeed obsolete,
> and the version text sounds redundant to me.  Stefan?

The idea is to have a softer transition when obsoleting commands -- `M-x
some-obsoleteTAB' will list the command if it's been "recently"
obsoleted, but also list the new command name (in parentheses).  This
will hopefully help people to learn the new command name.

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

bug#48404:

[Daniel Mendler]

If the check does not work reliably for third-party packages, it should
probably better reverted altogether. Why slap an ignore-errors around
the broken code now?

According to the commit message, which introduced this bug, there has
been some alternative solution, which clearly sounds better: Either omit
obsolete commands altogether or annotate them as obsolete. Omitting is
probably too problematic since people may rely on running obsolete commands.

What about making the behavior around obsolete commands a customizable
option? 1. omit obsolete commands, 2. annotate obsolete commands, 3. do
nothing.

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Tassilo Horn <tsdh@gnu.org>,  48404@debbugs.gnu.org,  stefan@marxist.se
> Date: Fri, 14 May 2021 09:43:37 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But I don't really understand why we compare versions in the snippet
> > above -- the command is already established as being obsolete, so
> > we've got to be running an Emacs version where it is indeed obsolete,
> > and the version text sounds redundant to me.  Stefan?
> 
> The idea is to have a softer transition when obsoleting commands -- `M-x
> some-obsoleteTAB' will list the command if it's been "recently"
> obsoleted, but also list the new command name (in parentheses).  This
> will hopefully help people to learn the new command name.

But this comes with a heavy price: commands from any package that is
not bundled with Emacs will not be able to take advantage of this
feature, ever.  Is it worth punishing those packages' users to have a
softer transition?  Given our conservative approach to obsoleting
stuff, I'm not sure.

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> > Since commit 1af91d271e077134e272055407fb8c4312a7579b I get strange
> > "Invalid version syntax: <package> <version> (must start with a number)"
> > errors during M-x completion in the minibuffer.  I can't reproduce with
> > emacs -Q but in the error, <package> is always some (M)ELPA package.
> >
> > For example, M-x mag TAB gives
> >
> >   completion--some: Invalid version syntax: ‘Magit 3.0.0’ (must start with a number)
>
> That commit uses byte-obsolete-info property of a symbol, and expects
> its caddr to be a valid version number.  It sounds like in some cases
> it is not an Emacs version, but something else.  So, questions:
>
>   . can you find out what kind of value does the byte-obsolete-info
>     property is there on Magit functions? and
>   . how come these properties were added to something that is not an
>     Emacs core code?
>
> In any case, it sounds like blindly trusting the value of that
> property is not a good idea, and we should wrap the call to
> version-to-list there in condition-case.  Could you try that?

The timing for Lars pushing my patch was a bit unfortunate, as I'm
currently away traveling and won't have any time to look at this in
the next week or two at least.  I unfortunately didn't say so in the
relevant bug thread, but I hadn't yet convinced myself that the patch
was the correct one, and had the intention of returning to it for a
closer look before pushing.

If any immediate issues can be resolved, this is on my todo list and I
will take a look at this when I'm back home.  It would also be fine to
just revert my patch for now and re-open the bug report.

Thanks.

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> If any immediate issues can be resolved, this is on my todo list and I
> will take a look at this when I'm back home.  It would also be fine to
> just revert my patch for now and re-open the bug report.

No prob -- I've pushed a fix that should stop `M-x' bugging out on
invalid versions strings now.

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

bug#48404:

[Daniel Mendler]

Why not revert the broken M-x predicate until a better solution is
found? For example as I mentioned adding an option which allows to
customize the treatment of obsolete commands (hide, annotate, keep).

It looks like the solution we got now is suboptimal: 1. The version
check does not work for third-party packages, it does not even work for
packages which are part of Emacs (so-long). 2. Since the version check
is broken, an `ignore-errors` wrapper is needed. 3. The behavior is not
customizable, some users may prefer to hide obsolete commands
altogether, some would like to see an annotation and some would like to
keep the obsolete commands in M-x.

Daniel

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> The idea is to have a softer transition when obsoleting commands -- `M-x
>> some-obsoleteTAB' will list the command if it's been "recently"
>> obsoleted, but also list the new command name (in parentheses).  This
>> will hopefully help people to learn the new command name.
>
> But this comes with a heavy price: commands from any package that is
> not bundled with Emacs will not be able to take advantage of this
> feature, ever.  Is it worth punishing those packages' users to have a
> softer transition?  Given our conservative approach to obsoleting
> stuff, I'm not sure.

I'm not quite sure I understand you here?  If the version can't be
parsed, then the command will still show up in `M-x TAB', which means
that the obsoletion of these commands is very soft indeed.

But I'm wondering whether `version-to-list' should be more lax here.
That is, currently it'll barf of things like:

(version-to-list "28.1 Magit/2.5")

It might make sense to allow the obsoletion versions to refer to both an
(approximate) Emacs version, as well as a package version.

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

bug#48404:

[Lars Ingebrigtsen]

Daniel Mendler <mail@daniel-mendler.de> writes:

> 3. The behavior is not customizable, some users may prefer to hide
> obsolete commands altogether, some would like to see an annotation and
> some would like to keep the obsolete commands in M-x.

Yes, it might make sense to allow `read-extended-command-predicate' to
be a list of predicates, and this obsoletion predicate might be in that
list by default.

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

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: tsdh@gnu.org,  48404@debbugs.gnu.org,  stefan@marxist.se
> Date: Sun, 16 May 2021 16:06:03 +0200
> 
> > But this comes with a heavy price: commands from any package that is
> > not bundled with Emacs will not be able to take advantage of this
> > feature, ever.  Is it worth punishing those packages' users to have a
> > softer transition?  Given our conservative approach to obsoleting
> > stuff, I'm not sure.
> 
> I'm not quite sure I understand you here?  If the version can't be
> parsed, then the command will still show up in `M-x TAB', which means
> that the obsoletion of these commands is very soft indeed.

For obsolete commands from 3rd-party packages, which state something
like "Magit 3.0" in the version since which they are obsolete, the
obsolescence will never happen, in the sense that they will _always_
appear in "M-x TAB", even 100 years from now.  Is that what we want?

> But I'm wondering whether `version-to-list' should be more lax here.
> That is, currently it'll barf of things like:
> 
> (version-to-list "28.1 Magit/2.5")

I'm not sure it's possible without introducing ambiguity into the
version string and complicating comparison of versions.  We already
support some non-numeric versions, and that's not easy.

> It might make sense to allow the obsoletion versions to refer to both an
> (approximate) Emacs version, as well as a package version.

How can this be done, even in principle?  Versions of unbundled
packages are unrelated to Emacs versions; typically, a given version
of an unbundled package supports quite a few Emacs versions.  So what
Emacs version will you put there?

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> For obsolete commands from 3rd-party packages, which state something
> like "Magit 3.0" in the version since which they are obsolete, the
> obsolescence will never happen, in the sense that they will _always_
> appear in "M-x TAB", even 100 years from now.  Is that what we want?

That is what was the case before applying the patch, so it's no change
for those commands.

>> But I'm wondering whether `version-to-list' should be more lax here.
>> That is, currently it'll barf of things like:
>> 
>> (version-to-list "28.1 Magit/2.5")
>
> I'm not sure it's possible without introducing ambiguity into the
> version string and complicating comparison of versions.  We already
> support some non-numeric versions, and that's not easy.

Sorry, I was imprecise here -- I didn't mean that we should change
`version-to-list' itself here, but add a new function that's more
permissive, just for use in this context.  It would basically be
something along the lines of (version-to-list (car (split-string string))) 

>> It might make sense to allow the obsoletion versions to refer to both an
>> (approximate) Emacs version, as well as a package version.
>
> How can this be done, even in principle?  Versions of unbundled
> packages are unrelated to Emacs versions; typically, a given version
> of an unbundled package supports quite a few Emacs versions.  So what
> Emacs version will you put there?

That would be up to the package maintainers -- they get to choose what
Emacs version(s) the obsolete command would show up in `M-x TAB'
completions.  

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

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: tsdh@gnu.org,  48404@debbugs.gnu.org,  stefan@marxist.se
> Date: Sun, 16 May 2021 16:38:58 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > For obsolete commands from 3rd-party packages, which state something
> > like "Magit 3.0" in the version since which they are obsolete, the
> > obsolescence will never happen, in the sense that they will _always_
> > appear in "M-x TAB", even 100 years from now.  Is that what we want?
> 
> That is what was the case before applying the patch, so it's no change
> for those commands.

No change for the worse, sure.  That's what I meant when I said they
will not be able to take advantage of this feature.  They will be
excluded.

> >> (version-to-list "28.1 Magit/2.5")
> >
> > I'm not sure it's possible without introducing ambiguity into the
> > version string and complicating comparison of versions.  We already
> > support some non-numeric versions, and that's not easy.
> 
> Sorry, I was imprecise here -- I didn't mean that we should change
> `version-to-list' itself here, but add a new function that's more
> permissive, just for use in this context.  It would basically be
> something along the lines of (version-to-list (car (split-string string))) 

The main purpose is to compare versions.  How would you do that with
loose version strings such as the one above?

> >> It might make sense to allow the obsoletion versions to refer to both an
> >> (approximate) Emacs version, as well as a package version.
> >
> > How can this be done, even in principle?  Versions of unbundled
> > packages are unrelated to Emacs versions; typically, a given version
> > of an unbundled package supports quite a few Emacs versions.  So what
> > Emacs version will you put there?
> 
> That would be up to the package maintainers -- they get to choose what
> Emacs version(s) the obsolete command would show up in `M-x TAB'
> completions.  

I don't think they will be able to solve this problem, not in
satisfactory ways anyway.

bug#48404:

[Daniel Mendler]

On 5/16/21 4:09 PM, Lars Ingebrigtsen wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
> 
>> 3. The behavior is not customizable, some users may prefer to hide
>> obsolete commands altogether, some would like to see an annotation and
>> some would like to keep the obsolete commands in M-x.
> 
> Yes, it might make sense to allow `read-extended-command-predicate' to
> be a list of predicates, and this obsoletion predicate might be in that
> list by default.

This sounds like a good resolution of the issue. Thanks!

Daniel

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> >> (version-to-list "28.1 Magit/2.5")
>> >
>> > I'm not sure it's possible without introducing ambiguity into the
>> > version string and complicating comparison of versions.  We already
>> > support some non-numeric versions, and that's not easy.
>> 
>> Sorry, I was imprecise here -- I didn't mean that we should change
>> `version-to-list' itself here, but add a new function that's more
>> permissive, just for use in this context.  It would basically be
>> something along the lines of (version-to-list (car (split-string string))) 
>
> The main purpose is to compare versions.  How would you do that with
> loose version strings such as the one above?

We're looking at the Emacs version (which we'd put first in the version
string, while the rest is essentially a free text used as a comment).

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

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: tsdh@gnu.org,  48404@debbugs.gnu.org,  stefan@marxist.se
> Date: Mon, 17 May 2021 15:54:43 +0200
> 
> >> >> (version-to-list "28.1 Magit/2.5")
> >> >
> >
> > The main purpose is to compare versions.  How would you do that with
> > loose version strings such as the one above?
> 
> We're looking at the Emacs version (which we'd put first in the version
> string, while the rest is essentially a free text used as a comment).

What do you mean by "first"?  We already support the likes of
"28.3 Beta3", and "Beta3" is not ignored there as some free-text
comment.

And that is assuming the developers of 3rd-party packages can have
some reasonable way of associating their versions with Emacs versions,
which I still think is a problem with no solution.

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> What do you mean by "first"?  We already support the likes of
> "28.3 Beta3", and "Beta3" is not ignored there as some free-text
> comment.

For this purpose, it doesn't matter -- we're only interested in the
major version.  But if you want to expand the language in a more regular
way, then a different separating character (between the part we parse
and the one that's free text) can be used, of course.  Semicolon?

> And that is assuming the developers of 3rd-party packages can have
> some reasonable way of associating their versions with Emacs versions,
> which I still think is a problem with no solution.

They don't really need to.  They decide "we think people with Emacs 25
shouldn't have this in their M-x TAX" and then put "26.1;Magit/1.2" in
the string.

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

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: tsdh@gnu.org,  48404@debbugs.gnu.org,  stefan@marxist.se
> Date: Mon, 17 May 2021 16:20:25 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What do you mean by "first"?  We already support the likes of
> > "28.3 Beta3", and "Beta3" is not ignored there as some free-text
> > comment.
> 
> For this purpose, it doesn't matter -- we're only interested in the
> major version.  But if you want to expand the language in a more regular
> way, then a different separating character (between the part we parse
> and the one that's free text) can be used, of course.  Semicolon?

Maybe, I don't know.

My point is that simply ignoring something after the first space is
not a good idea.

> > And that is assuming the developers of 3rd-party packages can have
> > some reasonable way of associating their versions with Emacs versions,
> > which I still think is a problem with no solution.
> 
> They don't really need to.  They decide "we think people with Emacs 25
> shouldn't have this in their M-x TAX" and then put "26.1;Magit/1.2" in
> the string.

We need to talk to them to get their agreement, I think.  We cannot
decide for them that this is what they should do.

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> They don't really need to.  They decide "we think people with Emacs 25
>> shouldn't have this in their M-x TAX" and then put "26.1;Magit/1.2" in
>> the string.
>
> We need to talk to them to get their agreement, I think.  We cannot
> decide for them that this is what they should do.

I think we just have to establish a convention that external packages
could use.  Whether they then want to use it is up to them.  That is, if
they don't want to, then Emacs' behaviour with regards to obsoleted
commands won't change -- but if they want to see this new behaviour,
they'll need to reformat their obsoletion strings.

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

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Tassilo Horn]

>> And that is assuming the developers of 3rd-party packages can have
>> some reasonable way of associating their versions with Emacs
>> versions, which I still think is a problem with no solution.
>
> They don't really need to.  They decide "we think people with Emacs 25
> shouldn't have this in their M-x TAX" and then put "26.1;Magit/1.2" in
> the string.

FWIW, as a 3rd-party package maintainer, I've never have the need to
define that some commands should not show up in completion with a
specific emacs version.  In auctex, we deal with emacs versions by just
requiring a minimal version (which gets bumpt very conservatively), and
that's about it.  But of course, YMMV.

In auctex, all our obsoletion versions are just version numbers "11.97"
like and represent the auctex version and our bug reports show that old
versions are still used much more often as one would expect.  So as long
as those "normal" versions don't cause the package version be compared
to the emacs version and just recently deprecated commands vanish from
completion, I'm neutral.

Bye,
Tassilo

bug#48404: 28.0.50; "Invalid version syntax" errors with read-extended-command

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, 48404@debbugs.gnu.org, stefan@marxist.se
> Date: Wed, 19 May 2021 13:54:22 +0200
> 
> In auctex, all our obsoletion versions are just version numbers "11.97"
> like and represent the auctex version and our bug reports show that old
> versions are still used much more often as one would expect.  So as long
> as those "normal" versions don't cause the package version be compared
> to the emacs version and just recently deprecated commands vanish from
> completion, I'm neutral.

AFAIU, they do get compared with Emacs version numbers.