Package: auctex;
Reported by: "Paul D. Nelson" <ultrono <at> gmail.com>
Date: Thu, 27 Feb 2025 18:31:02 UTC
Severity: normal
Found in version 14.0.9
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: "Paul D. Nelson" <ultrono <at> gmail.com> Cc: ikumi <at> ikumi.que.jp, arash <at> gnu.org, 76615 <at> debbugs.gnu.org Subject: bug#76615: 14.0.9; error with amsmath style hooks Date: Thu, 03 Apr 2025 09:50:04 -0400
> Also, if we're going to proceed with this, then I'd like to take the
> opportunity to add support for indirect buffers to RefTeX, which
> requires modifications to the same parts of the code as this patch.
I see you use `buffer-base-buffer` in the new patch. Is that all it
takes or does is there more in the waiting?
The patch looks fine to me. Can you push it or do you need me to do it?
While waiting for Arash's answer, I found a few more nitpicks.
Stefan
> + (let* ((master (reftex-TeX-master-file)))
> + (when (bufferp master)
> + (user-error "RefTeX phrases buffer requires a file buffer. Cannot create phrases for non-file buffers"))
This error message is too long. I'd drop one of the two sentences.
> + (true-master (reftex-get-truename-for-master master))
> + (master-dir (file-name-as-directory (reftex-get-directory-for-master master)))
> + (file (or file (reftex-get-buffer-identifier)))
> + (true-file (if file (file-truename file) (buffer-name)))
This overflows the 80 columns limit, as do many other places in your patch.
> +;;; =========================================================================
> +;;;
> +;;; Helper functions for handling both file paths and buffer objects
> +;;;
In GNU land, we call these file *names* not paths.
[ And please punctuate your comments. ]
Regarding the functions defined in this section, please use a "reftex--"
prefix to mark those functions as internal to RefTeX.
> +(defun reftex-get-buffer-identifier (&optional buffer)
> + "Return the base buffer's file name or buffer identifier.
> +For file buffers, returns the file name of the base buffer.
> +For non-file buffers, return the base buffer object itself.
> +When BUFFER is nil, use the current buffer."
> + (with-current-buffer (or (buffer-base-buffer buffer) buffer (current-buffer))
> + (or (buffer-file-name) (current-buffer))))
`with-current-buffer` is not super-expensive but it's not really cheap
either and it's significantly more expensive than `buffer-local-value`
or `buffer-file-name`, so better use one of those when you can, like here.
> +(defun reftex-abbreviate-or-get-name (master)
> + "Get a nice display name for MASTER.
> +For files, returns the abbreviated file name.
> +For buffers, returns the buffer name."
> + (if (bufferp master)
> + (prin1-to-string master)
> + (abbreviate-file-name master)))
Personally, I'd drop the "or-get" from the name.
> - (push (cons master index) reftex-master-index-list))
> + (push (cons master index) reftex-master-index-list)
> + (when (bufferp master)
> + (add-hook
> + 'kill-buffer-hook
> + (lambda ()
> + (setq reftex-master-index-list
> + (assq-delete-all master reftex-master-index-list))))))
Please avoid adding lambda expressions to hooks, better give a name to
the function and pass the function's name here: comparing function
names is easy, comparing functions is hard.
Also, I think you forgot to pass the `local` arg to `add-hook`.
> - (file (if (string-match "\\.[a-zA-Z]+\\'" master)
> - (concat (substring master 0 (match-beginning 0))
> - reftex-parse-file-extension)
> - (concat master reftex-parse-file-extension))))
> + (is-buffer (bufferp master))
> + (file (if is-buffer
> + nil
> + (if (string-match "\\.[a-zA-Z]+\\'" master)
> + (concat (substring master 0 (match-beginning 0))
> + reftex-parse-file-extension)
> + (concat master reftex-parse-file-extension)))))
Maybe call it `non-file` rather than `is-buffer`, since it seems to
better match the way we think about it (at least, based on the text used
in docstrings and comments 🙂).
> + (cond
> + (is-buffer
> + (error "Cannot restore for non-file buffer"))
Can't this be moved to the first branch of the surrounding `cond`,
along with readable, read, and kill?
> ((eq action 'read)
> (put reftex-docstruct-symbol 'modified nil)
> - (if (file-exists-p file)
[...]
> - nil))
> + (cond
> + (is-buffer nil)
I think we can't reach this branch because we handled that case earlier.
> ((eq action 'kill)
> ;; Remove the file
> - (when (and (file-exists-p file) (file-writable-p file))
> + (when (and file (file-exists-p file) (file-writable-p file))
> (message "Unlinking file %s" file)
> (delete-file file)))
Why?
> - (t
> +
> + (t ; write
> (put docstruct-symbol 'modified nil)
[...]
> + (if is-buffer
> + t
Is the `put` important in this case? If not, then this can also be
moved to the the first branch of the surrounding `cond`, so that this
first branch handles all the `is-buffer` (aka `non-file`) cases, and the
rest doesn't need to be touched at all.
> @@ -1240,10 +1319,13 @@ reftex-check-parse-consistency
> (let* ((real-master (reftex-TeX-master-file))
> (parsed-master
> (nth 1 (assq 'bof (symbol-value reftex-docstruct-symbol)))))
> - (unless (string= (file-truename real-master) (file-truename parsed-master))
> - (message "Master file name in load file is different: %s versus %s"
> - parsed-master real-master)
> - (error "Master file name error")))
> + ;; Skip this check for buffer objects since they don't need saved parse info
> + (when (and (stringp real-master) (stringp parsed-master))
> + (unless (string= (file-truename real-master)
> + (file-truename parsed-master))
> + (message "Master file name in load file is different: %s versus %s"
> + parsed-master real-master)
> + (error "Master file name error"))))
Beside the lack of punctuation in the comment, it leaves me wondering
about why we don't do
(unless (equal (reftex-get-truename-for-file real-master)
(reftex-get-truename-for-file parsed-master))
(message "Master file name in load file is different: %s versus %s"
parsed-master real-master)
(error "Master file name error")))
instead.
> + (cond
> + ((bufferp file) file)
> +
> + ;; Normal file path handling
> + ((stringp file)
> + (let ((buf (find-buffer-visiting file)))
> + (cond (buf
> + ;; We have it already as a buffer - just return it
> + buf)
> +
> + ((file-readable-p file)
> + ;; At least there is such a file and we can read it.
> +
> + (if (or (not mark-to-kill)
> + (eq t reftex-initialize-temporary-buffers))
> +
> + ;; Visit the file with full magic
> + (setq buf (find-file-noselect file))
> +
> + ;; Else: Visit the file just briefly, without or
> + ;; with limited Magic
> +
> + ;; The magic goes away
> + (cl-letf ((format-alist nil)
> + (auto-mode-alist (reftex-auto-mode-alist))
> + ((default-value 'major-mode) 'fundamental-mode)
> + (enable-local-variables nil)
> + (after-insert-file-functions nil))
> + (setq buf (find-file-noselect file)))
> +
> + ;; Is there a hook to run?
> + (when (listp reftex-initialize-temporary-buffers)
> + (with-current-buffer buf
> + (run-hooks 'reftex-initialize-temporary-buffers))))
> +
> + ;; Let's see if we got a license to kill :-|
> + (and mark-to-kill
> + (cl-pushnew buf reftex-buffers-to-kill))
> +
> + ;; Return the new buffer
> + buf)
> +
> + ;; If no such file exists, return nil
> + (t nil))))
> + ;;
> + (t nil)))
Why test (stringp file) here?
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.