[PATCH] unix: fix mch_call_shell_fork condition

[Felipe Contreras]

Clearly the code meant to check for SHELL_SILENT in order to send to
/dev/null, not SHELL_EXPAND.

This works because all the callers of get_cmd_output that use
SHELL_SILENT also use SHELL_EXPAND.

This has been there since Vim 5.0.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---
src/os_unix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/os_unix.c b/src/os_unix.c
index 20c61106f..79b71ca97 100644
--- a/src/os_unix.c
+++ b/src/os_unix.c
@@ -4710,7 +4710,7 @@ mch_call_shell_fork(
}
# endif

- if (!show_shell_mess || (options & SHELL_EXPAND))
+ if (!show_shell_mess || (options & SHELL_SILENT))
{
int fd;

--
2.32.0.rc2

[Bram Moolenaar]

Felipe Contreras wrote:

> Clearly the code meant to check for SHELL_SILENT in order to send to
> /dev/null, not SHELL_EXPAND.
>
> This works because all the callers of get_cmd_output that use
> SHELL_SILENT also use SHELL_EXPAND.
>
> This has been there since Vim 5.0.

I don't think the value should be changed. Unless you can show a
situation where the wrong thing happens.

There is one call where only SHELL_EXPAND is used in get_cmd_output().
Thus applying the change does have an effect. It's not so easy to see
how though.

The naming is confusing. It looks like SHELL_EXPAND is used whenever
the output of a non-interactive shell command is obtained, where we
don't what to check for typed characters. Perhaps
SHELL_IGNORE_TYPEAHEAD would be better?


--
Veni, Vidi, VW -- I came, I saw, I drove around in a little car.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Felipe Contreras]

On Tue, Jun 1, 2021 at 11:47 AM Bram Moolenaar <Br...@moolenaar.net> wrote:
> Felipe Contreras wrote:
>
> > Clearly the code meant to check for SHELL_SILENT in order to send to
> > /dev/null, not SHELL_EXPAND.
> >
> > This works because all the callers of get_cmd_output that use
> > SHELL_SILENT also use SHELL_EXPAND.
> >
> > This has been there since Vim 5.0.
>
> I don't think the value should be changed. Unless you can show a
> situation where the wrong thing happens.
>
> There is one call where only SHELL_EXPAND is used in get_cmd_output().
> Thus applying the change does have an effect. It's not so easy to see
> how though.

The only callers of get_cmd_output() are:

* get_cmd_output_as_rettv: sets SHELL_SILENT
* find_locales: sets SHELL_SILENT
* expand_backtick: sets SHELL_SILENT if EW_SILENT

All callers of expand_backtick set EW_SILENT, otherwise they might get
"shell returned ..." messages.

> The naming is confusing. It looks like SHELL_EXPAND is used whenever
> the output of a non-interactive shell command is obtained, where we
> don't what to check for typed characters. Perhaps
> SHELL_IGNORE_TYPEAHEAD would be better?

I think it's meant for stuff like expand("~/").

--
Felipe Contreras

[Bram Moolenaar]

Felipe Contreras wrote:

> > > Clearly the code meant to check for SHELL_SILENT in order to send to
> > > /dev/null, not SHELL_EXPAND.
> > >
> > > This works because all the callers of get_cmd_output that use
> > > SHELL_SILENT also use SHELL_EXPAND.
> > >
> > > This has been there since Vim 5.0.
> >
> > I don't think the value should be changed. Unless you can show a
> > situation where the wrong thing happens.
> >
> > There is one call where only SHELL_EXPAND is used in get_cmd_output().
> > Thus applying the change does have an effect. It's not so easy to see
> > how though.
>
> The only callers of get_cmd_output() are:
>
> * get_cmd_output_as_rettv: sets SHELL_SILENT
> * find_locales: sets SHELL_SILENT
> * expand_backtick: sets SHELL_SILENT if EW_SILENT
>
> All callers of expand_backtick set EW_SILENT, otherwise they might get
> "shell returned ..." messages.

get_arglist_exp() calls gen_expand_wildcards() without EW_SILENT,
which calls expand_backtick(). Then SHELL_SILENT won't be set.

> > The naming is confusing. It looks like SHELL_EXPAND is used whenever
> > the output of a non-interactive shell command is obtained, where we
> > don't what to check for typed characters. Perhaps
> > SHELL_IGNORE_TYPEAHEAD would be better?
>
> I think it's meant for stuff like expand("~/").

--

VOICE OVER: As the horrendous Black Beast lunged forward, escape for Arthur
and his knights seemed hopeless, when, suddenly ... the animator
suffered a fatal heart attack.
ANIMATOR: Aaaaagh!
VOICE OVER: The cartoon peril was no more ... The Quest for Holy Grail could
continue.
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

[Felipe Contreras]

On Tue, Jun 1, 2021 at 1:49 PM Bram Moolenaar <Br...@moolenaar.net> wrote:
>
>
> Felipe Contreras wrote:
>
> > > > Clearly the code meant to check for SHELL_SILENT in order to send to
> > > > /dev/null, not SHELL_EXPAND.
> > > >
> > > > This works because all the callers of get_cmd_output that use
> > > > SHELL_SILENT also use SHELL_EXPAND.
> > > >
> > > > This has been there since Vim 5.0.
> > >
> > > I don't think the value should be changed. Unless you can show a
> > > situation where the wrong thing happens.
> > >
> > > There is one call where only SHELL_EXPAND is used in get_cmd_output().
> > > Thus applying the change does have an effect. It's not so easy to see
> > > how though.
> >
> > The only callers of get_cmd_output() are:
> >
> > * get_cmd_output_as_rettv: sets SHELL_SILENT
> > * find_locales: sets SHELL_SILENT
> > * expand_backtick: sets SHELL_SILENT if EW_SILENT
> >
> > All callers of expand_backtick set EW_SILENT, otherwise they might get
> > "shell returned ..." messages.
>
> get_arglist_exp() calls gen_expand_wildcards() without EW_SILENT,
> which calls expand_backtick(). Then SHELL_SILENT won't be set.

The code cannot reach expand_backtick() because SPECIAL_WILDCHAR
contains `, and therefore has_special_wildchar() returns true.

The more we dig, the less likely it seems it will have an impact. And
of course, if get_arglist_exp() wants a silent gen_expand_wildcards()
it probably should set EW_SILENT.

Or better yet:

--- a/src/filepath.c
+++ b/src/filepath.c
@@ -3170,8 +3170,7 @@ expand_backtick(
buffer = eval_to_string(cmd + 1, TRUE);
else
#endif
- buffer = get_cmd_output(cmd, NULL,
- (flags & EW_SILENT) ? SHELL_SILENT : 0, NULL);
+ buffer = get_cmd_output(cmd, NULL, SHELL_SILENT, NULL);
vim_free(cmd);
if (buffer == NULL)
return -1;

Cheers.

--
Felipe Contreras