bugGNU nano - Bugs: bug #65741, clearing the keystroke buffer...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #65741: clearing the keystroke buffer after an error breaks existing string binds relying on errors for control flow

Submitter:  Tasos Papastylianou <tpapastylianou>
Submitted:  Tue 14 May 2024 11:49:56 AM UTC
   
 
Severity:  3 - Normal Status:  Wontfix
Assigned to:  None Open/Closed:  Closed

Jump to the original submission

Wed 19 Jun 2024 08:34:37 PM UTC, comment #18: 


>  You're saying that the proposed {enact} could be used to replace your "conditional hacks"?


No, not really, except in the same sense that offloading 'the entire buffer' to a highly specialised external script and then replacing 'the entire buffer' with its output, could be seen as a "replacement" for macros (which, I would argue, it is not).

Otherwise, {enact} and {persist} (or whatever you might want to call it) are otherwise orthogonal issues; if one wants to use macros (trivial or otherwise; enacted or otherwise), then, in my mind, being able to dictate that a macro should not be allowed to stop unpredictably, is desirable behaviour (and in fact, this would have been my preferred default). Otherwise, if a macro is able to halt prematurely in scenarios where you wouldn't want it to, then your only 'safe' choice is to offload all "editing operations" to an external script instead, which may be non-trivial compared to what you'd have done with the macro. (The example I gave below was supposed to be one such example, which had nothing to do with hacky 'conditionals'.)

Similarly, in my mind, a macro is a sequence of operations that are meant to be replayed verbatim, without too much thought; it's something that's meant to be powerful but brainless, without safeguards of this kind. If one needs safeguards in macros, presumably they should be looking to offload the risky parts onto external scripts instead. Having such safeguards is a bit more sophisticated (and unnecessarily so) than I'd expect from nano, especially since 'warnings' in nano aren't really errors, and there are other 'erroneous' situations that don't trigger warnings and hence the macro wouldn't halt (e.g. opening the wrong file, or a file that doesn't exist, or a file that is already open; nano will happily continue with the macro and do the wrong thing. which is exactly what a macro should be doing, if you ask me.)

Having said that, I agree it's a subtle issue, and I'm being a bit philosophical about it here. Though, to be honest, had I noticed the bug report you responded to when you made this fix, I would have been quite vocal against it, if I'm honest. My rationale is, if an executed macro does something silly unintentionally, then it's generally extremely easy to undo the mistakes (as long as the macro doesn't force nano to exit); whereas if a macro halts prematurely and you're unable to tell nano to let it complete, then that's it, you're stuck needing to do things manually. In theory, you could argue that, if the macro stops prematurely, then while it's true that it might never be able to achieve the intended result and cause you extra work in the process, this protection is desirable, as it also won't cause direct harm. But I would challenge that, since, if the macro doesn't work as intended, harm could be caused indirectly (e.g. macro fails to make all intended changes, and the user saves/exits without noticing), and it could still cause you extra work by having to do the things the macro halted from doing. I.e. just like if the macro had caused unintended harm by 'not' stopping. So the stopping behaviour only exchanges one problem for an almost identical one here, but at the cost of crippling existing macros / the ability to persist through an "error" during the execution of a macro, when macros shouldn't really care about things like that in the first place.

As for whether {enact} might allow me to work around this problem to some extent; maybe, though this would likely require equally ugly macros to achieve; e.g. if I can't trust that a macro won't stop half-way due to interrupts, then I could try saving a hidden file that represents 'state' just before the 'risky' step in the macro, and then reading that state and using it via an external script that outputs an appropriate string macro to 'enact' further as appropriate ... But this is already quite cumbersome simply to bypass unneeded protections. Plus, if the interruptions affect enacted macros too, then enact isn't really a solution for this problem anyway, since you would still likely require several manual macro invocations in a row to work around the halting behaviour.

In any case, please don't see this as an attempt on my part to persist on the issue; I just wanted to answer your question earnestly and explain my reasoning a bit better; even if the issue here remains wontfix, it may be useful context for someone else bumping into this thread in the future. :)

Tasos Papastylianou <tpapastylianou>
Mon 10 Jun 2024 06:35:40 AM UTC, comment #17: 

Marking as Wontfix the original report that clearing the keystroke buffer on errors broke existing string binds that relied on errors for control flow.

Benno Schulenberg <bens>
Group administrator
Mon 03 Jun 2024 01:52:07 PM UTC, comment #16: 

Good afternoon, Tasos,

You're saying that the proposed {enact} could be used to replace your "conditional hacks"?  If so, then that is the way to go: the patch is small and elegant, the feature is easy to understand, it is just one keyword, and it looks hugely powerful -- I just lack the imagination to see the things that it could be used for.

Benno Schulenberg <bens>
Group administrator
Mon 03 Jun 2024 09:45:51 AM UTC, comment #15: 

Morning Benno!

> As far as I know, you're the only one who creatively abuses string binds to implement conditionals


Just to clarify; while the 'clearing' update did affect my 'conditional hacks', I recognise these are hacks, and that probably not many people would do this. And, in any case, the {enact} solution we discussed previously is a much more elegant and natural solution to this problem, if you decide to include that in nano.

So, I didn't wish to imply that this patch/suggestion is simply to continue supporting my hacks. It's more a statement that this change might break existing user workflows (like it did for me), and if you wanted to consider offering a way around it just in case. The example binding I gave below is one such example (which you'll see has nothing to do with 'conditionals'; it simply allows the string replacement to happen as intended).

But, you're right, I have no idea exactly "how" many users might be affected by this change (it may be exactly N=1!), and I feel like I've stated my case and it's not something you're happy to consider, and I don't want to waste your time any further on this. So, please feel free to close the ticket; thanks for having entertained the discussion on the issue! :)

I will make a new ticket later to state my case for the 'enact' feature you proposed, as I think that's far more important; sorry I haven't done this yet! (bit overwhelmed at work right now, and I want to state my case properly :) )

Tasos Papastylianou <tpapastylianou>
Sun 02 Jun 2024 12:05:41 PM UTC, comment #14: 

Partially I mean the length: it is nearly double the code of my first proposed patch.  But mostly I mean that it doubles the number of extra keywords: two, {persist} and {dontpersist}, instead of just one, {dontclear}.  And even {flipkeep} is not acceptable to me, because its logic is harder than that of {dontclear}.

As far as I know, you're the only one who creatively abuses string binds to implement conditionals, and if there are more such creative users, I think they must be exceedingly few, because the logic that is involved is too hard (in my eyes) and people who can think that way will probably prefer editors like vim and emacs that allow a far larger field of play.

Anyway, I made that first patch with {dontclear} just to humor you, and might have applied it, but I won't go any further than that.  Also, on a sidenote, if commit 77d74b5d had been applied when it was made (as requested by Peter Passchier, to suit his way of recording macros), seven years ago, then it would never even have been possible to do the convoluted conditionals that you invented.  So... count your blessings.  :)


Benno Schulenberg <bens>
Group administrator
Sat 01 Jun 2024 05:25:53 PM UTC, comment #13: 


> If you want feedback about bracing mistakes, you could apply something like the attached patch.


Thanks! Good to know!

> Your patch looks okay, but is too much for nano.


Not sure what you mean by this; you mean in terms of length? Or, as in, you feel like this functionality would be beyond the scope of the project?

Obviously, lengthwise, the patch is more or less identical to what you suggested, except with separate 'on/off' switches rather than only a single 'on' or 'toggle' one. So I don't think this is what you meant.

If you mean the latter, then naturally, you have the final say and I will respect what you decide, since the quality of nano is for the most part a reflection of the quality of your decisions as its lead maintainer, and therefore as a happy user of nano I trust your instincts. But I do think that indiscriminately clearing the buffer will cause problems to any user relying on custom macros for anything but non-trivial stuff sooner or later, so having such a switch feels like a good idea to me (and my proposal above for a {persist}/{dontpersist} pair is simply a suggestion based on what would feel most natural / functional to me -- but obviously that's just a suggestion). Otherwise, the only alternative if the now default buffer-clearing behaviour makes creating such macros impossible, is to simply raise one's arms and say "well I can't do this with nano anymore (but I could before)". Which would be a bit unfortunate in my mind.
My personal feeling is that this is a reasonably small change which shouldn't cause much unnecessary bloat to the codebase, but would enable the few (?) users who rely on nano's custom macro capabilities, to continue using it as before, without nano itself deviating from its philosophy of lightweightedness and simplicity.

But like I said, I trust your instincts and will respect your decision; you have already spent more time than other maintainers would have one this, so I can only thank you for considering this issue as seriously as you have in the first place. :)

PS. Sorry, I keep saying macros when, as you pointed before, I should be saying something like "stringbinds". Though for philosophy's sake, I still think that they should be called macros, because, really, that's what they are in a technical sense: a sequence of bracefunctions/keystrokes that can be re-played verbatim (in my mind, the "interactive" macro is simply that, an interactive one, as opposed to pre-defined ones that can either be bound to keystrokes at the nanorc level, or loaded and 'enacted' dynamically, as per the other proposal). But, anyway, sorry, I don't mean to derail this discussion any further :)

Tasos Papastylianou <tpapastylianou>
Fri 31 May 2024 04:00:05 PM UTC, comment #12: 

Hello Tasos,

Your patch looks okay, but is too much for nano.

If you want feedback about bracing mistakes, you could apply something like the attached patch.


(file #56123)

Benno Schulenberg <bens>
Group administrator
Fri 31 May 2024 12:19:55 PM UTC, comment #11: 

Hi Benno,

I won't pretend I understand the nano codebase well enough yet, but based on your patches below, I have made one with my own suggestion. Please see attached.

This uses {persist} to disable the error-halting behaviour, and {dontpersist} to return to the default behaviour of halting on error.

Here is also an example (hopefully not too contrived) where something like this might be useful:

bind M-! "{replace}{regexp}\s+${enter}{persist}{enter}a{execute}{flipnewbuffer}{dontpersist} | externalcommand {enter}{savefile}" main


This clears trailing spaces, then runs an external command, then saves the file. A {persist} is used to prevent lack of trailing spaces from halting execution, but then a {dontpersist} then guarantees that if the external command fails, the file is not saved.

Incidentally, "unknown function" errors in the stringbinding are always halted, regardless of {persist}. (not that they shouldn't, but just something I noticed ... it had led to silent errors when bindings were being executed in the context of an interactive menu).

(file #56122)

Tasos Papastylianou <tpapastylianou>
Wed 29 May 2024 04:44:10 PM UTC, comment #10: 

The first implementation did not reset the 'keepstrokes' state when the string bind ended with a function name between braces -- it was reset only when the string bind ended with literal text.

Attached patch implements a toggling {flipkeep} instead.  But I don't like it: in my opinion it gets too confusing.


(file #56109)

Benno Schulenberg <bens>
Group administrator
Wed 29 May 2024 01:38:28 PM UTC, comment #9: 

I don't quite follow the difference in the two 'dontclear' patches, but they behave identically as far as I can tell. I don't think the first implementation had a problem either.

I was simply pointing out two things:

1. When you introduced the concept, I expected it would behave differently, but actually this behaviour is a lot better than I expected. E.g. I expected one would specifically have to use this instruction directly after an error-prone command to ignore that particular error. Whereas you implemented 'switch on dontclear effect from here on (until the macro is finished)' which makes a lot more sense.

2. Given the above behaviour, it would be very useful to have a second invocation of {dontclear} within a stringbind cancelling out the dontclear effect, (and a third invocation re-enabling it, a fourth invocation cancelling it again, and so on). I could imagine situations where one would want to enable the effect for some parts of the stringbind, but not others.

Thanks again for considering this!

Tasos Papastylianou <tpapastylianou>
Tue 28 May 2024 02:22:16 PM UTC, comment #8: 

The {dontclear} was meant as "apply where needed".  That it didn't work that way was a failure in my implementation.  See the attached patch for an updated version -- now the 'dontclear' status is forgotten whenever the keystroke buffer becomes empty.



(file #56108)

Benno Schulenberg <bens>
Group administrator
Tue 28 May 2024 11:58:24 AM UTC, comment #7: 

The {dontclear} patch works well, and is useful. Thank you!

I note that this is "apply in advance" and "menu independent", as opposed to "apply where needed".

I also note that once applied, it cannot be toggled "off" again; at the same time, its application is not 'retrospective'; i.e. the implementation does not look for the presence of {dontclear} first. So execution will still halt if an error occurs prior to encountering a {dontclear} instruction, but will respect the {dontclear} instruction after encountering one.

While I don't think this needs to become a formal toggleable option (which would be unnecessary overhead), I nevertheless think an on/off toggle within stringbinds would probably be useful. Otherwise you can only toggle 'on' at some point within a stringbind, but not 'off' again.

--

sorry for the delay replying -- delays at work. will reply regarding enact (and making a proper request for it) later!

Tasos Papastylianou <tpapastylianou>
Thu 23 May 2024 12:01:04 PM UTC, comment #6: 

Yes, a new feature request, including an example use case, for something like {enact} would be good.

Benno Schulenberg <bens>
Group administrator
Thu 23 May 2024 11:50:44 AM UTC, comment #5: 


> what might be feasible [...] looks easy enough to implement [...]


I think something like {enact} would be a super-powerful feature, if you were willing to consider it. Would you like me to open a new feature request and argue a proper case for it?

> The request for 'reloadrc' [...] is not an option.


Yes, understandable. I think something like {enact} would be simpler and make this unnecessary anyway.

> The idea of the proposed patch was to eventually incorporate it into nano


I think something like {dontclear} would still be useful regardless of the above, as there might be other legit reasons for not interrupting the macro on errors (e.g. say you have a macro which removes trailing spaces and then saves the file; you don't want this to fail if no trailing spaces are found!); so, yes, I'll give it a try and let you know, thanks!


Tasos Papastylianou <tpapastylianou>
Thu 23 May 2024 11:32:14 AM UTC, comment #4: 

Here's a provisional patch that implements {enact}.

But I think this is madness: a security nighmare, where nano is Turing complete and can do anything.  :|


(file #56091)

Benno Schulenberg <bens>
Group administrator
Thu 23 May 2024 10:26:44 AM UTC, comment #3: 

Hi Tasos,

Thanks for explaining how your "conditionals" work.  I now understand how "{exit}N" either stays in new buffer (because {exit} does nothing in the yesno menu), or closes and discards the new buffer (when the signature string was not found).

The idea of the proposed patch was to eventually incorporate it into nano, if it worked well, you had used it for a while, using it wasn't too bothersome, and at least one other person complained about the new keystroke-flushing behaviour.  But... if you are worried about this patch breaking when only you use it, then the easiest patch is to just delete the two lines that say `if (importance >= AHEM)` and `waiting_codes = 0;`.  That change is unlikely to break (and even if it were, it's easy to unbreak it).

The request for a 'reloadrc' function (in bug #61703) I rejected because it would require 1) a whole lot of code that frees all the data that is gathered while reading the rc files, and 2) some major restructuring of the initialization code, to allow to be called more than once.  Also, it's terribly wasteful: reprocess all of the rcfile when probably just one or two things have changed.  So, it really is not an option.

What might be feasible is: execute the first line of the current buffer (or the current cutbuffer, or the currently marked region, whatever you prefer) as if it were a string bind.  That would require just one extra function, say "{enact}", which looks easy enough to implement.

Benno Schulenberg <bens>
Group administrator
Wed 22 May 2024 09:27:25 PM UTC, comment #2: 

Hi Benno; only replying so as not to leave your reply unanswered, though I don't think my reply will necessarily further the discussion much at the moment ...

> in nano parlance they are "string binds"


Noted :D

> I don't understand how your example works


For the most part, the "logic" for such "conditional" (with very big air quotes here) stringbinds relies on the following three principles: 1. The same keystroke/function sequence may produce different results in different menu contexts; 2. some keystrokes (and functions) are effectively 'no-ops' in certain menus (or have a different effect, e.g. ^C cancelling in some menus, and doing effectively nothing in the main menu); and 3. You can manipulate 'state' subtly to make a keystroke/function sequence end up in a different menu before continuing with the remaining keystrokes.

So, in the example below, from our main tex file, we execute pdflatex and its compilation output is dumped into a new nano buffer, with the phrase COMPILATION_FAILED added if compilation fails. Then a replacement operation is started; if COMPILATION_FAILED is found, nano will trigger the all/yes/no/cancel menu (asking you how you want to proceed with replacement). If that happens, the issuing of an exit command, which does nothing in this menu, followed by an 'n', will simply exit the replacement menu, leaving the compilation buffer still open and untouched. Instead, if latex compiled cleanly and COMPILATION_FAILED was never inserted, replacement will fail (with an error) because this phrase is not found. In nano 7.2, the macro would then proceed normally to issue the {exit}n instruction, which effectively prompts nano to save the buffer before exiting, to which 'n' simply says no, closing the compilation buffer, and dropping us back into the main tex file for editing.

Obviously in theory this is clearly very hacky and limited, not least because you need to be careful to avoid accidentally pressing valid keystrokes for the 'conditional' menus, but even so, I've managed to produce some pretty crazy macros with this technique. I've even trolled a few vim users with my macros :p

All of that breaks with nano 8.0 of course, since many of these no-op operations are now simply treated as errors, voiding the rest of the macro.

> would it be possible to rethink your string binds with the new buffer-clearing behaviour to still achieve what you want?


Unlikely. But even if it is, it would be probably be even more hacky than the above. And that says something!

> I could propose is the attached patch (untested)


This would certainly help, but in general I don't think patching in this manner is the way to go with such things. For one, I'm assuming that such patches are liable to break between versions, right?

> if someone were to provide a patch...


(sad sigh) in theory I'd be ecstatic to do so ... in practice it's the kind of task that pragmatically needs to be way down my priority list, and my deadline count is ever-increasing rather than decreasing ... but if you're happy to leave this open for a while, I wouldn't mind having a go if and when I find such time to tinker...

If I may leave a final thought here, in the past (in one of the tickets, can't remember which one now) we had discussed the possibility of "hot-reloading" a .nanorc file as a potential feature, which would then allow one to modify a stringbind definition, reload the config, and then use the new stringbind. At the time you weren't too keen on this. But, if this is something you wanted to consider, if this existed, then proper conditionals could of course be offloaded to external scripts, which could then alter stringbind definitions appropriately in the nanorc, and then run them.

Alternatively, (and perhaps even more powerfully) if there was a way to read a stringbind definition from a file (or from the output of an 'execute' instruction) and either execute the resulting sequence on the spot or bind it to a standard keystroke, as opposed to simply dumping the literal control sequences in the buffer, then an external script could produce an appropriate sequence of operations for nano to perform on the fly, which would be very powerful. E.g., when reading a file or executing a command, there could be a toggle that says "interpret generated buffer as a stringbinding to be executed/captured, rather than as literal characters" (in not as many words, obviously). I think if you were to add this functionality, it would be a relatively small but very impactful change, since at the moment, keystrokes can trigger scripts, but not the other way round. Having scripts be able to trigger keystrokes would close the loop, allowing for all kinds of hackery and automation...

Feel free to ignore any and all of the above. Just bedtime thoughts. :)

Tasos Papastylianou <tpapastylianou>
Tue 14 May 2024 03:24:20 PM UTC, comment #1: 

Hello Tasos,

Hmm...  First off, I have to admit that I don't understand how your example snippet works -- my brain is too small to follow the convolutions.  :)

Second, for terminlogy sake, you call the things that have {function} parts "macros", but in nano parlance they are "string binds".  (A macro is the thing that is recorded with M-:.)

Third, you're not the only one to make heavy use of braced function names.  There is at least one other: https://github.com/AlanCristhian/nanorc-wizzard/blob/main/.nanorc#L351 (and further down -- found through https://savannah.gnu.org/bugs/?64019), but I don't know whether any of his string binds rely on some kind of control flow.

Fourth, would it be possible to rethink your string binds with the new buffer-clearing behaviour to still achieve what you want?

Fifth, if not, I do not want to add another option, if at all possible, and I certainly do not want to add control-flow commands to the current simple interpretation of braced function names.  (But if someone were to provide a patch...)

What I could propose is the attached patch (untested).  It would require that any string bind that relies on your special control flow starts with {dontclear}.  It has the advantage above a global option that not all string binds need to be designed for the same error behaviour.  Would that be a good-enough solution?


(file #56050)

Benno Schulenberg <bens>
Group administrator
Tue 14 May 2024 11:49:56 AM UTC, original submission:  

Hi Benno!

Let me start this by saying I'm fully aware of this: https://xkcd.com/1172/   (????)

The latest nano (v8.0) has a new feature where:

> "Whenever an error occurs, the keystroke buffer is cleared, thus stopping the execution of a macro or a string bind."


I can see the rationale why this might be considered a good idea; however, making this mandatory / untoggleable completely broke my workflow. This was "functionality" I was relying on to implement some clever control-flow in my macros.

Here's a simple example:

bind ^T "pdflatex -halt-on-error main.tex || echo COMPILATION_FAILED{enter}{lastline}{replace}COMPILATION_FAILED{enter}{enter}{exit}n" execute

With this snippet, old nano would behave as follows: pressing Ctrl+T+T compiles the pdf document; if it compiles successfully, nothing further happens (visibly) in the editor; but, if there was a compilation error, a new buffer "appears" showing you the compilation output with the error. This is one of many pdf-related macros I made (including full nano-to-evince and evince-to-nano latex search / synchronisation), all of which are now broken (hence I'm still using nano 7.2 instead).

Yes, I know this is a very hacky way of implementing control flows in macros, but I would argue that since macros are mostly intended to be sequences of keystrokes rather than anything containing explicit control-flow constructs, breaking on error by default severly limits them, and may or may not always be desirable behaviour.

I don't know if this only affects exactly a userbase of 1, or if others were also relying on clever macros too, but if you're happy to consider this, then my suggestion would be to make this behaviour configurable/toggleable, rather than enforced (no need for a default binding, as long as a user could create it for such a toggle).

Alternatively, now that the macro system supports "{function}" syntax, perhaps this might be a good opportunity to consider strengthening the macro system even further, by introducing control-flow "functions" that are only meaningful at the macro level, e.g. {stopiferror}, or some form of {iferror}{then}{else}{fi} construct?

Tasos Papastylianou <tpapastylianou>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by bens (Updated the item)
  • -email is unavailable- added by tpapastylianou (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-07-13 bens Open/ClosedOpen Closed
    2024-06-10 bens StatusNone Wontfix
    2024-05-31 bens Attached File- Added warn-about-bad-braced-names-in-menus.patch, #56123
    2024-05-31 tpapastylianou Attached File- Added add-persist-dontpersist-bindable-function-pair.patch, #56122
    2024-05-29 bens Attached File- Added 0001-rcfile-new-flipkeep-for-toggling-the-clearing-of-the.patch, #56109
    2024-05-28 bens Attached File- Added 0001-bindings-allow-suppressing-keystroke-buffer-clearing.patch, #56108
    2024-05-25 bens SummaryNano 8.0 behaviour of clearing macro-tail after error breaks existing macros relying on errors for control flow clearing the keystroke buffer after an error breaks existing string binds relying on errors for control flow
    2024-05-23 bens Attached File- Added 0001-possible-new-feature-allow-executing-the-top-line-as.patch, #56091
    2024-05-14 bens Attached File- Added add-a-dontclear-bindable-function.patch, #56050

    Back to the top

    Powered by Savane 3.13-e652.
    Corresponding source code