Mugdha Pattnaik [off-list ref] writes:
quoted
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.
On Wed, Nov 17, 2021 at 12:24 AM Junio C Hamano [off-list ref] wrote:
Mugdha Pattnaik [off-list ref] writes:
quoted
quoted
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