Jens Lehmann wrote:
> Currently using "git mv" on a submodule moves the submodule's work tree in
> that of the superproject.
It's not "Currently": it's the result of your last two patches. The
wording is very unclear. Perhaps: As a result of the last two patches
in this series, a 'git mv' moves the submodule directory to a new path
in the superproject's worktree, and updates the cache entry in the
index appropriately.
> But the submodule's path setting in .gitmodules
> is left untouched, which is now inconsistent with the work tree and makes
> git commands that rely on the proper path -> name mapping (like status and
> diff) behave strangely.
I'm frankly a little surprised that your previous two patches didn't
break any tests. That's probably because we don't have enough tests
to exercise git-core with submodules. I'm not saying that it's a bad
thing though: submodules are still immature, and tight tests would get
in the way of new patches.
> Let "git mv" help here by not only moving the submodule's work tree but
> also updating the "submodule.<submodule name>.path" setting from the
> .gitmodules file and stage both.
> This doesn't happen when no .gitmodules
> file is found and only issues a warning when it doesn't have a section for
> this submodule. This is because the user might just use plain gitlinks
> without the .gitmodules file
How will git-core work without a .gitmodules? Shouldn't we create a
fresh .gitmodules here?
> or has already updated the path setting by
> hand before issuing the "git mv" command (in which case the warning
> reminds him that mv would have done that for him).
Shouldn't these two cases issue different warnings?
Besides, why is this explanation missing in your rm patch's commit message?
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 609bbb8..36e5605 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -239,6 +242,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> rename_cache_entry_at(pos, dst);
> }
>
> + if (gitmodules_modified)
> + stage_updated_gitmodules();
> +
I'm unhappy with this, but we I'll have to live with my
dissatisfaction until we get rid of ".gitmodules" (if that ever
happens). mv has a clear task: it should perform a filesystem mv,
manipulate the index, and write out the changed index. Adding an
unrelated file to the index has nothing to do with its primary task.
> diff --git a/submodule.c b/submodule.c
> index eba9b42..fb742b4 100644
> --- a/submodule.c
> +++ b/submodule.c
All my comments in the rm review apply here too, so I won't repeat myself.
> @@ -10,6 +10,7 @@
> #include "string-list.h"
> #include "sha1-array.h"
> #include "argv-array.h"
> +#include "blob.h"
Interesting. Let's see why you need blob.h.
> static struct string_list config_name_for_path;
> static struct string_list config_fetch_recurse_submodules_for_name;
> @@ -30,6 +31,67 @@ static struct sha1_array ref_tips_after_fetch;
> */
> static int gitmodules_is_unmerged;
>
> +/*
> + * Try to update the "path" entry in the "submodule.<name>" section of the
> + * .gitmodules file.
> + */
> +int update_path_in_gitmodules(const char *oldpath, const char *newpath)
> +{
> + struct strbuf entry = STRBUF_INIT;
> + struct string_list_item *path_option;
> +
> + if (!file_exists(".gitmodules")) /* Do nothing whithout .gitmodules */
> + return -1;
s/whithout/without. Why are you not returning an error()? Don't you
want to tell the user that no ".gitmodules" was found?
> +void stage_updated_gitmodules(void)
void return? Don't you want the caller to know whether we were
successful or not?
Why does the name contain "updated"? Why does the function care if I
updated the .gitmodules or not? It's just staging .gitmodules, as it
is on the filesystem.
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct stat st;
> + int pos;
> + struct cache_entry *ce;
> + int namelen = strlen(".gitmodules");
> +
> + pos = cache_name_pos(".gitmodules", strlen(".gitmodules"));
And you forgot about namelen just like that.
> + if (pos < 0) {
> + warning(_("could not find .gitmodules in index"));
> + return;
> + }
What could this mean? I might have an untracked .gitmodules file in
my worktree, or I might not have a .gitmodules file at all. Since
you're already checking the latter case in the previous function,
can't you persist the result, and return a more sensible error?
> + ce = active_cache[pos];
> + ce->ce_flags = namelen;
> + if (strbuf_read_file(&buf, ".gitmodules", 0) < 0)
> + die(_("reading updated .gitmodules failed"));
You're reading it because? What does "_updated_ .gitmodules" mean here?
> + if (lstat(".gitmodules", &st) < 0)
> + die_errno(_("unable to stat updated .gitmodules"));
> + fill_stat_cache_info(ce, &st);
> + ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);
I don't understand why you're taking the pains to fill out a cache_entry.
> + if (remove_file_from_cache(".gitmodules") < 0)
> + die(_("unable to remove .gitmodules from index"));
You already have pos, so why not just remove_cache_entry_at()?
> + if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))
> + die(_("adding updated .gitmodules failed"));
Ah, you need blob.h for blob_type.
Wait, why are you just reading and writing .gitmodules? What changed?
And why are you manually writing a blob to the object store? Doesn't
git-core already do this if you just add it to the index? See the
S_IFLNK case in index_path().
What happens if there's a race? Shouldn't you be locking .gitmodules
before updating it, like we do with the index and just about
everything else?
> + if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
> + die(_("staging updated .gitmodules failed"));
This is all you need: it calls index_path() and writes your blob
object to the database.
--
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