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

Fix error setting PROMPT_COMMAND in bash #84

Closed
wants to merge 1 commit into from
Closed

Fix error setting PROMPT_COMMAND in bash #84

wants to merge 1 commit into from

Conversation

rocha
Copy link
Contributor

@rocha rocha commented Sep 5, 2018

The PROMPT_COMMAND environmental variable is setup incorrectly by pazi:

$ unset PROMPT_COMMAND
$ eval "$(pazi init bash)"
$ echo $PROMPT_COMMAND
__pazi_add_dir;$PROMPT_COMMMAND   # should be __pazi_add_dir;
$ PROMPT_COMMAND="__pazi_add_dir;" eval "$(pazi init bash)"
$ echo $PROMPT_COMMAND
__pazi_add_dir;$PROMPT_COMMMAND  # should be __pazi_add_dir;

The PROMPT_COMMAND environmental variable is setup incorrectly by `pazi`:
```bash
$ unset PROMPT_COMMAND
$ eval "$(pazi init bash)"
$ echo $PROMPT_COMMAND
 __pazi_add_dir;$PROMPT_COMMMAND   # should be __pazi_add_dir;
$ PROMPT_COMMAND="__pazi_add_dir;" eval "$(pazi init bash)"
$ echo $PROMPT_COMMAND
 __pazi_add_dir;$PROMPT_COMMMAND  # should be __pazi_add_dir;
```
Copy link
Owner

@euank euank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious what specific issue you ran into. Did your shell actually give you an error with things as they are?

It would be nice to add a regression test for what you ran into.

It might also be worth fixing upstream (since those couple lines were taken from fasd): https://github.com/whjvenyl/fasd/blob/3e6fd486b182224369ac312435fc02fc8c6c2f09/fasd#L172-L176

*__pazi_add_dir\;*) ;;
*) PROMPT_COMMAND="__pazi_add_dir;\$PROMPT_COMMMAND" ;;
case "${PROMPT_COMMAND}" in
*__pazi_add_dir;*) ;;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that escape is still needed.

With this change, it prints the error:

bash: eval: line 47: syntax error near unexpected token `;'
bash: eval: line 47: `    *__pazi_add_dir;*) ;;'

@euank
Copy link
Owner

euank commented Sep 5, 2018

After poking around a bit more, I think there's a deeper problem that this doesn't fix.

Any existing stuff in prompt command appears to just be being overwritten regardless.

I'm basing this specifically on the following:

$ cat ~/.bashrc
foo() {
  echo "Foo ran"
}

set -x
PROMPT_COMMAND="foo"

eval "$(pazi init bash)"
set +x

$ bash
+ PROMPT_COMMAND=foo
++ pazi init bash
+ eval '
__pazi_add_dir() {
    # TODO: should pazi keep track of this itself in its datadir?
    if [[ "${__PAZI_LAST_PWD}" != "${PWD}" ]]; then
        pazi visit "${PWD}"
    fi
    __PAZI_LAST_PWD="${PWD}"
}

case \$PROMPT_COMMAND in
    *__pazi_add_dir\;*);;
    *) PROMPT_COMMAND="__pazi_add_dir;${PROMPT_COMMMAND}";;
esac

pazi_cd() {
    if [ "$#" -eq 0 ]; then pazi view; return $?; fi
    local res; 
    res="$(__PAZI_EXTENDED_EXITCODES=1 pazi jump "$@")"
    local ret=$?
    case $ret in
    90) echo "${res}";;
    91) cd "${res}";;
    92) echo "${res}" && return 1;;
    93) return 1;;
    *) echo "${res}" && return $ret;;
    esac
}
alias z='\''pazi_cd'\'''
++ case \$PROMPT_COMMAND in
++ PROMPT_COMMAND='__pazi_add_dir;'
++ alias z=pazi_cd
+ set +x


$ echo $PROMPT_COMMAND 
__pazi_add_dir;

In the above, the foo function isn't called, either with or without the changes in this PR.

@rocha
Copy link
Contributor Author

rocha commented Sep 5, 2018

@euank The problem is that the script returned by pazi init bash is supposed to prepend __pazi_add_dir to the expanded value of $PROMPT_COMMAND. Since it is not expanding, any previous value of the variable is lost. For example:

$ echo $PROMPT_COMMAND
history -a; history -n;
$ eval "$(pazi init bash)"
$ echo $PROMPT_COMMAND
__pazi_add_dir;$PROMPT_COMMAND

@euank
Copy link
Owner

euank commented Sep 5, 2018

@rocha Does this change actually end up fixing the issue for you, as in does it sill execute your existing prompt command's contents as you expect?

Per my previous comment, on my system (bash 4.4.12) it seems to not respect any existing prompt command with or without this PR.

I'm a bit confused as to exactly why this PR doesn't work for me since it seems right. Before diving too far into that, I want to make sure we're at least observing the same thing.

@LinuxMercedes
Copy link
Collaborator

While attempting to reproduce all this with pure bash, I discovered why pazi clobbers PROMPT_COMMAND: I misspelled the variable name. Neat!

@rocha
Copy link
Contributor Author

rocha commented Sep 6, 2018

@euank yes, the fix works for me, provided I remove the extra M 😅

I am running bash 4.4.12.

I have found that following this guide and using the associated linter helps avoiding bash quoting headaches:
https://github.com/anordal/shellharden/blob/master/how_to_do_things_safely_in_bash.md

@euank
Copy link
Owner

euank commented Sep 6, 2018

Indeed, it turns out that extra M was tripping me up here too. Thanks for catching the other issue here and submitting a pr!

I hope you don't mind that @LinuxMercedes is picking this up... Trust me, you probably don't actually want to wade into the integ test harness

LinuxMercedes added a commit that referenced this pull request Sep 6, 2018
This test accomplishes two tasks:

1) Ensure that bash escape sequences are preserved by pazi (issue #41)
2) Ensure that the prompt command itself is preserved (pull #84)
LinuxMercedes added a commit that referenced this pull request Sep 6, 2018
This test accomplishes two tasks:

1) Ensure that bash escape sequences are preserved by pazi (issue #41)
2) Ensure that the prompt command itself is preserved (pull #84)
LinuxMercedes added a commit that referenced this pull request Sep 6, 2018
This test accomplishes two tasks:

1) Ensure that bash escape sequences are preserved by pazi (issue #41)
2) Ensure that the prompt command itself is preserved (pull #84)
LinuxMercedes added a commit that referenced this pull request Sep 8, 2018
This test accomplishes two tasks:

1) Ensure that bash escape sequences are preserved by pazi (issue #41)
2) Ensure that the prompt command itself is preserved (pull #84)
LinuxMercedes added a commit that referenced this pull request Sep 8, 2018
This test accomplishes two tasks:

1) Ensure that bash escape sequences are preserved by pazi (issue #41)
2) Ensure that the prompt command itself is preserved (pull #84)
@euank euank added this to the v0.1.1 milestone Nov 13, 2018
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

Successfully merging this pull request may close these issues.

3 participants