Re: [PATCH 2/2] promptinit: Fix prompt cleanups

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Mon, Jan 25, 2021 at 12:17 AM Arseny Maslennikov <ar@xxxxxxxxx> wrote:
>
> The promptinit framework fails to apply cleanup commands of the current
> theme on any theme change other than "prompt restore", as well as when
> invoking setup functions of a certain $theme as an implementation detail
> of `prompt -[hp] $theme'.

Thanks, I did have a chance to look at this.

> We fix it in the following way, hopefully without breaking compatibility:
> * Rename zstyle `cleanup' on the context `:prompt-theme' to `restore'
>   everywhere but in prompt_cleanup(). It is only used as a restore
>   mechanism now.
> * Ensure prompt_cleanup() continues to store its command list in the
>   `cleanup' style.
> * Clean up before theme switch at the end of set_prompt().

These all seem fine, conceptually.

> * Prepend every use of prompt_*_setup (which might modify the shell
>   state in ways that require cleanup) with a cleanup run.

Have you confirmed that this is a no-op when there has not been a
previous installation of a theme?

> * Adjust `prompt restore' to do both parts of the newly split restore
>   mechanism, cleanup first.

Also fine.  Just a couple of nit-picky things about the diff itself:

> diff --git a/Functions/Prompts/promptinit b/Functions/Prompts/promptinit
> index 5e42ebdd3..1c6d27ad7 100644
> --- a/Functions/Prompts/promptinit
> +++ b/Functions/Prompts/promptinit
> @@ -123,6 +125,7 @@ Use prompt -h <theme> for help on specific themes.'
>             # The next line is a bit ugly.  It (perhaps unnecessarily)
>             # runs the prompt theme setup function to ensure that if
>             # the theme has a _help function that it's been autoloaded.
> +           zstyle -t :prompt-theme cleanup
>             prompt_$2_setup
>           fi
>           if functions prompt_$2_help >/dev/null; then

Placement of the added zstyle call makes the comment incorrect, move
it above the comment?

> @@ -179,28 +182,41 @@ Use prompt -h <theme> for help on specific themes.'
> +  # Will add the hook this time.
> +  prompt_cleanup "$@"

This is logically correct, but I don't think it adds much in terms of
maintainability to make this a recursive call; there are only 2
commands executed in that event.