Re: .ONESEHLL not working as expected in 3.82

[Paul Smith]

On Sun, 2013-04-28 at 21:14 +0300, Eli Zaretskii wrote:
> > From: Paul Smith <address@hidden>
> > Cc: address@hidden, address@hidden
> > Date: Sat, 27 Apr 2013 12:54:10 -0400
> > 
> > On Sat, 2013-04-27 at 19:17 +0300, Eli Zaretskii wrote:
> > > The .ONESHELL feature is now supported on MS-Windows, for the default
> > > Windows shell (cmd.exe) or compatible replacements, in the development
> > > version (commit e56aad4).
> > 
> > Nice!
> 
> I see you followed this up with a commit (30843de) which moved code
> around that deals with the one_shell use case.  What was the reason
> for that change?  I think it breaks the use case where a Unixy shell
> is used on MS-Windows under .ONESHELL; this used to work before.

I moved it because the way you had it caused one of the test cases to
fail: you had changed that block of code to be inside the if-statement,
when before it was outside/after the if-statement.

> The code block that you moved is needed on MS-Windows as well, when
> the shell passes the is_bourne_compatible_shell test.

I think you're right, sorry.  I thought that code was inside a unix-only
section but I see now it wasn't.

The code used to look like this:

  if (is_bourne_compatible_shell())
    {
       ... modify the script to remove @-+ ...
       *t = '\0';
    }

  /* create an argv list ... */
  {
    ...
    new_argv[n++] = NULL;
  }
  return new_argv;

So, the setup of the new_argv[] was AFTER and outside the if-statement.
In the change you made, it was moved to be INSIDE the if-statement.
That caused a test case to fail because in the situation where we do
have one-shell and we do not have a POSIX shell, new_argv[] was empty
and no command was invoked.

I moved it back, but accidentally made it not work for Windows.

The goal of this code in the if-statement is to implement a special case
allowing ONESHELL to be easier to add in the case where you DO have a
standard shell.  In that case, and ONLY in that case, we remove the
internal @-+ characters.  This allows you to have something like:

  foo:
          @echo hi
          @echo there
          @echo how are you

And have it continue to work if you add ONESHELL (for performance
reasons) without rewriting all the recipes.

However, if you do NOT have a POSIX shell, then we do NOT remove these
internal characters: we simply provide the script as-is and only the
first line is checked for special characters.  This lets you use
something like Perl, where @ is a special character, for example:

  SHELL = /usr/bin/perl
  foo:
         @print "hi";
         @array = qw(there how are you);
         print "@array\n";

I think the implementation you have is not quite right.  I think the
parsing of the @-+ stuff is common across all platforms if we have a
shell, so you don't need the "else /* non-posix shell */".

I think it pseudo-code it would look something like this:

  if (posix-shell)
    {
      ...strip out @-+ from LINE...
    }
#ifdef WINDOWS32
  if (need a batch file)
    {
      ...write LINE to the batch file & setup new_argv for batch...
    }
  else
#endif
    {
      ...chop LINE up into new_argv...
    }
  return new_argv;

Or something.  Also, I'm not sure about adding things like @echo off to
the batch file.  That assumes that we'll always be using command.com to
run the batch file, but what if the user specified C:/perl/bin/perl.exe
or something as their SHELL?

I'm probably missing something about the implementation though.