Detect $status being carried over from last command

[soumya92]

fish, version 3.1.0

I did not expect this:

soumya@machine /h/soumya> false
soumya@machine /h/soumya [1]> set foo bar
soumya@machine /h/soumya [1]> sleep 2 &
Job 1, 'sleep 2 &' has ended
soumya@machine /h/soumya [1]>

soumya@machine /h/soumya> true
soumya@machine /h/soumya> set foo bar
soumya@machine /h/soumya> sleep 2 &
Job 1, 'sleep 2 &' has ended
soumya@machine /h/soumya> 

I understand why this is happening, since both set and & do not modify the status. And that behaviour is great for scripts.

But it was still surprising to me that the [1] carried over in the prompt. Would it be possible for $status to be cleared somehow in preexec, so that it's only set to a non-zero value if it was explicitly caused by the last command? Or introduce a variable that could indicate that the previous command had no status?

(I tried set status 0 in --on-event fish_preexec, but as expected, that did not work)

[faho]

Would it be possible for $status to be cleared somehow in preexec

No. We show $status as it is, anything else is more confusing, because now interactive fish and script-fish behave differently.

[soumya92]

Fair enough. What about introducing a variable to detect "empty status"? Then it could be up to prompt authors to handle that specially (e.g. indicate that the status "carried over"), without affecting the default prompt.

[soumya92]

The motivation behind this request is that I print a "summary" for commands that don't exit 0, indicating the error code and time. But it's weird to see " ✘ 1 ⬥ set ⬥ 12:04:20" since set did not exit with 1.

[soumya92]

anything else is more confusing, because now interactive fish and script-fish behave differently.

Actually, thinking about this some more, I don't agree completely. My mental model is that at the prompt, the status is "clean", and the displayed status is whatever the result of the previous command was. I don't consider my entire shell session similar to a single fish script, but rather I consider it similar to a bunch of tiny fish scripts.

So in my mind, the return value of a single command in interactive mode would be the same as the return value of fish -c "$commandline", and fish -c "set foo bar" returns 0.

But this is tangential, it's entirely possible that my mental model is in the minority, and adjusting that is not a big deal. The only problem here is that there's no way for me to detect the empty/carried-over status.

[faho]

I don't consider my entire shell session similar to a single fish script, but rather I consider it similar to a bunch of tiny fish scripts.

So in my mind, the return value of a single command in interactive mode would be the same as the return value of fish -c "$commandline", and fish -c "set foo bar" returns 0.

I would argue that your mind is... well, wrong. The commandline is positively dripping in state that carries over. Any set will affect all later lines, even if it's just local. Running fish -c twice should basically do the same thing (unless any global state changed, like exported variables or the working directory or files or something), but running two commands will often work differently.

Your shell session is a single fish script. It's "read line, execute line, read line, execute line". I don't think papering over that does any good.

[soumya92]

Huh, I didn't realise set -l would persist across the entire session. That definitely makes me rethink how sessions work 😄

In any case, all I really need here is some way to detect that the last command had an empty status.

I guess it's possible to hack that with some fish_indent and string checks to see if all commands were set without -q or backgrounded, but that sounds brittle, and it would be easier to just check (e.g.) $emptystatus. WDYT?

[soumya92]

Hmm, set explicitly returns the previous status, and it's not just a carried over status. So a simplistic solution would only work for the '&' case.

For set we would need some form of "optional" return, which seems like a rather large change to support just the one builtin command. 🤔

I'm out of ideas on this problem. For now I'll just hack it with some checks against argv in my postexec function, but it would be nice to have a proper solution.

[soumya92]

Nope, can't hack it in postexec either 😞

Consider the pathological case:

function return2; return 2; end; return2
set $varname bar

If varname has invalid characters, then the exit status of 2 is actually caused by set, otherwise it's caused by the preceding command "return2". Since there's no way to know that from just the commandline, any attempts to do so will be wrong under certain circumstances. So any changes to support this must happen in the shell itself.

[soumya92]

Okay, I hacked something together in #6820 that "works on my machine":tm:

This is just a proof of concept, but it could help further discussions by providing a concrete example implementation.

[krobelus]

I agree that the carried-over status is irritating and we should make it feel more natural. Reminds me of this thread on twitter.

Changing the language semantics or lying about $status is probably off-limits.
One idea would be to change the color of the status to something more modest, if the status was not written to. This would also communicate the fact.
Example:

printf '$ false\n'
printf '%s[1]%s $ set -l a b\n' (set_color --bold red) (set_color normal)
printf '%s[1]%s $\n' (set_color $fish_color_autosuggestion) (set_color normal)

[soumya92]

One idea would be to change the color of the status to something more modest, if the status was not written to. This would also communicate the fact.

That sounds good to me, even though I'm not very invested in what the default prompt looks like 😉

In my current implementation, that could be set status_was_written $argv[2] in fish_postexec, and then use that in fish_prompt to determine whether the status should be dimmed.

[soumya92]

Another point in favour of adding some form of optional return, at least for the builtins, is $pipestatus. Consider this hypothetical naive function that logs commands to some external source, maybe with different persistence for successful vs failed commands.

Because log_success/log_failure will change (pipe)status, the author makes sure the first thing they do is save the statuses. And because set does not change the status, this looks correct:

function log_and_summarize -a $cmdline
    set prev_status $status
    set prev_pipestatus $pipestatus
    if test $prev_status -eq 0
        log_success $cmdline
    else
        log_failure $cmdline
    end
    echo "exit(s): $prev_pipestatus"
end

However, this function will always print only one value in "exit(s):", regardless of how long the previous pipeline was. That's because set doesn't actually avoid modifying $status, it explicitly sets it to the same value. But it cannot do that for $pipestatus, since that's outside set's control. So the first set has clobbered $pipestatus.

The fix in this instance is to first save $pipestatus, and then save $status, but that's pretty surprising. It's not at all clear that the order of those two set commands is significant.

For this reason, I strongly think optional return (at the very least for the set builtin, but maybe for other fishscript functions) is worth adding. In the interest of simplicity and correctness, I think it would be better to actually re-use the previous status value (at the C++ level), rather than try to fake it with the set prev_status $status; ...; return $prev_status trick.

[krobelus]

In bash an assignment statement like a=b stomps both $? and $PIPESTATUS.
In zsh, an assignment doesn't modify either $status or $pipestatus.
Fish is odd here, because set a b keeps $status and writes $pipestatus. This is probably a bug.

I think it would be better to actually re-use the previous status value (at the C++ level)

Yeah, I think that's what we need to fix the above: keep track of whether a command in a job actually changed $status, and if it did, update the pipestatus.

I'm not sure what you mean by optional return for set. The builtin eval and & would need the same.
I haven't checked the details, but wouldn't it be the easiest to expose a counter $fish_status_generation that increments each time the status is actually written.

[soumya92]

optional return for set

As opposed to blanket not returning from set, since there are some cases where set needs to return values (set -q, for example).

& is handled in my proposed patch.

I'm not sure about how to handle eval, which might need a larger discussion outside the scope of this issue. For example, after eval "true|false" | true | false, is $pipestatus [0 1 0 1] or [1 0 1]?

expose a counter $fish_status_generation that increments each time the status is actually written.

It's possible my approach was overly complicated, but on my first attempt to get this working I discovered that there are many, many instances of $status being written during the regular fish_ functions (fish_title, fish_prompt, etc.), so the counter value would require similar filtering to what I've already implemented.

At that point it's just a matter of whether you prefer having a second argument to postexec, or whether you prefer to have the user store and later compare the value of $fish_status_generation. I would rather provide this information in postexec because that makes it clearer that this is an interactive-mode-only feature, but I have no strong preference as long as the information is available somehow.

[ridiculousfish]

For this issue, I think it would make sense to add a new status variable alongside $pipestatus and $status.

Perhaps $last_status, as parallel to $last_pid. The semantics would be that $last_status reflects only the last command run, and is never carried over.

[soumya92]

There are some complexities around introducing a new variable (either the "generation" or the "last_status"), when it comes to their values in the middle of a single prompt vs. multiple prompts.

For example, if I run false; set foo bar, vs. false and then set foo bar on two different prompts, should the $status and $last_status variables be the same after both cases, or different? By adding a parameter to postexec instead, we can completely avoid this issue.

This also doesn't address the inconsistency between status and pipestatus when running set foo bar. Most probably it's desirable to preserve the current status behaviour, so the solution would have to carry over pipestatus as well, which brings back much of the complexity from my PR.

And because of the way set is currently implemented, any simple solution will not work for it because it actually explicitly returns the same status code as what was previously stored. (i.e. the C++ equivalent of function set -a var value; set -l old_status $status; set $var value; return $old_status; end). Because of this, any system we use to track when to set and when to not set $last_status will not work for set without some additional complicated changes to the builtin.

It's possible I'm too focussed on one approach and I'm missing a much simpler solution. I'd love to hear your ideas on this topic.

[soumya92]

Referencing the PR, the first commit (223ab32) only handles the '&' case. This can be reasonably re-purposed to change last_status or update a generation variable instead (after resolving the multiple commands in one prompt issue).

The second commit (c41568d) is the not-as-nice code (in my opinion) that ends up making set work with the first commit. Without that, set foo bar will still be treated as a command that explicitly set status, because the builtin returned a value.

If your concern is primarily with the first commit, then I can easily make changes to accommodate your proposal (with some discussion on how you want to handle non-interactive mode, since it's basically tied to the prompt rendering right now).

[soumya92]

@ridiculousfish Have you had a chance to look at this? I'd like to hear what your thoughts are on the problem I ran into with the set case, and if you think there's a better way to fix the issue.

[ridiculousfish]

@soumya92 Thank you for your patience, I am catching up. I like the status_was_set approach, I think the PR can be made very simple:

1. Add the bool status_was_set or perhaps status_is_holdover to statuses_t. The idea is that anything that sets statuses_t will indicate if the value is new, or a holdover.

2. In job_t::continue_job() there's code that sets the status if the job is foreground and completed. Here's where you could add an else branch that gets the current statuses, marks them as a holdover, and then sets them back. That will take care of both non-foreground and stopped jobs.

That leaves set, and I think what you have done with adding -1 is OK for now. Ultimately I think we want to change the return type of all builtins to be a struct, and it could have a field status_is_holdover. I'll leave it up to you whether you want to tackle that or keep the -1 approach.

Thank you for the great contribution!

[soumya92]

@ridiculousfish Not a problem! Just making sure that the issue was not abandoned. I'll try to get something out this week that uses the approach you've outlined.

I think the difference between handling -1 and handling a status struct as the return value of builtins is probably fairly small, so I will try to make that update as part of the set changes and see how it goes.

[soumya92]

Okay, so I tried the job_t::continue_job approach, but that fails the case of true; sleep 1 &, by marking that as a holdover, even though the $status = 0 comes from true.

I think this is because

fish-shell/src/reader.cpp

Line 2199 in 3571754

parser.eval(cmd, io_chain_t{});

causes multiple job_t to be produced, and the last one ends up writing a held-over status, even though that status came from the one just before in the same commandline.

So I think I need to be able to track whether the status was set at any point in evaluating cmd there, which is most of what my previous commit did. A better approach could be to return all statuses_t for semicolon or newline separated portions of the commandline, and then check if any of them are not holdovers. But currently the statuses_t from true in true; sleep 1 & is lost, so this is not possible. add a field to parser_res_t that indicates whether status was set.

Before I go too deeply down a rabbit hole again, what are your thoughts on this matter?

[ridiculousfish]

I get it - from fish's point of view these are indeed separate jobs, but as a user you would like an indication that that the status was set since the last prompt. How about a generation count, then? Every time a new status is set, increment the number. Then if the number hasn't changed since the last prompt, the status is a holdover.

The gen count could potentially live in parser_t::library_data_t, we may even be able to combine with exec_count which serves a similar purpose.

[soumya92]

Ohh, yeah, that makes sense. For some reason when that was suggested earlier I just did not make the leap of putting in parser's libdata. I will put something together for review shortly.