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

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

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




Reply | Threaded
Open this post in threaded view
|

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




Reply | Threaded
Open this post in threaded view
|

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




Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov
In reply to this post by 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).



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Konstantin Kharlamov <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Konstantin Kharlamov
> From: Konstantin Kharlamov <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Cc: [hidden email]
> From: Konstantin Kharlamov <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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: [hidden email]
>> From: Konstantin Kharlamov <[hidden email]>
>> 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.
>



Reply | Threaded
Open this post in threaded view
|

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

Stefan Monnier
In reply to this post by Konstantin Kharlamov
>> 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



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov

В Пн, 18 фев. 2019 в 8:49 ПП (PM), Stefan Monnier
<[hidden email]> написал:
>>  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.





Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Konstantin Kharlamov

В Вт, 19 фев. 2019 в 12:51 ДП (AM), Stefan Monnier
<[hidden email]> написал:

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





Reply | Threaded
Open this post in threaded view
|

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