Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts

From: Jeff King <peff@peff.net>
Date: Oct 26, 2017, 06:32 UTC
Message-ID: <20171026063250.dckc22ocr3zjmsxv@sigill.intra.peff.net>
In-Reply-To: <e66694a3-98e2-33aa-4cdd-dac41d2a89d5@alum.mit.edu>

On Wed, Oct 25, 2017 at 10:03:09AM +0200, Michael Haggerty wrote:

Show 6 quoted lines

> > Yeah. It's supported by dash and many other shells, but we do try to
> > avoid it[1]. I think in this case we could just drop it (but keep
> > setting the "local foo" ones to empty with "foo=".
> 
> I do wish that we could allow "local", as it avoids a lot of headaches
> and potential breakage. According to [1],

Agreed.

Show 7 quoted lines

> > However, every single POSIX-compliant shell I've tested implements the
> > 'local' keyword, which lets you declare variables that won't be
> > returned from the current function. So nowadays you can safely count
> > on it working.
> 
> He mentions that ksh93 doesn't support "local", but that it differs from
> POSIX in many other ways, too.

Yes, the conclusion we came to in the thread I linked earlier is the same: ksh is affected, but that shell is a problem for other reasons. I don't know if anybody tested with "modern" ksh like mksh, though. Should be easy enough:

  cat >foo.sh <<\EOF
  fun() {
    local x=3
    echo inside: $x
  }
  x=2
  fun
  echo after: $x
  EOF

  mksh foo.sh

which produces the expected:

  inside: 3
  after: 2

So that's good.

> Perhaps we could slip in a couple of "local" as a compatibility test to
> see if anybody complains, like we did with a couple of C features recently.

That sounds reasonable to me. But from the earlier conversation, beware that:

  local x
  ...
  x=5

is not necessarily enough to notice the problem on broken shells (they may complain that "local" is not a command, and quietly stomp on the global). I think:

  local x=5

would be enough (though depend on how you use $x, the failure mode might be pretty subtle). Or we could even add an explicit test in t0000 like the example above.

> But I agree that this bug fix is not the correct occasion to change
> policy on something like this, so I'll reroll without "local".

Also agreed.

-Peff