Re: [PATCH v6] submodule: absorb git dir instead of dying on deinit

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2021, 18:54 UTC
Message-ID: <xmqqy25o9bie.fsf@gitster.g>
In-Reply-To: <CAA4dvxgNJ8eyuc5B6_GnSLHMWjdbJN5k_rTCLjWndEyjv_vOnw@mail.gmail.com>

Mugdha Pattnaik <mugdhapattnaik@gmail.com> writes:

Show 19 quoted lines

>> OK.  That sounds like an improvement, albeit possibly an overly
>> cautious one, as a casual "deinit" user will get an error as before
>> without "--force", which may or may not be a good thing.  Requiring
>> "--force" means it is safer by default by not changing the on-disk
>> data.  But requiring "--force" also means we end up training users
>> to say "--force" when it shouldn't have to be.
>> ...
>> Does "git submodule" currently reject a "deinit" request due to some
>> _other_ conditions or safety concerns and require the "--force"
>> option to continue?  Requiring the "--force" option to resolve ".git
>> is a directory, and the user wants to make it absorbed" means that
>> the user will be _forced_ to bypass these _other_ safety valves only
>> to save the submodule repository from destruction when running
>> "deinit", which may not be a good trade-off between the safety
>> requirements of these _other_ conditions, if exists, and the one we
>> are dealing with.
>
> This is definitely a situation we want to avoid. How about we try to run
> a check for uncommitted local modifications first?

I am not sure if I follow. If we stop (ab)using "--force" for the situation (i.e. where today's "deinit" would die because .git needs to be absorbed first), then the user would not have to say "--force" which may override other safety valve. You'd check if .git needs to be absorbed, make it absorbed as needed while reporting the fact that you did so, and then let the existing "deinit" code to take over. If there are other safety checks that needs "--force" to be overridden, that is handled (presumably) correctly by the existing code, no? So other than "do we need absorbing, and if so do it for the user" check, I do not think you'd need to add any new "we try to run a check for ..." at all.