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

[Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>]

On Wed, Mar 16, 2022 at 1:14 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> Hello!

Hi Derrick,

>
> > 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.
>
> > 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 for the recommendations, they are really helpful! I will try to
address them in the next patch :)

-- 
Thanks & Regards,
Shaoxuan