All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git List <git@vger.kernel.org>, Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v2] CodingGuidelines: explicitly allow "local" for test scripts
Date: Mon, 03 May 2021 11:01:27 +0200	[thread overview]
Message-ID: <877dkgxk9p.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqo8ds8k6r.fsf_-_@gitster.g>


On Mon, May 03 2021, Junio C Hamano wrote:

> 01d3a526 (t0000: check whether the shell supports the "local"
> keyword, 2017-10-26) raised a test balloon to see if those who build
> and test Git use a platform with a shell that lacks support for the
> "local" keyword.  After two years, 7f0b5908 (t0000: reword comments
> for "local" test, 2019-08-08) documented that "local" keyword, even
> though is outside POSIX, is allowed in our test scripts.
>
> Let's write it in the CodingGuidelines, too.  It might be tempting
> to allow it in scripted Porcelains (we have avoided getting them
> contaminiated by "local" so far), but they are on their way out and
> getting rewritten in C.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/CodingGuidelines | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 45465bc0c9..ea70676a30 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -175,6 +175,11 @@ For shell scripts specifically (not exhaustive):
>  
>     does not have such a problem.
>  
> + - Even though "local" is not part of POSIX, we make heavy use of it
> +   in our test suite.  We do not use it in scripted Porcelains, and
> +   hopefully nobody starts using "local" before they are reimplemented
> +   in C ;-)
> +

Is there any portability reason to avoid "local" in the porcelains? I
don't have any plans for using it, but I don't see why we'd explicitly
forbid it.

  reply	other threads:[~2021-05-03  9:02 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-03  4:10 [PATCH] CodingGuidelines: explicitly allow "local" for test scripts Junio C Hamano
2021-05-03  4:21 ` Eric Sunshine
2021-05-03  5:21   ` Junio C Hamano
2021-05-03  5:23     ` [PATCH v2] " Junio C Hamano
2021-05-03  9:01       ` Ævar Arnfjörð Bjarmason [this message]
2021-05-04  3:01         ` Junio C Hamano
2021-05-04 12:27           ` Ævar Arnfjörð Bjarmason
2021-05-04 12:50             ` Junio C Hamano
2021-05-04 15:09               ` Ævar Arnfjörð Bjarmason
2021-05-04 20:22                 ` Felipe Contreras
2021-05-04 23:17                 ` brian m. carlson
2021-05-04 23:55                   ` Junio C Hamano
2021-05-05  0:08                   ` Felipe Contreras
2021-05-05  1:58                     ` brian m. carlson
2021-05-05  3:59                       ` Felipe Contreras
2021-05-05 14:16                         ` Jeff King
2021-05-05 17:18                           ` Felipe Contreras

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

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

  git send-email \
    --in-reply-to=877dkgxk9p.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.