GNU bug report logs - #21262
25.0.50; Diff-mode gets confused about its narrowing if hunk source not found

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 21262 in the body.
You can then email your comments to 21262 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: Dima Kogan <dima <at> secretsauce.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.50;
 Diff-mode gets confused about its narrowing if hunk source not found
Date: Fri, 14 Aug 2015 22:12:09 -0700

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

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

[tst2.diff (text/x-diff, inline)]

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);
 }
 
 

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

From: charles <at> aurox.ch (Charles A. Roelli)
To: Dima Kogan <dima <at> secretsauce.net>
Cc: 21262 <at> debbugs.gnu.org
Subject: Re: bug#21262: 25.0.50;
 Diff-mode gets confused about its narrowing if hunk source not found
Date: Mon, 21 Aug 2017 21:49:50 +0200

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

> From: Dima Kogan <dima <at> 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.

[01-emacs-bug-21262-draft.patch (text/x-patch, attachment)]

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

From: npostavs <at> users.sourceforge.net
To: charles <at> aurox.ch (Charles A. Roelli)
Cc: 21262 <at> debbugs.gnu.org, Dima Kogan <dima <at> secretsauce.net>
Subject: Re: bug#21262: 25.0.50;
 Diff-mode gets confused about its narrowing if hunk source not found
Date: Mon, 21 Aug 2017 20:18:39 -0400

charles <at> 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.

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

From: charles <at> aurox.ch (Charles A. Roelli)
To: npostavs <at> users.sourceforge.net
Cc: 21262 <at> debbugs.gnu.org, dima <at> secretsauce.net
Subject: Re: bug#21262: 25.0.50;
 Diff-mode gets confused about its narrowing if hunk source not found
Date: Tue, 22 Aug 2017 16:02:17 +0200

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

> From: npostavs <at> users.sourceforge.net
> Cc: Dima Kogan <dima <at> secretsauce.net>,  21262 <at> 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.

[0001-Fix-diff-goto-source-when-buffer-is-narrowed-Bug-212.patch (text/x-patch, attachment)]

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

From: charles <at> aurox.ch (Charles A. Roelli)
To: npostavs <at> users.sourceforge.net
Cc: 21262-done <at> debbugs.gnu.org, dima <at> secretsauce.net
Subject: Re: bug#21262: 25.0.50;
 Diff-mode gets confused about its narrowing if hunk source not found
Date: Sun, 27 Aug 2017 14:21:05 +0200

> Date: Tue, 22 Aug 2017 16:02:17 +0200
> From: charles <at> aurox.ch (Charles A. Roelli) 
> 
> > From: npostavs <at> users.sourceforge.net
> > Cc: Dima Kogan <dima <at> secretsauce.net>,  21262 <at> 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.

Previous Next

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