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

[Mugdha Pattnaik <mugdhapattnaik@gmail.com>]

On Wed, Nov 17, 2021 at 12:24 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Mugdha Pattnaik <mugdhapattnaik@gmail.com> writes:
>
> >> 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.

Yes, I understand why "--force" should not be used. The reason why I
suggested the check for local modifications is because I thought we
should warn users that they have local modifications before we absorb
the gitdir. But I see now that this is okay, considering deinit would
die in such a situation anyway, and users would not lose their work.
The only side-effect of running deinit despite users having local
modifications, would be that the gitdir of the submodule has been
absorbed and that should be okay.

-- 
Mugdha