classification
Title: Adapt bash readline operate-and-get-next function
Type: enhancement Stage: patch review
Components: Extension Modules Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Rosuav, berker.peksag, lelit, martin.panter, steven.daprano, twouters
Priority: normal Keywords: patch

Created on 2014-08-19 09:42 by lelit, last changed 2019-02-08 13:28 by lelit.

Files
File name Uploaded Description Edit
readline.patch lelit, 2014-08-19 09:42 review
Messages (15)
msg225523 - (view) Author: Lele Gaifax (lelit) * Date: 2014-08-19 09:42
Bash implements an handy extension to the history/readline library called "operate-and-get-next" bound by default to Ctrl-O that lets you repeat an arbitrary sequence of input lines possibly editing some of them.

This patch adapts the extension to the Python readline module.

Since I have no way of testing it against the libedit alternative library, it targets only the real readline library.
msg225573 - (view) Author: Chris Angelico (Rosuav) * Date: 2014-08-20 15:20
Patch applies nicely to current default, and works for me on amd64 Linux. I'm liking how this is looking.
msg252768 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-10-11 05:59
Any other interest in this? It'd be nice to get this in trunk and start being able to recommend it to people.
msg252794 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-10-11 09:10
I left a comment on Rietveld. If I remember correctly, Modules/readline.c is basically a wrapper around libreadline and libedit. I'm not sure if we can add custom behaviors likes this. Perhaps we can expose some of the libreadline functions in Modules/readline.c and let users implement(I don't know if it's possible) it in their pythonrc?
msg268657 - (view) Author: Lele Gaifax (lelit) * Date: 2016-06-16 11:05
The patch does not apply cleanly anymore, with current 3.6a3.

If there is any chance it could be taken into consideration, I will try to rebase it on top of current version.

@Berker: as I don't use Rietveld, is it possible for me to reach the comment you mentioned?
msg268659 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-06-16 11:30
The review comments are at http://bugs.python.org/review/22228/#ps12743 (you can also click to the 'review' link above)
msg268661 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-16 11:59
I will try to have a closer look at this some time, but my immediate worry is it looks like you are directly copying Bash code (GPL) into Python (less restrictive license).

Perhaps a more general solution would be to expose rl_add_defun() to native Python code, if that is possible.
msg268665 - (view) Author: Lele Gaifax (lelit) * Date: 2016-06-16 13:14
I will try to address your concerns.
msg268794 - (view) Author: Lele Gaifax (lelit) * Date: 2016-06-18 10:54
In https://github.com/lelit/cpython/commit/3e5e557a876831a99c21f5a173623cb05ff48abf
I reimplemented the functionality in a slightly different and hopefully better
way, rebasing it on current master.

IANAL, but I think that the new approach is different enough from the original
GNU bash code to be considered safe from the license point of view.

I still could not test it against the editline alternative implementation:
AFAICT all the functions and symbols I used are exposed by that library too,
so it may work without resorting to #ifdefs.

In that regards however, I think we could and should take a different approach
in determining which underlying implementation is used: GNU readline >= 4.1
exposes a rl_gnu_readline_p flag that could be used at configure time to
define a IS_GNU_READLINE, and then rely on that to implement different code
paths within the readline.c module.

Please let me know if I should upload a traditional patch, instead of
referencing my branch on github.

As usual, thank you for any review and feedback!
msg269044 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-22 04:33
For the record, this “operate-and-get-next” function is documented in Bash at <https://www.gnu.org/software/bash/manual/html_node/Miscellaneous-Commands.html#index-operate_002dand_002dget_002dnext-_0028C_002do_0029>. It is supposed to finish entering the current line, and it was taken from the history, bring up the following history item for editing. It hasn’t been added to Readline directly because it relies on how the application uses history (e.g. it is flawed in Bash for me because I use HISTCONTROL=erasedups).

Python’s “readline” module currently has runtime detection of Editline vs Gnu Readline. I am not sure if it is strictly needed, or just that it was easier than build-time detection, as hinted in <https://bugs.python.org/issue6877#msg92654>. It is confusing, because we have other build-time detection of particular Readline features.

Perhaps you may be able to try out Editline using my patch for Issue 13501, but in my experience, the non-Apple patched Editline is too buggy to be useful in Python for much more than experimentation.

I left a couple comments on Git Hub.

I would prefer to expose more of this at the Python level, but that seems hard to do. See Issue 1690201 and Issue 1175004 for other attempts to add a custom function at the Python level. It is hard to do in a general way because rl_add_defun() only accepts a function pointer, and no opaque object to pass to the callback. I wonder how the ctypes library handles this when creating general function pointer objects.
msg269049 - (view) Author: Lele Gaifax (lelit) * Date: 2016-06-22 07:10
I addressed Martin's comments (thank you!) in
https://github.com/lelit/cpython/commits/issue22228_2, removing pointless
usage of a macro and avoiding usage of module's state to store the "next line
index", keeping it in a plain static variable.

Let me know how it looks now.

Martin Panter writes:

> Python’s “readline” module currently has runtime detection of Editline vs
> Gnu Readline. I am not sure if it is strictly needed, or just that it was
> easier than build-time detection, as hinted in
> <https://bugs.python.org/issue6877#msg92654>. It is confusing, because we
> have other build-time detection of particular Readline features.

Yes, it confused me too. Also, there is at least one spot in setup_readline()
that seems wrong to me, because it does not consider if it's effectively using
libedit:

    #ifndef __APPLE__
        if (!isatty(STDOUT_FILENO)) {
            /* Issue #19884: stdout is not a terminal. Disable meta modifier
               keys to not write the ANSI sequence "\033[1034h" into stdout. On
               terminals supporting 8 bit characters like TERM=xterm-256color
               (which is now the default Fedora since Fedora 18), the meta key is
               used to enable support of 8 bit characters (ANSI sequence
               "\033[1034h").

               With libedit, this call makes readline() crash. */
            rl_variable_bind ("enable-meta-key", "off");
        }
    #endif

> Perhaps you may be able to try out Editline using my patch for Issue 13501,
> but in my experience, the non-Apple patched Editline is too buggy to be
> useful in Python for much more than experimentation.

I will try to find some spare time to spend on this, but unfortunately I can
not promise I'll be able to do that in the near future, sorry.

> I would prefer to expose more of this at the Python level, but that seems
> hard to do.

Yeah, I know (I've been here nearly from the beginning ;-).
msg269096 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-23 00:50
It looks like ctypes uses ffi_closure_alloc() to allocate an executable function on demand. So it should be possible for readline to also call libffi and do this, but certainly not trivial.

>>> from ctypes import *
>>> @CFUNCTYPE(c_int, c_int, c_int)
... def operate_and_get_next(count, char):
...     print("Boo!", count, char)
...     return 0
... 
>>> libreadline = CDLL("libreadline.so.6")
>>> libreadline.rl_add_defun("operate-and-get-next", operate_and_get_next, ord("O") & 0x1F)
0
>>> # Press Ctrl-O ==>  Boo! 1 15
msg335003 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2019-02-07 05:18
If the licencing issue is resolved, can we reconsider this for 3.8?
msg335006 - (view) Author: Lele Gaifax (lelit) * Date: 2019-02-07 06:59
Luckily the referenced branch is still around on GH:

https://github.com/lelit/cpython-hg-mirror/tree/issue22228_2

As the name says, it is not related to modern CPython' git repo though.
msg335080 - (view) Author: Lele Gaifax (lelit) * Date: 2019-02-08 13:28
I rebased my work on current master of the cpython git repository, cherry picking the changes from the old branch (that was based on a at the time unofficial git mirror of the mercurial repository). 

To avoid any ambiguity I called it "issue22228_3", so it is now available here: https://github.com/lelit/cpython/tree/issue22228_3

Since now I own a Mac, I could find the energies to adapt the changes to libedit, if at all possible. That would be even more likely to happen should I receive some positive feedback on the chances this has to be accepted :-)
History
Date User Action Args
2019-02-08 13:28:16lelitsetmessages: + msg335080
2019-02-07 06:59:04lelitsetmessages: + msg335006
2019-02-07 05:18:27steven.dapranosetmessages: + msg335003
versions: + Python 3.8, - Python 3.6
2016-06-23 00:50:47martin.pantersetmessages: + msg269096
2016-06-22 07:10:19lelitsetmessages: + msg269049
2016-06-22 04:33:23martin.pantersetmessages: + msg269044
2016-06-18 14:46:06berker.peksagsetmessages: - msg268812
2016-06-18 14:45:58berker.peksagsetmessages: - msg268811
2016-06-18 14:45:52berker.peksagsetmessages: - msg268809
2016-06-18 14:45:46berker.peksagsetmessages: - msg268808
2016-06-18 14:45:27berker.peksagsetnosy: - Daniel Griffin, Daniel Griffin, Daniel Griffin
components: - Tests
2016-06-18 14:44:28berker.peksagsetfiles: - bugreport-2016-05-18-03-28-01.txt
2016-06-18 14:44:22berker.peksagsetfiles: - bugreport-2016-05-18-03-28-01.txt
2016-06-18 14:44:15berker.peksagsetfiles: - bugreport-2016-05-18-03-28-01.txt
2016-06-18 14:38:48lelitsetmessages: + msg268812
2016-06-18 14:30:17Daniel Griffinsetfiles: + bugreport-2016-05-18-03-28-01.txt
nosy: + Daniel Griffin
messages: + msg268811

2016-06-18 14:30:05Daniel Griffinsetfiles: + bugreport-2016-05-18-03-28-01.txt

nosy: + Daniel Griffin
messages: + msg268809

components: + Tests
2016-06-18 14:29:48Daniel Griffinsetfiles: + bugreport-2016-05-18-03-28-01.txt
nosy: + Daniel Griffin
messages: + msg268808

2016-06-18 10:54:16lelitsetmessages: + msg268794
2016-06-18 06:29:12BreamoreBoysetnosy: - BreamoreBoy
2016-06-16 13:14:20lelitsetmessages: + msg268665
2016-06-16 11:59:02martin.pantersetmessages: + msg268661
2016-06-16 11:31:02berker.peksagsetnosy: + martin.panter
2016-06-16 11:30:27berker.peksagsetmessages: + msg268659
2016-06-16 11:05:51lelitsetmessages: + msg268657
2015-10-11 09:10:48berker.peksagsetversions: + Python 3.6
nosy: + twouters, berker.peksag

messages: + msg252794

components: + Extension Modules
stage: patch review
2015-10-11 05:59:29Rosuavsetmessages: + msg252768
2014-08-20 15:20:25Rosuavsetmessages: + msg225573
2014-08-19 17:47:00BreamoreBoysetnosy: + BreamoreBoy, Rosuav
2014-08-19 09:42:08lelitcreate