Re: [PATCH] Add customizable `vcs` prompt theme (was Re: [RFC][PATCH] `newuser` prompt theme)

[Roman Perepelitsa <roman.perepelitsa@xxxxxxxxx>]

On Thu, Jun 10, 2021 at 3:48 PM Marlon Richert <marlon.richert@xxxxxxxxx> wrote:
>
> Thanks again. New version of the patch attached.

Thanks! A few more comments/questions. Those that I'm not quoting can
be considered resolved.

> On Thu, Jun 10, 2021 at 12:30 PM Roman Perepelitsa
> <roman.perepelitsa@xxxxxxxxx> wrote:
> >
> > I think invoking prompt_vcs_setup twice in the same shell will produce
> > an error. I haven't tried it though.
>
> I just tried it and I didn't get any errors. Why do you think it would
> produce an error?

I was expecting the readonly parameter at the top to print an error.

  % readonly -gHA _prompt_vcs_ps1_default=()
  % readonly -gHA _prompt_vcs_ps1_default=()
  zsh: read-only variable: _prompt_vcs_ps1_default

It's also unclear to me whether this prompt supports clean
deinitialization and repeated initialization. Doesn't look like it.
Does it?

> > Why is prompt_vcs_precmd checking whether prompt_vcs_fd is a readable
> > fd? How can it not be?
>
> I don't know how or why, but if I don't check for that, then I get
> this error repeatedly:
>
> prompt_vcs_precmd:6: failed to close file descriptor 11: bad file descriptor

Then the check must be masking a bug. This bug can result in the
closing of another file descriptor that is not owned by prompt_vcs.

I took a quick look and perhaps the bug is due to prompt_vcs_fd-widget
not unsetting prompt_vcs_fd?

> > There is a comment in prompt_vcs_precmd that says it's going to kill a
> > process while in fact it closes the file descriptor. That may
> > *eventually* kill the process when it decides to print something
> > (assuming it hasn't blocked SIGPIPE or is checking for write errors).
>
> True. I'll change it to actually kill the process, too.

I see that the code is sending SIGKILL now and not waiting for the
process to terminate. Both of these make me nervous. The code also
*looks like* it would work without monitor but it wouldn't (it would
result in unbounded background processes and zombies). What do you
think about not killing anything and ensuring that at most one
background process runs at once? This is what I do in my zsh theme and
it works well.

> > prompt_vcs_fd-widget has a check for [[ -n $1 ]]. When is it false?
>
> When prompt_vcs_fd-widget is being called incorrectly. :)

When can it be called incorrectly?

This is not a public function, it's a callback that is passed to `zle
-F`. Do you expect the latter to violate its contract in this specific
way? If not, I would remove the check. It's increasing the complexity
of the code without material gain.

> Perhaps it should not return 1 in that case, though.

The return code of `zle -F` callbacks is ignored. Adding specific
return codes makes it appear as if they matter. This makes the code
harder to understand similarly to conditions that are always false.

The file descriptor leaks to child processes. It's better to avoid
this. This can be done by using sysopen with cloexec instead of plain
exec.

Instead of printing the pid and reading it in the parent you can use
$sysparams[procsubstpid].

Why does _prompt_vcs_info quote its output? If it didn't quote, then
there would be no need to unquote after reading.

Handling errors from `read` would be nice.

Roman.