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