git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jerry Zhang <jerry@skydio.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH V2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after
Date: Mon, 31 Jan 2022 15:05:25 -0800	[thread overview]
Message-ID: <xmqqmtjbh5fu.fsf@gitster.g> (raw)
In-Reply-To: <20220129024757.24763-1-jerry@skydio.com> (Jerry Zhang's message of "Fri, 28 Jan 2022 18:47:57 -0800")

Jerry Zhang <jerry@skydio.com> writes:

> diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
> index 80f4a65b28..34a386aee2 100755
> --- a/t/t4204-patch-id.sh
> +++ b/t/t4204-patch-id.sh
> @@ -200,6 +200,36 @@ EOF
>  test_expect_success 'patch-id handles no-nl-at-eof markers' '
>  	cat nonl | calc_patch_id nonl &&
>  	cat withnl | calc_patch_id withnl &&
>  	test_cmp patch-id_nonl patch-id_withnl
>  '

This is not a new issue, but this old test script needs updating.
And worse yet, your new test copies the existing badness and make it
twice as bad.

We frown upon doing anything outside test_expect_* blocks these
days, so the commands to prepare nonl and withnl files before thie
precontext of this patch needs to be folded into the test body.

Also, sending a single file using "cat" into pipeline is an
antipattern.  Just redirect into the downstream command instead,
i.e.

	test_expect_success 'patch-id handles no-nl-at-eof markers' '
		cat >nonl <<-\EOF &&
                diff --git i/a w/a
                index e69de29..2e65efe 100644
                --- i/a
                +++ w/a
                @@ -0,0 +1 @@
                +a
                \ No newline at end of file
                diff --git i/b w/b
                index e69de29..6178079 100644
                --- i/b
                +++ w/b
                @@ -0,0 +1 @@
                +b
                EOF
		cat >withnl <<-\EOF &&
		... likewise ...
		EOF
		calc_patch_id nonl <nonl &&
		calc_patch_id withnl <withnl &&
		test_cmp patch-i_ nonl patch-id-withnl
	'
	
or something like that.

I think the "handles no-nl-at-eof markers" test at the end is the
first and only serious offender, so instead of leaving it after the
series, we probalby would want to have a preliminary clean-up patch
before this patch, and fix the test this patch adds.

Thanks.

      parent reply	other threads:[~2022-01-31 23:05 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-25 22:16 [PATCH] patch-id: fix scan_hunk_header on diffs with -U1 Jerry Zhang
2022-01-26  5:13 ` Junio C Hamano
2022-01-29  2:47 ` [PATCH V2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after Jerry Zhang
2022-01-31 18:55   ` Junio C Hamano
2022-01-31 22:54   ` Junio C Hamano
2022-01-31 23:05   ` Junio C Hamano [this message]

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=xmqqmtjbh5fu.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jerry@skydio.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).