On Mon, Nov 22 2021, Elijah Newren wrote:
> On Sun, Nov 21, 2021 at 9:59 AM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> wrote:
>>
>> On Sun, Nov 21 2021, Elijah Newren via GitGitGadget wrote:
>>
>> > From: Elijah Newren <newren@xxxxxxxxx>
>>
>> > +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@xxxxxxxxxx>) 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...
