GNU bug report logs - #23824
25.0.95; Prevent compare one buffer with itself

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 23824 in the body.
You can then email your comments to 23824 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: Tino Calancha <f92capac <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.95; Prevent compare one buffer with itself
Date: Wed, 22 Jun 2016 19:12:13 +0900 (JST)

When the current buffer, buf-a, is visiting FILE-B, buf-b should
be a temporary buffer on sync with FILE-B.

./emacs -r -Q -eval '(progn (with-temp-file "/tmp/foo" (insert "foo")) 
(find-file "/tmp/foo") (insert "bar"))'
M-: (highlight-compare-with-file "/tmp/foo") RET
n n ; Answer no to saving suggestions.
;; Current buffer content different than /tmp/foo but no face 
highlight-changes shown.


In GNU Emacs 25.0.95.2 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
Repository revision: 829733104db073f8abd67765eae162e7360281fa

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

From 47921b1f4898f9fad3acdaa15c2c70fa4b3f9061 Mon Sep 17 00:00:00 2001
From: Tino Calancha <f92capac <at> gmail.com>
Date: Wed, 22 Jun 2016 18:30:14 +0900
Subject: [PATCH] Prevent compare one buffer with itself

* lisp/hilit-chg.el (highlight-compare-with-file):
Use a new temporary buffer on sync with file-b; mark this
buffer as unmodified to prevent being prompt to save it (Bug#23824).
---
 lisp/hilit-chg.el | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/lisp/hilit-chg.el b/lisp/hilit-chg.el
index 8f042b6..89ca154 100644
--- a/lisp/hilit-chg.el
+++ b/lisp/hilit-chg.el
@@ -900,10 +900,18 @@ highlight-compare-with-file
   (let* ((buf-a (current-buffer))
 	 (file-a (buffer-file-name))
 	 (existing-buf (get-file-buffer file-b))
-	 (buf-b (or existing-buf
-		    (find-file-noselect file-b))))
+         (buf-b (cond ((eq buf-a existing-buf)
+                       (let ((buf-new (generate-new-buffer 
(generate-new-buffer-name
+                                                            (buffer-name 
buf-a)))))
+                         (with-current-buffer buf-new
+                           (insert-file-contents-literally file-b)
+                           (set-buffer-modified-p nil)) buf-new))
+                      (t
+                       (or existing-buf
+                           (find-file-noselect file-b))))))
     (highlight-markup-buffers buf-a file-a buf-b file-b (not 
existing-buf))
-    (unless existing-buf
+    (when (or (not existing-buf)
+              (eq buf-a existing-buf))
       (kill-buffer buf-b))
     ))

-- 
2.8.1

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tino Calancha <f92capac <at> gmail.com>
Cc: 23824 <at> debbugs.gnu.org
Subject: Re: bug#23824: 25.0.95; Prevent compare one buffer with itself
Date: Wed, 22 Jun 2016 18:19:15 +0300

> From: Tino Calancha <f92capac <at> gmail.com>
> Date: Wed, 22 Jun 2016 19:12:13 +0900 (JST)
> 
> When the current buffer, buf-a, is visiting FILE-B, buf-b should
> be a temporary buffer on sync with FILE-B.
> 
> ./emacs -r -Q -eval '(progn (with-temp-file "/tmp/foo" (insert "foo")) 
> (find-file "/tmp/foo") (insert "bar"))'
> M-: (highlight-compare-with-file "/tmp/foo") RET
> n n ; Answer no to saving suggestions.
> ;; Current buffer content different than /tmp/foo but no face 
> highlight-changes shown.

I think we need first to establish what exactly is the semantic of
this situation.  You are comparing a buffer with the file that the
buffer visits.  The doc string of this function tries to say something
about this situation:

  If the current buffer is visiting the file being compared against, it
  also will have its differences highlighted.  Otherwise, the file is
  read in temporarily but the buffer is deleted.

but I must confess that this is incomprehensible for me.  So I think
we should first establish what that means, or what the code is trying
to do.

> +                         (with-current-buffer buf-new
> +                           (insert-file-contents-literally file-b)

??? Why insert-file-contents-literally?  That definitely sounds wrong.

Thanks.

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

From: Tino Calancha <f92capac <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 23824 <at> debbugs.gnu.org
Subject: Re: bug#23824: 25.0.95; Prevent compare one buffer with itself
Date: Thu, 23 Jun 2016 09:58:02 +0900

On 06/23/2016 12:19 AM, Eli Zaretskii wrote:
>> From: Tino Calancha <f92capac <at> gmail.com>
>> Date: Wed, 22 Jun 2016 19:12:13 +0900 (JST)
>>
>> When the current buffer, buf-a, is visiting FILE-B, buf-b should
>> be a temporary buffer on sync with FILE-B.
>>
>> ./emacs -r -Q -eval '(progn (with-temp-file "/tmp/foo" (insert "foo"))
>> (find-file "/tmp/foo") (insert "bar"))'
>> M-: (highlight-compare-with-file "/tmp/foo") RET
>> n n ; Answer no to saving suggestions.
>> ;; Current buffer content different than /tmp/foo but no face
>> highlight-changes shown.
> I think we need first to establish what exactly is the semantic of
> this situation.  You are comparing a buffer with the file that the
> buffer visits.  The doc string of this function tries to say something
> about this situation:
>
>    If the current buffer is visiting the file being compared against, it
>    also will have its differences highlighted.  Otherwise, the file is
>    read in temporarily but the buffer is deleted.
>
> but I must confess that this is incomprehensible for me.  So I think
> we should first establish what that means, or what the code is trying
> to do.
 I understand what the doc means:  if the current buffer (buf-a) is 
visiting file-b,
then this func will perform a diff between buf-a and file-b.
* So, if  buf-a is modified, the command highlight you the differences 
with file-b, so
  let you decide if you want to save buf-a (overwritting file-b) or 
not.  It sounds useful.
* Current implementation doesn't match the doc string: even if buf-a is 
visiting file-b and
  modified, the func compare buf-a with buf-a, so that you never get 
nothing highlight
  in this case.

>> +                         (with-current-buffer buf-new
>> +                           (insert-file-contents-literally file-b)
> ??? Why insert-file-contents-literally?  That definitely sounds wrong.
>
> Thanks.

We can use: (insert-file-contents file-b)
it doesn't matter.  At the end, what this func does is comparing buffers
with ediff-diff-program so only the literal content would matter.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tino Calancha <f92capac <at> gmail.com>
Cc: 23824 <at> debbugs.gnu.org
Subject: Re: bug#23824: 25.0.95; Prevent compare one buffer with itself
Date: Thu, 23 Jun 2016 18:27:04 +0300

> Cc: 23824 <at> debbugs.gnu.org
> From: Tino Calancha <f92capac <at> gmail.com>
> Date: Thu, 23 Jun 2016 09:58:02 +0900
> 
> > I think we need first to establish what exactly is the semantic of
> > this situation.  You are comparing a buffer with the file that the
> > buffer visits.  The doc string of this function tries to say something
> > about this situation:
> >
> >    If the current buffer is visiting the file being compared against, it
> >    also will have its differences highlighted.  Otherwise, the file is
> >    read in temporarily but the buffer is deleted.
> >
> > but I must confess that this is incomprehensible for me.  So I think
> > we should first establish what that means, or what the code is trying
> > to do.
>   I understand what the doc means:  if the current buffer (buf-a) is 
> visiting file-b,
> then this func will perform a diff between buf-a and file-b.

But then what is that "also" word doing in the doc string?

> * So, if  buf-a is modified, the command highlight you the differences 
> with file-b, so
>    let you decide if you want to save buf-a (overwritting file-b) or 
> not.  It sounds useful.
> * Current implementation doesn't match the doc string: even if buf-a is 
> visiting file-b and
>    modified, the func compare buf-a with buf-a, so that you never get 
> nothing highlight
>    in this case.

I think there's more here than meets the eye.  Did you ask yourself
why the user is asked twice whether to save the same buffer to the
same file in your scenario?  Why does it do that?  What does it have
in mind?

> >> +                         (with-current-buffer buf-new
> >> +                           (insert-file-contents-literally file-b)
> > ??? Why insert-file-contents-literally?  That definitely sounds wrong.
> >
> > Thanks.
> 
> We can use: (insert-file-contents file-b)
> it doesn't matter.

Oh, it matters a lot.  insert-file-contents-literally will bypass any
decoding and leave the CR-LF EOLs untranslated, something that you
don't want to affect the comparison.

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

From: Tino Calancha <f92capac <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Tino Calancha <f92capac <at> gmail.com>, 23824 <at> debbugs.gnu.org
Subject: Re: bug#23824: 25.0.95; Prevent compare one buffer with itself
Date: Fri, 24 Jun 2016 14:07:29 +0900 (JST)

On Thu, 23 Jun 2016, Eli Zaretskii wrote:
>But then what is that "also" word doing in the doc string?
Nothing special:
It states that the person whom wrote this func took in account both cases:
*) When buf-a is not visiting file-b (let's call it 'default' case).
*) When buf-a is visiting file-b ('special' case).
Something like: "Dear doc readers, i let you know that i have _also_
considered the case when buf-a is visiting file-b".


>I think there's more here than meets the eye.  Did you ask yourself
>why the user is asked twice whether to save the same buffer to the
>same file in your scenario?  Why does it do that?  What does it have
>in mind?

I guess is just a bug in the logic.  The author of this code
overlooked that
(get-file-buffer file-b)
may return the very same buffer that buf-a (when buf-a is visiting 
file-b).
Then, calling
(highlight-markup-buffers BUF-A FILE-B BUF-A FILE-B)
will prompt you twice to save BUF-A when BUF-A is modified.
It prompts you to save buf-a again even if you saved buf-a after
the first prompt: this is because `highlight-markup-buffers'
save the bit on the modification status of BUF-A and BUF-B at the
top of the function.

In my patch, for this case (buf-a visiting file-b), i explicitely create a 
temporary buffer whose content equals file-b content.
Then, I call
(set-buffer-modified-p nil)
to prevent being prompted to save this temporary buffer.

>Oh, it matters a lot.  insert-file-contents-literally will bypass any
>decoding and leave the CR-LF EOLs untranslated, something that you
>don't want to affect the comparison.

I see.  Another example that reviewing a patch before applying it
is a good thing.
Then, my patch need to be modified:

insert-file-contents-literally -> insert-file-contents

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

From: Tino Calancha <f92capac <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: f92capac <at> gmail.com, 23824 <at> debbugs.gnu.org
Subject: Re: bug#23824: 25.0.95; Do not prompt twice to save a buffer
Date: Fri, 24 Jun 2016 22:16:06 +0900 (JST)

On Fri, 24 Jun 2016, Tino Calancha wrote:

>(highlight-markup-buffers BUF-A FILE-B BUF-A FILE-B)
>will prompt you twice to save BUF-A when BUF-A is modified.
>It prompts you to save buf-a again even if you saved buf-a after
>the first prompt: this is because `highlight-markup-buffers'
>save the bit on the modification status of BUF-A and BUF-B at the
>top of the function.

User should be prompted just one in this case.
I propose following patch:

From 67eb8473757392f893c3f83227cbdfd184499e25 Mon Sep 17 00:00:00 2001
From: Tino Calancha <f92capac <at> gmail.com>
Date: Fri, 24 Jun 2016 21:53:56 +0900
Subject: [PATCH] Do not prompt twice to save a buffer

* lisp/hilit-chg.el (highlight-markup-buffers): (Bug#23824).
---
 lisp/hilit-chg.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/hilit-chg.el b/lisp/hilit-chg.el
index 8f042b6..d4276ce 100644
--- a/lisp/hilit-chg.el
+++ b/lisp/hilit-chg.el
@@ -782,7 +782,7 @@ highlight-markup-buffers
 	   a-start a-end len-a
 	   b-start b-end len-b
 	   (bufa-modified (buffer-modified-p buf-a))
-	   (bufb-modified (buffer-modified-p buf-b))
+	   (bufb-modified (and (not (eq buf-a buf-b)) (buffer-modified-p buf-b)))
 	   (buf-a-read-only (with-current-buffer buf-a buffer-read-only))
 	   (buf-b-read-only (with-current-buffer buf-b buffer-read-only))
 	   temp-a temp-b)
-- 
2.8.1

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tino Calancha <f92capac <at> gmail.com>
Cc: f92capac <at> gmail.com, 23824 <at> debbugs.gnu.org
Subject: Re: bug#23824: 25.0.95; Do not prompt twice to save a buffer
Date: Sat, 25 Jun 2016 13:26:37 +0300

> From: Tino Calancha <f92capac <at> gmail.com>
> Date: Fri, 24 Jun 2016 22:16:06 +0900 (JST)
> cc: f92capac <at> gmail.com, 23824 <at> debbugs.gnu.org
> 
> >(highlight-markup-buffers BUF-A FILE-B BUF-A FILE-B)
> >will prompt you twice to save BUF-A when BUF-A is modified.
> >It prompts you to save buf-a again even if you saved buf-a after
> >the first prompt: this is because `highlight-markup-buffers'
> >save the bit on the modification status of BUF-A and BUF-B at the
> >top of the function.
> 
> User should be prompted just one in this case.
> I propose following patch:
> 
> >From 67eb8473757392f893c3f83227cbdfd184499e25 Mon Sep 17 00:00:00 2001
> From: Tino Calancha <f92capac <at> gmail.com>
> Date: Fri, 24 Jun 2016 21:53:56 +0900
> Subject: [PATCH] Do not prompt twice to save a buffer
> 
> * lisp/hilit-chg.el (highlight-markup-buffers): (Bug#23824).
> ---
>   lisp/hilit-chg.el | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lisp/hilit-chg.el b/lisp/hilit-chg.el
> index 8f042b6..d4276ce 100644
> --- a/lisp/hilit-chg.el
> +++ b/lisp/hilit-chg.el
> @@ -782,7 +782,7 @@ highlight-markup-buffers
>   	   a-start a-end len-a
>   	   b-start b-end len-b
>   	   (bufa-modified (buffer-modified-p buf-a))
> -	   (bufb-modified (buffer-modified-p buf-b))
> +	   (bufb-modified (and (not (eq buf-a buf-b)) (buffer-modified-p buf-b)))
>   	   (buf-a-read-only (with-current-buffer buf-a buffer-read-only))
>   	   (buf-b-read-only (with-current-buffer buf-b buffer-read-only))
>   	   temp-a temp-b)
> -- 
> 2.8.1

LGTM, thanks.

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

From: Tino Calancha <tino.calancha <at> gmail.com>
To: 23824-done <at> debbugs.gnu.org
Date: Sun, 26 Jun 2016 10:59:04 +0900 (JST)

Applied patch on master branch

Previous Next

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