bug#11318: [Patchset] A bugfix and an arguable bugfix for ediff-inferior-compare-regions

bug#11318: [Patchset] A bugfix and an arguable bugfix for ediff-inferior-compare-regions

[Dave Abrahams]

These patches apply against the emacs-24 branch from 2012-04-23 15:26:22 EST

The first one is definitely a bugfix.  The second one depends on the
first and corrects a behavior that I consider to be buggy, though
others may disagree.

[PATCH 1/2] BUGFIX: select current-diff-comparison regions for
[PATCH 2/2] Useful behavior for ediff-inferior-compare-regions when

bug#11319: [PATCH 1/2] BUGFIX: select current-diff-comparison regions for ediff-inferior-compare-regions correctly

[Dave Abrahams]

The buffer types passed to
ediff-clone-buffer-for-current-diff-comparison are used to select the
character ranges that participate in the inferior comparison.  Those
were being hardcoded as 'A and 'B, which was wrong.
---
 lisp/vc/ediff-util.el |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index 78a2163..046070d 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -3378,10 +3378,18 @@ Without an argument, it saves customized diff argument, if available
     (set-window-buffer wind cloned-buff)
     cloned-buff))
 
-(defun ediff-clone-buffer-for-current-diff-comparison (buff buf-type reg-name)
-  (let ((cloned-buff (ediff-make-cloned-buffer buff reg-name))
-	(reg-start (ediff-get-diff-posn buf-type 'beg))
-	(reg-end (ediff-get-diff-posn buf-type 'end)))
+(defun ediff-buffer-type (buffer)
+  (cond ((eq buffer ediff-buffer-A) 'A)
+        ((eq buffer ediff-buffer-B) 'B)
+        ((eq buffer ediff-buffer-C) 'C)
+        ((eq buffer ediff-ancestor-buffer) 'Ancestor)
+        (t nil)))
+
+(defun ediff-clone-buffer-for-current-diff-comparison (buff reg-name)
+  (let* ((cloned-buff (ediff-make-cloned-buffer buff reg-name))
+         (buf-type (ediff-buffer-type buff))
+         (reg-start (ediff-get-diff-posn buf-type 'beg))
+         (reg-end (ediff-get-diff-posn buf-type 'end)))
     (ediff-with-current-buffer cloned-buff
       ;; set region to be the current diff region
       (goto-char reg-start)
@@ -3555,7 +3563,7 @@ Ediff Control Panel to restore highlighting."
 
     (setq bufA (if use-current-diff-p
 		   (ediff-clone-buffer-for-current-diff-comparison
-		    bufA 'A "-Region.A-")
+		    bufA "-Region.A-")
 		 (ediff-clone-buffer-for-region-comparison bufA "-Region.A-")))
     (ediff-with-current-buffer bufA
       (setq begA (region-beginning)
@@ -3570,7 +3578,7 @@ Ediff Control Panel to restore highlighting."
 
     (setq bufB (if use-current-diff-p
 		   (ediff-clone-buffer-for-current-diff-comparison
-		    bufB 'B "-Region.B-")
+		    bufB "-Region.B-")
 		 (ediff-clone-buffer-for-region-comparison bufB "-Region.B-")))
     (ediff-with-current-buffer bufB
       (setq begB (region-beginning)
-- 
1.7.10

bug#11320: [PATCH 2/2] Useful behavior for ediff-inferior-compare-regions when merging with an ancestor

[Dave Abrahams]

The default, when "comparing currently-highlighted difference regions"
was to compare the selected region to the entire conflict in the merge
buffer.  Now it simply compares the region to the highlighted part of
the ancestor buffer.
---
 lisp/vc/ediff-util.el |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index 046070d..393c8ad 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -3557,9 +3557,11 @@ Ediff Control Panel to restore highlighting."
 		 bufB ediff-buffer-B
 		 possibilities nil)))
 
-    (if (and (ediff-valid-difference-p ediff-current-difference)
+    (when (and (ediff-valid-difference-p ediff-current-difference)
 	     (y-or-n-p "Compare currently highlighted difference regions? "))
-	(setq use-current-diff-p t))
+      (setq use-current-diff-p t)
+      (if ediff-merge-with-ancestor-job
+          (setq bufB ediff-ancestor-buffer)))
 
     (setq bufA (if use-current-diff-p
 		   (ediff-clone-buffer-for-current-diff-comparison
-- 
1.7.10

bug#11318: [Patchset] A bugfix and an arguable bugfix for ediff-inferior-compare-regions

[Glenn Morris]

merge 11318 11319
stop

> [PATCH 1/2] BUGFIX: select current-diff-comparison regions for
> [PATCH 2/2] Useful behavior for ediff-inferior-compare-regions when

As you can see, this style of sending patches is sub-optimal, since each
mail created a separate bug report, including this one, which I am now
merging with "patch 1".

bug#11318: [Patchset] A bugfix and an arguable bugfix for ediff-inferior-compare-regions

[Dave Abrahams]

on Wed May 09 2012, Glenn Morris <rgm-AT-gnu.org> wrote:

> merge 11318 11319
> stop
>
>> [PATCH 1/2] BUGFIX: select current-diff-comparison regions for
>> [PATCH 2/2] Useful behavior for ediff-inferior-compare-regions when
>
> As you can see, this style of sending patches is sub-optimal, since each
> mail created a separate bug report, including this one, which I am now
> merging with "patch 1".

Sorry about that.  I'll try something else next time around.

-- 
Dave Abrahams
BoostPro Computing
http://www.boostpro.com

bug#11318: [Patchset] A bugfix and an arguable bugfix for ediff-inferior-compare-regions

[Stefan Monnier]

> As you can see, this style of sending patches is sub-optimal, since each
> mail created a separate bug report, including this one, which I am now
> merging with "patch 1".

Arguably, debbugs (sh|c)ould pay attention to the References: (or
In-reply-to:) header to automatically put the replies in the same bug as
their "parent" message.


        Stefan

bug#11318: [Patchset] A bugfix and an arguable bugfix for ediff-inferior-compare-regions

[Glenn Morris]

Stefan Monnier wrote:

> Arguably, debbugs (sh|c)ould pay attention to the References: (or
> In-reply-to:) header to automatically put the replies in the same bug as
> their "parent" message.

It does (I added this feature), but:

a) It needs time (I think) to process the first message and update the
bug index with the message-id, so if all messages arrive at the same
instant, it doesn't work.

b) I made the decision to require a subject starting with "Re:" for this
to take effect, because sometimes people reply to an old message and
change the subject with the intention of creating a new report.
(I'm pretty sure I've said this before.)
I'm feeling like I should admit defeat on this aspect, since it seems
people just love messing around with the subject header.


I also added something that makes [patch] equivalent to

Tags:patch

But it's never been an advertised convention for any Emacs mailing list
AFAIK, and it makes me grumpy. :)
I've always assumed it came from the linux kernel and is now the [in thing].

But let's not turn this into another BEGIN_SRC derail...

bug#11318: [Patchset] A bugfix and an arguable bugfix for ediff-inferior-compare-regions

[Stefan Monnier]

>> Arguably, debbugs (sh|c)ould pay attention to the References: (or
>> In-reply-to:) header to automatically put the replies in the same bug as
>> their "parent" message.
> It does (I added this feature), but:

Good, thanks.  I did remember you adding such a thing, but I figured it
must have been something slightly different.

> a) It needs time (I think) to process the first message and update the
> bug index with the message-id, so if all messages arrive at the same
> instant, it doesn't work.

Oh, yes, that's a real bummer.  So we'd have to sequentialize processing
of incoming messages (if it's not done yet) and handle the message-ids
both ways: i.e. recognize when a message comes after its reply and put
it in the same bug-id.  That could be tricky/risky indeed.

> b) I made the decision to require a subject starting with "Re:" for this
> to take effect, because sometimes people reply to an old message and
> change the subject with the intention of creating a new report.
> (I'm pretty sure I've said this before.)
> I'm feeling like I should admit defeat on this aspect, since it seems
> people just love messing around with the subject header.

Is there an easy way to see (in retrospect) whether this "Re:" filtering
did the right thing more often than not?

> I also added something that makes [patch] equivalent to
> Tags:patch

Great, tho it doesn't seem to be used much.


        Stefan

bug#11318: [Patchset] A bugfix and an arguable bugfix for ediff-inferior-compare-regions

[Dave Abrahams]

on Wed May 09 2012, Glenn Morris <rgm-AT-gnu.org> wrote:

> merge 11318 11319
> stop
>
>> [PATCH 1/2] BUGFIX: select current-diff-comparison regions for
>> [PATCH 2/2] Useful behavior for ediff-inferior-compare-regions when
>
> As you can see, this style of sending patches is sub-optimal, since each
> mail created a separate bug report, including this one, which I am now
> merging with "patch 1".

Sub-optimal style aside, how about applying these patches?  As far as I
can tell (and I might be misreading), they have not yet been applied.

-- 
Dave Abrahams
BoostPro Computing
http://www.boostpro.com

bug#11319: [PATCH 1/2] BUGFIX: select current-diff-comparison regions for ediff-inferior-compare-regions correctly

[Stefan Monnier]

Version: 24.4

> The buffer types passed to
> ediff-clone-buffer-for-current-diff-comparison are used to select the
> character ranges that participate in the inferior comparison.  Those
> were being hardcoded as 'A and 'B, which was wrong.

Thank you, it looks OK, so I installed it in trunk,


        Stefan

bug#11319: [PATCH 1/2] BUGFIX: select current-diff-comparison regions for ediff-inferior-compare-regions correctly

[Dave Abrahams]

on Fri Dec 14 2012, Stefan Monnier <monnier-AT-iro.umontreal.ca> wrote:

> Version: 24.4
>
>> The buffer types passed to
>> ediff-clone-buffer-for-current-diff-comparison are used to select the
>> character ranges that participate in the inferior comparison.  Those
>> were being hardcoded as 'A and 'B, which was wrong.
>
> Thank you, it looks OK, so I installed it in trunk,
>
>         Stefan

Fantastic!  Now what about #11320, which depends on this one?  It takes
a feature from being completely useless and confusing to being
wonderful.  I'd love to be able to throw away my override of that file.

-- 
Dave Abrahams
BoostPro Computing                  Software Development        Training
http://www.boostpro.com             Clang/LLVM/EDG Compilers  C++  Boost

bug#11319: [PATCH 1/2] BUGFIX: select current-diff-comparison regions for ediff-inferior-compare-regions correctly

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 1802 bytes --]

bug#11319: [PATCH 1/2] BUGFIX: select current-diff-comparison regions for ediff-inferior-compare-regions correctly

[Dave Abrahams]

on Fri Dec 14 2012, Michael Kifer <michael.kifer-AT-stonybrook.edu> wrote:

> I think it is a good patch. One question: shouldn't the message
> "Which buffer to compare to the merge buffer (A or B)? ")
> be then changed to this?
> "Which buffer to compare to the ancestor buffer (A or B)? ")
>
> This is not part of the  #11320 patch.

I have no opinion about the change from "merge" to "ancestor."  It makes
absolutely no difference to me which of those words Emacs uses; I'll
interpret it exactly the same way.  I suggest you accept the patch as-is
and open a separate issue if you think the word is wrong.

bug#11319: [PATCH 1/2] BUGFIX: select current-diff-comparison regions for ediff-inferior-compare-regions correctly

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 1454 bytes --]

bug#11319: [PATCH 1/2] BUGFIX: select current-diff-comparison regions for ediff-inferior-compare-regions correctly

[Dave Abrahams]

on Mon Dec 17 2012, Michael Kifer <michael.kifer-AT-stonybrook.edu> wrote:

> On 12/17/2012 06:22 PM, Dave Abrahams wrote:
>
>     on Fri Dec 14 2012, Michael Kifer <michael.kifer-AT-stonybrook.edu> wrote:
>     
>     
>         I think it is a good patch. One question: shouldn't the message
>         "Which buffer to compare to the merge buffer (A or B)? ")
>         be then changed to this?
>         "Which buffer to compare to the ancestor buffer (A or B)? ")
>         
>         This is not part of the  #11320 patch.
>         
>     I have no opinion about the change from "merge" to "ancestor."  It makes
>     absolutely no difference to me which of those words Emacs uses; I'll
>     interpret it exactly the same way.  I suggest you accept the patch as-is
>     and open a separate issue if you think the word is wrong.
>     
> The merge buffer is not the ancestor buffer, so the message would be
> wrong and confusing.
> We should change both or none.

I can't understand the idea that you can't correct the behavior without
also correcting the text.  They are separate issues related only by the
region of code in which they appear.  Again, I have no opinion about
whether the text should change, so if you're going to tie them together,
please change the text too.  The behavior is all that matters to me

-- 
Dave Abrahams
BoostPro Computing                  Software Development        Training
http://www.boostpro.com             Clang/LLVM/EDG Compilers  C++  Boost

bug#11319: [PATCH 1/2] BUGFIX: select current-diff-comparison regions for ediff-inferior-compare-regions correctly

[Michael Kifer]

[-- Attachment #1: Type: text/html, Size: 2159 bytes --]

bug#11320: [PATCH 2/2] Useful behavior for ediff-inferior-compare-regions when merging with an ancestor

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

Dave Abrahams <dave@boostpro.com> writes:

> The default, when "comparing currently-highlighted difference regions"
> was to compare the selected region to the entire conflict in the merge
> buffer.

Right, this is pretty useless.

> -    (if (and (ediff-valid-difference-p ediff-current-difference)
> +    (when (and (ediff-valid-difference-p ediff-current-difference)
>  	     (y-or-n-p "Compare currently highlighted difference regions? "))
> -	(setq use-current-diff-p t))
> +      (setq use-current-diff-p t)
> +      (if ediff-merge-with-ancestor-job
> +          (setq bufB ediff-ancestor-buffer)))

Would it make sense to ask about ancestor vs merge buffer even before
the "currently highlighted" query?  I don't quite see why those two
should be tied together.  Although with the patch below, it might be too
many queries, perhaps it's better to only ask when given a prefix arg or
something.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2163 bytes --]

From b503830af00e38ba0d510467dea957e95668a74b Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 11 May 2018 21:56:56 -0400
Subject: [PATCH] Let ediff '=' compare against ancestor buffer (Bug#11320)

* lisp/vc/ediff-util.el (ediff-inferior-compare-regions): Ask user
whether to compare against the ancestor or merge buffer.  Use
read-multiple-choice for A vs B buffer query.
---
 lisp/vc/ediff-util.el | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index 104a578268..c121df9eca 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -3546,25 +3546,20 @@ ediff-inferior-compare-regions
     (ediff-paint-background-regions 'unhighlight)
 
     (cond ((ediff-merge-job)
-	   (setq bufB ediff-buffer-C)
+	   (setq bufB (if (and ediff-ancestor-buffer
+                               (y-or-n-p "Compare against ancestor buffer?"))
+                          ediff-ancestor-buffer
+                        ediff-buffer-C))
 	   ;; ask which buffer to compare to the merge buffer
-	   (while (cond ((eq answer ?A)
-			 (setq bufA ediff-buffer-A
-			       possibilities '(?B))
-			 nil)
-			((eq answer ?B)
-			 (setq bufA ediff-buffer-B
-			       possibilities '(?A))
-			 nil)
-			((equal answer ""))
-			(t (beep 1)
-			   (message "Valid values are A or B")
-			   (sit-for 2)
-			   t))
-	     (let ((cursor-in-echo-area t))
-	       (message
-		"Which buffer to compare to the merge buffer (A or B)? ")
-	       (setq answer (capitalize (read-char-exclusive))))))
+	   (setq answer (car (read-multiple-choice
+                              (format "Which buffer to compare to the %s buffer?"
+                                      (if (eq bufB ediff-ancestor-buffer)
+                                          "ancestor" "merge"))
+                              '((?a "A")
+                                (?b "B")))))
+           (if (eq answer ?a)
+               (setq bufA ediff-buffer-A)
+             (setq bufA ediff-buffer-B)))
 
 	  ((ediff-3way-comparison-job)
 	   ;; ask which two buffers to compare
-- 
2.11.0

bug#11320: [PATCH 2/2] Useful behavior for ediff-inferior-compare-regions when merging with an ancestor

[Noam Postavsky]

tags 11320 fixed
close 11320 27.1
quit

Noam Postavsky <npostavs@gmail.com> writes:

> Would it make sense to ask about ancestor vs merge buffer even before
> the "currently highlighted" query?  I don't quite see why those two
> should be tied together.  Although with the patch below, it might be too
> many queries, perhaps it's better to only ask when given a prefix arg or
> something.

I changed the order of the queries, so that the user can hit two "y"s in
order.  I found this fairly okay.  I've pushed to master, we'll see if
someone comes up with something better later.

[1: 591bb3d900]: 2018-06-28 22:09:04 -0400
  Let ediff '=' compare against ancestor buffer (Bug#11320)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=591bb3d90018ebbcf79e6d496ed73ef396a58887