Re: en/keep-cwd (Was: Re: What's cooking in git.git (Dec 2021, #01; Fri, 3))

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Dec 06 2021, Elijah Newren wrote:

> On Mon, Dec 6, 2021 at 5:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> * en/keep-cwd (2021-12-01) 11 commits
>>  - t2501: simplify the tests since we can now assume desired behavior
>>  - dir: new flag to remove_dir_recurse() to spare the original_cwd
>>  - dir: avoid incidentally removing the original_cwd in remove_path()
>>  - stash: do not attempt to remove startup_info->original_cwd
>>  - rebase: do not attempt to remove startup_info->original_cwd
>>  - clean: do not attempt to remove startup_info->original_cwd
>>  - symlinks: do not include startup_info->original_cwd in dir removal
>>  - unpack-trees: add special cwd handling
>>  - unpack-trees: refuse to remove startup_info->original_cwd
>>  - setup: introduce startup_info->original_cwd
>>  - t2501: add various tests for removing the current working directory
>>
>>  Many git commands that deal with working tree files try to remove a
>>  directory that becomes empty (i.e. "git switch" from a branch that
>>  has the directory to another branch that does not would attempt
>>  remove all files in the directory and the directory itself).  This
>>  drops users into an unfamiliar situation if the command was run in
>>  a subdirectory that becomes subject to removal due to the command.
>>  The commands have been taught to keep an empty directory if it is
>>  the directory they were started in to avoid surprising users.
>
> Very nicely written summary.
>
>>
>>  Needs review.
>>  There are some comments on earlier rounds; the latest one needs a
>>  serious review or at least Acks from past commentors.
>>  source: <pull.1140.v5.git.git.1638340854.gitgitgadget@gmail.com>
>
> If it helps, there are two parts to the review:
> - Do we want this feature?
> - Does this patch series implement the feature correctly?
> Let me mention both:
>
> Much of the discussion from commenters was actually related to the
> first point.  While Peff suggested the idea, and Taylor and Phillip
> (Wood) spoke up in favor (and I obviously cared enough to write
> patches), Ævar didn't like it at first.  After a lot of back and
> forth, we eventually discovered some misunderstanding after which
> Ævar, while still not a proponent, dropped his strong objection ("I'm
> much more sympathetic to this approach now."[1])  I was unable to
> determine the opinion of other reviewers/commenters on this point,
> though if I had to guess I'd say Junio is at least marginally in
> favor.

I just really wanted you to pick up & run with the "let's not die then"
WIP work I had, but I also want a pony. So ... :)

IOW I do think it would be really worthwhile to pull at the thread of
not dying, and I just find it a bit conceptually unclean to not /try/
and just die (or recover) on the actual OS error we get.

But I also think I'm just nitpicking / being overly pedantic there.

I can't really think of a realistic scenario where users will really
mind the behavior change you're introducing (in a bad way), but the "in
a good way" case is clearly something that addresses an actual problem.

So I've got no objections to these changes going in as they stand. If
anyone is interested in pulling at the "let's not make it die" thread
that can always be done later, or we can make it configurable or
whatever.

I don't think this needs to be part of your initial series, but just a
question:

Isn't a logical follow-up after it to change the various things like
"git bisect that refuse to run in subdirectories to happily do so?
I.e. the reason for those is specifically this "cwd goes away". Both
because of our bad handling of it, and due to any user confusion that'll
be addressed by that cwd sticking around".