git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Jeff King <peff@peff.net>
Cc: Michael Haggerty <mhagger@alum.mit.edu>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Junio C Hamano <gitster@pobox.com>,
	Git List <git@vger.kernel.org>
Subject: Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
Date: Sat, 28 Oct 2017 16:42:49 +0000	[thread overview]
Message-ID: <20171028164249.ufro5weobwadfonv@genre.crustytoothpaste.net> (raw)
In-Reply-To: <20171026063250.dckc22ocr3zjmsxv@sigill.intra.peff.net>

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

On Wed, Oct 25, 2017 at 11:32:51PM -0700, Jeff King wrote:
> On Wed, Oct 25, 2017 at 10:03:09AM +0200, Michael Haggerty wrote:
> 
> > > 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.

This would be useful.  Debian requires that all implementations that
implement /bin/sh support local and a small number of other features.

There is discussion in the Austin Group issue tracker about adding this
feature to POSIX, but it's gotten bogged down over lexical versus
dynamic scoping.  Everyone agrees that it's a desirable feature, though.

> > 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:

As far as I can tell, bash, dash, posh, mksh, pdksh, zsh, and busybox sh
all support local.  From my reading of the documentation, so does sh on
FreeBSD, NetBSD, and OpenBSD.  Not all of these are good choices for a
POSIXy sh, though.

ksh93 will support local if you alias it to typeset, but only when
called from functions defined with "function", not normal shell-style
functions.  I have a gist[0] that does absurd things to work around
that, but I wouldn't recommend that for production use.

Solaris 11.1's man page doesn't document local in sh (which is a ksh88
variant) and ksh is ksh93, so it doesn't appear to support it.  Solaris
11.3 documents bash, so it's a non-issue there.

It's my understanding that using ksh as a POSIXy sh variant is very
common on proprietary Unices, so its lack of compatibility may be a
dealbreaker.  Then again, many of those systems may have bash installed.

> > 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.

I'd recommend an explicit test for this.  It's much easier to track down
that way than seeing other failure scenarios.  People will also usually
complain about failing tests.

[0] https://gist.github.com/bk2204/9dd24df0e4499a02a300578ebdca4728
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

  reply	other threads:[~2017-10-28 16:43 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-24 15:16 [PATCH 0/2] Fix an error-handling path when locking refs Michael Haggerty
2017-10-24 15:16 ` [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts Michael Haggerty
2017-10-24 16:19   ` Eric Sunshine
2017-10-24 19:46     ` Jeff King
2017-10-25  8:03       ` Michael Haggerty
2017-10-25  8:23         ` Michael Haggerty
2017-10-26  6:32         ` Jeff King
2017-10-28 16:42           ` brian m. carlson [this message]
2017-10-29  0:05             ` Junio C Hamano
2017-10-29 16:12               ` brian m. carlson
2017-10-24 15:16 ` [PATCH 2/2] files_transaction_prepare(): fix handling of ref lock failure Michael Haggerty
2017-10-24 19:49   ` Jeff King
2017-10-25  2:29   ` Junio C Hamano
2017-10-24 19:45 ` [PATCH 0/2] Fix an error-handling path when locking refs Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171028164249.ufro5weobwadfonv@genre.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mhagger@alum.mit.edu \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).