Re: [RFC PATCH (WIP)] Show a dirty working tree and a detached HEAD in status for submodule

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 11.01.2010 23:45, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> * It makes git show submodules as modified in the superproject when
>>   one or more of these conditions are met:
>>
>>     a) The submodule contains untracked files
>>     b) The submodule contains modified files
>>     c) The submodules HEAD is not on a local or remote branch
>>
>>   That can be seen when using either "git status", "git diff[-files]"
>>   & "git diff[-index] HEAD" (and with "git gui" & gitk).
> 
> If the submodule is checked out, _and_ if the HEAD there, either detached
> or not, does not agree with what the "other" one records (i.e. the commit
> recorded in an entry in the index, or in the tree, that you are comparing
> your work tree against), then it also should be considered modified.  I
> don't think your (a)-(c) cover this case.

Right, i did not to add the current (and unchanged) behavior to this
list, i just wrote down the new cases (and these new cases only come
into play when the submodule has been checked out).


> Also I don't understand why you want to treat (c) any specially at all.

To avoid possible loss of commits.

Before doing something like "git checkout -f" or "git reset --hard", it
is a good idea to check via "git status" if you have local changes. I
hope checkout and reset will recurse into submodules in the near future.
when they do, all commits in the submodule which are not on any branch
are lost (at least when the reflog expired). Or the remote branch the
user thinks the submodule is tracking has been deleted or rebased. You
might want to know that before e.g. committing it in the superproject.

Maybe compare it to new or modified files in a git repo: They don't
necessarily pose a problem when committing, you might be able to push
and clone the repo somewhere else and nothing breaks. But you wanna
know about these new and modifies files, in case you just forgot to add
them. So i think the HEAD of a submodule not on any branch is a bit like
a new or modified file in a regular repo, both will not show up in a
different repo than yours unless you do something about it. And a
modification is lost by a checkout or reset just as the dangling commits
will be.

Yes, this test can't provide 100% safety against loss of commits, but at
least we should try to warn if we can detect it. Does it give false
positives (saying the submodules HEAD is dangling when it shouldn't)?
I doubt it. Does it give false negatives? Yes, but we can't do anything
about that due to the distributed nature of git.


> Even if (c) is something we _should_ report, please do not call that as
> "detached" in its implementation.

Correct, that term is misleading in this context. Maybe call it
something like "The submodule contains a HEAD not on any branch"
then? Or "The submodule has a dangling HEAD"?


>> * This behavior is not configurable but activated by default. A config
>>   option is needed here.
> 
> I doubt it.
> 
> My gut feeling is that this should be _always_ on for a submodule
> directory that has been "submodule init/update".  The user is interested
> in that particular submodule, and any change to it should be reported for
> both classes of users.  Theose who meant to use the submodule read-only
> need to be able to notice that they accidentally made the submodule dirty
> before making a commit in the superproject.  Those who wanted to work in
> submodule needs to know if the state is in sync with what they expect
> before making a commit in the superproject.

Yes, me too thinks it should default to on for every initialized
submodule.

But this is a major change in behavior, so it might be a good idea to be
able to turn it off (e.g. if it breaks scripts). Maybe a config option
really isn't such a bright idea, but what about having something like a
"--no-dirty-submodules" command line option?


> That of course is provided if the unconditional check does not trigger for
> submodules that the user hasn't "submodue init"ed; I think you did that
> correctly at the beginning of your is_submodule_modified() implementation.

Yes, that's what that test is for. Will add a comment there.


> But the thing is, in a distributed environment, the submodule HEAD being
> at the tip of _some_ branch (either local or remote) you have doesn't mean
> anything to help them.  IOW, for protect others, you would need a check
> when you _push out_ (either in 'push' or on the receiving end).

This is something on my TODO list: Add a change to "git push" to assert
that all HEADs of initialized submodules lie on a /remote/ branch before
doing the push in the superproject.


> So I'd suggest dropping this condition in "status/diff" that is about
> preparing to make the next commit in your _local_ history.

I would rather have this patch merged without c) than not at all. But i
think it is a worthwhile and rather cheap test. And i would prefer to
change the default behavior of "git status" only once now and not again
later.