Add a $status_generation variable to track when $status was changed

[soumya92]

Description

Attempt to differentiate between explicitly setting $status and implicitly re-using a previous $status value through a $status_generation electric variable. Currently this only affects backgrounded jobs and variable assignments.

This variable will be automatically incremented after every interactive command, except command & or set foo bar.

This will allow tools like "undistract me" to correctly report the exit status of the last command, by ignoring $status in fish_postexec iff the value of $status_generation is the same in pre and post exec.

It could potentially be extended to support something like return --previous in fish script, if the feature is useful enough to warrant that complexity.

Fixes #6815 #6998

TODOs:

• Changes to fish usage are reflected in user documentation/manpages.
• Tests have been added for regressions fixed
• User-visible changes noted in CHANGELOG.md

[ridiculousfish]

Apologies for delay in review. I recognize the need here and I want to find a solution, but I think this particular approach is too complex. Let's continue discussion in #6815.

[krobelus]

Thanks for your continued effort, this looks great!

I have some minor comments. If you don't want to bother I can do the cleanups.

[soumya92]

Any idea why switching int -> maybe_t in the builtins is causing undefined reference to '__atomic_store' and undefined reference to '__atomic_load' on clang?

I don't know enough C++ to figure this one out. I can confirm that without that change clang configure and make works fine, (even though -latomic is not present in the build flags), and after that change it fails (-latomic is still not present in the build flags).

[krobelus]

The build failure is mysterious.. I could reproduce it at first and now it builds fine with clang.

[soumya92]

Added a change to cmake that I think will fix the build issue on clang

[krobelus]

Alright, I think this is basically ready!

One more thing: the new parameter to fish_postexec could break people who are using $argv instead of $argv[1].
A quick search on grep.app revealed just a couple:

https://github.com/gazorby/fish-abbreviation-tips/blob/b02b13b32db0aedd000e293167cba59e1235ac41/conf.d/abbr_tips.fish
https://github.com/qznc/dot/blob/662f4122a3d01cb088f097c88523edef51bfd65b/config/fish/config.fish
https://github.com/razzius/fish-functions/blob/af3b6b408c2e67254143b732a86f4bb162325298/config.fish
https://github.com/martinklepsch/dotfiles/blob/master/config/fish/functions/notify-me.fish

I guess we can just not care about this, since it's not that many.

However, thinking about this, couldn the interface be better if we expose the holdover status not as hook parameter, but instead - true to its name - in the "status" builtin. I think something like status is-holdover would be sweet, it simplifies fish_prompt since we won't need the postexec hook anymore, and (unlike a special variable) it is super easy to look up what it does with help status or status -h.

(Also if you end up making more changes you can run a git clang-format @{u} but that's not a requirement.)

[soumya92]

Yeah, good point regarding compat of this solution. I don't know if status is-holdover is simple to implement and explain, since it needs to use some reference point for checking the holdover state.

I've implemented @ridiculousfish's suggestion on the linked issue (to use a "generation" variable), which does have the downside of documentation as you mention, but probably works better and was much easier to implement. PTAL, thanks.

[soumya92]

I tried clang-format, but it made no changes. Is there some configuration I need to import?

Rewrite 8f14bf700e9a486b3b97b889e65945521a34b034 (1/5) (0 seconds passed, remaining 0 predicted)    clang-format did not modify any files
Rewrite 3014b94dcddd681219593ffcac59fd8863783c55 (2/5) (1 seconds passed, remaining 1 predicted)    clang-format did not modify any files
Rewrite e61d66669aa5d04903463590678c23795a686c59 (2/5) (1 seconds passed, remaining 1 predicted)    clang-format did not modify any files
Rewrite 4047391a680c7e13d4bc66ada2567e79b88d1078 (2/5) (1 seconds passed, remaining 1 predicted)    no modified files to format
Rewrite 42ceecba55fccb189d5ab9aab9aaed57f59cefee (5/5) (3 seconds passed, remaining 0 predicted)    no modified files to format

WARNING: Ref 'refs/heads/explicit-status-tracking' is unchanged

[krobelus]

Oh maybe my clang-format 10 is newer than yours, don't worry about it.

[krobelus]

Yeah, it looks like status is-holdover would require more explanation, so directly exposing the generation count seems like the simplest solution after all.

We could expose a status status-generation instead of $status_generation, like we have status current-command as a replacement for the old-style $_. The idea is to not clutter the variables with information most users don't need. Similar for $pipestatus, but that's already out there.
Then again, $status_generation fits right next to $status. This solution is also fine by me. Maybe we can have another opinion, @ridiculousfish?

Empty commands don't cause a "holdover" status anymore as they don't fire fish_postexec since #7085. This feels a bit strange but that's very minor.

[soumya92]

Empty commands don't cause a "holdover" status anymore as they don't fire fish_postexec since #7085. This feels a bit strange but that's very minor.

With your suggestion of checking generation in fish_prompt, now they do :)

[ridiculousfish]

I think this is awesome!! I love the subtle tweak to the default prompt. Also really well factored, easily reviewable change set.

$status_generation is going to be a "low level" feature, not something users encounter every day, so it's OK if it's a little un-ergonomic. I think we can improve the name, but I don't want name bikeshedding to block merge, so I'll merge now.

Big thanks to @soumya92 for this contribution!

[soumya92]

Thanks! I'm glad I was able to add this feature, since I will use it a lot :)