[RFC][PATCH] Add zrestart()

[RFC][PATCH] Add zrestart()

[Marlon Richert]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

Attached patch adds the zrestart() function that was discussed
previously in workers/48408 and before (and also after).

It was suggested originally that `zle -fn` be used to validate the
dotfiles. However, I discovered that this can fail needlessly, for
example, if a dotfile references dynamically named directories.
Instead, to verify that the shell can restart without errors, I start
an interactive subshell with an empty command and capture its return
value and standard error.

Likewise, it was suggested at some point that some form of `zsh -l &&
exit` be used instead of `exec zsh`. However, it was later brought up
that, if the user would restart the shell many times this way, this
could potentially exhaust the available process IDs. Thus, I use `exec
zsh` after all. Hopefully, the validation above provides enough
safeguards. At least the user doesn't have to worry about mistyping
`zsh` as the argument to `exec` this way.

[-- Attachment #2: 0001-Add-zrestart.txt --]
[-- Type: text/plain, Size: 2256 bytes --]

From f0fbd0f7769d675620168d4109a9a1e7410f9e78 Mon Sep 17 00:00:00 2001
From: Marlon Richert <marlon.richert@gmail.com>
Date: Sat, 24 Apr 2021 23:34:47 +0300
Subject: [PATCH] Add zrestart()

---
 Doc/Zsh/contrib.yo      |  5 +++++
 Functions/Misc/zrestart | 42 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 Functions/Misc/zrestart

diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo
index 8bf1a208e..efd190f7a 100644
--- a/Doc/Zsh/contrib.yo
+++ b/Doc/Zsh/contrib.yo
@@ -4649,6 +4649,11 @@ See `Recompiling Functions'
 ifzman(above)\
 ifnzman((noderef(Utilities))).
 )
+findex(zrestart)
+item(tt(zrestart))(
+This function tests whether the shell is able to restart without error and, if 
+so, restarts the shell.
+)
 findex(zstyle+)
 item(tt(zstyle+) var(context) var(style) var(value) [ tt(+) var(subcontext) var(style) var(value) ... ])(
 This makes defining styles a bit simpler by using a single `tt(+)' as a
diff --git a/Functions/Misc/zrestart b/Functions/Misc/zrestart
new file mode 100644
index 000000000..1fc17c03b
--- /dev/null
+++ b/Functions/Misc/zrestart
@@ -0,0 +1,42 @@
+#
+# Restarts the shell if and only if it can restart without error.
+#
+
+emulate -LR zsh
+{
+  # Some users export $ZDOTDIR, which can mess things up.
+  local zdotdir=$ZDOTDIR
+  unset ZDOTDIR
+  
+  print 'Validating...'
+  
+  # Try if the shell can start up without errors. Passing an empty command 
+  # ensures that the subshell exits immediately after executing all dotfiles.
+  # We suppress standard out, since we're interested in standard error only.
+  setopt multios
+  local err="$(zsh --interactive --monitor --zle -c '' 2>&1 > /dev/null)"
+  local -i ret=$?
+  
+  if (( ret )) || [[ -n $err ]]; then
+    [[ -n $err ]] &&
+        print -ru2 -- "$err"
+    print -nu2 'Validation failed'
+    (( ret )) &&
+        print -nu2 " with exit status $ret"
+    [[ -n $err ]] &&
+        print -nu2 '. Please fix the error(s) above'
+    print -u2 '.'
+    print -u2 'Restart aborted.'
+    
+    (( ret )) ||
+        (( ret = 64 ))  # EX_DATAERR; see `man 3 sysexits`.
+        
+    return ret
+    
+  else
+    print 'Restarting...'
+    exec zsh
+  fi
+} always {
+  ZDOTDIR=$zdotdir
+}
-- 
2.31.1

Re: [RFC][PATCH] Add zrestart()

[Daniel Shahaf]

> Subject: [PATCH] Add zrestart()

Nitpick, but the parentheses are wrong.  They imply zrestart() is a C
function, which it isn't.

> +++ b/Doc/Zsh/contrib.yo
> @@ -4649,6 +4649,11 @@ See `Recompiling Functions'
>  ifzman(above)\
>  ifnzman((noderef(Utilities))).
>  )
> +findex(zrestart)
> +item(tt(zrestart))(
> +This function tests whether the shell is able to restart without error and, if 
> +so, restarts the shell.
> +)
> +++ b/Functions/Misc/zrestart
> @@ -0,0 +1,42 @@
> +#
> +# Restarts the shell if and only if it can restart without error.
> +#
> +
> +emulate -LR zsh
> +{
> +  # Some users export $ZDOTDIR, which can mess things up.
> +  local zdotdir=$ZDOTDIR
> +  unset ZDOTDIR

The comment implies those users are doing something unusual, which isn't
the case.  Exporting ZDOTDIR is normal.

More importantly, I don't see any reason to munge $ZDOTDIR in the first
place.  It amounts to second-guessing the user.

> +  print 'Validating...'

Error and progress messages should name their originator.

> +  # Try if the shell can start up without errors. Passing an empty command 
> +  # ensures that the subshell exits immediately after executing all dotfiles.
> +  # We suppress standard out, since we're interested in standard error only.
> +  setopt multios

This option will already be on, and in any case isn't required for the
next line.

> +  local err="$(zsh --interactive --monitor --zle -c '' 2>&1 > /dev/null)"

This doesn't necessarily restart the _same_ zsh, if there's more than
one installed.

Also, I think it's quite a stretch to describe this line as "_tests_
whether the shell is able to restart".  This line executes a whole bunch
of code you have no control over.  I don't think that's an acceptable
approach.

Re: [RFC][PATCH] Add zrestart()

[Marlon Richert]

On Mon, Apr 26, 2021 at 6:22 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > +{
> > +  # Some users export $ZDOTDIR, which can mess things up.
> > +  local zdotdir=$ZDOTDIR
> > +  unset ZDOTDIR
>
> More importantly, I don't see any reason to munge $ZDOTDIR in the first
> place.  It amounts to second-guessing the user.

It's a workaround for the following problem that I've observed in the wild:
1. $HOME/.zshenv does `export ZDOTDIR=foo` and <bar>.
2. $ZDOTDIR/.zshrc does something that relies on <bar> having happened.
3. User calls zrestart (or just `exec zsh`, for that matter).
4. The new shell now looks for foo/.zshenv instead of $HOME/.zshenv,
but no such file exits.
5. .zshrc throws an error because <bar> hasn't happened in this shell.


> > +  local err="$(zsh --interactive --monitor --zle -c '' 2>&1 > /dev/null)"
>
> Also, I think it's quite a stretch to describe this line as "_tests_
> whether the shell is able to restart".

I originally had `zsh -fn <all the dotfiles>`, along the lines of what
was suggested earlier, but that test can fail on a valid dotfile that
uses dynamically named dirs. Plus, if any dotfile sources other files,
those files aren't checked this way at all. The approach above is the
only one I've found so far that appears to be completely reliable in
determining whether the shell can start up successfully.


> This line executes a whole bunch of code you have no control over.

It is, however, exactly the code we want to test here.


> I don't think that's an acceptable approach.

What kind of approach would be acceptable?

Re: [RFC][PATCH] Add zrestart()

[Daniel Shahaf]

Marlon Richert wrote on Mon, 26 Apr 2021 19:03 +00:00:
> On Mon, Apr 26, 2021 at 6:22 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > > +{
> > > +  # Some users export $ZDOTDIR, which can mess things up.
> > > +  local zdotdir=$ZDOTDIR
> > > +  unset ZDOTDIR
> >
> > More importantly, I don't see any reason to munge $ZDOTDIR in the first
> > place.  It amounts to second-guessing the user.
> 
> It's a workaround for the following problem that I've observed in the wild:
> 1. $HOME/.zshenv does `export ZDOTDIR=foo` and <bar>.
> 2. $ZDOTDIR/.zshrc does something that relies on <bar> having happened.
> 3. User calls zrestart (or just `exec zsh`, for that matter).
> 4. The new shell now looks for foo/.zshenv instead of $HOME/.zshenv,
> but no such file exits.
> 5. .zshrc throws an error because <bar> hasn't happened in this shell.

If the user set up their dotfiles such as typing «zsh» at the prompt
doesn't launch zsh, that's not your problem to fix.  Drop the unset.

(And anyway, even if you didn't drop the unset, you'd still have to make
it local to the function in case it returns.)

> > > +  local err="$(zsh --interactive --monitor --zle -c '' 2>&1 > /dev/null)"
> >
> > Also, I think it's quite a stretch to describe this line as "_tests_
> > whether the shell is able to restart".
> 

Care to comment about the part of my answer before the "Also", which you
had snipped?

> I originally had `zsh -fn <all the dotfiles>`, along the lines of what
> was suggested earlier, but that test can fail on a valid dotfile that
> uses dynamically named dirs.

A minimal example of this would not be out of place.

> Plus, if any dotfile sources other files,
> those files aren't checked this way at all. The approach above is the
> only one I've found so far that appears to be completely reliable in
> determining whether the shell can start up successfully.
> 
> 
> > This line executes a whole bunch of code you have no control over.
> 
> It is, however, exactly the code we want to test here.
> 

I'm aware.  However, you aren't "testing" it, you are *running* it.

First, that means the docs are wrong.

Second, that code might do things that are inappropriate for the use-case
of "testing" the startup code.

Or, in other words: the trick is to throw the bathwater out and
keep the baby.  Keeping *both* the baby and the bathwater isn't an
ideal solution.

> 
> > I don't think that's an acceptable approach.
> 
> What kind of approach would be acceptable?

I think you've basically run into the halting problem here, so, try
a different universe.  The current one was compiled with some funny
#define's.

Daniel

Re: [RFC][PATCH] Add zrestart()

[Bart Schaefer]

On Mon, Apr 26, 2021 at 12:30 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> > What kind of approach would be acceptable?
>
> I think you've basically run into the halting problem here

Consequently we need to decide collectively whether we're just
rejecting "zrestart" as impossible, or if there's a sub-optimal
solution we can agree on.  I suspect that executing the entire startup
twice in order to be sure it'll work once, is not one we'll agree on.

Not really a solution, but an interesting (?) observation:

If you start an interactive shell in the background, it will stop when
it tries to print the first prompt:

% echo $$
277859
% Src/zsh -f &
[1] 277861
%
[1]  + suspended (tty output)  Src/zsh -f

You can then do:

% exec fg
[1]  + continued  Src/zsh -f
% echo $$
277861
%

Now you're at the prompt for the previously backgrounded shell.  If
you exit from that, the parent is gone.

So if you had some way to detect that a backgrounded shell had
actually reached the PS1 prompt, you could cause the parent shell to
replace itself.

Re: [RFC][PATCH] Add zrestart()

[Marlon Richert]

On Mon, Apr 26, 2021 at 10:29 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > > > +  local err="$(zsh --interactive --monitor --zle -c '' 2>&1 > /dev/null)"
> > >
> > > Also, I think it's quite a stretch to describe this line as "_tests_
> > > whether the shell is able to restart".
> >
>
> Care to comment about the part of my answer before the "Also", which you
> had snipped?

Sure:

On Mon, Apr 26, 2021 at 6:22 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > +  local err="$(zsh --interactive --monitor --zle -c '' 2>&1 > /dev/null)"
>
> This doesn't necessarily restart the _same_ zsh, if there's more than
> one installed.

I think your observation is correct and I was planning to fix it in
the next version of my patch. :)

When I don't reply to a point you make, it means that I either agree
or think it's not worth arguing about, and I've put it on my TODO list
for the next version of my patch. I was trying to be brief and not
post a bunch of agrees/will-dos or quote unnecessarily. But if it's
prefered that I do respond to every point I agree with and/or quote
each part of every email I reply to, just let me know. :)


> > I originally had `zsh -fn <all the dotfiles>`, along the lines of what
> > was suggested earlier, but that test can fail on a valid dotfile that
> > uses dynamically named dirs.
>
> A minimal example of this would not be out of place.

% zsh_directory_name() { [[ $2 == home ]] && reply=($HOME) }
% cd ~[home]; print $?
0
% print 'zsh_directory_name() { [[ $2 == home ]] && reply=($HOME) }
cd ~[home]' > tmp
% zsh -fn tmp
tmp:2: no directory expansion: ~[home]
%


> > Plus, if any dotfile sources other files,
> > those files aren't checked this way at all. The approach above is the
> > only one I've found so far that appears to be completely reliable in
> > determining whether the shell can start up successfully.
> >
> > > This line executes a whole bunch of code you have no control over.
> >
> > It is, however, exactly the code we want to test here.
>
> I'm aware.  However, you aren't "testing" it, you are *running* it.
>
> First, that means the docs are wrong.
>
> Second, that code might do things that are inappropriate for the use-case
> of "testing" the startup code.

When you execute `make check`, does it not _run_ the code? How else
are you going to test it? :)


> Or, in other words: the trick is to throw the bathwater out and
> keep the baby.  Keeping *both* the baby and the bathwater isn't an
> ideal solution.

What exactly here is the "bathwater"? Would it help to mock out
certain features in the subshell that you wouldn't want to actually
run in this test? Would it help to invoke the subshell with -o
RESTRICTED?

Re: [RFC][PATCH] Add zrestart()

[Marlon Richert]

On Tue, Apr 27, 2021 at 2:55 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Mon, Apr 26, 2021 at 12:30 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > > What kind of approach would be acceptable?
> >
> > I think you've basically run into the halting problem here
>
> Consequently we need to decide collectively whether we're just
> rejecting "zrestart" as impossible, or if there's a sub-optimal
> solution we can agree on.  I suspect that executing the entire startup
> twice in order to be sure it'll work once, is not one we'll agree on.
>
> Not really a solution, but an interesting (?) observation:
>
> If you start an interactive shell in the background, it will stop when
> it tries to print the first prompt:
>
> % echo $$
> 277859
> % Src/zsh -f &
> [1] 277861
> %
> [1]  + suspended (tty output)  Src/zsh -f
>
> You can then do:
>
> % exec fg
> [1]  + continued  Src/zsh -f
> % echo $$
> 277861
> %
>
> Now you're at the prompt for the previously backgrounded shell.  If
> you exit from that, the parent is gone.
>
> So if you had some way to detect that a backgrounded shell had
> actually reached the PS1 prompt, you could cause the parent shell to
> replace itself.

Failing that, perhaps a better option would just be to add a note in
the newuser .zshrc to use `zsh` or `zsh && exit` to apply changes?

Or perhaps zrestart could be just this:

zrestart() {
  exec zsh
}

At least then the user has no risk of mistyping the argument to exec.

By the way: What kind of error in the dotfiles would cause the shell
not to be able to start?

Re: [RFC][PATCH] Add zrestart()

[Roman Perepelitsa]

On Tue, Apr 27, 2021 at 1:43 PM Marlon Richert <marlon.richert@gmail.com> wrote:
>
> By the way: What kind of error in the dotfiles would cause the shell
> not to be able to start?

Adding the following line to zshrc should do it:

    exec true

Something like this does happen in the real world. E.g., it's not
uncommon to exec into tmux from rc files.

Roman.

Re: [RFC][PATCH] Add zrestart()

[Marlon Richert]

On Tue, Apr 27, 2021 at 2:49 PM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> On Tue, Apr 27, 2021 at 1:43 PM Marlon Richert <marlon.richert@gmail.com> wrote:
> >
> > By the way: What kind of error in the dotfiles would cause the shell
> > not to be able to start?
>
> Adding the following line to zshrc should do it:
>
>     exec true
>
> Something like this does happen in the real world. E.g., it's not
> uncommon to exec into tmux from rc files.

Thanks! Now I know how to test that. And now I also know that my test
doesn't catch that. :(

Back to the drawing board…

Re: [RFC][PATCH] Add zrestart()

[Marlon Richert]

On Tue, Apr 27, 2021 at 8:49 PM Marlon Richert <marlon.richert@gmail.com> wrote:
>
> On Tue, Apr 27, 2021 at 2:49 PM Roman Perepelitsa
> <roman.perepelitsa@gmail.com> wrote:
> >
> > On Tue, Apr 27, 2021 at 1:43 PM Marlon Richert <marlon.richert@gmail.com> wrote:
> > >
> > > By the way: What kind of error in the dotfiles would cause the shell
> > > not to be able to start?
> >
> > Adding the following line to zshrc should do it:
> >
> >     exec true
> >
> > Something like this does happen in the real world. E.g., it's not
> > uncommon to exec into tmux from rc files.
>
> Thanks! Now I know how to test that. And now I also know that my test
> doesn't catch that. :(

Bart's solution fails for that case, too, btw:

% cd $(mktemp -d)
% HOME=$PWD ZDOTDIR=$PWD exec zsh -f
% print 'exec true' >| .zshrc
% exec zsh -d &
[1] 81931
%
[1]  + suspended (tty output)  exec zsh -d
% exec fg
[1]  + continued  exec zsh -d
zsh: can't set tty pgrp: interrupt

[Process completed]

Re: [RFC][PATCH] Add zrestart()

[Bart Schaefer]

On Tue, Apr 27, 2021 at 10:58 AM Marlon Richert
<marlon.richert@gmail.com> wrote:
>
> Bart's solution fails for that case, too, btw:

Well, you can't just wait for the SIGTTOU, you also have to guarantee
that the PS1 prompt has been printed.  That's the tricky bit.

Re: [RFC][PATCH] Add zrestart()

[Daniel Shahaf]

Marlon Richert wrote on Tue, Apr 27, 2021 at 14:42:47 +0300:
> On Tue, Apr 27, 2021 at 2:55 AM Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > On Mon, Apr 26, 2021 at 12:30 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > >
> > > > What kind of approach would be acceptable?
> > >
> > > I think you've basically run into the halting problem here
> >
> > Consequently we need to decide collectively whether we're just
> > rejecting "zrestart" as impossible, or if there's a sub-optimal
> > solution we can agree on.  I suspect that executing the entire startup
> > twice in order to be sure it'll work once, is not one we'll agree on.
> >
> > Not really a solution, but an interesting (?) observation:
> >
> > If you start an interactive shell in the background, it will stop when
> > it tries to print the first prompt:
> >
> > % echo $$
> > 277859
> > % Src/zsh -f &
> > [1] 277861
> > %
> > [1]  + suspended (tty output)  Src/zsh -f
> >
> > You can then do:
> >
> > % exec fg
> > [1]  + continued  Src/zsh -f
> > % echo $$
> > 277861
> > %
> >
> > Now you're at the prompt for the previously backgrounded shell.  If
> > you exit from that, the parent is gone.
> >
> > So if you had some way to detect that a backgrounded shell had
> > actually reached the PS1 prompt, you could cause the parent shell to
> > replace itself.
> 
> Failing that, perhaps a better option would just be to add a note in
> the newuser .zshrc to use `zsh` or `zsh && exit` to apply changes?
> 
> Or perhaps zrestart could be just this:
> 
> zrestart() {
>   exec zsh
> }
> 
> At least then the user has no risk of mistyping the argument to exec.

Hang on; these are two separate problems.

1. «exec typo».  We could add a setopt that makes this not kill the
shell.  That'd be analogous to execve(3):

    RETURN VALUE
           On success, execve() does not return, on error -1 is returned, and errno is set appropriately.

2. «exec zsh» that then immediately exits non-zero.  I'd argue this
should be fixed in the terminal emulator by configuring it to not close
a tab when the command in that tab exited non-zero — for instance,
because that usually causes the command's stderr to be lost.  (Compare
tmux's remain-on-exit option, albeit that one isn't conditional on the
exit code.)

----

For (2), there may be some fancier way along the lines of:
.
    # Arrange for the timestamp of $SOME_WELL_KNOWN_FILENAME to
    # approximate the last successful start of an interactive shell
    pre${either_cmd_or_exec_but_I_forget_which}() {
        touch -- $SOME_WELL_KNOWN_FILENAME
        add-zsh-hook -d -- … ${funcstack[1]}
    }
.
and then in .zshrc:
.
    if (( ${foo}++ == 0 )) && [[ ${ZDOTDIR:-${HOME:-/dev/null}}/.zshrc -nt $SOME_WELL_KNOWN_FILENAME ]]; then
        eval source -- ${(q)${ZDOTDIR:-${HOME}}/.zshrc
        case $? in
            (0) return 0;;
            ((#b)(*)) add-zsh-hook -d … pre${either_cmd_or_exec_but_I_forget_which}; return ${match};;
        esac
    fi
    unset ${foo} # "Run only on the topmost recursive invocation" mechanism

But I think deliberately ignoring errors like this falls under
"possible, but not advisable".

Again, that's just a sketch / pseudocode, not an actual tested solution.

The /dev/null trick is just to avoid stat()ing /.zshrc if ZDOTDIR and
HOME are both unset.

Cheers,

Daniel

Re: [RFC][PATCH] Add zrestart()

[Daniel Shahaf]

Marlon Richert wrote on Tue, Apr 27, 2021 at 14:37:30 +0300:
> On Mon, Apr 26, 2021 at 10:29 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > > > > +  local err="$(zsh --interactive --monitor --zle -c '' 2>&1 > /dev/null)"
> > > >
> > > > Also, I think it's quite a stretch to describe this line as "_tests_
> > > > whether the shell is able to restart".
> > >
> >
> > Care to comment about the part of my answer before the "Also", which you
> > had snipped?
> 
> Sure:
> 
> On Mon, Apr 26, 2021 at 6:22 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > > +  local err="$(zsh --interactive --monitor --zle -c '' 2>&1 > /dev/null)"
> >
> > This doesn't necessarily restart the _same_ zsh, if there's more than
> > one installed.
> 
> I think your observation is correct and I was planning to fix it in
> the next version of my patch. :)
> 
> When I don't reply to a point you make, it means that I either agree
> or think it's not worth arguing about, and I've put it on my TODO list
> for the next version of my patch. I was trying to be brief and not
> post a bunch of agrees/will-dos or quote unnecessarily. But if it's
> prefered that I do respond to every point I agree with and/or quote
> each part of every email I reply to, just let me know. :)

When you don't respond to a point, it's not possible for the reviewer to
distinguish the situation you describe from other situations —
especially when, as in this instance, the fix isn't obvious.  (proc(5)
isn't portable, argv[0] can lie, etc..)  So, it helps to be explicit.

But so long as we're on this level of detail, it's also fair to just
acknowledge once that all snipped points are agreed with and will be
implemented.

> > > I originally had `zsh -fn <all the dotfiles>`, along the lines of what
> > > was suggested earlier, but that test can fail on a valid dotfile that
> > > uses dynamically named dirs.
> >
> > A minimal example of this would not be out of place.
> 
> % zsh_directory_name() { [[ $2 == home ]] && reply=($HOME) }
> % cd ~[home]; print $?
> 0
> % print 'zsh_directory_name() { [[ $2 == home ]] && reply=($HOME) }
> cd ~[home]' > tmp
> % zsh -fn tmp
> tmp:2: no directory expansion: ~[home]
> %

Thanks.  Is this a bug or expected behaviour?

> > > Plus, if any dotfile sources other files,
> > > those files aren't checked this way at all. The approach above is the
> > > only one I've found so far that appears to be completely reliable in
> > > determining whether the shell can start up successfully.
> > >
> > > > This line executes a whole bunch of code you have no control over.
> > >
> > > It is, however, exactly the code we want to test here.
> >
> > I'm aware.  However, you aren't "testing" it, you are *running* it.
> >
> > First, that means the docs are wrong.
> >
> > Second, that code might do things that are inappropriate for the use-case
> > of "testing" the startup code.
> 
> When you execute `make check`, does it not _run_ the code?

It runs code that I wrote on data that I control in an isolated
environment that takes care not to modify anything else on the system
whereon the tests are run.

> How else are you going to test it? :)

The operative word here is "you".  Whoever wrote the code is best placed
to know how to test it.  You don't know what the user's zshrc does, so
how can you know whether it's safe to run it twice?

> > Or, in other words: the trick is to throw the bathwater out and
> > keep the baby.  Keeping *both* the baby and the bathwater isn't an
> > ideal solution.
> 
> What exactly here is the "bathwater"?

Side effects of the test.  (The baby is the test's soundness and
completeness.)

> Would it help to mock out certain features in the subshell that you
> wouldn't want to actually run in this test? Would it help to invoke
> the subshell with -o RESTRICTED?

That'd still let a zshrc run, say, «ssh foo lorem» or «date | tee -a
my-login-log | read -r REPLY» [jumping through some hoops to avoid
actual output redirections], which are both things that shouldn't be run
twice.

To be clear, these are just two examples.  They are not exhausive.

Re: [RFC][PATCH] Add zrestart()

[Marlon Richert]

On Thu, Apr 29, 2021 at 5:12 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Marlon Richert wrote on Tue, Apr 27, 2021 at 14:37:30 +0300:
> > I think your observation is correct and I was planning to fix it in
> > the next version of my patch. :)
> >
> > When I don't reply to a point you make, it means that I either agree
> > or think it's not worth arguing about, and I've put it on my TODO list
> > for the next version of my patch. I was trying to be brief and not
> > post a bunch of agrees/will-dos or quote unnecessarily. But if it's
> > prefered that I do respond to every point I agree with and/or quote
> > each part of every email I reply to, just let me know. :)
>
> When you don't respond to a point, it's not possible for the reviewer to
> distinguish the situation you describe from other situations —
> especially when, as in this instance, the fix isn't obvious.  (proc(5)
> isn't portable, argv[0] can lie, etc..)  So, it helps to be explicit.
>
> But so long as we're on this level of detail, it's also fair to just
> acknowledge once that all snipped points are agreed with and will be
> implemented.

Thanks for clearing that up. I'll try to be more clear, next time. :)


> > > > I originally had `zsh -fn <all the dotfiles>`, along the lines of what
> > > > was suggested earlier, but that test can fail on a valid dotfile that
> > > > uses dynamically named dirs.
> > >
> > > A minimal example of this would not be out of place.
> >
> > % zsh_directory_name() { [[ $2 == home ]] && reply=($HOME) }
> > % cd ~[home]; print $?
> > 0
> > % print 'zsh_directory_name() { [[ $2 == home ]] && reply=($HOME) }
> > cd ~[home]' > tmp
> > % zsh -fn tmp
> > tmp:2: no directory expansion: ~[home]
> > %
>
> Thanks.  Is this a bug or expected behaviour?

Well, if zsh -n is intended to be used for checking whether a Zsh
source file can be parsed (is it?), then yes, I think that's a bug.


As for your other points: I don't really have anything to comment on
them, as this function hasn't been my idea in the first place. I
originally just had a note in the newuser .zshrc telling the user to
use `exec zsh` to apply changes, rather than `source ~/.zshrc`.
However, in workers/48026, Oliver expressed apprehension about this,
to which Bart, in workers/48031, suggested a different solution, which
I ultimately grew into the patch submitted here. Perhaps the three of
you could discuss this further?

Re: [RFC][PATCH] Add zrestart()

[Lawrence Velázquez]

On Fri, Apr 30, 2021, at 1:27 PM, Marlon Richert wrote:
> As for your other points: I don't really have anything to comment on
> them, as this function hasn't been my idea in the first place. I
> originally just had a note in the newuser .zshrc telling the user to
> use `exec zsh` to apply changes, rather than `source ~/.zshrc`.
> However, in workers/48026, Oliver expressed apprehension about this,
> to which Bart, in workers/48031, suggested a different solution, which
> I ultimately grew into the patch submitted here. Perhaps the three of
> you could discuss this further?

Anything else on this?

vq

Re: [RFC][PATCH] Add zrestart()

[Bart Schaefer]

On Sun, May 9, 2021 at 2:00 PM Lawrence Velázquez <larryv@zsh.org> wrote:
>
> Anything else on this?

There's really no ideal solution for this.  The only "good" way to
handle it is to have two different windows/terminals/whatever from
which to log in, so you can leave the old shell running while you test
whether a fresh login works properly.

Given we can't assure that, a scheme that occurs to me, but might be
tricky to make work smoothly:

At the beginning of $HOME/.zshenv, check for existence of a
$HOME/.zsh-safe file (exact name TBD).
If that file exists, set ZDOTDIR to something
innocuous/nonexistent/freshly-created, and print a message telling the
user he needs to repair his startup files and try again.  Sleep for a
few seconds and then return from .zshenv.
If the file does not exist, create it, and print a message advising
the user that if a shell prompt is reached, to remove the file and
also remove the test code from $HOME/.zshenv.  Sleep for a few seconds
and then go on with whatever else is in ~/.zshenv.

It's then safe to use "exec zsh" (or "exec zsh -l" if the shell is
already a login shell) because even if the user is kicked all the way
out of his remote session, he'll be able to log back in under the
"safe mode" created in ~/.zshenv and try again.

zrestart should thus unset ZDOTDIR, insert the test code into
~/.zshenv, and remove the .zsh-safe file.

Various details that might also be considered:

When creating the file, set up a precmd hook that cleans up the above
and unhooks itself, so the user doesn't have to do anything.  This
could be clobbered by later code, though, so printing the instructions
is probably still useful.

Do some or all of this only if .zshenv is run from an interactive shell.

There's no way to prevent /etc/zshenv from foiling all of this, e.g.,
by setting ZDOTDIR.