Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"make" completion is overwriting readline keybindings #190

Closed
dsifford opened this issue Mar 3, 2018 · 43 comments
Closed

"make" completion is overwriting readline keybindings #190

dsifford opened this issue Mar 3, 2018 · 43 comments

Comments

@dsifford
Copy link

dsifford commented Mar 3, 2018

For whatever reason, the make completion is overwriting the globally set tab keybinding (which I have set to menu-complete).

This is easily reproducible. Check the example below.

asciicast

Why could this be happening?

@dsifford dsifford changed the title "make" "make" completion is overwriting readline keybindings Mar 3, 2018
@scop
Copy link
Owner

scop commented Mar 5, 2018

No idea offhand. Is make completion the only thing this happens with?

@dsifford
Copy link
Author

dsifford commented Mar 5, 2018

To the best of my knowledge, yes.

I haven't systematically tested this with any of the others, but I can say with some level of certainty that this is the first time that I've ever ran into this. I use a pretty wide array of tools that have associated completions on a frequent basis, so I'm somewhat certain that this issue stems from the make completion itself.

@bewuethr
Copy link

bewuethr commented Mar 5, 2018

As far as I can tell, the completion for make is the only one that checks the COMP_TYPE variable, i.e., checks what type of completion is being attempted – maybe related? I don't see where the binding could be affected, though.

@dsifford
Copy link
Author

dsifford commented Mar 5, 2018

Are either of you able to reproduce this on your end @scop @bewuethr ?

@bewuethr
Copy link

bewuethr commented Mar 5, 2018

Yes, I can reproduce it; haven't tried to narrow it down, though.

@bewuethr
Copy link

bewuethr commented Mar 6, 2018

I tried isolating the cause, but now (on a different machine) I am unable to reproduce. The binding isn't removed any longer. Can you reproduce with a minimal makefile, or is anything special about it?

@dsifford
Copy link
Author

dsifford commented Mar 6, 2018

Nope, nothing special about the file.

I'm able to reproduce with the following Makefile on my machine:

.PHONY: foo
foo:
        @echo foo

.PHONY: bar
bar:
        @echo bar
$ uname -a
Linux archlinux 4.15.5-1-ARCH #1 SMP PREEMPT Thu Feb 22 22:15:20 UTC 2018 x86_64 GNU/Linux
$ bash --version
GNU bash, version 4.4.19(1)-release (x86_64-unknown-linux-gnu)
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

@scop
Copy link
Owner

scop commented Mar 6, 2018

Can't reproduce using the given Makefile, current bash-completion master, and

$ uname -a
Linux uncleman 4.13.0-36-generic #40-Ubuntu SMP Fri Feb 16 20:07:48 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
$ bash --version | head -n 1
GNU bash, version 4.4.12(1)-release (x86_64-pc-linux-gnu)

@bewuethr
Copy link

bewuethr commented Mar 6, 2018

Hmmm, and now, on the same machine where I observed it yesterday, I can't see it any longer ☹️

@dsifford
Copy link
Author

dsifford commented Mar 6, 2018

Well, shoot.

I was going to suggest that maybe this is an issue with bleeding edge bash since @scop is running 4.4.12 and I'm on 4.4.19, but now I'm not so sure.

What system/bash version were you running when you were able to trigger this, @bewuethr?

@dsifford
Copy link
Author

dsifford commented Mar 6, 2018

Update: Just tested using the official docker bash:4 image and I can't reproduce there either. That image is also running 4.4.19....

bash-4.4# uname -a
Linux fd318dffa380 4.15.5-1-ARCH #1 SMP PREEMPT Thu Feb 22 22:15:20 UTC 2018 x86_64 Linux

The plot thickens....

@bewuethr
Copy link

bewuethr commented Mar 7, 2018

I used Bash 4.4.18, I think, I have to check when I'm back at the machine.

Here's a way to narrow down the cause: if you set your PS4 to

PS4='$(bind -q menu-complete):${BASH_SOURCE##*/}:${LINENO}: '

and then attempt completion for make, you should be able to see exactly where the binding gets removed.

@dsifford
Copy link
Author

dsifford commented Mar 8, 2018

@bewuethr Good idea!

I think I spotted it...

menu-complete can be invoked via "\C-i", "\C-n".:make:65: local mode=--
menu-complete can be invoked via "\C-i", "\C-n".:make:66: ((  COMP_TYPE != 9  ))
menu-complete can be invoked via "\C-i", "\C-n".:make:67: mode=-d
mmenu-complete can be invoked via "\C-i", "\C-n".:make:0: shopt -po posix
menu-complete can be invoked via "\C-i", "\C-n".:make:70: local 'reset=set +o posix'
menu-complete can be invoked via "\C-i", "\C-n".:make:70: set +o posix
menu-complete can be invoked via "\C-n".:make:74: COMPREPLY=($( LC_ALL=C             $1 -npq __BASH_MAKE_COMPLETION__=1                 "${makef[@]}" "${makef_dir[@]}" .DEFAULT 2>/dev/null |             command sed -nf <(_make_target_extract_script $mode "$cur") ))
mmenu-complete can be invoked via "\C-n".:make:0: command sed -nf /dev/fd/63
mmmenu-complete can be invoked via "\C-n".:make:0: _make_target_extract_script -d ''
mmenu-complete can be invoked via "\C-n".:make:0: LC_ALL=C
mmenu-complete can be invoked via "\C-n".:make:0: sed -nf /dev/fd/63
mmmenu-complete can be invoked via "\C-n".:make:1: local mode=-d

Looks like it's either the set +o posix call on line 70 or the the expression called when setting COMPREPLY on line 74 of the make completion file.

Interestingly, for whatever reason, the line numbers of those calls aren't actually 70 and 74, but 155 and 156, respectively.

Thoughts?

@bewuethr
Copy link

bewuethr commented Mar 8, 2018

I can't figure out how to reproduce this separately; does it not happen if you remove set +o posix? If so, that'd feel like a Bash bug.

@dsifford
Copy link
Author

dsifford commented Mar 8, 2018

@bewuethr I'll give that a shot when I get back to my main machine later this evening.

One thing that I am doing that many (or most) other bash users probably don't do is I set PS1 variables with PROMPT_COMMAND. I'm not sure how that would even relate to this, but I figured I might toss that out there as that adds another thin layer of complexity.

@bewuethr
Copy link

bewuethr commented Mar 9, 2018

I use PROMPT_COMMAND as well to set my PS1, FWIW ;)

@dsifford
Copy link
Author

dsifford commented Mar 9, 2018

@scop @bewuethr

Huzzah!

Commenting out line 155 (and, by necessity, the call to $reset on line 160) completely resolves this issue for me.

Why is that posix call even there to begin with? What's the rationale for it?

@bewuethr
Copy link

bewuethr commented Mar 9, 2018

If POSIX mode is enabled, process substitution <(...) is disabled because it's a Bash extension, but they use the feature for how they call sed:

command sed -nf <(_make_target_extract_script $mode "$cur")

They don't have to do that, though. This is equivalent and doesn't require process substitution:

_make_target_extract_script $mode "$cur" | command sed -n -f -

This would work without messing with the POSIX shell option.

@scop
Copy link
Owner

scop commented Mar 9, 2018

Good stuff on debugging, but I'm afraid the fix is not that simple -- currently the sed command reads data it needs to operate on from stdin, so it cannot read the sed script from stdin.

@scop
Copy link
Owner

scop commented Mar 9, 2018

Forgot to note that I agree if unsetting posix mode causes this, it sounds like a bash bug and should be reported there.

@bewuethr
Copy link

bewuethr commented Mar 9, 2018

Oh, right, duh. Not that easy.

@dsifford
Copy link
Author

dsifford commented Mar 9, 2018

@scop Can you explain what you mean here:

currently the sed command reads data it needs to operate on from stdin, so it cannot read the sed script from stdin.

I read that like 6 times and I can't seem to wrap my head around what you mean. I'm familiar and somewhat competent in sed, so the confusion is not what sed is, but really instead just need clarification on what you're referring to when you say "it" the few times that you use it.

Also....

it sounds like a bash bug and should be reported there.

How do you suggest going about reporting this seeing as I'm ostensibly the only person out of us 3 here that can reliably reproduce this?

@bewuethr
Copy link

bewuethr commented Mar 9, 2018

currently the sed command reads data it needs to operate on from stdin, so it cannot read the sed script from stdin.

Sed can operate on either a file, like

sed 's/old/new/' file

or on standard input, like

cat file | sed 's/old/new/'

Instead of declaring the commands directly, you can read them from a file with the -f option:

cat file | sed -f sedscript.sed

or, operating on a file instead of stdin:

sed -f sedscript.sed file

Instead of making sed modify standard input when piping to sed (as in the second example), you can tell it to read the commands from standard input by specifying - as the file to read commands from:

cat sedscript.sed | sed -f - file

In the make completion script, they pipe to sed and read commands from process substitution, like

<commands> | sed -f <(other_commands)

where other_commands dynamically generate the sed commands. My suggestion to read the sed commands from standard input instead of using -f <(...) doesn't work because standard input is already occupied by the data we're modifying with sed, being piped to sed.

@dsifford
Copy link
Author

dsifford commented Mar 9, 2018

@bewuethr Aha.. didn't catch that sneakly little -f in there. Got it. Thanks for clarifying.

@dsifford
Copy link
Author

What is preventing us from just passing the output of _make_target_extract_script as a parameter instead of a file redirect?

@scop
Copy link
Owner

scop commented Mar 11, 2018 via email

scop added a commit that referenced this issue Mar 11, 2018
…tution

Unsetting posix mode may reportedly interfere with keybindings (#190).
@scop
Copy link
Owner

scop commented Mar 11, 2018 via email

@scop scop closed this as completed Mar 11, 2018
@scop
Copy link
Owner

scop commented Jun 15, 2020

@dsifford @bewuethr did we ever get to the bottom of in which environments does this occur? The video thingy (?) in the original report is no longer available, the comments don't idenfify the readline version etc.

I'm contemplating starting to use the <(...) construct in a lot of places. Alternatively, I could use the lastpipe shopt, but it is somewhat more verbose to write than process substitution, and requires also toggling on/off options.

Would be nice to be able to reproduce, know how widespread the problem is, which versions are affected, and if it's specifically posix mode or potentially other options (lastpipe and monitor in this case, and other options elsewhere in bash-completion) whose toggling provokes it.

@dsifford
Copy link
Author

@scop AFAIK my issue was resolved? I'm having a hard time remembering the specifics here as well.

I can try reproducing again using my example i posted and report back if you like.

I attempted to unarchive the asciinima recording but unfortunately it looks like it's gone for good.

@scop
Copy link
Owner

scop commented Jun 15, 2020

It was resolved, right, but as said I'm considering use of similar constructs again, much more than just make -- perhaps even the exact one that caused problems in your scenario if we can be confident enough the conditions in which it will bite are rare "enough". See my previous comment for details. Testing and reports welcome.

@dsifford
Copy link
Author

Happy to help test. What would you like from me?

@scop
Copy link
Owner

scop commented Jun 15, 2020

If you can source the following snippet and then invoke test-lastpipe Tab and test-posix Tab and report if the readline bindings change.

_completion_test_lastpipe()
{
    printf "\n%s\n" "testing lastpipe"
    bind -q menu-complete
    local reset=$(
        shopt -op monitor
        shopt -p lastpipe
    )
    shopt -ou monitor
    shopt -s lastpipe
    printf "%s\n" "hello"
    $reset
    bind -q menu-complete
}
complete -F _completion_test_lastpipe test-lastpipe

_completion_test_posix()
{
    printf "\n%s\n" "testing posix"
    bind -q menu-complete
    local reset=$(
        shopt -op posix
    )
    shopt -ou posix
    printf "%s\n" "hello"
    $reset
    bind -q menu-complete
}
complete -F _completion_test_posix test-posix

I can't get any problems to arise:

$ test-lastpipe <Tab>
testing lastpipe
menu-complete can be invoked via "\C-i", "\C-n".
hello
menu-complete can be invoked via "\C-i", "\C-n".
^C
$ test-posix <Tab>
testing posix
menu-complete can be invoked via "\C-i", "\C-n".
hello
menu-complete can be invoked via "\C-i", "\C-n".
^C

@dsifford
Copy link
Author

@scop test-posix <Tab> does indeed cause this break to happen for me.

test-lastpipe <Tab> does not.

@dsifford
Copy link
Author

asciicast

@scop
Copy link
Owner

scop commented Jun 16, 2020

Ooh, excellent. Can you help me try to replicate it? What kind of environment is this -- distribution, bash, and readline versions, inputrc or other readline config, bashrc's? Feel free to send me email if you'd rather not upload configs in public.

@dsifford
Copy link
Author

Certainly.

dsifford ~
(ins) $ uname -a
Linux archlinux.desktop 5.7.2-arch1-1 #1 SMP PREEMPT Wed, 10 Jun 2020 20:36:24 +0000 x86_64 GNU/Linux
dsifford ~
(ins) $ bash --version
GNU bash, version 5.0.17(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
dsifford ~
(ins) $ pacman -Qi readline
Name            : readline
Version         : 8.0.004-1

My dotfiles are here: https://github.com/dsifford/.dotfiles

Relevant files:

@scop
Copy link
Owner

scop commented Jun 16, 2020

Thanks, got it. set editing-mode vi apparently gives the C-n menu-complete binding, and that binding is also the one lost on posix mode unset. An explicitly set C-n in inputrc does not disappear. So a minimal reproducer ~/.inputrc would be

set editing-mode vi
Tab: menu-complete

There was a brief mention of filing a bash bug about this in the original discussion, did anyone get around to do that?

@dsifford
Copy link
Author

From memory, I did not report this.

I may have and just forgot about it though.

@bewuethr
Copy link

I didn't report anything either.

@scop
Copy link
Owner

scop commented Jun 21, 2020

@bewuethr
Copy link

Apparently fixed in bash 5.1, according to Chet's response!

@scop
Copy link
Owner

scop commented Jun 21, 2020

Yep, that's nice -- however for bash-completion it means we should refrain from touching posix mode until 5.1 or later is the oldest supported version. I wonder if that's going to happen during this decade ;)

@dsifford
Copy link
Author

Pretty crazy that they fixed this February of 2019 for Bash 5.1 and now (in nearly July 2020) they don't even have an RC for 5.1.

What a wild release cycle that must be. Must be a backwards compatibility nightmare.

scop added a commit that referenced this issue Mar 26, 2023
Problematic because it can cause keybindings to change.

#190
scop added a commit that referenced this issue Mar 26, 2023
Problematic because it can cause keybindings to change.

#190
scop added a commit that referenced this issue Mar 26, 2023
Problematic because it can cause keybindings to change.

#190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants