Re: [RFC PATCH 1/1] mv: integrate with sparse-index

From: Victoria Dye <vdye@github.com>
Date: Mar 15, 2022, 16:07 UTC
Message-ID: <1ab24e4b-1feb-e1bc-4ae4-c28a69f77e05@github.com>
In-Reply-To: <20220315100145.214054-2-shaoxuan.yuan02@gmail.com>

Shaoxuan Yuan wrote:

Show 6 quoted lines

> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/mv.c                             |  3 +++
>  t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
>

Hi Shaoxuan!

I'll answer your question "are the tests on the right track?" [1] inline with the tests here.

[1] https://lore.kernel.org/git/20220315100145.214054-1-shaoxuan.yuan02@gmail.com/

Show 25 quoted lines

> diff --git a/builtin/mv.c b/builtin/mv.c
> index 83a465ba83..111360ebf5 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -138,6 +138,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  
>  	git_config(git_default_config, NULL);
>  
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.command_requires_full_index = 0;
> +
>  	argc = parse_options(argc, argv, prefix, builtin_mv_options,
>  			     builtin_mv_usage, 0);
>  	if (--argc < 1)
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 2a04b532f9..0a8164c5f6 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1521,4 +1521,38 @@ test_expect_success 'checkout behaves oddly with df-conflict-2' '
>  	test_cmp full-checkout-err sparse-index-err
>  '
>  
> +test_expect_success 'mv' '
> +	init_repos &&
> +

In 't1092', I've tried to write test cases around some of the characteristics relevant to sparse checkout/sparse index. For example:

- files inside vs. outside of sparse cone (e.g., 'deep/a' vs 'folder1/a')
- "normal" directories vs. sparse directories (e.g., 'deep/' vs. 'folder1/')
- directories inside a sparse directory vs. "toplevel" sparse directories
  (e.g., 'folder1/0/' vs. 'folder1/')
- options that follow different code paths, especially if those code paths
  interact with the index differently (e.g., 'git reset --hard' vs 'git
  reset --mixed')
- (probably not relevant for 'git mv') files with vs. without staged changes
  in the index

I've found that exercising these characteristics provides good baseline coverage for a sparse index integration, not leaving any major gaps. I'll also typically add cases specific to any workarounds I need to add to a command (like for 'git read-tree --prefix' [2]).

Also, if some of the information about the test repos (e.g., what's inside vs. outside cone, or what's in the repos in the first place) isn't clear, I'm happy to give a deeper dive into how they're set up.

With all of that in mind, let's go over the cases you have so far.

[2] https://lore.kernel.org/git/90ebcb7b8ff4b4f1ba09abcbe636d639fa597e74.1646166271.git.gitgitgadget@gmail.com/

Show 6 quoted lines

> +	# test first form <source> <destination>
> +	test_all_match git mv deep/a deep/a_mod &&
> +	test_all_match git mv deep/deeper1 deep/deeper1_mod &&
> +	test_all_match git mv deep/deeper2/deeper1/deepest2/a \
> +	deep/deeper2/deeper1/deepest2/a_mod &&
> +

This is a good basis for "inside cone" to "inside cone" moves. That said, I don't think you need all three (since they're all testing effectively the same inside-to-inside cone move). I'd suggest instead adding cases like:

	test_all_match git mv <inside cone> <outside cone> &&
	test_all_match git mv <outside cone> <inside cone> &&
	test_all_match git mv <outside cone> <outside cone> &&

to see how the sparse index behaves when files are moved in and out of sparse directories. Similarly, you may want to try 'git mv <sparse directory> <somewhere else>' to see if that triggers any unintended behavior.

Additionally, I don't *think* 'git mv' prints out the state of the index, so you'll probably want to follow these cases with:

	test_all_match git status --porcelain=v2 &&

which prints the status info in a machine-readable format.

Show 5 quoted lines

> +	run_on_all git reset --hard &&
> +
> +	test_all_match git mv -f deep/a deep/before/a &&
> +	test_all_match git mv -f deep/before/a deep/a &&
> +

Good! The '-f' option will allow one file to overwrite another in the index, which is definitely interesting in a sparse index. Same as above, though, you should verify 'git status --porcelain=v2'.

Show 5 quoted lines

> +	run_on_all git reset --hard &&
> +
> +	test_all_match git mv -k deep/a deep/before/a &&
> +	test_all_match git mv -k deep/before/a deep/a &&
> +

The '-k' option might be interesting in the context of the index, since it pushes past errors that would normally make it exit early. However, if it just skips things that fail rather than exiting with an error, it probably isn't testing anything more than the 'git mv' cases.

Show 7 quoted lines

> +	run_on_all git reset --hard &&
> +
> +	test_all_match git mv -v deep/a deep/a_mod &&
> +	test_all_match git mv -v deep/deeper1 deep/deeper1_mod &&
> +	test_all_match git mv -v deep/deeper2/deeper1/deepest2/a \
> +	deep/deeper2/deeper1/deepest2/a_mod &&
> +

Looking at 'builtin/mv.c', the '-v' "verbose" option only controls whether some verbose printouts are emitted. This might be relevant if the printouts were printing index information that didn't match between 'full-checkout', 'sparse-checkout' and 'sparse-index', but if you haven't seen that, I'd leave these cases out.

Show 6 quoted lines

> +	# test second form <source> ... <destination directory>
> +	run_on_all git reset --hard &&
> +	run_on_all mkdir deep/folder &&
> +	test_all_match git mv deep/a deep/folder &&
> +	test_all_match git mv -v deep/deeper1 deep/folder &&
> +	test_all_match git mv -f deep/deeper2/deeper1/deepest2/a deep/folder

This is a good variation on the standard "inside cone" to "inside cone", and I'd like to see something similar done inside sparse directories. And, similar to above, I don't think '-v' needs to be tested.

> +'
> +
>  test_done

Overall, this is a great start! You've got a good pattern set up (it's very clear to follow), I think it mainly needs some more variety to the test cases. Also, if you find that this test gets way too large after adding more cases, feel free to split it into multiple named tests if one gets too long (e.g. "mv", "mv -f").

My recommendations:

- add tests covering outside-of-sparse-cone 'mv' arguments
- add tests covering 'mv' attempting to move directories (in-cone and
  sparse)
- add some "test_must_fail" tests to see what happens when you do something
  "wrong", e.g. to try to overwrite a file without '-f' (I've found some
  really interesting issues in the past where you expect something to fail
  and it doesn't)
- add 'git status --porcelain=v2' checks to confirm that the 'mv' worked the
  same across the different checkouts
- remove multiples of test cases that test the same general behavior (e.g.,
  'git mv <in-cone file> <in-cone file>' only needs to be done once)
- double-check whether '-v' and '-k' have the ability to affect
  full-checkout/sparse-checkout/sparse-index differently - if not, you
  probably don't need to test them

Thanks for working on this, and I hope this helps!