* bug#21262: 25.0.50; Diff-mode gets confused about its narrowing if hunk source not found
@ 2015-08-15 5:12 Dima Kogan
2017-08-21 19:49 ` Charles A. Roelli
0 siblings, 1 reply; 5+ messages in thread
From: Dima Kogan @ 2015-08-15 5:12 UTC (permalink / raw)
To: 21262
[-- Attachment #1: Type: text/plain, Size: 672 bytes --]
Hi. This bug has been there since at least emacs23 it seems like. To
reproduce:
1. emacs -Q
2. Open any patch file that has hunks from more than one file in it. For
instance, the attached patch file works, but it really doesn't matter
3. Narrow to just the second file's diffs. For instance: M-N M-N C-SPC
C-> C-x n n SPC (last SPC to confirm the narrowing)
4. Try to see the source of the hunk. Move the point to the guts of the
hunk, and M-enter. Emacs asks for the directory where the sources live.
In the process, it changes the narrowing to the wrong hunk. This is
wrong. The narrowing shouldn't change.
5. If we cancel with C-g, the wrong narrowing persists
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tst2.diff --]
[-- Type: text/x-diff, Size: 1214 bytes --]
diff --git a/src/charset.c b/src/charset.c
index b19e344..eeebf17 100644
--- a/src/charset.c
+++ b/src/charset.c
@@ -555,7 +555,7 @@ load_charset_map_from_vector (struct charset *charset, Lisp_Object vec, int cont
if (len % 2 == 1)
{
- add_to_log ("Failure in loading charset map: %V", vec, Qnil);
+ add_to_log ("Failure in loading charset map: %V", vec);
return;
}
diff --git a/src/image.c b/src/image.c
index 066db74..313419b 100644
--- a/src/image.c
+++ b/src/image.c
@@ -629,16 +629,19 @@ valid_image_p (Lisp_Object object)
}
-/* Log error message with format string FORMAT and argument ARG.
+/* Log error message with format string FORMAT and trailing arguments.
Signaling an error, e.g. when an image cannot be loaded, is not a
good idea because this would interrupt redisplay, and the error
message display would lead to another redisplay. This function
therefore simply displays a message. */
static void
-image_error (const char *format, Lisp_Object arg1, Lisp_Object arg2)
+image_error (const char *format, ...)
{
- add_to_log (format, arg1, arg2);
+ va_list ap;
+ va_start (ap, format);
+ vadd_to_log (format, ap);
+ va_end (ap);
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#21262: 25.0.50; Diff-mode gets confused about its narrowing if hunk source not found
2015-08-15 5:12 bug#21262: 25.0.50; Diff-mode gets confused about its narrowing if hunk source not found Dima Kogan
@ 2017-08-21 19:49 ` Charles A. Roelli
2017-08-22 0:18 ` npostavs
0 siblings, 1 reply; 5+ messages in thread
From: Charles A. Roelli @ 2017-08-21 19:49 UTC (permalink / raw)
To: Dima Kogan; +Cc: 21262
[-- Attachment #1: Type: text/plain, Size: 2394 bytes --]
> From: Dima Kogan <dima@secretsauce.net>
> Date: Fri, 14 Aug 2015 22:12:09 -0700
>
> Hi. This bug has been there since at least emacs23 it seems like. To
> reproduce:
>
> 1. emacs -Q
>
> 2. Open any patch file that has hunks from more than one file in it. For
> instance, the attached patch file works, but it really doesn't matter
>
> 3. Narrow to just the second file's diffs. For instance: M-N M-N C-SPC
> C-> C-x n n SPC (last SPC to confirm the narrowing)
>
> 4. Try to see the source of the hunk. Move the point to the guts of the
> hunk, and M-enter. Emacs asks for the directory where the sources live.
> In the process, it changes the narrowing to the wrong hunk. This is
> wrong. The narrowing shouldn't change.
>
> 5. If we cancel with C-g, the wrong narrowing persists
>
>
>
> diff --git a/src/charset.c b/src/charset.c
> index b19e344..eeebf17 100644
> --- a/src/charset.c
> +++ b/src/charset.c
> @@ -555,7 +555,7 @@ load_charset_map_from_vector (struct charset *charset, Lisp_Object vec, int cont
>
> if (len % 2 == 1)
> {
> - add_to_log ("Failure in loading charset map: %V", vec, Qnil);
> + add_to_log ("Failure in loading charset map: %V", vec);
> return;
> }
>
> diff --git a/src/image.c b/src/image.c
> index 066db74..313419b 100644
> --- a/src/image.c
> +++ b/src/image.c
> @@ -629,16 +629,19 @@ valid_image_p (Lisp_Object object)
> }
>
>
> -/* Log error message with format string FORMAT and argument ARG.
> +/* Log error message with format string FORMAT and trailing arguments.
> Signaling an error, e.g. when an image cannot be loaded, is not a
> good idea because this would interrupt redisplay, and the error
> message display would lead to another redisplay. This function
> therefore simply displays a message. */
>
> static void
> -image_error (const char *format, Lisp_Object arg1, Lisp_Object arg2)
> +image_error (const char *format, ...)
> {
> - add_to_log (format, arg1, arg2);
> + va_list ap;
> + va_start (ap, format);
> + vadd_to_log (format, ap);
> + va_end (ap);
> }
>
>
This still affects 26.0.50.
The attached change fixes it -- but I know nothing of diff-mode, so
better ideas are welcome. Note that the buffer will be widened while
Emacs asks where to look for the sources, but it will return to the
original narrowing after the user has selected a file or hit C-g.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 01-emacs-bug-21262-draft.patch --]
[-- Type: text/x-patch, Size: 4937 bytes --]
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index aa8d778..b470406 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -874,52 +874,54 @@ diff-find-file-name
;; Flush diff-remembered-files-alist if the default-directory is changed.
(set (make-local-variable 'diff-remembered-defdir) default-directory)
(set (make-local-variable 'diff-remembered-files-alist) nil))
- (save-excursion
- (unless (looking-at diff-file-header-re)
- (or (ignore-errors (diff-beginning-of-file))
- (re-search-forward diff-file-header-re nil t)))
- (let ((fs (diff-hunk-file-names old)))
- (if prefix (setq fs (mapcar (lambda (f) (concat prefix f)) fs)))
- (or
- ;; use any previously used preference
- (cdr (assoc fs diff-remembered-files-alist))
- ;; try to be clever and use previous choices as an inspiration
- (cl-dolist (rf diff-remembered-files-alist)
- (let ((newfile (diff-merge-strings (caar rf) (car fs) (cdr rf))))
- (if (and newfile (file-exists-p newfile)) (cl-return newfile))))
- ;; look for each file in turn. If none found, try again but
- ;; ignoring the first level of directory, ...
- (cl-do* ((files fs (delq nil (mapcar 'diff-filename-drop-dir files)))
- (file nil nil))
- ((or (null files)
- (setq file (cl-do* ((files files (cdr files))
- (file (car files) (car files)))
- ;; Use file-regular-p to avoid
- ;; /dev/null, directories, etc.
- ((or (null file) (file-regular-p file))
- file))))
- file))
- ;; <foo>.rej patches implicitly apply to <foo>
- (and (string-match "\\.rej\\'" (or buffer-file-name ""))
- (let ((file (substring buffer-file-name 0 (match-beginning 0))))
- (when (file-exists-p file) file)))
- ;; If we haven't found the file, maybe it's because we haven't paid
- ;; attention to the PCL-CVS hint.
- (and (not prefix)
- (boundp 'cvs-pcl-cvs-dirchange-re)
- (save-excursion
- (re-search-backward cvs-pcl-cvs-dirchange-re nil t))
- (diff-find-file-name old noprompt (match-string 1)))
- ;; if all else fails, ask the user
- (unless noprompt
- (let ((file (expand-file-name (or (car fs) ""))))
- (setq file
- (read-file-name (format "Use file %s: " file)
- (file-name-directory file) file t
- (file-name-nondirectory file)))
- (set (make-local-variable 'diff-remembered-files-alist)
- (cons (cons fs file) diff-remembered-files-alist))
- file))))))
+ (save-restriction
+ (widen)
+ (save-excursion
+ (unless (looking-at diff-file-header-re)
+ (or (ignore-errors (diff-beginning-of-file))
+ (re-search-forward diff-file-header-re nil t)))
+ (let ((fs (diff-hunk-file-names old)))
+ (if prefix (setq fs (mapcar (lambda (f) (concat prefix f)) fs)))
+ (or
+ ;; use any previously used preference
+ (cdr (assoc fs diff-remembered-files-alist))
+ ;; try to be clever and use previous choices as an inspiration
+ (cl-dolist (rf diff-remembered-files-alist)
+ (let ((newfile (diff-merge-strings (caar rf) (car fs) (cdr rf))))
+ (if (and newfile (file-exists-p newfile)) (cl-return newfile))))
+ ;; look for each file in turn. If none found, try again but
+ ;; ignoring the first level of directory, ...
+ (cl-do* ((files fs (delq nil (mapcar 'diff-filename-drop-dir files)))
+ (file nil nil))
+ ((or (null files)
+ (setq file (cl-do* ((files files (cdr files))
+ (file (car files) (car files)))
+ ;; Use file-regular-p to avoid
+ ;; /dev/null, directories, etc.
+ ((or (null file) (file-regular-p file))
+ file))))
+ file))
+ ;; <foo>.rej patches implicitly apply to <foo>
+ (and (string-match "\\.rej\\'" (or buffer-file-name ""))
+ (let ((file (substring buffer-file-name 0 (match-beginning 0))))
+ (when (file-exists-p file) file)))
+ ;; If we haven't found the file, maybe it's because we haven't paid
+ ;; attention to the PCL-CVS hint.
+ (and (not prefix)
+ (boundp 'cvs-pcl-cvs-dirchange-re)
+ (save-excursion
+ (re-search-backward cvs-pcl-cvs-dirchange-re nil t))
+ (diff-find-file-name old noprompt (match-string 1)))
+ ;; if all else fails, ask the user
+ (unless noprompt
+ (let ((file (expand-file-name (or (car fs) ""))))
+ (setq file
+ (read-file-name (format "Use file %s: " file)
+ (file-name-directory file) file t
+ (file-name-nondirectory file)))
+ (set (make-local-variable 'diff-remembered-files-alist)
+ (cons (cons fs file) diff-remembered-files-alist))
+ file)))))))
(defun diff-ediff-patch ()
^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#21262: 25.0.50; Diff-mode gets confused about its narrowing if hunk source not found
2017-08-21 19:49 ` Charles A. Roelli
@ 2017-08-22 0:18 ` npostavs
2017-08-22 14:02 ` Charles A. Roelli
0 siblings, 1 reply; 5+ messages in thread
From: npostavs @ 2017-08-22 0:18 UTC (permalink / raw)
To: Charles A. Roelli; +Cc: 21262, Dima Kogan
charles@aurox.ch (Charles A. Roelli) writes:
> This still affects 26.0.50.
>
> The attached change fixes it -- but I know nothing of diff-mode, so
> better ideas are welcome. Note that the buffer will be widened while
> Emacs asks where to look for the sources, but it will return to the
> original narrowing after the user has selected a file or hit C-g.
Looks like a reasonable fix to me (though I know only a little bit of
diff-mode). Here it is again ignoring whitespace, which I think makes
it easier to see what is happening:
modified lisp/vc/diff-mode.el
@@ -874,6 +874,8 @@ diff-find-file-name
;; Flush diff-remembered-files-alist if the default-directory is changed.
(set (make-local-variable 'diff-remembered-defdir) default-directory)
(set (make-local-variable 'diff-remembered-files-alist) nil))
+ (save-restriction
+ (widen)
(save-excursion
(unless (looking-at diff-file-header-re)
(or (ignore-errors (diff-beginning-of-file))
@@ -919,7 +921,7 @@ diff-find-file-name
(file-name-nondirectory file)))
(set (make-local-variable 'diff-remembered-files-alist)
(cons (cons fs file) diff-remembered-files-alist))
- file))))))
+ file)))))))
Perhaps we should swap the save-restriction and save-excursion around
though? The elisp manual says:
If you use both `save-restriction' and `save-excursion' together,
`save-excursion' should come first (on the outside). Otherwise,
the old point value would be restored with temporary narrowing
still in effect.
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#21262: 25.0.50; Diff-mode gets confused about its narrowing if hunk source not found
2017-08-22 0:18 ` npostavs
@ 2017-08-22 14:02 ` Charles A. Roelli
2017-08-27 12:21 ` Charles A. Roelli
0 siblings, 1 reply; 5+ messages in thread
From: Charles A. Roelli @ 2017-08-22 14:02 UTC (permalink / raw)
To: npostavs; +Cc: 21262, dima
[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]
> From: npostavs@users.sourceforge.net
> Cc: Dima Kogan <dima@secretsauce.net>, 21262@debbugs.gnu.org
> Date: Mon, 21 Aug 2017 20:18:39 -0400
>
> Looks like a reasonable fix to me (though I know only a little bit of
> diff-mode). Here it is again ignoring whitespace, which I think makes
> it easier to see what is happening:
>
> modified lisp/vc/diff-mode.el
> @@ -874,6 +874,8 @@ diff-find-file-name
> ;; Flush diff-remembered-files-alist if the default-directory is changed.
> (set (make-local-variable 'diff-remembered-defdir) default-directory)
> (set (make-local-variable 'diff-remembered-files-alist) nil))
> + (save-restriction
> + (widen)
> (save-excursion
> (unless (looking-at diff-file-header-re)
> (or (ignore-errors (diff-beginning-of-file))
> @@ -919,7 +921,7 @@ diff-find-file-name
> (file-name-nondirectory file)))
> (set (make-local-variable 'diff-remembered-files-alist)
> (cons (cons fs file) diff-remembered-files-alist))
> - file))))))
> + file)))))))
>
> Perhaps we should swap the save-restriction and save-excursion around
> though? The elisp manual says:
>
> If you use both `save-restriction' and `save-excursion' together,
> `save-excursion' should come first (on the outside). Otherwise,
> the old point value would be restored with temporary narrowing
> still in effect.
Good point, thank you. I'll apply the attached patch in a few days if
there's no further discussion.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-diff-goto-source-when-buffer-is-narrowed-Bug-212.patch --]
[-- Type: text/x-patch, Size: 5415 bytes --]
From aac6f9eb7aba0cc6b67b56a748e8ee25f755e8f5 Mon Sep 17 00:00:00 2001
From: "Charles A. Roelli" <charles@aurox.ch>
Date: Tue, 22 Aug 2017 15:57:01 +0200
Subject: [PATCH] Fix 'diff-goto-source' when buffer is narrowed (Bug#21262)
* lisp/vc/diff-mode.el (diff-find-file-name): Save the current
narrowing, and widen the buffer before searching for the name of the
file corresponding to the diff.
With thanks to Noam Postavsky.
---
lisp/vc/diff-mode.el | 92 +++++++++++++++++++++++++++-------------------------
1 file changed, 47 insertions(+), 45 deletions(-)
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index aa8d778..1d4af54 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -875,51 +875,53 @@ diff-find-file-name
(set (make-local-variable 'diff-remembered-defdir) default-directory)
(set (make-local-variable 'diff-remembered-files-alist) nil))
(save-excursion
- (unless (looking-at diff-file-header-re)
- (or (ignore-errors (diff-beginning-of-file))
- (re-search-forward diff-file-header-re nil t)))
- (let ((fs (diff-hunk-file-names old)))
- (if prefix (setq fs (mapcar (lambda (f) (concat prefix f)) fs)))
- (or
- ;; use any previously used preference
- (cdr (assoc fs diff-remembered-files-alist))
- ;; try to be clever and use previous choices as an inspiration
- (cl-dolist (rf diff-remembered-files-alist)
- (let ((newfile (diff-merge-strings (caar rf) (car fs) (cdr rf))))
- (if (and newfile (file-exists-p newfile)) (cl-return newfile))))
- ;; look for each file in turn. If none found, try again but
- ;; ignoring the first level of directory, ...
- (cl-do* ((files fs (delq nil (mapcar 'diff-filename-drop-dir files)))
- (file nil nil))
- ((or (null files)
- (setq file (cl-do* ((files files (cdr files))
- (file (car files) (car files)))
- ;; Use file-regular-p to avoid
- ;; /dev/null, directories, etc.
- ((or (null file) (file-regular-p file))
- file))))
- file))
- ;; <foo>.rej patches implicitly apply to <foo>
- (and (string-match "\\.rej\\'" (or buffer-file-name ""))
- (let ((file (substring buffer-file-name 0 (match-beginning 0))))
- (when (file-exists-p file) file)))
- ;; If we haven't found the file, maybe it's because we haven't paid
- ;; attention to the PCL-CVS hint.
- (and (not prefix)
- (boundp 'cvs-pcl-cvs-dirchange-re)
- (save-excursion
- (re-search-backward cvs-pcl-cvs-dirchange-re nil t))
- (diff-find-file-name old noprompt (match-string 1)))
- ;; if all else fails, ask the user
- (unless noprompt
- (let ((file (expand-file-name (or (car fs) ""))))
- (setq file
- (read-file-name (format "Use file %s: " file)
- (file-name-directory file) file t
- (file-name-nondirectory file)))
- (set (make-local-variable 'diff-remembered-files-alist)
- (cons (cons fs file) diff-remembered-files-alist))
- file))))))
+ (save-restriction
+ (widen)
+ (unless (looking-at diff-file-header-re)
+ (or (ignore-errors (diff-beginning-of-file))
+ (re-search-forward diff-file-header-re nil t)))
+ (let ((fs (diff-hunk-file-names old)))
+ (if prefix (setq fs (mapcar (lambda (f) (concat prefix f)) fs)))
+ (or
+ ;; use any previously used preference
+ (cdr (assoc fs diff-remembered-files-alist))
+ ;; try to be clever and use previous choices as an inspiration
+ (cl-dolist (rf diff-remembered-files-alist)
+ (let ((newfile (diff-merge-strings (caar rf) (car fs) (cdr rf))))
+ (if (and newfile (file-exists-p newfile)) (cl-return newfile))))
+ ;; look for each file in turn. If none found, try again but
+ ;; ignoring the first level of directory, ...
+ (cl-do* ((files fs (delq nil (mapcar 'diff-filename-drop-dir files)))
+ (file nil nil))
+ ((or (null files)
+ (setq file (cl-do* ((files files (cdr files))
+ (file (car files) (car files)))
+ ;; Use file-regular-p to avoid
+ ;; /dev/null, directories, etc.
+ ((or (null file) (file-regular-p file))
+ file))))
+ file))
+ ;; <foo>.rej patches implicitly apply to <foo>
+ (and (string-match "\\.rej\\'" (or buffer-file-name ""))
+ (let ((file (substring buffer-file-name 0 (match-beginning 0))))
+ (when (file-exists-p file) file)))
+ ;; If we haven't found the file, maybe it's because we haven't paid
+ ;; attention to the PCL-CVS hint.
+ (and (not prefix)
+ (boundp 'cvs-pcl-cvs-dirchange-re)
+ (save-excursion
+ (re-search-backward cvs-pcl-cvs-dirchange-re nil t))
+ (diff-find-file-name old noprompt (match-string 1)))
+ ;; if all else fails, ask the user
+ (unless noprompt
+ (let ((file (expand-file-name (or (car fs) ""))))
+ (setq file
+ (read-file-name (format "Use file %s: " file)
+ (file-name-directory file) file t
+ (file-name-nondirectory file)))
+ (set (make-local-variable 'diff-remembered-files-alist)
+ (cons (cons fs file) diff-remembered-files-alist))
+ file)))))))
(defun diff-ediff-patch ()
--
2.9.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#21262: 25.0.50; Diff-mode gets confused about its narrowing if hunk source not found
2017-08-22 14:02 ` Charles A. Roelli
@ 2017-08-27 12:21 ` Charles A. Roelli
0 siblings, 0 replies; 5+ messages in thread
From: Charles A. Roelli @ 2017-08-27 12:21 UTC (permalink / raw)
To: npostavs; +Cc: 21262-done, dima
> Date: Tue, 22 Aug 2017 16:02:17 +0200
> From: charles@aurox.ch (Charles A. Roelli)
>
> > From: npostavs@users.sourceforge.net
> > Cc: Dima Kogan <dima@secretsauce.net>, 21262@debbugs.gnu.org
> > Date: Mon, 21 Aug 2017 20:18:39 -0400
> >
> > Looks like a reasonable fix to me (though I know only a little bit of
> > diff-mode). Here it is again ignoring whitespace, which I think makes
> > it easier to see what is happening:
> >
> > modified lisp/vc/diff-mode.el
> > @@ -874,6 +874,8 @@ diff-find-file-name
> > ;; Flush diff-remembered-files-alist if the default-directory is changed.
> > (set (make-local-variable 'diff-remembered-defdir) default-directory)
> > (set (make-local-variable 'diff-remembered-files-alist) nil))
> > + (save-restriction
> > + (widen)
> > (save-excursion
> > (unless (looking-at diff-file-header-re)
> > (or (ignore-errors (diff-beginning-of-file))
> > @@ -919,7 +921,7 @@ diff-find-file-name
> > (file-name-nondirectory file)))
> > (set (make-local-variable 'diff-remembered-files-alist)
> > (cons (cons fs file) diff-remembered-files-alist))
> > - file))))))
> > + file)))))))
> >
> > Perhaps we should swap the save-restriction and save-excursion around
> > though? The elisp manual says:
> >
> > If you use both `save-restriction' and `save-excursion' together,
> > `save-excursion' should come first (on the outside). Otherwise,
> > the old point value would be restored with temporary narrowing
> > still in effect.
>
> Good point, thank you. I'll apply the attached patch in a few days if
> there's no further discussion.
I've pushed the fix, closing the bug now.
commit 208a3cb05f4d954abc9dd6c8cd858ef2bedd7cb4
Date: Tue Aug 22 15:57:01 2017 +0200
Fix 'diff-goto-source' when buffer is narrowed (Bug#21262)
* lisp/vc/diff-mode.el (diff-find-file-name): Save the current
narrowing, and widen the buffer before searching for the name of the
file corresponding to the diff.
With thanks to Noam Postavsky.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-27 12:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-15 5:12 bug#21262: 25.0.50; Diff-mode gets confused about its narrowing if hunk source not found Dima Kogan
2017-08-21 19:49 ` Charles A. Roelli
2017-08-22 0:18 ` npostavs
2017-08-22 14:02 ` Charles A. Roelli
2017-08-27 12:21 ` Charles A. Roelli
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).