Implement safe shell reloading inside omf plugin

[derekstavis]

This commit adds an improved reload code for Oh My Fish, besides
saving the history now the reloading technique keeps directory
history and stack and clears fish_greeting, for a transparent
transition.

The reloading code is now safe regarding to background jobs. exec
wipes fish job control, so the user-facing code under the (just-
introduced by this commit) omf reload, and usages under
omf remove and omf update are now kept safe by a warning.

Recap of the commit:

• Add improved reload code (omf.reload)
• Add a safe reload code (omf.cli.reload)
• Add omf reload command

[oranja]

👍 💯 for the code.

The new CLI command needs docs.

[derekstavis]

I always forget the docs

[derekstavis]

Docs are there. Rebased to current master.

[oranja]

Recommends #276

[bobthecow]

I’m a bit hesitant to merge this right now, partially because we haven’t heard from @CoderStephen @bpinto or @scorphus, and partially because refreshing to replace the shell process on omf update has done weird things in the past, and we removed it from update and remove for that reason.

Since core doesn’t call refresh anywhere, I was okay deprecating it before adding reload. I’m also okay adding an explicit reload function. I’m just not 100% comfortable making the decision to run it automatically on every update and remove.

If we remove the calls from omf.cli.remove and omf.cli.update, I’m happy to merge this right now. Let’s to that and ship the function, then get more general buy-in on doing it automatically every update and remove?

[bobthecow]

Or we could add a config var so we could all opt in to the automatic reload, but not inflict it on our users for a bit longer. I think I like that best of all.

[bpinto]

👍 I like the config var option.

[derekstavis]

(...) refreshing to replace the shell process on omf update has done weird things in the past

This PR works hard to fix the side effects of exec. I had already fixed lost history in the past, and now I'm also addressing two major concerns of exec:

• Keep directory history/stack between old/new session
• Don't exec if background jobs are found

This makes the reload process very safe regarding to shell state.

(...) more general buy-in on doing it automatically every update and remove?

The expected behaviour of omf remove fonts is to, right after its completion, have fonts command unavailable. The same is true when you omf update. It's expected to see the shiny new feature of updated packages available right after update is finished. Because of how autoloading works, not reloading the shell after an omf update also imposes a risk of having version mismatch between already sourced functions and the updated ones.

(...) add a config var so we could all opt in to the automatic reload

Automatic reload is a great feature. I remember friends of mine hating legacy omf because of the constant shell close/reopen, before I taught them exec. If the automatic behaviour bothers or harm someone, it's a bug in reload process, so in this case I'd prefer an opt-out config var -- but more than a config var, I'd prefer having the bug reported and somehow fixed.

[sagebind]

I'm actually going to agree with @derekstavis on this. Not only would omf remove fonts be confusing if some of the package's functions are still available, it could potentially cause severe errors with the package if you do try to run said functions; dependent functions might not be available or an uninstall hook might have removed some necessary files.

Adding a config var to disable automatic reload would be OK I guess, but I prefer things to just work simply without configuration.

If omf update has buggy behavior with automatic reload that omf update; omf reload doesn't manually, then that would be a bug somewhere. I reload my shell a lot (I have it key-bound actually) because when things change in the environment a lot, it helps to prevent accidental bugs or misbehaving.

[sagebind]

With clarification from @bobthecow, I suggest we introduce a config variable for now that enables automatic reload. The variable must be set to enable the feature; then it can undergo a lot of use over the next few months when we can eventually enable it permanently.

I'm 👍 for a merge after this is done.