Jens Lehmann <Jens.Lehmann@xxxxxx> writes:
> Am 30.05.2013 01:58, schrieb Junio C Hamano:
>> * jl/submodule-mv (2013-04-23) 5 commits
>> (merged to 'next' on 2013-04-23 at c04f574)
>> + submodule.c: duplicate real_path's return value
>> (merged to 'next' on 2013-04-19 at 45ae3c9)
>> + rm: delete .gitmodules entry of submodules removed from the work tree
>> + Teach mv to update the path entry in .gitmodules for moved submodules
>> + Teach mv to move submodules using a gitfile
>> + Teach mv to move submodules together with their work trees
>>
>> "git mv A B" when moving a submodule A does "the right thing",
>> inclusing relocating its working tree and adjusting the paths in
>> the .gitmodules file.
>
> There are only two issues I'm aware of:
>
> *) When the .gitmodules file is already modified but unchanged
> running rm or mv on a submodule will stage those changes too.
>
> *) There is a harmless but unnecessary double invocation of strlen()
> in the function (fixed by the diff below).
>
> I plan to fix the first issue in another patch which would also get
> rid of the second issue, as exactly that code would have to be touched
> anyways.
>
> Does that make sense?
In general I think whatever you think that makes sense in this area
would make sense ;-).
I do not feel confident that I am reading what you mean by "already
modified but unchanged" right. Do you mean the working tree version
is different from both HEAD and the index (HEAD and the index may or
may not match and that does not change the situation)? Assuming
that is the case, i.e. the situation you are worried about is:
When ".gitmodules" has a local modification the user chose not
to "add" yet. Then the user does "git rm/mv" on a submodule
that is unrelated to the submodule whose setting the user has
changes for.
I am curious what the plan is to "fix" this. An obviously safe
thing to do is to error out with a "You have local modification;
please 'git add .gitmodules' first." but if that advice/suggestion
is always the right course of action for the user, it invites "then
why doesn't Git do so for me?", which would in turn support that it
is not an issue in the first place (it deserves to be mentioned in a
warning, "adding your local modifications together with change
needed for rm/mv", though).
I think in the ideal world, you may want to apply the change needed
for rm/mv to the version in the index, and then update the working
tree version by doing a 3-way merge. We already know that eventually
we would need a merge driver that is specific to the file format
that git-config uses, possibly even taking an advantage of the
knowledge of not just the file format but also the semantics of
individual variables [*1*], so we may want to keep it in mind that a
three-way merge would be the eventual goal, while settling on an
"error out on local mod" just like "git checkout anotherbranch" used
to always stop (before we taught the "-m" option to it) when a local
change needed a 3-way merge to be carried along to the new branch.
So my gut feeling of the "fix" at this point in the evolution of the
program may be to error out with a message like "You have local
changes to .gitmodules; please stash it before moving or removing".
[Footnote]
*1* I think all the variables in .gitmodules are single-valued, so
the original submodule.dir.path's value was "dir", the local
modification by the user was to make it "folder", and rm wants
to delete an unrelated submodule.mod.* altogether, we can apply
the usual 3-way merge policy per variable basis to update
submodule.dir.pah to "folder".
A more general merge driver to handle git-config format files
would have a way to be told that some variables are additive
with the -X<driver-specific-option> mechanism. When variable
foo.bar is specified as a multi-valued set of variables, the
original has a single foo.bar="xyzzy", one side adds a
foo.bar="frotz while the other side modifies the original to
foo.bar="nitfol", the ideal way for such a merge driver to
operate is to leave two definitions (xyzzy will be gone and
frotz and nitfol remain).
But I highly suspect that would need a much larger change to the
configuration file parser and writer that is totally out of
scope with this change, and that is why my recommendation at
this point is just to follow the example of pre-"-m" era of "git
checkout anotherbranch".
> ----------8<-----------------
> diff --git a/submodule.c b/submodule.c
> index edfc23c..4670af7 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -102,7 +102,7 @@ void stage_updated_gitmodules(void)
> struct cache_entry *ce;
> int namelen = strlen(".gitmodules");
>
> - pos = cache_name_pos(".gitmodules", strlen(".gitmodules"));
> + pos = cache_name_pos(".gitmodules", namelen);
> if (pos < 0) {
> warning(_("could not find .gitmodules in index"));
> return;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html