Add fixes for target-shell prompt #943
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #943 +/- ##
==========================================
- Coverage 77.73% 77.72% -0.01%
==========================================
Files 326 326
Lines 28573 28575 +2
==========================================
Hits 22210 22210
- Misses 6363 6365 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| """alias for help""" | ||
| return self.do_help(line) | ||
| self.do_help(line) |
There was a problem hiding this comment.
iirc all Cmd functions should explicitly return a boolean value.
There was a problem hiding this comment.
the do_help of Cmd doesn't return any value: https://github.com/python/cpython/blob/fb0b642bf1aa3ec276304b7170deedd5040c1698/Lib/cmd.py#L292
There was a problem hiding this comment.
That's interesting, perhaps do_help is exempted from this.
There was a problem hiding this comment.
IIRC we decided that everything should be consistent in returning booleans across the board in the refactor.
The standard library isn't, but we are.
There was a problem hiding this comment.
then I'll just return a True in that function and be done with it
| @@ -352,16 +352,14 @@ def __init__(self, target: Target): | |||
| self.histfile = pathlib.Path(getattr(target._config, "HISTFILE", self.DEFAULT_HISTFILE)).expanduser() | |||
|
|
|||
| # prompt format | |||
| self.prompt_ps1 = "\x1b[1;32m{base}\x1b[0m:\x1b[1;34m{cwd}\x1b[0m$ " | |||
| if ps1 := getattr(target._config, "PS1", None): | |||
| if "{cwd}" in ps1 and "{base}" in ps1: | |||
| self.prompt_ps1 = ps1 | |||
|
|
|||
There was a problem hiding this comment.
Perhaps there should be an else here to warn the user of an incomplete PS1 config value?
There was a problem hiding this comment.
That is a good idea, I'll add that
There was a problem hiding this comment.
I added it at L360
Miauwkeru
force-pushed
the
fixes-target-shell
branch
from
58a51a2 to
82f4b4b
Compare
dissect/target/tools/shell.py
Outdated
| """alias for help""" | ||
| self.do_help(line) | ||
| return True |
There was a problem hiding this comment.
I believe it should return False. If a do_ or cmd_ function returns True it is a sign the Cli should exit.
There was a problem hiding this comment.
aah, TIL. Will change it
There was a problem hiding this comment.
Kind of counter intuitive if you ask me, but that's how it works 😅. Perhaps it is inspired by 0 and 1 exit codes.
There was a problem hiding this comment.
Yeah, it definitely feels counterintuitive. I guess its the exit code thing to make it feel like a shell where a non zero value gets treated as an error. Anyway, fixed!
Miauwkeru
force-pushed
the
fixes-target-shell
branch
from
82f4b4b to
b28fc72
Compare
And some miscellaneous fixes of typing and consistency
Co-authored-by: Erik Schamper <[email protected]>
Miauwkeru
force-pushed
the
fixes-target-shell
branch
from
2384f1e to
f70001b
Compare
There was an issue with the
TargetCmdin the case the prompt was defined in the config file, yet didn't contain{cmd}or{base}. This fixes that issueand some fixes for typing consistency