Re: submodules' shortcomings, was Re: RFC: display dirty submodule working directory in git gui and gitk

From: Jens Lehmann <jens.lehmann@web.de>
Date: Jan 5, 2010, 11:57 UTC
Message-ID: <4B43292C.5060106@web.de>
In-Reply-To: <7v1vi428w0.fsf@alter.siamese.dyndns.org>

Am 05.01.2010 10:33, schrieb Junio C Hamano:

Show 13 quoted lines

> So it is not necessarily a bad thing if the commit checked out in the
> submodule repository is different from what the superproject records in
> its index when a commit is made in the superproject.  We allow committing
> with local changes in regular files, while we do notify the users about
> them to avoid mistakes.  We should give the same kind of notification
> about submodules, but the "local changes" need to be thought out more
> carefully than plain files in the superproject itself.  Does uncommitted
> changes in the index of submodule repository count?  Local changes in the
> work tree files?  What about untracked files that the user might have
> forgot to add?  Should they be warned?  What about the commit in the
> submodule repository being a non-descendant of the commit recorded in the
> HEAD of the superproject's tree, resulting in a non-ff change at the
> submodule level?

Committing in the superproject with any dirty state in a submodule should always work (same as it does with local changes in regular files), but be visible for the user (again as local changes in regular files are). Right now we do not show enough information about a submodule to protect the user from accidentally throwing away changes made inside it. The only thing we show right now are the differences between submodule commits and what the superproject has in its index and in its commits. Missing are:

  a) modified files
     I think these have to be shown, no matter if they are checked into
     the submodules index or not (because until they are committed, they
     can't be staged in the superproject anyway).

  b) new unignored files
     IMO these files should show up too (the superproject doesn't show
     ignored files, the submodule state shouldn't do that either). But
     OTOH i don't see a possibility for loss of data when this state is
     not shown.

  c) a detached HEAD not on any local *or* remote branch
     This can be fatal when doing a reset, revert or checkout, so it
     should be shown. Alternatively when applied on a submodule, forcing
     could be disabled to let the command fail instead of throwing stuff
     away.

  d) a detached HEAD not on any remote branch
     AFAICS this is only important for a push, and could just error out
     there.

(But i don't think it is necessary to show detailed information, just what type of states are found in the submodule)

Concerning Dscho's remarks about the performace impact: We could control this behavior via .gitmodules too (and later have different settings for the submodules depending on the group the user chose). So you could turn these checks off for repos where you don't care, saving the time to go through the whole working directory of the submodule. But i would vote for the default to show at least case a) and maybe even c) to follow the principle of least surprise.

Show 12 quoted lines

> I think "clone" has a chicken-and-egg problem.  If all of your project
> participant are expected to check out all the submodules, are expected to
> make commits in all of them, and essentially have to track everything in
> sync, then "clone" can obviously do that without asking what kind of
> participant you are [*1*].  Otherwise, you need to have some mechanism
> (e.g. "group mapping" you mentioned earlier) for the user to specify "I am
> interested in these submodules" before the actual sub-clones to happen,
> but until you clone the superproject that has some description for that
> mechanism to use, and the user to see what's available, you cannot say
> what kind of participant you are.  It has to become two-step process;
> either "clone" going interactive in the middle, or you let the clone to
> happen and then "submodule init" to express that information.

Yes, we can leave it that way for now (first "clone" and then "submodule init <the submodules you need>"). We can migrate to the "group mapping" functionality later (which would then allow to force certain submodules to always be populated because they appear in every group).