Re: [PATCH v2 0/4] rm: integrate with sparse-index

[Victoria Dye <vdye@github.com>]

Junio C Hamano wrote:
> Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:
> 
>> Turn on sparse-index feature within `git-rm` command.
> 
> That is a clearly written single-line summary.
> 
>> Add necessary modifications and test them.
> 
> This states an obvious without adding any useful information.  What
> modifications were necessary and why they were necessary, what old
> behaviour was undesirable and added tests prevent them to appear
> again?  These details are better left to the proposed log message of
> individual patches.
> 
> This series, when queued on top of 'master' without anything else,
> seems to pass its own tests, but when combined with the "reset and
> checkout fixes" <pull.1312.v2.git.1659841030.gitgitgadget@gmail.com>
> by Victoria, the last one t1092 fails.
> 
> ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- 
> expecting success of 1092.27 'reset hard with removed sparse dir':
>         init_repos &&
> 
>         test_all_match git rm -r --sparse folder1 &&
>         test_all_match git status --porcelain=v2 &&
> 
>         test_all_match git reset --hard &&
>         test_all_match git status --porcelain=v2 &&
> 
>         cat >expect <<-\EOF &&
>         folder1/
>         EOF
> 
>         git -C sparse-index ls-files --sparse folder1 >out &&
>         test_cmp expect out
> 
> HEAD is now at 703fd3e initial commit
> HEAD is now at 703fd3e initial commit
> HEAD is now at 703fd3e initial commit
> --- full-checkout-out   2022-08-08 17:19:19.820840016 +0000
> +++ sparse-index-out    2022-08-08 17:19:19.836841239 +0000
> @@ -1,3 +1 @@
> -rm 'folder1/0/0/0'
> -rm 'folder1/0/1'
> -rm 'folder1/a'
> +rm 'folder1/'
> not ok 27 - reset hard with removed sparse dir
> #
> #               init_repos &&
> #
> #               test_all_match git rm -r --sparse folder1 &&
> #               test_all_match git status --porcelain=v2 &&
> #
> #               test_all_match git reset --hard &&
> #               test_all_match git status --porcelain=v2 &&
> #
> #               cat >expect <<-\EOF &&
> #               folder1/
> #               EOF
> #
> #               git -C sparse-index ls-files --sparse folder1 >out &&
> #               test_cmp expect out
> #
> ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- 
> 
> When we have the index (incorrectly) fully expanded, and may have
> (incorrectly) working tree files outside of our sparse-cone of
> interest, we may have paths under the 'folder1/' that we may need to
> remove (and report as removed), but after the bug that causes us to
> "incorrectly check out" gets fixed, perhaps the 'folder1/' is the
> only thing that needs removed if it is outside our sparse-cone of
> interest?  IOW, is the test hardcoding the behaviour of a bug that
> was fixed?  I dunno.
> 

This test failure is a result of a behavior change in the logging of 'git
rm' in this series when removing a sparse directory. Patch 4 talks about it
in more detail [1]; I failed to account for it in my series.

I'll re-roll my series and replace the 'test_all_match' on that line to
'run_on_all' to avoid the failure. This isn't the first conflict my series
has caused with this one, so I'll make sure everything builds and tests pass
with the changes from both series before resubmitting.

Thanks for catching this, and sorry for the inconvenience.

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