Re: [PATCH 1/8] t2501: add various tests for removing the current working directory

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 23, 2021, 02:19 UTC
Message-ID: <211123.86czmrwqqo.gmgdl@evledraar.gmail.com>
In-Reply-To: <CABPp-BEp3OL7F2J_LzqtC-x-8pBUPO8ZR1fTx_6XbqZeOH1kRw@mail.gmail.com>

On Mon, Nov 22 2021, Elijah Newren wrote:

Show 47 quoted lines

> On Sun, Nov 21, 2021 at 9:59 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Sun, Nov 21 2021, Elijah Newren via GitGitGadget wrote:
>>
>> > From: Elijah Newren <newren@gmail.com>
>>
>> > +test_expect_failure 'checkout fails if cwd needs to be removed' '
>> > +     git checkout foo/bar/baz &&
>> > +     test_when_finished "git clean -fdx" &&
>> > +
>> > +     mkdir dirORfile &&
>> > +     (
>> > +             cd dirORfile &&
>> > +
>> > +             test_must_fail git checkout fd_conflict 2>../error &&
>> > +             grep "Refusing to remove the current working directory" ../error
>> > +     ) &&
>> > +
>> > +     test_path_is_dir dirORfile
>>
>>
>> I'd find this & the rest of this series much easier to understand if we
>> started out by positively asserting the current behavior here, and
>> didn't test_cmp/grep for erro r messages that don't exist anymore.
>
> Yeah, this is my fault for my bad commit message.  I stated I was
> adding tests checking for the problems of interest, making it sound
> like I was testing existing behavior, but I should have stated I was
> adding tests with the behavior we'd prefer to have (i.e. basically a
> test-driven-development) setup.
>
> Also, there really wouldn't need to be so many tests for describing
> the existing behavior.  It's basically just `git
> $OPERATION_THAT_REMOVES_CWD_AS_SIDE_EFFECT` followed by nearly any
> other git command will cause the second and later commands to fail
> with:
>
> ```
> shell-init: error retrieving current directory: getcwd: cannot access
> parent directories: No such file or directory
> fatal: Unable to read current working directory: No such file or directory
> ```
>
> However, we do need a lot of tests for corrected behavior, because
> there are so many different codepaths we can follow which will lead to
> deletion of the current working directory.

Currently if I do e.g.:

    git checkout master
    git clean -dxf
    cd perl
    git checkout v0.99
    cd ../
    git clean -dxfn

Nothing breaks and I don't end up with an empty perl/ to remove. With these patches we'd either die on the "checkout" (I think) keep the "perl" and have an empty perl/ to report in the "git clean -dxfn" at the end (I'm not sure which, I forgot and haven't re-read this series just now).

I think changing it anyway might be justifiable, but changing the behavior of things like that tickles my spidey sense a bit. I.e. I can see people having written scripts like that which would break (it's often easier to cd around after globbing than staying at the top-level, then jump back).

So I wonder (especially with Glen's comment in <20211123003958.3978-1-chooglen@google.com>) if this is being done at the right API level. E.g. maybe it would be better for some commands to ease into this with an advise() or warning() and not a die() or error(), or have the die() be in the likes of "git switch" but not "reset --hard".

Or maybe not, just food for thought...