git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: git <git@vger.kernel.org>, Elijah Newren <newren@gmail.com>,
	Johannes Sixt <j6t@kdbg.org>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Junio C Hamano <gitster@pobox.com>,
	Luke Diamand <luke@diamand.org>, Jeff King <peff@peff.net>
Subject: Re: [PATCH 00/25] fix buggy tests, modernize tests, fix broken &&-chains
Date: Mon, 2 Jul 2018 11:27:14 -0700
Message-ID: <CAGZ79kZXhZyhDL_+cFK6BzL-RL7Ac0zKeKux97v9shc+qm+nOA@mail.gmail.com> (raw)
In-Reply-To: <20180702002405.3042-1-sunshine@sunshineco.com>

On Sun, Jul 1, 2018 at 5:24 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> This series fixes several buggy tests which went undetected due to
> broken &&-chains in subshells, modernizes some tests to take advantage
> of test functions (test_might_fail(), test_write_lines(), etc.), and
> fixes a lot of broken &&-chains in subshells. It applies atop
> 'master'. Happily, there are no broken &&-chains in subshells in any
> in-flight topic.
>
> It is split out from an earlier series[1] which additionally extended
> --chain-lint to work within subshells. I decided to make this an
> independent series because these (hopefully) non-controversial changes
> all stand on their own merit, and because I don't want to flood the list
> repeatedly with this lengthy series as I re-roll the "extend
> --chain-lint to work in subshells" functionality from [1].
>
> To ease review burden, I largely avoided noisy modernizations and
> cleanups, and limited changes to merely adding "&&" even when some other
> transformation would have made the fix nicer overall. (For example, I
> resisted the urge to replace a series of 'echo' statements with a simple
> here-doc.)
>
> Changes since [1]:
>
> * Found and fixed more &&-chain breakage, and converted a couple more
>   'unset' instances (which were hidden behind a MINGW prerequisite) to
>   sane_unset().
>
> * Rewrote commit messages to sell changes on their own merit rather than
>   leaning on --chain-lint as a crutch. (junio, jrnieder)
>
> * Changed a modernization/cleanup to use "! non-git-command" rather than
>   test_must_fail(), moving it to its own patch in the process. (j6t)
>
> * Changed "printf '%s\n'" idiom to test_write_lines(). (junio)
>
> * Incorporated Stefan's fix[2] for a broken 't/lib-submodule-update'
>   test since my interpretation of the problem was incorrect.
>
> * Converted a subshell 'echo' sequence to write_script().
>
> * Dropped patches which existed primarily to pacify --chain-lint; they
>   are no longer needed since I re-wrote the "linter" to detect &&-chain
>   breakage itself (by pure textual inspection) rather than by
>   incorporating subshell bodies into the main &&-chain:
>
>   t0001: use "{...}" block around "||" expression rather than subshell
>   t3303: use standard here-doc tag "EOF" to avoid fooling --chain-lint
>   t9104: use "{...}" block around "||" expression rather than subshell
>   t9401: drop unnecessary nested subshell
>
> * Dropped a patch which pretty much duplicated Junio's 037714252f
>   (tests: clean after SANITY tests, 2018-06-15), which graduated to
>   'master':
>
>   t7508: use test_when_finished() instead of managing exit code manually
>
> An interdiff against [1] is below, although I stripped out all the noisy
> "printf '%s\n'" to test_write_lines() differences, of which there were a
> lot, since they drowned out the other more significant changes.
>
> Thanks to Elijah, Hannes, Jonathan Nieder, Jonathan Tan, Junio, Luke,
> Peff, and Stefan for comments on [1].

Thanks for this series,
I think it is good to include it as is and build on top if needed. I had some
comments on the earlier part of the series, but that is really just the cherry
on top of the cake.

Thanks,
Stefan

  parent reply index

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-02  0:23 Eric Sunshine
2018-07-02  0:23 ` [PATCH 01/25] t: use test_might_fail() instead of manipulating exit code manually Eric Sunshine
2018-07-02 17:44   ` Stefan Beller
2018-07-02 20:58     ` Eric Sunshine
2018-07-03 19:20   ` Junio C Hamano
2018-07-02  0:23 ` [PATCH 02/25] t: use test_write_lines() instead of series of 'echo' commands Eric Sunshine
2018-07-02 17:53   ` Stefan Beller
2018-07-02  0:23 ` [PATCH 03/25] t: use sane_unset() rather than 'unset' with broken &&-chain Eric Sunshine
2018-07-02  0:23 ` [PATCH 04/25] t: drop unnecessary terminating semicolon in subshell Eric Sunshine
2018-07-03 19:23   ` Junio C Hamano
2018-07-02  0:23 ` [PATCH 05/25] t/lib-submodule-update: fix "absorbing" test Eric Sunshine
2018-07-02  0:23 ` [PATCH 06/25] t5405: use test_must_fail() instead of checking exit code manually Eric Sunshine
2018-07-02  0:23 ` [PATCH 07/25] t5406: use write_script() instead of birthing shell script manually Eric Sunshine
2018-07-02  0:23 ` [PATCH 08/25] t5505: modernize and simplify hard-to-digest test Eric Sunshine
2018-07-02  0:23 ` [PATCH 09/25] t6036: fix broken "merge fails but has appropriate contents" tests Eric Sunshine
2018-07-02  0:23 ` [PATCH 10/25] t7201: drop pointless "exit 0" at end of subshell Eric Sunshine
2018-07-02  0:23 ` [PATCH 11/25] t7400: fix broken "submodule add/reconfigure --force" test Eric Sunshine
2018-07-16 14:43   ` Johannes Schindelin
2018-07-16 15:51     ` Johannes Schindelin
2018-07-16 18:50       ` Eric Sunshine
2018-07-16 21:37         ` Junio C Hamano
2018-07-16 23:05           ` Eric Sunshine
2018-07-17 10:09             ` Johannes Schindelin
2018-07-17 17:53           ` Junio C Hamano
2018-07-02  0:23 ` [PATCH 12/25] t7810: use test_expect_code() instead of hand-rolled comparison Eric Sunshine
2018-07-02 18:05   ` Stefan Beller
2018-07-02 21:12     ` Eric Sunshine
2018-07-02  0:23 ` [PATCH 13/25] t9001: fix broken "invoke hook" test Eric Sunshine
2018-07-02  0:23 ` [PATCH 14/25] t9814: simplify convoluted check that command correctly errors out Eric Sunshine
2018-07-02  0:23 ` [PATCH 15/25] t0000-t0999: fix broken &&-chains Eric Sunshine
2018-07-02  0:23 ` [PATCH 16/25] t1000-t1999: " Eric Sunshine
2018-07-02  0:23 ` [PATCH 17/25] t2000-t2999: " Eric Sunshine
2018-07-02  0:23 ` [PATCH 18/25] t3000-t3999: " Eric Sunshine
2018-07-02  0:23 ` [PATCH 19/25] t3030: " Eric Sunshine
2018-07-02  0:24 ` [PATCH 20/25] t4000-t4999: " Eric Sunshine
2018-07-02  0:24 ` [PATCH 21/25] t5000-t5999: " Eric Sunshine
2018-07-12 12:37   ` SZEDER Gábor
2018-07-12 17:44     ` Eric Sunshine
2018-07-12 18:19       ` Jeff King
2018-07-12 18:31       ` Junio C Hamano
2018-07-12 18:35         ` Junio C Hamano
2018-07-12 18:46           ` Eric Sunshine
2018-07-12 18:50             ` Junio C Hamano
2018-07-12 18:53               ` Eric Sunshine
2018-07-17  8:15           ` SZEDER Gábor
2018-07-02  0:24 ` [PATCH 22/25] t6000-t6999: " Eric Sunshine
2018-07-02  0:24 ` [PATCH 23/25] t7000-t7999: " Eric Sunshine
2018-07-02  0:24 ` [PATCH 24/25] t9000-t9999: " Eric Sunshine
2018-07-02  0:24 ` [PATCH 25/25] t9119: " Eric Sunshine
2018-07-02 18:27 ` Stefan Beller [this message]
2018-07-02 21:20   ` [PATCH 00/25] fix buggy tests, modernize tests, " Eric Sunshine
2018-07-03 19:40 ` Junio C Hamano

Reply instructions:

You may reply publically 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=CAGZ79kZXhZyhDL_+cFK6BzL-RL7Ac0zKeKux97v9shc+qm+nOA@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=jonathantanmy@google.com \
    --cc=jrnieder@gmail.com \
    --cc=luke@diamand.org \
    --cc=newren@gmail.com \
    --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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox