bug#23824: 25.0.95; Prevent compare one buffer with itself

bug#23824: 25.0.95; Prevent compare one buffer with itself

[Tino Calancha]

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@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

bug#23824: 25.0.95; Prevent compare one buffer with itself

[Eli Zaretskii]

> From: Tino Calancha <f92capac@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.

bug#23824: 25.0.95; Prevent compare one buffer with itself

[Tino Calancha]

On 06/23/2016 12:19 AM, Eli Zaretskii wrote:
>> From: Tino Calancha <f92capac@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.

bug#23824: 25.0.95; Prevent compare one buffer with itself

[Eli Zaretskii]

> Cc: 23824@debbugs.gnu.org
> From: Tino Calancha <f92capac@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.

bug#23824: 25.0.95; Prevent compare one buffer with itself

[Tino Calancha]

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

bug#23824: 25.0.95; Do not prompt twice to save a buffer

[Tino Calancha]

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@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

bug#23824: 25.0.95; Do not prompt twice to save a buffer

[Eli Zaretskii]

> From: Tino Calancha <f92capac@gmail.com>
> Date: Fri, 24 Jun 2016 22:16:06 +0900 (JST)
> cc: f92capac@gmail.com, 23824@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@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.

bug#23824: (no subject)

[Tino Calancha]

Applied patch on master branch