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

[Elijah Newren <newren@gmail.com>]

On Mon, Nov 22, 2021 at 6:27 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Mon, Nov 22 2021, Elijah Newren wrote:
>
> > 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).

You'd have an empty perl/ left after the checkout, which would be
cleaned up by your final git clean command.

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

I disagree this would break any user scripts.  If people expect a 'git
checkout' or 'git rebase' to always work, their script is _already_
broken.  The presence of any untracked files within the directory
already results in a hard error -- we refuse to remove non-empty
directories (unless all files are tracked and unmodified).  This rule
deserves a clarification: treat the current working directory as
non-empty since the parent process is likely still parked there..

Further, our own commands are broken/misbehaving due to us not
erroring out; see e.g.
https://lore.kernel.org/git/xmqqv93n7q1v.fsf@gitster.g/ and its
grandparent.  User scripts likely have lurking problems too.

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

Glen's comment was interesting, but provided no specifics about how
the changes I made could cause any possible harm in his case.
Further, the fact that others are adding extra places doing cleanup
sound like additional codepaths that should be protected for the exact
same reasons.  I think we absolutely want my changes affecting his new
codepaths.

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

The commands that don't need to remove the current working directory
but just were as a convenience, no longer do and continue on just
fine.  Commands that need to remove the current working directory in
order to place a file there will error out, just as they would have
when trying to remove a directory with untracked files.  I see no need
to ease into anything here.

> Or maybe not, just food for thought...

You may also be interested in reading more of the other thread I
linked to from my cover letter; all these cases were discussed in good
detail over there.  For example, look at
https://lore.kernel.org/git/CABPp-BFmU+RaAjq4_0-PSfRgH1Jc63nN0fMuDWk2+iDbdz7CCA@mail.gmail.com/.
Peff's previous suggestion was to just make the commands error out if
they'd normally remove the current working directory and require the
user to run from a different directory instead.  My version lightened
that requirement so it only errors out if the current working
directory needed to be removed in order to place something else there
(and if nothing else was needed to be placed there, then just leaving
the directory around).