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

Output from chpwd hook contaminates script output #3524

Closed
apjanke opened this issue Jan 28, 2015 · 5 comments · Fixed by #3525
Closed

Output from chpwd hook contaminates script output #3524

apjanke opened this issue Jan 28, 2015 · 5 comments · Fixed by #3525

Comments

@apjanke
Copy link
Contributor

apjanke commented Jan 28, 2015

The recent change for Apple Terminal style pwd notification #3429 causes terminal control characters to be emitted upon each chpwd. This happens even if it's not about to display a prompt; e.g. if a cd is done inside a function or script.

This can contaminate the output of the function or script if it's being captured by anything. In particular, David Riley reports:


The recent change breaks Android development on OSX because Android
has a function:

get_build_var () {
T=$(gettop)
if [ ! "$T" ]
then
echo "Couldn't locate the top of the tree.  Try setting TOP." >&2
return
fi
(
\cd $T
CALLED_FROM_SETUP=true BUILD_SYSTEM=build/core command make
--no-print-directory -f build/core/config.mk dumpvar-$1
)
}

The cd commands ends up printing some terminal commands which then get
returned from get_build_var which causes problems later on since it's
expecting a simple string instead of some terminal codes.


The pwd reporting mechanism needs to be changed to emit output only when it's safe to do so, i.e. when displaying an interactive prompt. (It might be convenient to do it on any cd change, so interactive scripts reflect their current location, but I don't think it can be done safely since the terminal control mechanism isn't out-of-band.)

This probably means moving the pwd reporting output back to a precmd hook.

@apjanke
Copy link
Contributor Author

apjanke commented Feb 4, 2015

Interestingly, I just saw today that the zsh FAQ suggests using the chpwd hook this way to emit control characters. http://zsh.sourceforge.net/FAQ/zshfaq03.html#l23 If I find time, I'll have a look at their mailing list and drop them a line about this issue; maybe they could revise it or add a note.

@apjanke
Copy link
Contributor Author

apjanke commented Mar 7, 2015

For future reference: I asked the zsh-workers list about this, and it turns out the FAQ example is fine: it's protected with a [[ -t 1 ]] || return so it only emits the control sequences if the output is connected to a terminal.

http://www.zsh.org/mla/workers/2015/msg00599.html

This might be a better way of doing it than doing the update in precmd since it would reduce a lot of redundant updates. Probably not a performance issue, but could reduce visible flicker in the terminal window, and just seems like a cleaner design.

@mcornella
Copy link
Member

Perfect, so that's going to be our solution. Love it that the answer was so succint and yet so obscure LOL.
👍

@apjanke
Copy link
Contributor Author

apjanke commented Mar 7, 2015

Yeah, I think so. I'll work on another patch for this, but am going to read up on the -t and related stuff first to make sure I get it right; the "is this a terminal?" support is new to me.

@apjanke
Copy link
Contributor Author

apjanke commented Mar 31, 2015

Well, looks like there's another problem with using chpwd, pointed out on the zsh-workers mailing list: if you cd inside a subshell, and then exit the subshell, the terminal pwd won't get update to reflect exiting from the subshell.

More generally, not everything that changes the pwd of the foreground shell process is going to be caused by a cd and trigger the chpwd hook. This could also happen if you had a couple zsh processes or pipelines running as background jobs and switched the foreground job around. Or suspended a subshell.

It looks like precmd is the right place to do this after all: I think we want the terminal to have the current directory of the controlling or foreground interactive shell every time we're looking at a prompt. And just updating on chpwd is not sufficient to get that.

Might still be worth adding the if [[ -t 1 ]] check to the precmd code. It's possible the shell is hooked up to a script or similar, in which case you might not want it cluttered with terminal escape sequences. Though I'm a little leery of setting a precedent like that, because it might interfere with debugging.

For future reference, another workaround might be redirecting the control characters to /dev/tty to force them to go to the terminal instead of a captured stdout.

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 a pull request may close this issue.

2 participants