git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH v2 1/4] t4014: clean up style
Date: Mon, 19 Aug 2019 22:41:22 -0400
Message-ID: <CAPig+cSzq7KB4y+wuuVtVS7oi=uK2FY0+UkfOXx1jp-oe=fs4Q@mail.gmail.com> (raw)
In-Reply-To: <76a0a274fde564f709d294acd7d6da17167f05b5.1566258525.git.liu.denton@gmail.com>

On Mon, Aug 19, 2019 at 7:53 PM Denton Liu <liu.denton@gmail.com> wrote:
> In Git's tests, there is typically no space between the redirection
> operator and the filename. Remove these spaces.
>
> Since output is silenced when running without `-v` and debugging
> output is useful with `-v`, remove redirections to /dev/null.
>
> Change here-docs from `<<\EOF` to `<<-\EOF` so that they can be indented
> along with the rest of the test case.
>
> Convert all instances of `cnt=$(... | wc -l) && test $cnt = N` into
> uses of `test_line_count()`.
>
> For style, move the ending sq of test cases onto its own line whenever
> they do not conform.
>
> Rename output files from "expected" to "expect" to conform with the
> usual convention.
>
> Finally, refactor to remove Git commands upstream of pipe as well as Git
> commands that are in a non-variable-assignment subshell (e.g. `echo
> "base-commit: $(git rev-parse HEAD)"`. This way, if an invocation of a
> Git command fails, the return code won't be lost. Keep upstream non-Git
> commands since we have to assume a base level of sanity.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> This patch is getting a little unwieldy. Perhaps we could split it into
> several smaller patches? Unfortunately, I'm not really sure where a
> logical place to split it would be.

The bullet points in the commit message, each of which is a distinct
change, give a strong hint as to how the commit could be split into
smaller pieces. It would make repeated review easier (though I'm not
sure it's worth re-rolling just for that).

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> @@ -62,26 +63,24 @@ test_expect_success setup '
>  test_expect_success "format-patch --ignore-if-in-upstream" '
>
>         git format-patch --stdout \
>                 --ignore-if-in-upstream master..side >patch1 &&
> -       cnt=$(grep "^From " patch1 | wc -l) &&
> -       test $cnt = 2
> -
> +       grep "^From " patch1 >from1 &&
> +       test_line_count = 2 from1
>  '

Here you've removed the blank line following the body before the
closing quote, which brings the formatting more in line with current
style, however, you could do the same with the blank line before the
body. Ditto for other tests.

Another style fix would be to change the double quotes in the test
title to single quotes (here and in other tests).

> @@ -357,7 +369,7 @@ test_expect_success 'reroll count (-v)' '
>  check_threading () {
>         expect="$1" &&
>         shift &&
> -       (git format-patch --stdout "$@"; echo $? > status.out) |
> +       git format-patch --stdout "$@" >patch &&
>         # Prints everything between the Message-ID and In-Reply-To,
>         # and replaces all Message-ID-lookalikes by a sequence number
>         perl -ne '
> @@ -372,12 +384,11 @@ check_threading () {
>                         print;
>                 }
>                 print "---\n" if /^From /i;
> -       ' > actual &&
> -       test 0 = "$(cat status.out)" &&
> +       ' <patch >actual &&
>         test_cmp "$expect" actual
>  }

If you do break this patch into smaller pieces, this might deserve its
own patch (or not) because it requires a bit of extra reasoning by the
reviewer.

> @@ -852,20 +866,21 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
> -git_version="$(git --version | sed "s/.* //")"
> -
>  signature() {
> +       git_version="$(git --version >version && sed "s/.* //" version)" &&
>         printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
>  }

Windows folks are not going to like you for this change since process
creation is so expensive on that platform, and the point of setting
that variable globally was to avoid repeated invocation (especially
since the output of "git --version" won't change). This function
appears to be invoked only three times presently, but that number
could increase in the future as new tests are added. Consequently, it
might be best to drop this change.

  reply index

Thread overview: 91+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-19 23:52 [PATCH v2 0/4] format-patch: learn --infer-cover-subject option Denton Liu
2019-08-19 23:52 ` [PATCH v2 1/4] t4014: clean up style Denton Liu
2019-08-20  2:41   ` Eric Sunshine [this message]
2019-08-19 23:52 ` [PATCH v2 2/4] Doc: add more detail for git-format-patch Denton Liu
2019-08-20  2:44   ` Eric Sunshine
2019-08-20  7:07     ` Denton Liu
2019-08-19 23:52 ` [PATCH v2 3/4] config/format.txt: make clear the default value of format.coverLetter Denton Liu
2019-08-20  2:47   ` Eric Sunshine
2019-08-19 23:52 ` [PATCH v2 4/4] format-patch: learn --infer-cover-letter option Denton Liu
2019-08-20  3:46   ` Eric Sunshine
2019-08-20  7:18 ` [PATCH v3 00/13] format-patch: learn --infer-cover-subject option (also t4014 cleanup) Denton Liu
2019-08-20  7:18   ` [PATCH v3 01/13] t4014: drop unnecessary blank lines from test cases Denton Liu
2019-08-20  7:18   ` [PATCH v3 02/13] t4014: s/expected/expect/ Denton Liu
2019-08-20 21:31     ` Eric Sunshine
2019-08-20  7:18   ` [PATCH v3 03/13] t4014: move closing sq onto its own line Denton Liu
2019-08-20  7:18   ` [PATCH v3 04/13] t4014: use sq for test case names Denton Liu
2019-08-20  7:18   ` [PATCH v3 05/13] t4014: remove spaces after redirect operators Denton Liu
2019-08-20  7:18   ` [PATCH v3 06/13] t4014: use indentable here-docs Denton Liu
2019-08-20  7:19   ` [PATCH v3 07/13] t4014: drop redirections to /dev/null Denton Liu
2019-08-20  7:19   ` [PATCH v3 08/13] t4014: use test_line_count() where possible Denton Liu
2019-08-20  7:19   ` [PATCH v3 09/13] t4014: remove confusing pipe in check_threading() Denton Liu
2019-08-20  7:19   ` [PATCH v3 10/13] t4014: stop losing return codes of git commands Denton Liu
2019-08-20  7:31     ` Denton Liu
2019-08-20 19:04       ` Johannes Sixt
2019-08-20  7:19   ` [PATCH v3 11/13] Doc: add more detail for git-format-patch Denton Liu
2019-08-21 18:26     ` Junio C Hamano
2019-08-20  7:19   ` [PATCH v3 12/13] config/format.txt: specify default value of format.coverLetter Denton Liu
2019-08-20  7:19   ` [PATCH v3 13/13] format-patch: learn --infer-cover-subject option Denton Liu
2019-08-21 19:32     ` Junio C Hamano
2019-08-23 18:15       ` Denton Liu
2019-08-23 18:46         ` Philip Oakley
2019-08-23 20:18           ` Junio C Hamano
2019-08-24  8:03             ` Denton Liu
2019-08-24 13:59               ` Philip Oakley
2019-08-26 14:30                 ` Junio C Hamano
2019-08-26 14:26               ` Junio C Hamano
2019-08-26 16:05                 ` Junio C Hamano
2019-08-22 20:18   ` [PATCH v3 00/13] format-patch: learn --infer-cover-subject option (also t4014 cleanup) Junio C Hamano
2019-08-23 18:19     ` Denton Liu
2019-08-23 20:25       ` Junio C Hamano
2019-08-24  8:25   ` [PATCH 00/13] format-patch: clean up tests and documentation Denton Liu
2019-08-24  8:26     ` [PATCH 01/13] t4014: drop unnecessary blank lines from test cases Denton Liu
2019-08-24  8:26     ` [PATCH 02/13] t4014: s/expected/expect/ Denton Liu
2019-08-24  8:26     ` [PATCH 03/13] t4014: move closing sq onto its own line Denton Liu
2019-08-24  8:26     ` [PATCH 04/13] t4014: use sq for test case names Denton Liu
2019-08-24  8:26     ` [PATCH 05/13] t4014: remove spaces after redirect operators Denton Liu
2019-08-24  8:27     ` [PATCH 06/13] t4014: use indentable here-docs Denton Liu
2019-08-24  8:27     ` [PATCH 07/13] t4014: drop redirections to /dev/null Denton Liu
2019-08-24  8:27     ` [PATCH 08/13] t4014: let sed open its own files Denton Liu
2019-08-26  0:42       ` Eric Sunshine
2019-08-24  8:27     ` [PATCH 09/13] t4014: use test_line_count() where possible Denton Liu
2019-08-24  8:27     ` [PATCH 10/13] t4014: remove confusing pipe in check_threading() Denton Liu
2019-08-24  8:27     ` [PATCH 11/13] t4014: stop losing return codes of git commands Denton Liu
2019-08-24  8:27     ` [PATCH 12/13] Doc: add more detail for git-format-patch Denton Liu
2019-08-26 15:20       ` Junio C Hamano
2019-08-26 16:07         ` Junio C Hamano
2019-08-24  8:27     ` [PATCH 13/13] config/format.txt: specify default value of format.coverLetter Denton Liu
2019-08-24  8:28     ` [PATCH 00/13] format-patch: clean up tests and documentation Denton Liu
2019-08-26 15:21       ` Junio C Hamano
2019-08-27  4:04     ` [PATCH v2 " Denton Liu
2019-08-27  4:04       ` [PATCH v2 01/13] t4014: drop unnecessary blank lines from test cases Denton Liu
2019-08-27  4:04       ` [PATCH v2 02/13] t4014: s/expected/expect/ Denton Liu
2019-08-27  4:04       ` [PATCH v2 03/13] t4014: move closing sq onto its own line Denton Liu
2019-08-27  4:04       ` [PATCH v2 04/13] t4014: use sq for test case names Denton Liu
2019-08-27  4:05       ` [PATCH v2 05/13] t4014: remove spaces after redirect operators Denton Liu
2019-08-27  4:05       ` [PATCH v2 06/13] t4014: use indentable here-docs Denton Liu
2019-08-27  4:05       ` [PATCH v2 07/13] t4014: drop redirections to /dev/null Denton Liu
2019-08-27  4:05       ` [PATCH v2 08/13] t4014: let sed open its own files Denton Liu
2019-08-27  4:05       ` [PATCH v2 09/13] t4014: use test_line_count() where possible Denton Liu
2019-08-27  4:05       ` [PATCH v2 10/13] t4014: remove confusing pipe in check_threading() Denton Liu
2019-08-27  4:05       ` [PATCH v2 11/13] t4014: stop losing return codes of git commands Denton Liu
2019-08-27  4:05       ` [PATCH v2 12/13] Doc: add more detail for git-format-patch Denton Liu
2019-08-27  4:05       ` [PATCH v2 13/13] config/format.txt: specify default value of format.coverLetter Denton Liu
2019-09-04 11:21       ` [PATCH v2 00/13] format-patch: clean up tests and documentation Denton Liu
2019-09-05 19:56         ` Junio C Hamano
2019-09-05 21:40           ` Denton Liu
2019-10-11 19:12   ` [PATCH v4 0/3] format-patch: learn --cover-from-description option Denton Liu
2019-10-11 19:12     ` [PATCH v4 1/3] format-patch: remove erroneous and condition Denton Liu
2019-10-11 19:12     ` [PATCH v4 2/3] format-patch: use enum variables Denton Liu
2019-10-12  2:16       ` Junio C Hamano
2019-10-11 19:12     ` [PATCH v4 3/3] format-patch: teach --cover-from-description option Denton Liu
2019-10-12  2:36       ` Junio C Hamano
2019-10-11 19:23     ` [PATCH v4 4/3] fixup! " Denton Liu
2019-10-12  4:18     ` [PATCH v4 0/3] format-patch: learn " Junio C Hamano
2019-10-14 20:46     ` [PATCH v5 " Denton Liu
2019-10-14 20:46       ` [PATCH v5 1/3] format-patch: change erroneous and condition Denton Liu
2019-10-15  2:16         ` Junio C Hamano
2019-10-15  3:45           ` Denton Liu
2019-10-14 20:47       ` [PATCH v5 2/3] format-patch: use enum variables Denton Liu
2019-10-14 20:47       ` [PATCH v5 3/3] format-patch: teach --cover-from-description option Denton Liu
2019-10-15  2:25         ` 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='CAPig+cSzq7KB4y+wuuVtVS7oi=uK2FY0+UkfOXx1jp-oe=fs4Q@mail.gmail.com' \
    --to=sunshine@sunshineco.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=liu.denton@gmail.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 list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror http://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

Example config snippet for mirrors

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.io/gmane.comp.version-control.git

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

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