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

From: Elijah Newren <newren@gmail.com>
Date: Nov 23, 2021, 03:11 UTC
Message-ID: <CABPp-BH-MtGLAk3vkFCXr8sB3KmMEp98J1wpjO=v6MbXH4tPHg@mail.gmail.com>
In-Reply-To: <211123.86czmrwqqo.gmgdl@evledraar.gmail.com>

On Mon, Nov 22, 2021 at 6:27 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 65 quoted lines

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

Show 5 quoted lines

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