Re: [PATCH v2 01/14] t3705: test that 'sparse_entry' is unstaged

From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Matheus Tavares Bernardino <matheus.bernardino@usp.br>,
	Derrick Stolee <stolee@gmail.com>,
	vdye@github.com, Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v2 01/14] t3705: test that 'sparse_entry' is unstaged
Date: Tue, 14 Sep 2021 22:22:56 -0700	[thread overview]
Message-ID: <CABPp-BF9rhAK47KQUJtZ9yDTkJ-qoeZ0Q7T4ycLcLSgLoxkzTw@mail.gmail.com> (raw)
In-Reply-To: <8aefce6254c0bcbbbca909a62d033c74c90f980b.1631453010.git.gitgitgadget@gmail.com>

On Sun, Sep 12, 2021 at 6:23 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The tests in t3705-add-sparse-checkout.sh check to see how 'git add'
> behaves with paths outside the sparse-checkout definition. These
> currently check to see if a given warning is present but not that the
> index is not updated with the sparse entries. Add a new
> 'test_sparse_entry_unstaged' helper to be sure 'git add' is behaving
> correctly.
>
> We need to modify setup_sparse_entry to actually commit the sparse_entry
> file so it exists at HEAD but is not already staged in the index.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t3705-add-sparse-checkout.sh | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
> index 2b1fd0d0eef..af81b4b6846 100755
> --- a/t/t3705-add-sparse-checkout.sh
> +++ b/t/t3705-add-sparse-checkout.sh
> @@ -19,6 +19,7 @@ setup_sparse_entry () {
>         fi &&
>         git add sparse_entry &&
>         git update-index --skip-worktree sparse_entry &&
> +       git commit --allow-empty -m "ensure sparse_entry exists at HEAD" &&
>         SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry)
>  }
>
> @@ -36,6 +37,11 @@ setup_gitignore () {
>         EOF
>  }
>
> +test_sparse_entry_unstaged () {
> +       git status --porcelain >actual &&
> +       ! grep "^M  sparse_entry\$" actual

Is there a reason this is ^M rather than ^D?  Granted, both would be
bugs so I wouldn't expect either to appear, but the point of the check
is looking for likely errors.  Wouldn't the more likely error case for
a file not present in the working tree be that we stage the deletion
of the file?

> +}
> +
>  test_expect_success 'setup' "
>         cat >sparse_error_header <<-EOF &&
>         The following pathspecs didn't match any eligible path, but they do match index
> @@ -55,6 +61,7 @@ test_expect_success 'git add does not remove sparse entries' '
>         setup_sparse_entry &&
>         rm sparse_entry &&
>         test_must_fail git add sparse_entry 2>stderr &&
> +       test_sparse_entry_unstaged &&
>         test_cmp error_and_hint stderr &&
>         test_sparse_entry_unchanged
>  '
> @@ -73,6 +80,7 @@ test_expect_success 'git add . does not remove sparse entries' '
>         rm sparse_entry &&
>         setup_gitignore &&
>         test_must_fail git add . 2>stderr &&
> +       test_sparse_entry_unstaged &&
>
>         cat sparse_error_header >expect &&
>         echo . >>expect &&
> @@ -88,6 +96,7 @@ do
>                 setup_sparse_entry &&
>                 echo modified >sparse_entry &&
>                 test_must_fail git add $opt sparse_entry 2>stderr &&
> +               test_sparse_entry_unstaged &&
>                 test_cmp error_and_hint stderr &&
>                 test_sparse_entry_unchanged
>         '
> @@ -98,6 +107,7 @@ test_expect_success 'git add --refresh does not update sparse entries' '
>         git ls-files --debug sparse_entry | grep mtime >before &&
>         test-tool chmtime -60 sparse_entry &&
>         test_must_fail git add --refresh sparse_entry 2>stderr &&
> +       test_sparse_entry_unstaged &&
>         test_cmp error_and_hint stderr &&
>         git ls-files --debug sparse_entry | grep mtime >after &&
>         test_cmp before after
> @@ -106,6 +116,7 @@ test_expect_success 'git add --refresh does not update sparse entries' '
>  test_expect_success 'git add --chmod does not update sparse entries' '
>         setup_sparse_entry &&
>         test_must_fail git add --chmod=+x sparse_entry 2>stderr &&
> +       test_sparse_entry_unstaged &&
>         test_cmp error_and_hint stderr &&
>         test_sparse_entry_unchanged &&
>         ! test -x sparse_entry
> @@ -116,6 +127,7 @@ test_expect_success 'git add --renormalize does not update sparse entries' '
>         setup_sparse_entry "LINEONE\r\nLINETWO\r\n" &&
>         echo "sparse_entry text=auto" >.gitattributes &&
>         test_must_fail git add --renormalize sparse_entry 2>stderr &&
> +       test_sparse_entry_unstaged &&
>         test_cmp error_and_hint stderr &&
>         test_sparse_entry_unchanged
>  '
> @@ -124,6 +136,7 @@ test_expect_success 'git add --dry-run --ignore-missing warn on sparse path' '
>         setup_sparse_entry &&
>         rm sparse_entry &&
>         test_must_fail git add --dry-run --ignore-missing sparse_entry 2>stderr &&
> +       test_sparse_entry_unstaged &&
>         test_cmp error_and_hint stderr &&
>         test_sparse_entry_unchanged
>  '
> @@ -148,6 +161,7 @@ test_expect_success 'do not warn when pathspec matches dense entries' '
>  test_expect_success 'add obeys advice.updateSparsePath' '
>         setup_sparse_entry &&
>         test_must_fail git -c advice.updateSparsePath=false add sparse_entry 2>stderr &&
> +       test_sparse_entry_unstaged &&
>         test_cmp sparse_entry_error stderr
>
>  '
> --
> gitgitgadget

Looks fine otherwise.