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

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 15, 2022, 17:14 UTC
Message-ID: <20ffd93d-e3dd-4df6-5ec7-d3577cac910d@github.com>
In-Reply-To: <1ab24e4b-1feb-e1bc-4ae4-c28a69f77e05@github.com>

On 3/15/2022 12:07 PM, Victoria Dye wrote:

Show 9 quoted lines

> Shaoxuan Yuan wrote:
>> 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!

Hello!

Show 19 quoted lines

> I'll answer your question "are the tests on the right track?" [1] inline
> with the tests here. 
> 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]).

This, and other advice that Victoria mentions, are really good points to keep in mind.

Show 18 quoted lines

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

You mention in your cover letter that the ensure_not_expanded tests are not added yet (same with performance tests). Now that you've gotten feedback on this version of the patch, I might recommend the organization you might want for a full series:

1. Add these 'mv' tests to t1092 _without_ the code change. These
   tests should work when the index is expanded, and making the
   code change to not expand the index shouldn't change the
   behavior.

2. Add the performance test so we have a baseline to measure how
   well 'mv' does in the normal case (and how it is slower when
   expanding the index).

3. Make the code change and add the ensure_not_expanded test,
   since the functionality from the tests added in (1) will not
   change and we can report the results from the perf tests
   added in (2). The only thing to test is the new, internal
   behavior that the index is not expanded when doing these
   actions. (Keep in mind that we expect the index to be
   expanded for out-of-cone moves, but it's the in-cone moves
   that we expect to not expand.)

Thanks! -Stolee