git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>,
	Johannes Sixt <j6t@kdbg.org>,
	Jonathan Tan <jonathantanmy@google.com>,
	Stefan Beller <sbeller@google.com>,
	Junio C Hamano <gitster@pobox.com>,
	Luke Diamand <luke@diamand.org>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Date: Mon, 30 Jul 2018 11:13:56 -0700	[thread overview]
Message-ID: <20180730181356.GA156463@aiede.svl.corp.google.com> (raw)
In-Reply-To: <20180711064642.6933-2-sunshine@sunshineco.com>

Hi,

Eric Sunshine wrote:

> The --chain-lint option detects broken &&-chains by forcing the test to
> exit early (as the very first step) with a sentinel value. If that
> sentinel is the test's overall exit code, then the &&-chain is intact;
> if not, then the chain is broken. Unfortunately, this detection does not
> extend to &&-chains within subshells even when the subshell itself is
> properly linked into the outer &&-chain.
>
> Address this shortcoming by feeding the body of the test to a
> lightweight "linter" which can peer inside subshells and identify broken
> &&-chains by pure textual inspection.

Interesting.

>                                        Although the linter does not
> actually parse shell scripts, it has enough knowledge of shell syntax to
> reliably deal with formatting style variations (as evolved over the
> years) and to avoid being fooled by non-shell content (such as inside
> here-docs and multi-line strings).

This is causing contrib/subtree tests to fail for me: running "make -C
contrib/subtree test" produces

[...]
	*** t7900-subtree.sh ***
	ok 1 - no merge from non-existent subtree
	ok 2 - no pull from non-existent subtree
	ok 3 - add subproj as subtree into sub dir/ with --prefix
	ok 4 - add subproj as subtree into sub dir/ with --prefix and --message
	ok 5 - add subproj as subtree into sub dir/ with --prefix as -P and --message as -m
	ok 6 - add subproj as subtree into sub dir/ with --squash and --prefix and --message
	ok 7 - merge new subproj history into sub dir/ with --prefix
	ok 8 - merge new subproj history into sub dir/ with --prefix and --message
	ok 9 - merge new subproj history into sub dir/ with --squash and --prefix and --message
	ok 10 - merge the added subproj again, should do nothing
	ok 11 - merge new subproj history into subdir/ with a slash appended to the argument of --prefix
	ok 12 - split requires option --prefix
	ok 13 - split requires path given by option --prefix must exist
	ok 14 - split sub dir/ with --rejoin
	ok 15 - split sub dir/ with --rejoin from scratch
	ok 16 - split sub dir/ with --rejoin and --message
	ok 17 - split "sub dir"/ with --branch
	ok 18 - check hash of split
	ok 19 - split "sub dir"/ with --branch for an existing branch
	ok 20 - split "sub dir"/ with --branch for an incompatible branch
	error: bug in the test script: broken &&-chain or run-away HERE-DOC: 
		subtree_test_create_repo "$subtree_test_count" &&
[...]
		)

	Makefile:44: recipe for target 't7900-subtree.sh' failed

The problematic test code looks like this:

	(
		cd "$subtree_test_count/sub proj" &&
		git fetch .. subproj-br &&
		git merge FETCH_HEAD &&

		chks="sub1
sub2
sub3
sub4" &&
		chks_sub=$(cat <<TXT | sed '\''s,^,sub dir/,'\''
$chks
TXT
) &&
		chkms="main-sub1
main-sub2
main-sub3
main-sub4" &&
		chkms_sub=$(cat <<TXT | sed '\''s,^,sub dir/,'\''
$chkms
TXT
) &&

		subfiles=$(git ls-files) &&
		check_equal "$subfiles" "$chkms
$chks"
	)

Ugly quoting, useless use of "cat", etc, aside, I don't think it's
missing any &&.  Hints?

Thanks,
Jonathan

  parent reply	other threads:[~2018-07-30 18:14 UTC|newest]

Thread overview: 123+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-26  7:29 [PATCH 00/29] t: detect and fix broken &&-chains in subshells Eric Sunshine
2018-06-26  7:29 ` [PATCH 01/29] t7508: use test_when_finished() instead of managing exit code manually Eric Sunshine
2018-06-26  7:29 ` [PATCH 02/29] t0001: use "{...}" block around "||" expression rather than subshell Eric Sunshine
2018-06-26  7:29 ` [PATCH 03/29] t1300: use sane_unset() to avoid breaking &&-chain Eric Sunshine
2018-06-26  7:29 ` [PATCH 04/29] t3303: use standard here-doc tag "EOF" to avoid fooling --chain-lint Eric Sunshine
2018-06-26  7:29 ` [PATCH 05/29] t5505: modernize and simplify hard-to-digest test Eric Sunshine
2018-06-26  7:29 ` [PATCH 06/29] t6036: fix broken "merge fails but has appropriate contents" tests Eric Sunshine
2018-06-26  8:44   ` Elijah Newren
2018-06-26  7:29 ` [PATCH 07/29] t7201: drop pointless "exit 0" at end of subshell Eric Sunshine
2018-06-26  7:29 ` [PATCH 08/29] t7400: fix broken "submodule add/reconfigure --force" test Eric Sunshine
2018-06-27 18:04   ` Stefan Beller
2018-06-26  7:29 ` [PATCH 09/29] t7810: use test_expect_code() instead of hand-rolled comparison Eric Sunshine
2018-06-26  7:29 ` [PATCH 10/29] t9001: fix broken "invoke hook" test Eric Sunshine
2018-06-26 17:07   ` Jonathan Tan
2018-06-26  7:29 ` [PATCH 11/29] t9104: use "{...}" block around "||" expression rather than subshell Eric Sunshine
2018-06-26  7:29 ` [PATCH 12/29] t9401: drop unnecessary nested subshell Eric Sunshine
2018-06-26  7:29 ` [PATCH 13/29] t/lib-submodule-update: fix broken "replace submodule must-fail" test Eric Sunshine
2018-06-27 18:30   ` [PATCH] t/lib-submodule-update: fix absorbing test Stefan Beller
2018-06-27 18:38     ` Eric Sunshine
2018-06-26  7:29 ` [PATCH 14/29] t: drop subshell with missing &&-chain in favor of simpler construct Eric Sunshine
2018-06-26 19:31   ` Junio C Hamano
2018-06-26 20:06     ` Eric Sunshine
2018-06-26  7:29 ` [PATCH 15/29] t: drop unnecessary terminating semicolons in subshell Eric Sunshine
2018-06-26  7:29 ` [PATCH 16/29] t: use test_might_fail() instead of manipulating exit code manually Eric Sunshine
2018-06-26  7:29 ` [PATCH 17/29] t: use test_must_fail() instead of checking " Eric Sunshine
2018-06-26  7:59   ` Luke Diamand
2018-06-26  8:58   ` Elijah Newren
2018-06-26  9:21     ` Eric Sunshine
2018-06-26 18:05       ` Johannes Sixt
2018-06-26 18:14         ` Eric Sunshine
2018-06-26 21:00           ` Johannes Sixt
2018-06-26  7:29 ` [PATCH 18/29] t0000-t0999: fix broken &&-chains in subshells Eric Sunshine
2018-06-26  7:29 ` [PATCH 19/29] t1000-t1999: " Eric Sunshine
2018-06-26  7:29 ` [PATCH 20/29] t2000-t2999: " Eric Sunshine
2018-06-26  7:29 ` [PATCH 21/29] t3000-t3999: " Eric Sunshine
2018-06-26  7:29 ` [PATCH 22/29] t3030: " Eric Sunshine
2018-06-26  7:29 ` [PATCH 23/29] t4000-t4999: " Eric Sunshine
2018-06-26  7:29 ` [PATCH 24/29] t5000-t5999: " Eric Sunshine
2018-06-26  7:29 ` [PATCH 25/29] t6000-t6999: " Eric Sunshine
2018-06-26  7:29 ` [PATCH 26/29] t7000-t7999: " Eric Sunshine
2018-06-26  7:29 ` [PATCH 27/29] t9000-t9999: " Eric Sunshine
2018-06-26  7:30 ` [PATCH 28/29] t9119: " Eric Sunshine
2018-06-26  7:30 ` [PATCH 29/29] t/test-lib: teach --chain-lint to detect " Eric Sunshine
2018-06-26 19:15   ` Junio C Hamano
2018-06-26 19:52     ` Eric Sunshine
2018-06-26 20:17       ` Jeff King
2018-06-26 20:22         ` Jeff King
2018-06-26 20:59           ` Eric Sunshine
2018-06-26 21:33           ` Elijah Newren
2018-06-26 21:42             ` Eric Sunshine
2018-06-26 20:46         ` Eric Sunshine
2018-06-26 21:01           ` Jeff King
2018-06-26 21:13             ` Eric Sunshine
2018-06-28 14:35               ` Jeff King
2018-06-27  2:15             ` Elijah Newren
2018-06-27  6:27               ` Johannes Sixt
2018-06-27  6:48                 ` Eric Sunshine
2018-06-28 14:37               ` Jeff King
2018-06-26 21:09         ` Junio C Hamano
2018-06-26  9:20 ` [PATCH 00/29] t: detect and fix " Elijah Newren
2018-06-26  9:31   ` Eric Sunshine
2018-06-26 15:34     ` Elijah Newren
2018-06-26 19:38 ` Junio C Hamano
2018-06-26 21:25   ` Eric Sunshine
2018-06-26 22:31     ` Junio C Hamano
2018-06-27  0:22       ` Jonathan Nieder
2018-07-11  6:46 ` [PATCH v2 00/10] detect " Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 01/10] t/test-lib: teach --chain-lint to " Eric Sunshine
2018-07-11 21:37     ` Junio C Hamano
2018-07-12 10:50       ` Eric Sunshine
2018-07-12 16:56         ` Jeff King
2018-07-12 19:32           ` Eric Sunshine
2018-07-12 19:54             ` Junio C Hamano
2018-07-30 18:13     ` Jonathan Nieder [this message]
2018-07-30 19:06       ` [PATCH 0/2] subtree: fix &&-chain and simplify tests (Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells) Jonathan Nieder
2018-07-30 19:07         ` [PATCH 1/2] subtree test: add missing && to &&-chain Jonathan Nieder
2018-07-30 19:07         ` [PATCH 2/2] subtree test: simplify preparation of expected results Jonathan Nieder
2018-07-30 20:25       ` [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells Eric Sunshine
2018-07-30 20:59         ` Jonathan Nieder
2018-07-30 21:38           ` Eric Sunshine
2018-07-31 12:50             ` Jeff King
2018-07-31 18:55               ` Eric Sunshine
2018-07-31 19:08                 ` Jeff King
2018-08-23 18:02     ` Ævar Arnfjörð Bjarmason
2018-08-23 18:27       ` Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 02/10] t/Makefile: add machinery to check correctness of chainlint.sed Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 03/10] t/chainlint: add chainlint "basic" test cases Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 04/10] t/chainlint: add chainlint "whitespace" " Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 05/10] t/chainlint: add chainlint "one-liner" " Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 06/10] t/chainlint: add chainlint "nested subshell" " Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 07/10] t/chainlint: add chainlint "loop" and "conditional" " Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 08/10] t/chainlint: add chainlint "cuddled" " Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 09/10] t/chainlint: add chainlint "complex" " Eric Sunshine
2018-07-11  6:46   ` [PATCH v2 10/10] t/chainlint: add chainlint "specialized" " Eric Sunshine
2018-08-07  8:21   ` [PATCH 0/5] chainlint: improve robustness against "unusual" shell coding Eric Sunshine
2018-08-07  8:21     ` [PATCH 1/5] chainlint: match arbitrary here-docs tags rather than hard-coded names Eric Sunshine
2018-08-08 22:50       ` Jeff King
2018-08-09  5:58         ` Eric Sunshine
2018-08-09 14:26           ` Jeff King
2018-08-07  8:21     ` [PATCH 2/5] chainlint: recognize multi-line $(...) when command cuddled with "$(" Eric Sunshine
2018-08-07  8:21     ` [PATCH 3/5] chainlint: let here-doc and multi-line string commence on same line Eric Sunshine
2018-08-07  8:21     ` [PATCH 4/5] chainlint: recognize multi-line quoted strings more robustly Eric Sunshine
2018-08-07  8:21     ` [PATCH 5/5] chainlint: add test of pathological case which triggered false positive Eric Sunshine
2018-08-08 22:53     ` [PATCH 0/5] chainlint: improve robustness against "unusual" shell coding Jeff King
2018-08-09  0:44       ` Junio C Hamano
2018-08-13  8:47     ` [PATCH v2 0/6] " Eric Sunshine
2018-08-13  8:47       ` [PATCH v2 1/6] chainlint: match arbitrary here-docs tags rather than hard-coded names Eric Sunshine
2018-08-13  8:47       ` [PATCH v2 2/6] chainlint: match 'quoted' here-doc tags Eric Sunshine
2018-08-13 19:27         ` Junio C Hamano
2018-08-13 20:12           ` Eric Sunshine
2018-08-13  8:47       ` [PATCH v2 3/6] chainlint: recognize multi-line $(...) when command cuddled with "$(" Eric Sunshine
2018-08-13  8:47       ` [PATCH v2 4/6] chainlint: let here-doc and multi-line string commence on same line Eric Sunshine
2018-08-13  8:47       ` [PATCH v2 5/6] chainlint: recognize multi-line quoted strings more robustly Eric Sunshine
2018-08-13  8:47       ` [PATCH v2 6/6] chainlint: add test of pathological case which triggered false positive Eric Sunshine
2018-08-15 18:45       ` [PATCH v3 0/6] chainlint: improve robustness against "unusual" shell coding Eric Sunshine
2018-08-15 18:45         ` [PATCH v3 1/6] chainlint: match arbitrary here-docs tags rather than hard-coded names Eric Sunshine
2018-08-15 18:45         ` [PATCH v3 2/6] chainlint: match quoted here-doc tags Eric Sunshine
2018-08-15 18:45         ` [PATCH v3 3/6] chainlint: recognize multi-line $(...) when command cuddled with "$(" Eric Sunshine
2018-08-15 18:45         ` [PATCH v3 4/6] chainlint: let here-doc and multi-line string commence on same line Eric Sunshine
2018-08-15 18:45         ` [PATCH v3 5/6] chainlint: recognize multi-line quoted strings more robustly Eric Sunshine
2018-08-15 18:45         ` [PATCH v3 6/6] chainlint: add test of pathological case which triggered false positive Eric Sunshine
2018-08-29  9:45         ` [PATCH] chainlint: match "quoted" here-doc tags Eric Sunshine
2018-08-29 17:57           ` Junio C Hamano

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=20180730181356.GA156463@aiede.svl.corp.google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=jonathantanmy@google.com \
    --cc=luke@diamand.org \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=sbeller@google.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.
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).