\D{format} yields the result of calling strftime(3) with the provided
format and the current time.
When PS4 can use this, it will enable us to easily generate timestamps
when tracing script execution.
Details
Details
- Reviewers
jilles pauamma_gundo.com - Group Reviewers
manpages - Commits
- rGa675eaec5aef: sh: implement PS1 \D to print current time
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Place the result of strftime() in the right place instead of always at the beginning of the buffer.
Simplify conditional early exit.
Comment Actions
A little unfortunate to have these strange backslash sequences spread out further, but it's nothing that bash doesn't do.
| bin/sh/eval.c | ||
|---|---|---|
| 764 | This order of operations and lack of escaping means that a parameter expansion in a directory name (\w) will be expanded. If expandstr is extended to PS1 and PS2 and with command substitution, this may lead to command execution from changing into a weirdly-named directory. I haven't thought of a good solution here yet, but this one is not the right one. | |
| bin/sh/sh.1 | ||
| 1426–1429 | and with the current time | |
| 1430 | Brackets are []. | |
| 1431 | Perhaps "Empty" is clearer. | |
Comment Actions
A little unfortunate to have these strange backslash sequences spread out further, but it's nothing that bash doesn't do.
In a way, this has nothing to do with bash. I want sh PS4 to be able to generate some kind of timestamps when executing a script with tracing on. The way bash does it is just a convention which I think is not worth diverging from.
| bin/sh/eval.c | ||
|---|---|---|
| 764 |
Isn't this true already? Currently we call expandstr() on ps4val() and getprompt() will not be much different. | |
| bin/sh/eval.c | ||
|---|---|---|
| 764 | I now understand what you mean. Formatting sequences like \w are first translated to, for example, the CWD, and then subject to parameter expansion. So if CWD is /usr/$X, \w is translated to that and then the $X is expanded. | |
| bin/sh/eval.c | ||
|---|---|---|
| 764 | Perhaps the solution is to escape all $ at the end of getprompt() and then apply expandstr() to both PS1 and PS2? | |
Comment Actions
The more POSIX-like way would be to add some level of command substitution to expandstr() (ensuring set -x doesn't apply to the nested command) and put in something like $(date +%s) in PS4. However, this is slower and harder to implement. It is also more flexible.
| bin/sh/eval.c | ||
|---|---|---|
| 764 | That would be all $ and ` generated by backslash sequences. Actually applying expandstr() to PS1 and PS2 will require some way to deal with re-entering the parser, so it shouldn't be done as part of this review. | |
| bin/sh/sh.1 | ||
|---|---|---|
| 1429 | ||
Comment Actions
Code is OK like this.
| bin/sh/sh.1 | ||
|---|---|---|
| 1426–1430 | Perhaps this could be formulated a bit differently so no prior knowledge of strftime is assumed: "The current time formatted using the .Xr strftime 3 format .Ar format." | |
Comment Actions
In this iteration I tried to address all new feedback.
I also was annoyed by the current formatting of the file, so this is a diff against D37926 where I reduce indentation.
Comment Actions
When I was looking at this code today, I thought that the unconditional memccpy() is a bit wasteful in the \D{} case, because the one copied byte is a special case and when it's noticed, the copied byte is overwriten by strcpy() with "%X".
There is little to be gained in either performance or legibility of the code, but when I rewrote this locally to be less wasteful, it didn't look much worse, nor did it look much better.
Opinions?
Comment Actions
Minutes after posting the previous version, I thought about handling the rare corner case when strftime() returns 0.
Comment Actions
Back out the previous update.
The -1 was there for the i++ that the loop always performs, so it's needed regardless of whether strftime() returned 0 or more.
Split the calculation that was there before into two parts, one of which just decrements i and makes a comment on why it's done.