All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org, Phillip Wood <phillip.wood123@gmail.com>,
	Michael J Gruber <git@grubix.eu>
Subject: Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter
Date: Wed, 29 Mar 2023 02:28:43 -0400	[thread overview]
Message-ID: <20230329062843.GC1793752@coredump.intra.peff.net> (raw)
In-Reply-To: <CAPig+cSMYt81fcU8fRNhN9qZ=95jvRNv9YGhkM6PxgdMywWnCA@mail.gmail.com>

On Tue, Mar 28, 2023 at 11:07:17PM -0400, Eric Sunshine wrote:

> > I just think chainlint.pl is doing a good enough job of catching it that
> > we can rely on it. I'll be curious if Eric has input there on whether it
> > can do even better, which would remove all of the caveats from the
> > commit message.
> 
> It was an intentional design choice, for the sake of simplicity, _not_
> to make chainlint.pl a shell syntax checker, despite the fact that it
> is genuinely parsing shell code. After all, the shell itself -- by
> which test code will ultimately be executed -- is more than capable of
> diagnosing syntax errors, so why burden chainlint.pl with all that
> extra baggage? Instead, chainlint.pl is focussed on detecting semantic
> problems -- such as broken &&-chain and missing `return 1` -- which
> are of importance to _this_ project.

Yeah, I'm not too surprised to hear any of that, and I almost wrote off
chainlint.pl for this purpose (hence the hand-waving in my commit
message). But after realizing it has to find here-docs anyway to ignore
them, it seemed reasonable to bend it to my will. ;)

Thanks again for your patch.

> [1]: Why is that, by the way? Is `eval` not complaining about the
> unclosed here-doc? Is that something that can be fixed more generally?
> Are there other syntactic problems that go unnoticed?

The behavior varies from shell to shell. Historically, I suspect it was
a deliberate decision to read until EOF, because it lets you stick
arbitrary data at the end of a script, like:

  $ cat foo.sh
  my_prog <<\EOF
  1 2 3 4
  5 6 7 8
  [imagine kilobytes of data tables here]

without having to worry about terminating it. I think I've seen it with
something like:

  {
    echo "uudecode <<\EOF | tar tf -"
    tar cf - Documentation | uuencode -
  } >foo.sh

which makes foo.sh a sort of self-extracting tarball. (As an aside, I
was disappointed that I did not have uuencode installed by default on my
system. How times have changed. ;) ).

These days bash will warn about it, but dash will not:

  $ bash foo.sh | wc -l
  foo.sh: line 129028: warning: here-document at line 1 delimited by end-of-file (wanted `EOF')
  901

  $ dash foo.sh | wc -l
  901

Bash still has a zero exit code, though, so aside from the stderr cruft
(which is hidden unless you are digging in "-v" output), the tests would
pass. I can't remember when bash started warning, though "git log -S" in
its repo suggests it was bash 4.0 in 2009.

-Peff

  reply	other threads:[~2023-03-29  6:28 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-28 20:20 [PATCH 0/4] some chainlint fixes and performance improvements Jeff King
2023-03-28 20:22 ` [PATCH 1/4] tests: run internal chain-linter under "make test" Jeff King
2023-03-29 10:20   ` Ævar Arnfjörð Bjarmason
2023-03-29 15:49     ` Junio C Hamano
2023-03-29 23:28       ` Jeff King
2023-03-30 18:45         ` Junio C Hamano
2023-03-28 20:23 ` [PATCH 2/4] tests: replace chainlint subshell with a function Jeff King
2023-03-28 20:40   ` Junio C Hamano
2023-03-28 20:28 ` [PATCH 3/4] tests: drop here-doc check from internal chain-linter Jeff King
2023-03-28 21:46   ` Junio C Hamano
2023-03-29  2:37     ` Jeff King
2023-03-29  3:04       ` Jeff King
2023-03-29  3:13         ` Eric Sunshine
2023-03-29  3:46           ` Eric Sunshine
2023-03-29  4:02             ` Eric Sunshine
2023-03-29  6:07             ` Jeff King
2023-03-29  6:28               ` Eric Sunshine
2023-03-29  3:07       ` Eric Sunshine
2023-03-29  6:28         ` Jeff King [this message]
2023-03-28 20:28 ` [PATCH 4/4] tests: skip test_eval_ in internal chain-lint Jeff King
2023-03-28 21:08 ` [PATCH 0/4] some chainlint fixes and performance improvements Jeff King
2023-03-30 22:08   ` Jeff King
2023-03-30 22:16     ` Junio C Hamano
2023-03-30 19:27 ` [PATCH v2 0/5] " Jeff King
2023-03-30 19:27   ` [PATCH v2 1/5] tests: run internal chain-linter under "make test" Jeff King
2023-03-30 19:27   ` [PATCH v2 2/5] tests: replace chainlint subshell with a function Jeff King
2023-03-30 19:30   ` [PATCH v2 3/5] tests: diagnose unclosed here-doc in chainlint.pl Jeff King
2023-03-30 21:26     ` Eric Sunshine
2023-03-30 19:30   ` [PATCH v2 4/5] tests: drop here-doc check from internal chain-linter Jeff King
2023-03-30 19:30   ` [PATCH v2 5/5] tests: skip test_eval_ in internal chain-lint Jeff King
2023-03-30 20:32   ` [PATCH v2 0/5] some chainlint fixes and performance improvements Junio C Hamano
2023-03-30 22:09     ` 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

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

  git send-email \
    --in-reply-to=20230329062843.GC1793752@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@grubix.eu \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood123@gmail.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.