Re: [PATCH] rebase, cherry-pick, revert: only run from toplevel

From: Jeff King <peff@peff.net>
Date: Sep 1, 2021, 06:30 UTC
Message-ID: <YS8eEtwQvF7TaLCb@coredump.intra.peff.net>
In-Reply-To: <CABPp-BFET9PhgAh+xvMWzTGD+pW3ujOUXO6JOF1rtFMMpDHZvg@mail.gmail.com>

On Tue, Aug 31, 2021 at 11:08:35PM -0700, Elijah Newren wrote:

Show 34 quoted lines

> On Tue, Aug 31, 2021 at 10:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Elijah Newren <newren@gmail.com> writes:
> >
> > > Ooh, this sounds intriguing to me...but what if we changed that rule
> > > slightly and just decided to never make the cwd go away?  Currently,
> > > the checkout code removes directories if they have no tracked or
> > > untracked or ignored files left, i.e. if they're empty.  What if we
> > > decide to only have remove_scheduled_dirs() remove directories that
> > > are empty AND they are not the current working directory?
> >
> > Is that generally doable?  What would we do when the directory the
> > subcommand was started from (or one of its parent directories) is
> > not just missing but has to be a file in the revision the subcommand
> > is trying to checkout?
> 
> The same problem (an untracked directory is in the way) already exists
> and has to be handled by all relevant subcommands, right?
> 
> In particular, if the current working directory only has untracked
> files in it, then the directory cannot be removed.  That will prevent
> us from checking out the revision we want, so we have to throw an
> error.
> 
> So my idea just piggy backs on that, resulting in the same error also
> being shown when the current working directory has 0 untracked files
> within it.
> 
> Since the whole thread started from, "maybe we should throw an error
> instead of continuing if it would result in the current working
> directory getting deleted", I believe this idea does exactly what we
> were looking for...and nicely tailors the new error cases to precisely
> the situations we wanted them for -- when the current working
> directory would have been removed by the old code.

Yeah, this is definitely what I had in mind for my original suggestion: extending the "do not delete directory if it contains an untracked file" rule to the cwd. I guess I was thinking of it as an error, but it really _isn't_ an error in most cases, as long as we don't need to recreate another entry there (and if we do, then as you say, the existing error paths should handle it).

It does leave crufty directories behind in the working tree that Git would not then delete (because it only cleans up directories after it removes their contents, and there are no contents left to remove). That's probably not a big deal, though. If you are moving around history with things like rebase or bisect, you'd likely move back to a working tree with the directory anyway. And it's not like you can't end up with such directories through other means (and "git clean -d" takes care of them).

So I do like the approach.

-Peff

PS I'm assuming this is all taking the cwd of the process based on the
   original "prefix" (since Git processes generally move up to the
   top-level of the working tree). But that still leaves the problem for
   _other_ processes. E.g., if I am working in two terminals, one could
   be in "subdir/" and I could issue a bisect or rebase command from the
   other at the top-level, which would put one of them into the
   confusing state. You could strengthen the rule to "is _any_ process
   using this as a cwd", but it's hard to do so portably. And I know
   I've been annoyed by similar such safety valves before ("why can't I
   delete this? Argh, some process I don't care about is squatting on
   it").