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

On Tue, Aug 31, 2021 at 09:43:15PM -0700, Elijah Newren wrote:

On Tue, Aug 31, 2021 at 7:55 PM Taylor Blau [off-list ref] wrote:

quoted

On Tue, Aug 31, 2021 at 01:14:55PM -0700, Elijah Newren wrote:

quoted

quoted

Now I have spent zero time looking into actually coding this, so it may
turn out to be much trickier than I am suggesting. But this seems like a
much more fruitful direction, where we can protect users in cases where
they benefit (and give them sensible and actionable error messages),
without bothering people in the majority of cases where their cwd
doesn't go away.

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?

Hmm. My first thought after reading this is that it would cause
surprising behavior for anybody who had 'git add --all' in their 'rebase
-x' script. But would it?

I.e., imagine somebody doing an in-place sed in a rebase and then `git
add --all`-ing the result at each point in history. If the directory
they were in ever went away, then the *next* revision would add that
directory right back.

That behavior seems somewhat surprising to me, or at least I could
imagine it being surprising to users.

I'm not following.  `git add --all` doesn't add empty directories, so
I don't see how my proposed change would cause any problems in such a
case; nothing would be added back.

Ahh, it was I who wasn't following. You were proposing to leave the
directories in place but empty. Agreed that there wouldn't be any
problems with that.

quoted

Another thought is what should happen when the current directory goes
away and then comes back as a file? We wouldn't be able to checkout that
file, I don't think, so it might be a dead end.

I'm not following this either.  Peff's original suggestion was to
error out only when we knew it could cause problems, in particular
when the working directory would be removed.  Here I've shifted the
way the problem is viewed by just not removing the working directory,
but the end result is the same -- it errors out when the removal was
needed.  Given that erroring out is exactly what we wanted for a case
like this, why does that make it a dead end?

The way you shifted the problem makes it possible for us to discover
that only right before we're about to fail, right? In other words, if
you're doing a rebase then you're potentially leaving a lot of wasted
work on the table if you realize halfway through the operation that you
couldn't complete it.

Even though I think that's *not* what you want for rebase, it ironically
*is* what you might want for bisect, since the path we'll take isn't
known ahead of time. So even if there are some paths that would result
in a directory -> file conversion in the cwd, we don't need to fail the
operation ahead of time if the bisection doesn't actually take that
path.

On the other hand, it does still leave a lot of work on the table if the
bisection does eventually want to change the current working directory
into a file, depending on where that change happens.

Maybe those cases are pretty niche in practice. I have to imagine that
they are. If my guess is right, then I think your approach makes sense.

Thanks,
Taylor