Re: [PATCH v5 00/11] Avoid removing the current working directory, even if it becomes empty

From: Derrick Stolee <stolee@gmail.com>
Date: Dec 7, 2021, 21:00 UTC
Message-ID: <a6730006-bc30-d1c6-2d9a-c00df505d38d@gmail.com>
In-Reply-To: <CABPp-BHRse4BwXTjd4cRruOdymt=DJRPDL1jyPzX5HDqx4ymDw@mail.gmail.com>

On 12/7/2021 3:43 PM, Elijah Newren wrote:

Show 31 quoted lines

> On Tue, Dec 7, 2021 at 8:09 AM Derrick Stolee <stolee@gmail.com> 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.

Show 18 quoted lines

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