bug#34421: [PATCH 1/2] smerge-mode: factor out going to next conflict

bug#34421: [PATCH 1/2] smerge-mode: factor out going to next conflict

[Konstantin Kharlamov]

* lisp/vc/smerge-mode.el (smerge-goto-next-conflict)
---
 lisp/vc/smerge-mode.el | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
index 569797e18dd..929cd85432a 100644
--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -269,6 +269,9 @@ font-lock-keywords
 
 (defconst smerge-match-names ["conflict" "upper" "base" "lower"])
 
+(defsubst smerge-goto-next-conflict (&optional bound)
+  (re-search-forward smerge-begin-re bound t))
+
 (defun smerge-ensure-match (n)
   (unless (match-end n)
     (error "No `%s'" (aref smerge-match-names n))))
@@ -276,7 +279,7 @@ smerge-ensure-match
 (defun smerge-auto-leave ()
   (when (and smerge-auto-leave
 	     (save-excursion (goto-char (point-min))
-			     (not (re-search-forward smerge-begin-re nil t))))
+			     (not (smerge-goto-next-conflict))))
     (when (and (listp buffer-undo-list) smerge-mode)
       (push (list 'apply 'smerge-mode 1) buffer-undo-list))
     (smerge-mode -1)))
@@ -312,7 +315,7 @@ smerge-combine-with-next
       (push (if (match-end i) (copy-marker (match-end i) t)) ends))
     (setq ends (apply 'vector ends))
     (goto-char (aref ends 0))
-    (if (not (re-search-forward smerge-begin-re nil t))
+    (if (not (smerge-goto-next-conflict))
 	(error "No next conflict")
       (smerge-match-conflict)
       (let ((match-data (mapcar (lambda (m) (if m (copy-marker m)))
@@ -407,7 +410,7 @@ smerge-popup-context-menu
     ;; There's no conflict at point, the text-props are just obsolete.
     (save-excursion
       (let ((beg (re-search-backward smerge-end-re nil t))
-	    (end (re-search-forward smerge-begin-re nil t)))
+	    (end (smerge-goto-next-conflict)))
 	(smerge-remove-props (or beg (point-min)) (or end (point-max)))
 	(push event unread-command-events)))))
 
@@ -651,7 +654,7 @@ smerge-resolve-all
   (interactive)
   (save-excursion
     (goto-char (point-min))
-    (while (re-search-forward smerge-begin-re nil t)
+    (while (smerge-goto-next-conflict)
       (condition-case nil
           (progn
             (smerge-match-conflict)
@@ -812,7 +815,7 @@ smerge-match-conflict
 	  (cond
 	   ((save-excursion
 	      (goto-char upper-start)
-	      (re-search-forward smerge-begin-re end t))
+	      (smerge-goto-next-conflict end))
 	    ;; There's a nested conflict and we're after the beginning
 	    ;; of the outer one but before the beginning of the inner one.
 	    ;; Of course, maybe this is not a nested conflict but in that
@@ -887,7 +890,7 @@ smerge-find-conflict
                (goto-char pos))))
     ;; If we're not already inside a conflict, look for the next conflict
     ;; and add/update its overlay.
-    (while (and (not found) (re-search-forward smerge-begin-re limit t))
+    (while (and (not found) (smerge-goto-next-conflict limit))
       (condition-case nil
           (progn
             (smerge-match-conflict)
-- 
2.20.1

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Konstantin Kharlamov]

* lisp/vc/smerge-mode.el (smerge-vc-next-conflict)
---
 lisp/vc/smerge-mode.el | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
index 929cd85432a..8ac0b3ed370 100644
--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -1435,6 +1435,18 @@ smerge-start-session
         (smerge-next))
     (error (smerge-auto-leave))))
 
+(require 'vc)
+
+(defun smerge-vc-next-conflict ()
+  (interactive)
+  (let ((buffer (current-buffer)))
+    (when (not (smerge-goto-next-conflict))
+      (vc-find-conflicted-file)
+      (if (eq buffer (current-buffer))
+          (message "No conflicts found")
+        (goto-char 0)
+        (smerge-goto-next-conflict)))))
+
 (provide 'smerge-mode)
 
 ;;; smerge-mode.el ends here
-- 
2.20.1

bug#34420: [PATCH v2 2/2] smerge-mode: new function to go to next conflict

[Konstantin Kharlamov]

* lisp/vc/smerge-mode.el (smerge-vc-next-conflict)
---

v2: added documentation for the function

 lisp/vc/smerge-mode.el | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
index 929cd85432a..0bc11fee23c 100644
--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -1435,6 +1435,20 @@ smerge-start-session
         (smerge-next))
     (error (smerge-auto-leave))))
 
+(require 'vc)
+
+(defun smerge-vc-next-conflict ()
+  "Tries to go to next conflict in current file, otherwise tries
+to open next conflicted file version-control-system wise"
+  (interactive)
+  (let ((buffer (current-buffer)))
+    (when (not (smerge-goto-next-conflict))
+      (vc-find-conflicted-file)
+      (if (eq buffer (current-buffer))
+          (message "No conflicts found")
+        (goto-char 0)
+        (smerge-goto-next-conflict)))))
+
 (provide 'smerge-mode)
 
 ;;; smerge-mode.el ends here
-- 
2.20.1

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Konstantin Kharlamov]

To give some context: these patches follow this thread 
http://lists.gnu.org/archive/html/help-gnu-emacs/2019-01/msg00111.html 
(note: the thread continues the next month, i.e. February, too).

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Eli Zaretskii]

> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Date: Thu, 14 Feb 2019 00:44:03 +0300
> 
> To give some context: these patches follow this thread 
> http://lists.gnu.org/archive/html/help-gnu-emacs/2019-01/msg00111.html 
> (note: the thread continues the next month, i.e. February, too).

I hope Stafen (CC'ed) could comment on those.

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Eli Zaretskii]

> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Date: Thu, 14 Feb 2019 00:44:03 +0300
> 
> To give some context: these patches follow this thread 
> http://lists.gnu.org/archive/html/help-gnu-emacs/2019-01/msg00111.html 
> (note: the thread continues the next month, i.e. February, too).

I hope Stefan (CC'ed) could comment on those.

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Stefan Monnier]

>> To give some context: these patches follow this thread 
>> http://lists.gnu.org/archive/html/help-gnu-emacs/2019-01/msg00111.html 
>> (note: the thread continues the next month, i.e. February, too).
> I hope Stefan (CC'ed) could comment on those.

Hmm...

In terms of implementation, there's nothing very deep and it looks OK
(tho maybe it points to the need to autoload vc-find-conflicted-file,
and obviously I'd use (point-min) rather than a hardcoded 0 and change
the docstring to use the imperative rather than present tense).

In terms of UI it's a question of taste: I didn't provide such a command
because I never felt the need for it (I think I like to be in control of
changing buffer).  Maybe I'd change it to first emit a warning "no more
conflicts in this buffer" and only go to the next buffer if you repeat
the command (as a form of confirmation that you're done with the current
buffer).  Also, I'd save the file before switching to the next buffer.


        Stefan

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Konstantin Kharlamov]

On 16.02.2019 16:41, Stefan Monnier wrote:
>>> To give some context: these patches follow this thread
>>> http://lists.gnu.org/archive/html/help-gnu-emacs/2019-01/msg00111.html
>>> (note: the thread continues the next month, i.e. February, too).
>> I hope Stefan (CC'ed) could comment on those.
> 
> Hmm...
> 
> In terms of implementation, there's nothing very deep and it looks OK
> (tho maybe it points to the need to autoload vc-find-conflicted-file,
> and obviously I'd use (point-min) rather than a hardcoded 0 and change
> the docstring to use the imperative rather than present tense).

Thanks for your comments!

> In terms of UI it's a question of taste: I didn't provide such a command
> because I never felt the need for it (I think I like to be in control of
> changing buffer).  Maybe I'd change it to first emit a warning "no more
> conflicts in this buffer" and only go to the next buffer if you repeat
> the command (as a form of confirmation that you're done with the current
> buffer).

Well, I wouldn't call changing a buffer upon explicit command from a 
user a "taking control from them". The command name being explicit (at 
least should be — I'm not a native English speaker, please tell me if 
it's not) about going to the next conflict in the repository, and 
conflicts are markers in different files.

Emitting a warning in this case would be annoying, because the user 
expects to be taken to the next conflict if there's any. Imagine the 
case with many files having one small conflict, and that the user have 
to press "take me to the next conflict" twice every time.

> Also, I'd save the file before switching to the next buffer.
Okay, I can add that. I can't really comment anything about usability in 
this case, because I have a habit of saving files myself way too often :)

------

Since this discussion may take time, and you even may not even like the 
change — please, could you push the first patch in the series? When I 
first started looking at smerge-mode.el, I was confused about how going 
to the next conflict is implemented, and why the code does not call 
`smerge-next` anywhere. Extracting the code to an inline function, I 
think, should make it a bit easier for future contributors.

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Eli Zaretskii]

> Cc: 34420@debbugs.gnu.org
> From: Konstantin Kharlamov <hi-angel@yandex.ru>
> Date: Sun, 17 Feb 2019 12:18:53 +0300
> 
> > In terms of UI it's a question of taste: I didn't provide such a command
> > because I never felt the need for it (I think I like to be in control of
> > changing buffer).  Maybe I'd change it to first emit a warning "no more
> > conflicts in this buffer" and only go to the next buffer if you repeat
> > the command (as a form of confirmation that you're done with the current
> > buffer).
> 
> Well, I wouldn't call changing a buffer upon explicit command from a 
> user a "taking control from them". The command name being explicit (at 
> least should be — I'm not a native English speaker, please tell me if 
> it's not) about going to the next conflict in the repository, and 
> conflicts are markers in different files.

I'm not sure this is always the case.  I can certainly envision a user
who might be surprised by being switched to another buffer.  Asking a
question like "No more conflicts in this file; go to next one?" might
in that case be a good idea.

> Emitting a warning in this case would be annoying, because the user 
> expects to be taken to the next conflict if there's any.

We could have an option to control this behavior.

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Konstantin Kharlamov]

On 17.02.2019 18:37, Eli Zaretskii wrote:
>> Cc: 34420@debbugs.gnu.org
>> From: Konstantin Kharlamov <hi-angel@yandex.ru>
>> Date: Sun, 17 Feb 2019 12:18:53 +0300
>>
>>> In terms of UI it's a question of taste: I didn't provide such a command
>>> because I never felt the need for it (I think I like to be in control of
>>> changing buffer).  Maybe I'd change it to first emit a warning "no more
>>> conflicts in this buffer" and only go to the next buffer if you repeat
>>> the command (as a form of confirmation that you're done with the current
>>> buffer).
>>
>> Well, I wouldn't call changing a buffer upon explicit command from a
>> user a "taking control from them". The command name being explicit (at
>> least should be — I'm not a native English speaker, please tell me if
>> it's not) about going to the next conflict in the repository, and
>> conflicts are markers in different files.
> 
> I'm not sure this is always the case.  I can certainly envision a user
> who might be surprised by being switched to another buffer.  Asking a
> question like "No more conflicts in this file; go to next one?" might
> in that case be a good idea.

The thing is, we already have a function that only works within focused 
buffer, it is smerge-next.

This patch is about adding a more "global" function, one that works 
within the focused repository instead of just the focused buffer.

>> Emitting a warning in this case would be annoying, because the user
>> expects to be taken to the next conflict if there's any.
> 
> We could have an option to control this behavior.
> 

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Stefan Monnier]

>> In terms of UI it's a question of taste: I didn't provide such a command
>> because I never felt the need for it (I think I like to be in control of
>> changing buffer).  Maybe I'd change it to first emit a warning "no more
>> conflicts in this buffer" and only go to the next buffer if you repeat
>> the command (as a form of confirmation that you're done with the current
>> buffer).
> Well, I wouldn't call changing a buffer upon explicit command from a user
> a "taking control from them".

I didn't mean it in the sense that the command misbehaves.  Just that
the command does 2 things (go to the next conflict in the same file
sometimes, and go the next conflict in another file other times) and as
a user I'd prefer to know which one's going to happen before
it happens.  As I said it's a question of taste.

I think I could be tempted to use your command (in place of smerge-next)
if it requested confirmation before going to the next file.

I'll install your code as a first step,


        Stefan

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Stefan Monnier]

> I'll install your code as a first step,

Done, as well as a subsequent patch which makes it save the buffer and
request confirmation.  Try it out and let me know of any
remaining problems.


        Stefan

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[hi-angel]

В Пн, 18 фев. 2019 в 8:49 ПП (PM), Stefan Monnier 
<monnier@IRO.UMontreal.CA> написал:
>>  I'll install your code as a first step,
> 
> Done, as well as a subsequent patch which makes it save the buffer and
> request confirmation.  Try it out and let me know of any
> remaining problems.
> 
> 
>         Stefan

Ok, thanks, so, I'm trying the latest smerge-mode.el out, and two 
issues I see:

1. On the first call of the function I get "smerge-vc-next-conflict: 
Symbol’s function definition is void: vc-find-conflicted-file". This 
starts working once I execute (require 'vc)
2. Although I didn't set smerge-change-buffer-confirm to nil, there's 
no confirmation before going to another buffer.

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Stefan Monnier]

> 1. On the first call of the function I get "smerge-vc-next-conflict:
> Symbol’s function definition is void: vc-find-conflicted-file". This starts
> working once I execute (require 'vc)

Did you take only smerge-mode.el ?

I changed vc.el so vc-find-conflicted-file is now autoloaded in the
`master` branch, so you'll only see it if you use the `master` branch.

> 2. Although I didn't set smerge-change-buffer-confirm to nil, there's no
> confirmation before going to another buffer.

I assume it's because you run it via `M-x`.
The confirmation only kicks in if you run it via a keybinding.


        Stefan

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[hi-angel]

В Вт, 19 фев. 2019 в 12:51 ДП (AM), Stefan Monnier 
<monnier@IRO.UMontreal.CA> написал:
>>  1. On the first call of the function I get "smerge-vc-next-conflict:
>>  Symbol’s function definition is void: vc-find-conflicted-file". 
>> This starts
>>  working once I execute (require 'vc)
> 
> Did you take only smerge-mode.el ?
> 
> I changed vc.el so vc-find-conflicted-file is now autoloaded in the
> `master` branch, so you'll only see it if you use the `master` branch.
> 
>>  2. Although I didn't set smerge-change-buffer-confirm to nil, 
>> there's no
>>  confirmation before going to another buffer.
> 
> I assume it's because you run it via `M-x`.
> The confirmation only kicks in if you run it via a keybinding.
> 
> 
>         Stefan

Ah, works for me, thanks! I didn't want to upgrade to latest master 
because I've been using harfbuzz branch, but I just installed and 
tested master — everything is okay.

bug#34420: [PATCH 2/2] smerge-mode: new function to go to next conflict

[Stefan Monnier]

> Ah, works for me, thanks! I didn't want to upgrade to latest master because
> I've been using harfbuzz branch, but I just installed and tested master —
> everything is okay.

Thanks, closing,


        Stefan