Move push_timer to measure command substitution timing too #10466
Conversation
src/parse_execution.rs
Outdated
| @@ -1672,6 +1672,8 @@ impl<'a> ParseExecutionContext { | |||
| job.internal_job_id, | |||
| ); | |||
|
|
|||
| // Start the timer here to ensure command substitution is measured | |||
| let _timer = push_timer(job.wants_timing()); | |||
There was a problem hiding this comment.
the behavior change looks good (bash does the same) although I think we should consolidate the two calls to push_timer() into one. I don't think there is a reason to treat "simple blocks" specially here.
Then we can also get rid of JobProperties::wants_timing.
Once we support timing background functions we would want to add something similar back
There was a problem hiding this comment.
seems fair, I made some changes, but it feels a little weird.
it maintains the current behavior of time begin; echo -n (sleep 1); end & returning timing even though timing a background job is an error, since the block isn't run as a background job
not sure if that should be an error too? technically it doesn't do anything wrong since the block isn't backgrounded, but it feels weird syntactically for that to be accepted
ignore that, not a good morning, i understand whats happening now..
There was a problem hiding this comment.
from what i understand, the timing background rule seems to not apply to blocks?
There was a problem hiding this comment.
begin; end & is not supported yet and the & is simply ignored. So it's fine if the time behavior is weird.
There was a problem hiding this comment.
Okay sure, in that case I think the PR is ready for another review, I don't believe the changes I made differ in behavior for the begin; end & situation
Accurate0
force-pushed
the
measure-command-subst
branch
from
14451ff
to
682b296
Compare
Accurate0
force-pushed
the
measure-command-subst
branch
from
682b296
to
d612bc7
Compare
Description
I've moved the push timer to happen before where command substitution seems to occur, works correctly with the simple repro described on the ticket:
Fixes #9100
TODOs: