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 the printf unescaping in "_quote_readline_by_ref" #526

Merged
merged 5 commits into from
May 22, 2021

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented May 22, 2021

This is the PR for the fix discussed in the conversation #492 (comment). We want to revert the following change:

-          printf -v value "$value" # Decode escape sequences of \....
+            printf -v value %s "$value" # Decode escape sequences of \....

I have included the tests along with the fix.

conftest.py (52814dd): add an optional argument rendered_cmd

Because I need to make a test that involves a TAB character in the filenames, I would like to add this new argument to support command lines in which the actual input sequence and the result rendered to the terminal are different. For example, suppose we think about a command-line string "echo alpha\\\tb":

  • A. Inputs from a keyboard would be "echo alpha\\\026\tb" i.e., e c h o SP a l p h a \ C-v TAB b
  • B. The internal command-line string becomes "echo alpha\\\tb" i.e., e c h o SP a l p h a \ TAB b
  • C. Bash renders the command in the terminal as "/@echo alpha\\ b" i.e. / @ e c h o SP a l p h a \ SP SP SP b where /@ is the prompt.

The current problem of the testing framework is that it assumes that A == C and just performs bash.expect_exact for the input string A. In this commit, I allow users to optionally specify C through the named argument rendered_cmd="...". As I'm not a native English speaker, I don't have a good sense of choosing the argument name. If you have a better idea for the name of the new argument, could you please specify it?

unit/test_quote_readline.py (608badf): add a failing test

This tests the completion results for "echo alpha\\\tb".

bash_completion (ab58390): fix _quote_readline_by_ref

This is the suggested change of removing %s from printf.

@akinomyoga akinomyoga force-pushed the fix-quote_readline-printf-unescape branch from ab58390 to 63f944f Compare May 22, 2021 08:47
@scop
Copy link
Owner

scop commented May 22, 2021

Cool, thanks. This is pretty intricate stuff, frankly much more low level than I'm comfortable with, but not at all your fault and seems necessary here. Added some lint fixes (highly recommended to install pre-commit, added a blurb about that in e1c400a).

I guess the only bit of an issue I'd like to see addressed is that the rendered cmd appears to assume PS1 length of 2, seems kind of magical. Maybe add some comment clarifying that, or even define it in terms of PS1 length, e.g.

rendered_cmd="echo alpha\\"
rendered_cmd += " " * (8 - (len(conftest.PS1 + rendered_cmd) % 8)) + "b"

...but that's not too clear either, maybe a comment would do better.

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented May 22, 2021

(highly recommended to install pre-commit, added a blurb about that in e1c400a).

Thank you for the instruction! I have installed pre-commit (and also perl-CPAN required by pre-commit, and mypy). But I'm not sure if I correctly understand the usage. If I just run the command pre-commit, everything is skipped. I don't know how to use it but ended up with the following command:

$ pre-commit run --from-ref @~ --to-ref @ -v --show-diff-on-failure --all-files

Is there any other correct (shorter) command to perform the lint check on the previous commit? Or do I need to run this long command every time?

Anyway, some lint errors have been automatically corrected and reflected in the actual files. But there are still errors by mypy, and I don't know how to check the details of the error:

mypy.....................................................................Failed
- hook id: mypy
- duration: 0.03s
- exit code: 1

If I directly run mypy (though I'm not sure if I correctly run mypy), I got the following errors:

$ mypy --config-file=test/setup.cfg test/t/unit/test_unit_quote_readline.py
test/t/unit/test_unit_quote_readline.py:3: error: Skipping analyzing 'pytest': found module but no type hints or library stubs  [import]
test/t/unit/test_unit_quote_readline.py:3: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
test/t/unit/test_unit_quote_readline.py:5: error: Cannot find implementation or library stub for module named 'conftest'  [import]
Found 2 errors in 1 file (checked 1 source file)

Anyway, I push the commit of the current status e4ef29e.


I guess the only bit of an issue I'd like to see addressed is that the rendered cmd appears to assume PS1 length of 2, seems kind of magical.

Yes.

Maybe add some comment clarifying that, or even define it in terms of PS1 length, e.g.

[...]

...but that's not too clear either, maybe a comment would do better.

I added a comment in unit/test_unit_quote_readline.py.

@scop
Copy link
Owner

scop commented May 22, 2021

pre-commit install will install it so that it'll run automatically on commit, in git's pre-commit hook. I've encountered the mypy failure myself too, will have a look sometime.

But very nice, thanks a bunch again!

@scop scop merged commit 8a0334d into scop:master May 22, 2021
@akinomyoga
Copy link
Collaborator Author

pre-commit install will install it so that it'll run automatically on commit, in git's pre-commit hook

Ah, OK. I see. Thank you!

@akinomyoga akinomyoga deleted the fix-quote_readline-printf-unescape branch May 22, 2021 20:42
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.

None yet

2 participants