Re: Undo and narrow-to-region (was Re: PATCH: narrow-to-region (was ...))

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

On 22 Jul, Bart wrote:
> So you mean that part of undo becomes
> 
>         if (unapplychange(prev))
>             curchange = prev;
>     } while (last_change >= (zlong)0 || (curchange->flags & CH_PREV));
> 
> ??
> 
> What must be happening is that unapplychange() alters the history line,
> returns zero, and then is called again with exactly the same struct,

Yes. Changes to the history number are not collected up as change events
in nextchanges. This means that several history line changes are undone
in a single go.

It also has the, perhaps less desirable, effect that you can't use redo
to revert a change to the history line. Is that worth fixing?

> So the "else break;" looks like paranoia to prevent an infinite loop in
> the event that unapplychange() fails every time.  Which could happen if
> quietgethist() is a no-op?  Perhaps when a line has fallen out of the
> history because of HISTSIZE?

I tried to find a way to induce such an infinite loop and couldn't. Even
so, how about the following patch which checks histline had changed to
detect a failure to set the history line.

> Anyway modulo infinite-loop-prevention, it looks to me as if removing
> that else-clause is the right thing to do, and similarly in redo() a
> bit further along.  I'll let Oliver do the patch if we all agree.

Not sure it's necessary for redo because there's no option to redo more
than one change.

Oliver

diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 8b55403..d512c7a 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -1590,8 +1590,8 @@ undo(char **args)
 	    return 1;
 	if (unapplychange(prev))
 	    curchange = prev;
-	else
-	    break;
+	else if (histline != prev->hist)
+	    break; /* avoid infinite loop if history line unchanged */
     } while (last_change >= (zlong)0 || (curchange->flags & CH_PREV));
     setlastline();
     return 0;