avoid infinite loop in replace when empty string matches

[matthias314]

When the empty string matches during an interactive replacement, then the replacement text will always be inserted at the same spot and the procedure never moves forward. As a result, the user is caught in an infinite loop and has to cancel the command. An example would be something like replace '$' ',' which will add all commas at the end of the current line without moving to the next.

This PR fixes this by not allowing an empty match right after a previous match. This is also what replaceall does (because it's done in Go's regexp library). As a result, replace behaves like replaceall.

Well, almost: Currently a command of the form replace '' 'x' doesn't replace anything, unlike replaceall '' 'x'. This is caused by the test

micro/internal/buffer/search.go

Lines 110 to 113 in fb20818

	func (b *Buffer) FindNext(s string, start, end, from Loc, down bool, useRegex bool) ([2]Loc, bool, error) {
	if s == "" {
	return [2]Loc{}, false, nil
	}

If this PR is merged, then one could think about delete this test. Then replace would match the behavior of replaceall.

[dmaluka]

Why not do this just immediately after we found and replaced an empty match, rather than postpone it to the next iteration? i.e.

--- a/internal/action/command.go
+++ b/internal/action/command.go
@@ -993,6 +993,14 @@ func (h *BufPane) ReplaceCmd(args []string) {
 					if end.Y == locs[1].Y {
 						end = end.Move(nrunes, h.Buf)
 					}
+					if locs[0] == locs[1] {
+						// advance after empty match
+						if searchLoc == end {
+							searchLoc = start
+						} else {
+							searchLoc = searchLoc.Move(1, h.Buf)
+						}
+					}
 					h.Cursor.Loc = searchLoc
 					nreplaced++
 				} else if !canceled && !yes {

[dmaluka]

...Also, this addresses the issue for replace but not for find. I.e.: we press Ctrl-f and type $, the cursor correctly moves to the end of the line, then we press Ctrl-n to find the next match but the cursor stays where it is, rather than moves to the end of the next line.

So shouldn't we move this logic into h.Buf.FindNext() instead?

[dmaluka]

Also a side note: matching ^ (beginning of the line) still works not quite correctly (for a different reason). Looks like we should handle patterns beginning with ^ in a special way.

[matthias314]

Why not do this just immediately after we found and replaced an empty match

That's not the same. For example, saying replace '(z|$)' '@' for

abc
xyz

would give

abc@
xy@@

with your approach, but

abc@
xy@

with replaceall and my variant. Of course, one can implement anything one wants, but I think it would be nice if replace with answer y all the give (until the search wraps over) gives the same result as replaceall.

this addresses the issue for replace but not for find

That's true.

So shouldn't we move this logic into h.Buf.FindNext() instead?

FindNext itself cannot move the cursor forward. How would FindNext know if an empty match is acceptable? Would that be another argument?

matching ^ (beginning of the line) still works not quite correctly

Can you share an example?

[dmaluka]

That's not the same. For example, saying replace '(z|$)' '@' for [...]

Indeed. Yeah, your version seems better then.

FindNext itself cannot move the cursor forward. How would FindNext know if an empty match is acceptable? Would that be another argument?

Yep, I hadn't thought about the details, I was hoping we could generalize it between "find" and "replace" cases but seems like we need to address them separately...

matching ^ (beginning of the line) still works not quite correctly

Can you share an example?

Just replace ^ @, for example. Or e.g. replace ^foo @ when the cursor is at foo but not at the beginning of a line.

[matthias314]

I was hoping we could generalize it between "find" and "replace" cases but seems like we need to address them separately...

If we add a field like LastMatch to Buffer (besides LastSearch), then h.Buf.FindNext() could take this into account. So I now think it could work to make essentially all changes only once in h.Buf.FindNext(). At least it's worth a try.

[matthias314]

I don't think anymore that it's a good idea to modify h.Buf.FindNext(). That function may be called from scripts that shouldn't modify the search status of the buffer. One might be able to reuse a modified h.FindNext() in ReplaceCmd. Maybe it's better to do the necessary changes in h.FindNext() first and then see for an overlap.

I've just force-pushed a new version where I've just renamed prevMatchEnd to lastMatchEnd to align it with the naming of variables like LastSearch. Otherwise this PR seems fine to me provided that one only wants to modify ReplaceCmd.