On 12/7/2021 3:43 PM, Elijah Newren wrote: > On Tue, Dec 7, 2021 at 8:09 AM Derrick Stolee <stolee@xxxxxxxxx> wrote: >> There is _also_ more work to do, as follow-ups. In particular, the thing >> that I thought about was sparse-checkout and created this test which still >> fails at the end of your series (as an addition to t1092) > > Interesting testcase... > >> test_expect_success 'remove cwd' ' >> init_repos && >> >> test_sparse_match git sparse-checkout set deep/deeper1/deepest && >> for repo in sparse-checkout sparse-index >> do >> ( >> cd $repo/deep/deeper1 && >> test-tool getcwd >"$TRASH_DIRECTORY/expect" && >> git sparse-checkout set && >> >> test-tool getcwd >"$TRASH_DIRECTORY/actual" && >> test_sparse_match git status --porcelain && > > However, this line is broken even if the directory weren't removed. > Not because of the "git status --porcelain" part, but because of two > other reasons: > > 1) test_sparse_match presumes it is run from the directory above the > repos while you still have it in $repo/deep/deeper1 > 2) The point of test_sparse_match is to compare the results in two > different repositories, but it'll do this twice and the first time > without the changes having been made to the sparse-index repo. > Perhaps this belonged outside the surrounding for loop? You're right! I had first written this test with test_sparse_match everywhere, but test_sparse_match uses subshells to 'cd' into different places, so it doesn't let us get a failure in the 'test-tool getcwd'. I missed this one because the test fails with 'test-tool getcwd' so I never get to the broken line. > I think I'd either drop the "test_sparse_match" or else just drop the > whole line; the real comparison is the expect/actual files. Dropping > this line makes it a good test. I agree. >> cd "$TRASH_DIRECTORY" && >> test_cmp expect actual >> ) >> done >> ' >> >> Please do not let this test delay the advancement of this series. As we >> find these kinds of issues, we can fix them one-by-one as needed. > > Yeah, sounds good. Since you piqued my interest, though, the problem > is that we're passing an absolute path to remove_dir_recursively() > inside clean_tracked_sparse_directories() when we should be passing a > relative path. (We always chdir(r->worktree) in setup.c, so there's > no need to prepend the path with r->worktree+'/'.) > > Still, the current series is long enough and unless there are issues > others have spotted with it, I'd rather just let it proceed as-is and > then send this fix and a correction of your testcase in separately. Absolutely. Let's pick up this fix another time. Thanks, -Stolee
