git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: "Raymond E. Pasco" <ray@ameretat.dev>
Cc: "Git List" <git@vger.kernel.org>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Martin Ågren" <martin.agren@gmail.com>
Subject: Re: [PATCH] t4069: test diff behavior with i-t-a paths
Date: Mon, 10 Aug 2020 12:23:04 -0400	[thread overview]
Message-ID: <CAPig+cSn_wrBuMKzoUZ720Hy-Y9RuPpJtmZ1mr--cnyAP866-Q@mail.gmail.com> (raw)
In-Reply-To: <20200810085343.43717-1-ray@ameretat.dev>

On Mon, Aug 10, 2020 at 4:54 AM Raymond E. Pasco <ray@ameretat.dev> wrote:
> Add a small test suite to test the behavior of diff with intent-to-add
> paths. Specifically, the diff between an i-t-a entry and a file in the
> worktree should be a "new file" diff, and the diff between an i-t-a
> entry and no file in the worktree should be a "deleted file" diff.
> However, if --ita-visible-in-index is passed, the former should instead
> be a diff from the empty blob.
>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
> diff --git a/t/t4069-diff-intent-to-add.sh b/t/t4069-diff-intent-to-add.sh
> @@ -0,0 +1,30 @@
> +test_expect_success 'diff between i-t-a and file should be new file' '
> +       cat blueprint >test-file &&
> +       git add -N test-file &&
> +       git diff >output &&
> +       grep "new file mode 100644" output
> +'

If someone comes along and inserts new tests above this one and those
new tests make their own changes to the index or worktree, how can
this test be sure that the "new file mode" line is about 'test-file'
rather than some other entry? It might be better to tighten this test,
perhaps like this:

    git diff -- test-file &&

Same comment applies to the other tests.

> +test_expect_success 'diff between i-t-a and no file should be deletion' '
> +       rm -f test-file &&
> +       git diff >output &&
> +       grep "deleted file mode 100644" output
> +'
> +
> +test_expect_success '--ita-visible-in-index diff should be from empty blob' '
> +       cat blueprint >test-file &&
> +       git diff --ita-visible-in-index >output &&
> +       grep "index e69de29" output
> +'

The hard-coded SHA-1 value in the "index" line is going to cause the
test to fail when the test suite is configured to run with SHA-256.
You could fix it by preparing two hash values -- one for SHA-1 and one
for SHA-256 -- and then looking up the value with test_oid() for use
with grep. On the other hand, if you're not interested in the exact
value, but care only that _some_ hash value is present, then you could
just grep for a hex-string.

But what is this test actually checking? In my experiments, this grep
expression will also successfully match the output from the test
preceding this one, which means that the conditions of this test are
too loose.

To tighten this test, perhaps it makes sense to take a different
approach and check the exact output rather than merely grepping for a
particular string. In other words, something like this might be better
(typed in email, so untested):

    cat >expect <<-\EOF &&
    diff --git a/test-file b/test-file
    index HEX..HEX HEX
    --- a/test-file
    +++ b/test-file
    EOF
    cat blueprint >test-file &&
    git diff --ita-visible-in-index -- test-file >raw &&
    sed "s/[0-9a-f][0-9a-f]*/HEX/g' raw >actual &&
    test_cmp expect actual

In fact, this likely would be a good model to use for all the tests.

  parent reply	other threads:[~2020-08-10 16:23 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-04 16:33 [PATCH] apply: Allow "new file" patches on i-t-a entries Raymond E. Pasco
2020-08-04 19:30 ` Junio C Hamano
2020-08-04 20:59   ` Raymond E. Pasco
2020-08-04 22:31   ` [PATCH v2] apply: allow " Raymond E. Pasco
2020-08-04 23:40     ` [PATCH v3] " Raymond E. Pasco
2020-08-04 23:49     ` [PATCH v2] " Junio C Hamano
2020-08-05  0:32       ` Raymond E. Pasco
2020-08-06  6:01         ` [PATCH v4 0/3] apply: handle i-t-a entries in index Raymond E. Pasco
2020-08-06  6:01           ` [PATCH v4 1/3] apply: allow "new file" patches on i-t-a entries Raymond E. Pasco
2020-08-06  6:01           ` [PATCH v4 2/3] apply: make i-t-a entries never match worktree Raymond E. Pasco
2020-08-06 21:00             ` Junio C Hamano
2020-08-06 21:47               ` Raymond E. Pasco
2020-08-06  6:01           ` [PATCH v4 3/3] t4140: test apply with i-t-a paths Raymond E. Pasco
2020-08-06 21:07             ` Junio C Hamano
2020-08-07  3:44               ` Raymond E. Pasco
2020-08-08  7:49           ` [PATCH v5 0/3] apply: handle i-t-a entries in index Raymond E. Pasco
2020-08-08  7:49             ` [PATCH v5 1/3] apply: allow "new file" patches on i-t-a entries Raymond E. Pasco
2020-08-08 13:47               ` Phillip Wood
2020-08-08  7:49             ` [PATCH v5 2/3] apply: make i-t-a entries never match worktree Raymond E. Pasco
2020-08-08 13:46               ` Phillip Wood
2020-08-08 14:07                 ` Raymond E. Pasco
2020-08-08 15:48                   ` Phillip Wood
2020-08-08 15:58                     ` Raymond E. Pasco
2020-08-09 15:25                       ` Phillip Wood
2020-08-09 17:58                       ` Junio C Hamano
2020-08-10 11:03                   ` [PATCH] git-apply.txt: correct description of --cached Raymond E. Pasco
2020-08-10 16:18                     ` Junio C Hamano
2020-08-12 13:32                       ` Phillip Wood
2020-08-12 19:23                         ` Junio C Hamano
2020-08-12 20:52                           ` Raymond E. Pasco
2020-08-12 13:59                       ` Phillip Wood
2020-08-08  7:49             ` [PATCH v5 3/3] t4140: test apply with i-t-a paths Raymond E. Pasco
2020-08-23 15:58               ` Phillip Wood
2020-08-08  7:53           ` [PATCH 1/1] diff-lib: use worktree mode in diffs from i-t-a entries Raymond E. Pasco
2020-08-08  8:48             ` Martin Ågren
2020-08-08 10:46               ` Raymond E. Pasco
2020-08-08 14:21                 ` Martin Ågren
2020-08-09 18:09             ` Junio C Hamano
2020-08-10  8:53             ` [PATCH] t4069: test diff behavior with i-t-a paths Raymond E. Pasco
2020-08-10  8:57               ` [PATCH] diff-lib: use worktree mode in diffs from i-t-a entries Raymond E. Pasco
2020-08-10  9:03               ` [RESEND PATCH v2] " Raymond E. Pasco
2020-08-10 16:22               ` [PATCH] t4069: test diff behavior with i-t-a paths Junio C Hamano
2020-08-10 16:23               ` Eric Sunshine [this message]
2020-08-10 21:47                 ` Eric Sunshine
2020-08-10 22:09                   ` Junio C Hamano
2020-08-10 22:13                     ` Eric Sunshine
2020-08-10 22:22                       ` Junio C Hamano
2020-08-10 23:02                 ` Raymond E. Pasco
2020-08-10 23:21                   ` Eric Sunshine
2020-08-11  3:29                     ` Junio C Hamano
2020-08-08  7:58           ` [HYPOTHETICAL PATCH 0/2] apply: reject modification diffs to i-t-a entries Raymond E. Pasco
2020-08-08  7:58             ` [HYPOTHETICAL PATCH 1/2] " Raymond E. Pasco
2020-08-08  7:58             ` [HYPOTHETICAL PATCH 2/2] t4140: test failure of diff from empty blob to i-t-a path Raymond E. Pasco

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+cSn_wrBuMKzoUZ720Hy-Y9RuPpJtmZ1mr--cnyAP866-Q@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=martin.agren@gmail.com \
    --cc=ray@ameretat.dev \
    --subject='Re: [PATCH] t4069: test diff behavior with i-t-a paths' \
    /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

Code repositories for project(s) associated with this 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).