Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121653002:
Fix prompt exporting escaped characters, opt into PS1 mod. (Closed)
DescriptionFix prompt exporting escaped characters, opt into PS1 mod.
Shells (bash, zsh at least) fail to interpret escaped characters, notably
newline, when evaluating a script via "eval". This means that PS1 prompt
augmentation will break promps containing newlines.
This can be solved by exporting the escaped literals of those escape
characters and using the export FOO=$'...' notation, causing the prompt
to acknowledge and interpret those characters when it evaluates them. This
patch does this for commonly-encountered escaped characters.
BUG=chromium:626210
TEST=local
- Ran without fix, mangles prompt. Ran with fix, doesn't.
Committed: https://chromium.googlesource.com/infra/infra/+/665cd3ab5c23babeaec3594a98a72c0e37b86fad
Patch Set 1 #Patch Set 2 : Actually fix the problem, since people apparently care about this. #Patch Set 3 : Only use $'...' notation if there was actually an escaped character. #
Total comments: 4
Patch Set 4 : Re-enable by default, document the tag, comments. #
Total comments: 4
Patch Set 5 : Splellnig #Patch Set 6 : Fix pylint error. #
Messages
Total messages: 29 (13 generated)
dnj@google.com changed reviewers: + dnj@google.com, nodir@chromium.org, vadimsh@chromium.org
PTAL. This has been annoying me for about a week, I think.
It was not fixed by https://codereview.chromium.org/2089403002/ ? On Sun, Jul 3, 2016 at 6:20 PM <dnj@google.com> wrote: > PTAL. This has been annoying me for about a week, I think. > > https://codereview.chromium.org/2121653002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Remove PS1 prompt modification. The PS1 prompt modification is not compatible with all PS1 values, and has been noted several times in the CL in which it was introduced. Unfortunately, no steps have been taken to fix this, so I am suggesting that we remove it for now to stop the breakage. BUG=None TEST=None ========== to ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. Additionally, PS1 prompt modification was forced onto the user (unless they opt out with an environment variable) by "bootstrap.py" and "env.py". This patch changes this so that the prompt is only modified if the user opts in by defining the "INFRA_PROMPT_TAG" variable. BUG=None TEST=None ==========
On 2016/07/04 02:19:32, nodir wrote: > It was not fixed by https://codereview.chromium.org/2089403002/ ? Nope. I was opting for "this breaks stuff so revert", but it looks like people actually care about this addition, so I've updated this CL to: 1) Fix the underlying problem (see CL description) 2) Make it so the prompt modification is opt-in rather than opt-out. WDYT?
nodir@chromium.org changed reviewers: + tansell@chromium.org
deferring to vadimsh@ and tansell@
It'd be super cool to land this so I can actually use my prompt again :)
LGTM with a couple of minor nits. (I don't think my LGTM will be enough though?) If this works as described, I'm okay with making this compromise. With this version I don't think anyone will discover this functionality, but it was originally added for the quickstart script, so that should be okay. Thanks for taking the time to resolve this issue rather than just reverting or complaining! I was a little worried when I saw the original subject. I think you should use the first line of the text (Fix prompt exporting escaped characters, opt into PS1 mod.) as the subject instead. Couple of other things; * Do we have a policy of prefixing commit messages with `infra:` or `infra/go:` or similar? That is really helpful for us people who work across multiple different repro... * It is generally good to have a bug associated with this. Could we get one? Thanks once again! Tim 'mithro' Ansell https://codereview.chromium.org/2121653002/diff/40001/go/env.py File go/env.py (right): https://codereview.chromium.org/2121653002/diff/40001/go/env.py#newcode35 go/env.py:35: # Replace special characters with their escaped form. This will allow them Can you move this into a function? https://codereview.chromium.org/2121653002/diff/40001/go/env.py#newcode40 go/env.py:40: ('\n', r'\n'), This is kind of confusing, I think ('\n', '\\n') Would probably be clearer.
Description was changed from ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. Additionally, PS1 prompt modification was forced onto the user (unless they opt out with an environment variable) by "bootstrap.py" and "env.py". This patch changes this so that the prompt is only modified if the user opts in by defining the "INFRA_PROMPT_TAG" variable. BUG=None TEST=None ========== to ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. Additionally, PS1 prompt modification was forced onto the user (unless they opt out with an environment variable) by "bootstrap.py" and "env.py". This patch changes this so that the prompt is only modified if the user opts in by defining the "INFRA_PROMPT_TAG" variable. BUG=None TEST=None ==========
Description was changed from ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. Additionally, PS1 prompt modification was forced onto the user (unless they opt out with an environment variable) by "bootstrap.py" and "env.py". This patch changes this so that the prompt is only modified if the user opts in by defining the "INFRA_PROMPT_TAG" variable. BUG=None TEST=None ========== to ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. BUG=None TEST=None ==========
> If this works as described, I'm okay with making this compromise. With this > version I don't think anyone will discover this functionality, but it was > originally added for the quickstart script, so that should be okay. Yeah, the more I think about it, the more I agree that most people will appreciate the prompt. I'll add some documentation about the feature and return to enabled-by-default with the supported ability to set the variable to "" to disable. > * Do we have a policy of prefixing commit messages with `infra:` or `infra/go:` > or similar? That is really helpful for us people who work across multiple > different repro... Not really a formal policy, but I'll go ahead and add it though, since agreed that it is useful. > * It is generally good to have a bug associated with this. Could we get one? Done.
Description was changed from ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. BUG=None TEST=None ========== to ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. BUG=chromium:626210 TEST=None ==========
https://codereview.chromium.org/2121653002/diff/40001/go/env.py File go/env.py (right): https://codereview.chromium.org/2121653002/diff/40001/go/env.py#newcode35 go/env.py:35: # Replace special characters with their escaped form. This will allow them On 2016/07/07 02:09:39, mithro wrote: > Can you move this into a function? Done. https://codereview.chromium.org/2121653002/diff/40001/go/env.py#newcode40 go/env.py:40: ('\n', r'\n'), On 2016/07/07 02:09:38, mithro wrote: > This is kind of confusing, I think > > ('\n', '\\n') > > Would probably be clearer. Done.
Description was changed from ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. BUG=chromium:626210 TEST=None ========== to ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. BUG=chromium:626210 TEST=local - Ran without fix, mangles prompt. Ran with fix, doesn't. ==========
Definitely LGTM (with spelling fixes). (However, I haven't actually tested this myself :). https://codereview.chromium.org/2121653002/diff/60001/go/README.md File go/README.md (right): https://codereview.chromium.org/2121653002/diff/60001/go/README.md#newcode100 go/README.md:100: `go/env.py`, the new environment will include a modified `PS1` proppt containing s/proppt/prompt/ https://codereview.chromium.org/2121653002/diff/60001/go/README.md#newcode103 go/README.md:103: a different value or disabled by exporting an empty value: s/:/./ ?
Thanks for the preview! Ping nodir@ or vadimsh@, this is not a hugely risky CL, let's just land? https://codereview.chromium.org/2121653002/diff/60001/go/README.md File go/README.md (right): https://codereview.chromium.org/2121653002/diff/60001/go/README.md#newcode100 go/README.md:100: `go/env.py`, the new environment will include a modified `PS1` proppt containing On 2016/07/07 04:48:14, mithro wrote: > s/proppt/prompt/ Done. https://codereview.chromium.org/2121653002/diff/60001/go/README.md#newcode103 go/README.md:103: a different value or disabled by exporting an empty value: On 2016/07/07 04:48:14, mithro wrote: > s/:/./ ? Ah that used to have a shell snippet following it, oops!
On 2016/07/07 06:58:06, dnj (Google) wrote: > Thanks for the preview! Ping nodir@ or vadimsh@, this is not a hugely risky CL, > let's just land? > > https://codereview.chromium.org/2121653002/diff/60001/go/README.md > File go/README.md (right): > > https://codereview.chromium.org/2121653002/diff/60001/go/README.md#newcode100 > go/README.md:100: `go/env.py`, the new environment will include a modified `PS1` > proppt containing > On 2016/07/07 04:48:14, mithro wrote: > > s/proppt/prompt/ > > Done. > > https://codereview.chromium.org/2121653002/diff/60001/go/README.md#newcode103 > go/README.md:103: a different value or disabled by exporting an empty value: > On 2016/07/07 04:48:14, mithro wrote: > > s/:/./ ? > > Ah that used to have a shell snippet following it, oops! s/preview/review good typing skills tonight.
lgtm
The CQ bit was checked by dnj@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tansell@chromium.org Link to the patchset: https://codereview.chromium.org/2121653002/#ps80001 (title: "Splellnig")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Presubmit on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Presubmit/build...)
The CQ bit was checked by dnj@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org, tansell@chromium.org Link to the patchset: https://codereview.chromium.org/2121653002/#ps100001 (title: "Fix pylint error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. BUG=chromium:626210 TEST=local - Ran without fix, mangles prompt. Ran with fix, doesn't. ========== to ========== Fix prompt exporting escaped characters, opt into PS1 mod. Shells (bash, zsh at least) fail to interpret escaped characters, notably newline, when evaluating a script via "eval". This means that PS1 prompt augmentation will break promps containing newlines. This can be solved by exporting the escaped literals of those escape characters and using the export FOO=$'...' notation, causing the prompt to acknowledge and interpret those characters when it evaluates them. This patch does this for commonly-encountered escaped characters. BUG=chromium:626210 TEST=local - Ran without fix, mangles prompt. Ran with fix, doesn't. Committed: https://chromium.googlesource.com/infra/infra/+/665cd3ab5c23babeaec3594a98a72... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/infra/infra/+/665cd3ab5c23babeaec3594a98a72... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
