Re: [PATCH] t4069: test diff behavior with i-t-a paths

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 10, 2020, 16:23 UTC
Message-ID: <CAPig+cSn_wrBuMKzoUZ720Hy-Y9RuPpJtmZ1mr--cnyAP866-Q@mail.gmail.com>
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:

Show 17 quoted lines

> 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.

Show 11 quoted lines

> +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.