Moving submodules with nested submodules
cover Moving submodules with nested submodules
This fixes the bug reported in [1] ("Bug: moving submodules that have submodules inside them causes a fatal error in git status")
[1] https://public-inbox.org/git/[email protected]/
Thanks,
Stefan
Stefan Beller (5):
submodule.h: drop declaration of connect_work_tree_and_git_dir
submodule-config: allow submodule_free to handle arbitrary
repositories
submodule-config: add repository argument to submodule_from_{name,
path}
submodule-config: remove submodule_from_cache
submodule: fixup nested submodules after moving the submodule
.../technical/api-submodule-config.txt | 2 +- builtin/grep.c | 2 +- builtin/mv.c | 6 +- builtin/submodule--helper.c | 17 +++-- dir.c | 70 ++++++++++++++++++- dir.h | 12 +++- repository.c | 2 +- submodule-config.c | 29 +++----- submodule-config.h | 9 +-- submodule.c | 40 ++++++----- submodule.h | 1 - t/helper/test-submodule-config.c | 8 ++- t/t7001-mv.sh | 2 +- unpack-trees.c | 2 +- 14 files changed, 135 insertions(+), 67 deletions(-)
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH 1/5 submodule.h: drop declaration of connect_work_tree_and_git_dir
The function connect_work_tree_and_git_dir is declared in both submodule.h and dir.h, such that one of them is redundant. As the function is implemented in dir.c, drop the declaration from submodule.h
--- submodule.h | 1 - 1 file changed, 1 deletion(-)
submodule.h +0 -1
@@ -105,7 +105,6 @@ extern int push_unpushed_submodules(struct oid_array *commits, const char **refspec, int refspec_nr, const struct string_list *push_options, int dry_run); -extern void connect_work_tree_and_git_dir (const char *work_tree,const char *git_dir);/* * Given a submodule path (as in the index), return the repository * path of that submodule in 'buf'. Return -1 on error or when the
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH 2/5 submodule-config: allow submodule_free to handle arbitrary repositories
In reply to Stefan Beller--- Documentation/technical/api-submodule-config.txt | 2 +- builtin/grep.c | 2 +- submodule-config.c | 6 +++--- submodule-config.h | 2 +- t/helper/test-submodule-config.c | 2 +- unpack-trees.c | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-)
Documentation/technical/api-submodule-config.txt +1 -1
@@ -38,7 +38,7 @@ Data Structures Functions --------- -`void submodule_free()`:: +`void submodule_free(struct repository *r)`:: Use these to free the internally cached values.
builtin/grep.c +1 -1
@@ -651,7 +651,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,/* load the gitmodules file for this rev */ if (recurse_submodules) { -submodule_free (); +submodule_free (repo);gitmodules_config_oid (&real_obj->oid); } if (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].path,
submodule-config.c +3 -3
@@ -642,8 +642,8 @@ const struct submodule *submodule_from_cache(struct repository *repo, key, lookup_path); } -void submodule_free (void ) +void submodule_free (struct repository *r) { -if (the_repository->submodule_cache) -submodule_cache_clear (the_repository->submodule_cache); +if (r->submodule_cache) +submodule_cache_clear (r->submodule_cache); }
submodule-config.h +1 -1
@@ -46,6 +46,6 @@ extern const struct submodule *submodule_from_path(extern const struct submodule *submodule_from_cache (struct repository *repo,const struct object_id *treeish_name,const char *key); -extern void submodule_free (void ); +extern void submodule_free (struct repository *r);#endif /* SUBMODULE_CONFIG_H */
t/helper/test-submodule-config.c +1 -1
@@ -64,7 +64,7 @@ int cmd_main(int argc, const char **argv) arg +=2 ; } -submodule_free (); +submodule_free (the_repository);return 0 ; }
unpack-trees.c +1 -1
@@ -290,7 +290,7 @@ static void load_gitmodules_file(struct index_state *index,if (!state && ce->ce_flags &CE_WT_REMOVE ) {repo_read_gitmodules (the_repository); }else if (state && (ce->ce_flags &CE_UPDATE )) { -submodule_free (); +submodule_free (the_repository);checkout_entry (ce, state,NULL );repo_read_gitmodules (the_repository); }
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Re: [PATCH 2/5] submodule-config: allow submodule_free to handle arbitrary repositories
Quoted 1 line · Stefan Beller
Signed-off-by Stefan Beller <[email protected]>
Looks good. At some point we may want to rename this function so that it describes what it actually does as 'submodule_free' doesn't quite describe that this clears a repository's submodule cache. But that's beyond the scope of this.
Quoted 92 lines
--- Documentation/technical/api-submodule-config.txt | 2 +- builtin/grep.c | 2 +- submodule-config.c | 6 +++--- submodule-config.h | 2 +- t/helper/test-submodule-config.c | 2 +- unpack-trees.c | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-)Documentation/technical/api-submodule-config.txt +1
-1@@ -38,7 +38,7 @@ Data Structures Functions --------- -`void submodule_free()`:: +`void submodule_free(struct repository *r)`:: Use these to free the internally cached values.builtin/grep.c +1
-1@@ -651,7 +651,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,/* load the gitmodules file for this rev */ if (recurse_submodules) { -submodule_free (); +submodule_free (repo);gitmodules_config_oid (&real_obj->oid); } if (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].path,submodule-config.c +3
-3@@ -642,8 +642,8 @@ const struct submodule *submodule_from_cache(struct repository *repo, key, lookup_path); } -void submodule_free (void ) +void submodule_free (struct repository *r) { -if (the_repository->submodule_cache) -submodule_cache_clear (the_repository->submodule_cache); +if (r->submodule_cache) +submodule_cache_clear (r->submodule_cache); }submodule-config.h +1
-1@@ -46,6 +46,6 @@ extern const struct submodule *submodule_from_path(extern const struct submodule *submodule_from_cache (struct repository *repo,const struct object_id *treeish_name,const char *key); -extern void submodule_free (void ); +extern void submodule_free (struct repository *r);#endif /* SUBMODULE_CONFIG_H */ t/helper/test-submodule-config.c +1
-1@@ -64,7 +64,7 @@ int cmd_main(int argc, const char **argv) arg +=2 ; } -submodule_free (); +submodule_free (the_repository);return 0 ; }unpack-trees.c +1
-1@@ -290,7 +290,7 @@ static void load_gitmodules_file(struct index_state *index,if (!state && ce->ce_flags &CE_WT_REMOVE ) {repo_read_gitmodules (the_repository); }else if (state && (ce->ce_flags &CE_UPDATE )) { -submodule_free (); +submodule_free (the_repository);checkout_entry (ce, state,NULL );repo_read_gitmodules (the_repository); }Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Signature · 1 line
Brandon Williams
PATCH 1/1 grep: remove "repo" arg from non-supporting funcs
In reply to Stefan BellerAs part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct repository'", 2017-08-02), many functions in builtin/grep.c were converted to also take "struct repository *" arguments. Among them were grep_object() and grep_objects().
However, at least grep_objects() was converted incompletely - it calls gitmodules_config_oid(), which references the_repository.
But it turns out that the conversion was extraneous anyway - there has been no user-visible effect - because grep_objects() is never invoked except with the_repository.
Revert the changes to grep_objects() and grep_object() (which conversion is also extraneous) to show that both these functions do not support repositories other than the_repository.
---
Patch 1/5 of your series is obviously correct.
I investigated the change to grep_objects() in patch 2/5, and here is a patch summarizing my findings. Consider including this patch before 2/5 (or before 1/5). You'll probably need to write "submodule_free(the_repository);" instead of what you have currently, but other than that, I don't think this affects your patch set much.
--- builtin/grep.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
builtin/grep.c +5 -7
@@ -601,8 +601,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, }static int grep_object (struct grep_opt *opt,const struct pathspec *pathspec, -struct object *obj,const char *name,const char *path, -struct repository *repo) +struct object *obj,const char *name,const char *path) {if (obj->type ==OBJ_BLOB )return grep_oid (opt, &obj->oid, name,0 , path);
@@ -629,7 +628,7 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, }init_tree_desc (&tree, data, size); hit =grep_tree (opt, pathspec, &tree, &base, base.len, - obj->type ==OBJ_COMMIT , repo); + obj->type ==OBJ_COMMIT , the_repository);strbuf_release (&base);free (data);return hit;
@@ -638,7 +637,6 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, }static int grep_objects (struct grep_opt *opt,const struct pathspec *pathspec, -struct repository *repo,const struct object_array *list) {unsigned int i;
@@ -654,8 +652,8 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,submodule_free ();gitmodules_config_oid (&real_obj->oid); } -if (grep_object (opt, pathspec, real_obj, list->objects[i].name, list->objects[i].path, - repo)) { +if (grep_object (opt, pathspec, real_obj, list->objects[i].name, + list->objects[i].path)) { hit =1 ;if (opt->status_only)break ;
@@ -1107,7 +1105,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)if (cached)die (_ ("both --cached and trees are given." )); - hit =grep_objects (&opt, &pathspec, the_repository, &list); + hit =grep_objects (&opt, &pathspec, &list); }if (num_threads)
Signature · 1 line
2.16.0.rc0.35.ga4d78ce26
Re: [PATCH] grep: remove "repo" arg from non-supporting funcs
Quoted 8 lines · Jonathan Tan
As part of commit
f9ee2fcdfa("grep: recurse in-process using 'struct repository'", 2017-08-02), many functions inbuiltin/grep.cwere converted to also take "struct repository *" arguments. Among them weregrep_object()andgrep_objects().However, at least
grep_objects()was converted incompletely - it callsgitmodules_config_oid(), which referencesthe_repository.
Quoted 7 lines
But it turns out that the conversion was extraneous anyway - there has been no user-visible effect - because
grep_objects()is never invoked except withthe_repository.Revert the changes to
grep_objects()andgrep_object()(which conversion is also extraneous) to show that both these functions do not support repositories other thanthe_repository.
I'd rather convert gitmodules_config_oid instead of reverting the other functions into a world without an arbitrary repository object.
Quoted 10 lines
Signed-off-by Jonathan Tan <[email protected]>---
Patch 1/5 of your series is obviously correct.I investigated the change to
grep_objects()in patch 2/5, and here is a patch summarizing my findings. Consider including this patch before 2/5 (or before 1/5). You'll probably need to write "submodule_free(the_repository);" instead of what you have currently, but other than that, I don't think this affects your patch set much.
Thanks for looking at the patches!
I'd think this patch is orthogonal to the series, as this is about the effort of converting parts of git-grep whereas this series is fixing a bug (by converting parts of the submodule config machinery))?
Thanks,
Stefan
Re: [PATCH] grep: remove "repo" arg from non-supporting funcs
Quoted 13 lines · On Tue, 27 Mar 2018 16:20:25 -0700 Stefan Beller <[email protected]> wrote:
Quoted 12 lines · Jonathan Tan
As part of commit
f9ee2fcdfa("grep: recurse in-process using 'struct repository'", 2017-08-02), many functions inbuiltin/grep.cwere converted to also take "struct repository *" arguments. Among them weregrep_object()andgrep_objects().However, at least
grep_objects()was converted incompletely - it callsgitmodules_config_oid(), which referencesthe_repository.But it turns out that the conversion was extraneous anyway - there has been no user-visible effect - because
grep_objects()is never invoked except withthe_repository.
Quoted 6 lines
Quoted 3 lines
Revert the changes to
grep_objects()andgrep_object()(which conversion is also extraneous) to show that both these functions do not support repositories other thanthe_repository.I'd rather convert
gitmodules_config_oidinstead of reverting the other functions into a world without an arbitrary repository object.
I don't really think of it as a reversion, since grep_objects() didn't fully support repos other than the_repository anyway.
Besides that, that's fine, but then:
(1) You would have to explain both in the gitmodules_config_oid() and in this patch why "gitmodules_config_oid(...)" -> "gitmodules_config_oid(repo, ...)" and "submodule_free()" -> "submodule_free(repo)" are safe, since they have different behavior upon first glance. (They have the same behavior only because grep_objects() is always called with the_repository.) I was trying to explain this in as clear a way as possible, by showing (with code) that grep_objects() only works with, and is only called with, the_repository.
(2) The code path where repo != the_repository would still never be exercised, and I prefer to not have such code paths.
I don't feel too strongly about (1), since they just concern commit messages, of which there are many valid opinions on how to write them. I feel a bit more strongly about (2), but can concede my point if the project is OK with a code path not being exercised.
Quoted 4 lines
Thanks for looking at the patches!
I'd think this patch is orthogonal to the series, as this is about the effort of converting parts of git-grep whereas this series is fixing a bug (by converting parts of the submodule config machinery))?
It is orthogonal in the same way as your patch 1/5, I think - a preparatory patch to make your "real" patches easier to understand.
Re: [PATCH] grep: remove "repo" arg from non-supporting funcs
Quoted 40 lines · Jonathan Tan
Quoted 13 lines · On Tue, 27 Mar 2018 16:20:25 -0700 Stefan Beller <[email protected]> wrote:
Quoted 12 lines · Jonathan Tan
As part of commit
f9ee2fcdfa("grep: recurse in-process using 'struct repository'", 2017-08-02), many functions inbuiltin/grep.cwere converted to also take "struct repository *" arguments. Among them weregrep_object()andgrep_objects().However, at least
grep_objects()was converted incompletely - it callsgitmodules_config_oid(), which referencesthe_repository.But it turns out that the conversion was extraneous anyway - there has been no user-visible effect - because
grep_objects()is never invoked except withthe_repository.Quoted 6 lines
Quoted 3 lines
Revert the changes to
grep_objects()andgrep_object()(which conversion is also extraneous) to show that both these functions do not support repositories other thanthe_repository.I'd rather convert
gitmodules_config_oidinstead of reverting the other functions into a world without an arbitrary repository object.I don't really think of it as a reversion, since
grep_objects()didn't fully support repos other thanthe_repositoryanyway.Besides that, that's fine, but then:
(1) You would have to explain both in the
gitmodules_config_oid()and in this patch why "gitmodules_config_oid(...)" -> "gitmodules_config_oid(repo, ...)" and "submodule_free()" -> "submodule_free(repo)" are safe, since they have different behavior upon first glance. (They have the same behavior only becausegrep_objects()is always called withthe_repository.) I was trying to explain this in as clear a way as possible, by showing (with code) thatgrep_objects()only works with, and is only called with,the_repository.
(2) The code path where repo !=the_repositorywould still never be exercised, and I prefer to not have such code paths.
Quoted 4 lines
I don't feel too strongly about (1), since they just concern commit messages, of which there are many valid opinions on how to write them. I feel a bit more strongly about (2), but can concede my point if the project is OK with a code path not being exercised.
Ok. I admit not having looked at the code beforehand, and I was just arguing based on intuition. Turns out I was wrong.
I thought that we'd have to have arbitrary repos for proper submodule recursion, but this specific code is for grepping thru given tree objects, which for now is assumed that the user can only give trees of the_repository and we'd error out in case of a submodule tree. Makes sense.
In that case, I agree with your patch. Thanks for writing it. I'll take it into the series.
Thanks,
Stefan
PATCH 3/5 submodule-config: add repository argument to submodule_from_{name, path}
In reply to Stefan BellerThis enables submodule_from_{name, path} to handle arbitrary repositories. All callers just pass in the_repository, a later patch will pass in other repos.
--- builtin/submodule--helper.c | 14 +++++++------- submodule-config.c | 14 ++++++++------ submodule-config.h | 4 ++-- submodule.c | 30 ++++++++++++++++-------------- t/helper/test-submodule-config.c | 6 ++++-- 5 files changed, 37 insertions(+), 31 deletions(-)
builtin/submodule--helper.c +7 -7
@@ -454,7 +454,7 @@ static void init_submodule(const char *path, const char *prefix, displaypath =get_submodule_displaypath (path, prefix); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub) die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -621,7 +621,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,struct rev_info rev;int diff_files_result; -if (!submodule_from_path (&null_oid, path)) +if (!submodule_from_path (the_repository, &null_oid, path))die (_ ("no submodule mapping found in .gitmodules for path '%s'" ), path);
@@ -741,7 +741,7 @@ static int module_name(int argc, const char **argv, const char *prefix)if (argc !=2 )usage (_ ("git submodule--helper name <path>" )); - sub =submodule_from_path (&null_oid, argv[1 ]); + sub =submodule_from_path (the_repository, &null_oid, argv[1 ]);if (!sub) die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -772,7 +772,7 @@ static void sync_submodule(const char *path, const char *prefix,if (!is_submodule_active (the_repository, path))return ; - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (sub && sub->url) { if (starts_with_dot_dot_slash(sub->url) ||
@@ -925,7 +925,7 @@ static void deinit_submodule(const char *path, const char *prefix,struct strbuf sb_config =STRBUF_INIT ;char *sub_git_dir =xstrfmt ("%s/.git" , path); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub || !sub->name) goto cleanup;
@@ -1367,7 +1367,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, goto cleanup; } - sub =submodule_from_path (&null_oid, ce->name); + sub =submodule_from_path (the_repository, &null_oid, ce->name);if (suc->recursive_prefix) displaypath = relative_path(suc->recursive_prefix,
@@ -1650,7 +1650,7 @@ static const char *remote_submodule_branch(const char *path)const char *branch =NULL ;char *key; - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)return NULL ;
submodule-config.c +8 -6
@@ -619,18 +619,20 @@ static void gitmodules_read_check(struct repository *repo)repo_read_gitmodules (repo); } -const struct submodule *submodule_from_name (const struct object_id *treeish_name, +const struct submodule *submodule_from_name (struct repository *r, +const struct object_id *treeish_name,const char *name) { -gitmodules_read_check (the_repository); -return config_from (the_repository->submodule_cache, treeish_name, name, lookup_name); +gitmodules_read_check (r); +return config_from (r->submodule_cache, treeish_name, name, lookup_name); } -const struct submodule *submodule_from_path (const struct object_id *treeish_name, +const struct submodule *submodule_from_path (struct repository *r, +const struct object_id *treeish_name,const char *path) { -gitmodules_read_check (the_repository); -return config_from (the_repository->submodule_cache, treeish_name, path, lookup_path); +gitmodules_read_check (r); +return config_from (r->submodule_cache, treeish_name, path, lookup_path); } const struct submodule *submodule_from_cache(struct repository *repo,
submodule-config.h +2 -2
@@ -39,9 +39,9 @@ extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg)extern int parse_push_recurse_submodules_arg (const char *opt,const char *arg);extern void repo_read_gitmodules (struct repository *repo);extern void gitmodules_config_oid (const struct object_id *commit_oid); -extern const struct submodule *submodule_from_name ( +extern const struct submodule *submodule_from_name (struct repository *r,const struct object_id *commit_or_tree,const char *name); -extern const struct submodule *submodule_from_path ( +extern const struct submodule *submodule_from_path (struct repository *r,const struct object_id *commit_or_tree,const char *path); extern const struct submodule *submodule_from_cache(struct repository *repo, const struct object_id *treeish_name,
submodule.c +16 -14
@@ -95,7 +95,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)if (is_gitmodules_unmerged (&the_index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); - submodule =submodule_from_path (&null_oid, oldpath); + submodule =submodule_from_path (the_repository, &null_oid, oldpath);if (!submodule || !submodule->name) {warning (_ ("Could not find section in .gitmodules where path=%s" ), oldpath);return -1 ;
@@ -129,7 +129,7 @@ int remove_path_from_gitmodules(const char *path)if (is_gitmodules_unmerged (&the_index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); - submodule =submodule_from_path (&null_oid, path); + submodule =submodule_from_path (the_repository, &null_oid, path);if (!submodule || !submodule->name) {warning (_ ("Could not find section in .gitmodules where path=%s" ), path);return -1 ;
@@ -173,7 +173,8 @@ static int add_submodule_odb(const char *path)void set_diffopt_flags_from_submodule_config (struct diff_options *diffopt,const char *path) { -const struct submodule *submodule =submodule_from_path (&null_oid, path); +const struct submodule *submodule =submodule_from_path (the_repository, + &null_oid, path);if (submodule) {const char *ignore;char *key;
@@ -673,7 +674,7 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)if (!should_update_submodules ())return NULL ; -return submodule_from_path (&null_oid, ce->name); +return submodule_from_path (the_repository, &null_oid, ce->name); } static struct oid_array *submodule_commits(struct string_list *submodules,
@@ -730,13 +731,14 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,if (!S_ISGITLINK (p->two->mode))continue ; - submodule =submodule_from_path (commit_oid, p->two->path); + submodule =submodule_from_path (the_repository, + commit_oid, p->two->path);if (submodule) name = submodule->name;else { name =default_name_or_path (p->two->path);/* make sure name does not collide with existing one */ - submodule =submodule_from_name (commit_oid, name); + submodule =submodule_from_name (the_repository, commit_oid, name);if (submodule) {warning ("Submodule in commit %s at path: " "'%s' collides with a submodule named "
@@ -944,7 +946,7 @@ int find_unpushed_submodules(struct oid_array *commits,const struct submodule *submodule;const char *path =NULL ; - submodule =submodule_from_name (&null_oid, name->string); + submodule =submodule_from_name (the_repository, &null_oid, name->string);if (submodule) path = submodule->path;else
@@ -1112,7 +1114,7 @@ static void calculate_changed_submodule_paths(void)const struct string_list_item *name;/* No need to check if there are no submodules configured */ -if (!submodule_from_path (NULL ,NULL )) +if (!submodule_from_path (the_repository,NULL ,NULL ))return ;argv_array_push (&argv,"--" );/* argv[0] program name */
@@ -1133,7 +1135,7 @@ static void calculate_changed_submodule_paths(void)const struct submodule *submodule;const char *path =NULL ; - submodule =submodule_from_name (&null_oid, name->string); + submodule =submodule_from_name (the_repository, &null_oid, name->string);if (submodule) path = submodule->path;else
@@ -1161,7 +1163,7 @@ int submodule_touches_in_range(struct object_id *excl_oid,int ret;/* No need to check if there are no submodules configured */ -if (!submodule_from_path (NULL ,NULL )) +if (!submodule_from_path (the_repository,NULL ,NULL ))return 0 ;argv_array_push (&args,"--" );/* args[0] program name */
@@ -1603,7 +1605,7 @@ int submodule_move_head(const char *path,if (old_head && !is_submodule_populated_gently (path, error_code_ptr))return 0 ; - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)die ("BUG: could not get submodule information for '%s'" , path);
@@ -1885,7 +1887,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix, real_old_git_dir =real_pathdup (old_git_dir,1 ); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path);
@@ -1941,7 +1943,7 @@ void absorb_git_dir_into_superproject(const char *prefix,* superproject did not rewrite the git file links yet, * fix it now. */ - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path); connect_work_tree_and_git_dir(path,
@@ -2087,7 +2089,7 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)strbuf_addstr (buf, git_dir); }if (!is_git_directory (buf->buf)) { - sub =submodule_from_path (&null_oid, submodule); + sub =submodule_from_path (the_repository, &null_oid, submodule);if (!sub) { ret =-1 ; goto cleanup;
t/helper/test-submodule-config.c +4 -2
@@ -48,9 +48,11 @@ int cmd_main(int argc, const char **argv)die_usage (argc, argv,"Commit not found." );if (lookup_name) { - submodule =submodule_from_name (&commit_oid, path_or_name); + submodule =submodule_from_name (the_repository, + &commit_oid, path_or_name); }else - submodule =submodule_from_path (&commit_oid, path_or_name); + submodule =submodule_from_path (the_repository, + &commit_oid, path_or_name);if (!submodule)die_usage (argc, argv,"Submodule not found." );
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Re: [PATCH 3/5] submodule-config: add repository argument to submodule_from_{name, path}
submodule-config.h extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg) · line 45
Quoted 6 lines · On Tue, 27 Mar 2018 14:39:16 -0700 Stefan Beller <[email protected]> wrote:
submodule-config.h +2
-2-extern const struct submodule *submodule_from_name ( +extern const struct submodule *submodule_from_name (struct repository *r,const struct object_id *commit_or_tree,const char *name); -extern const struct submodule *submodule_from_path ( +extern const struct submodule *submodule_from_path (struct repository *r,const struct object_id *commit_or_tree,const char *path);
There is a recent change to CodingGuidelines that states to not include "extern" in 89a9f2c862 ("CodingGuidelines: mention "static" and "extern"", 2018-02-08), so consider removing "extern" since you're already changing this file.
Other than that,
Re: [PATCH 3/5] submodule-config: add repository argument to submodule_from_{name, path}
Quoted 14 lines · Jonathan Tan
submodule-config.h extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg) · line 45
Quoted 6 lines · On Tue, 27 Mar 2018 14:39:16 -0700 Stefan Beller <[email protected]> wrote:
submodule-config.h +2
-2-extern const struct submodule *submodule_from_name ( +extern const struct submodule *submodule_from_name (struct repository *r,const struct object_id *commit_or_tree,const char *name); -extern const struct submodule *submodule_from_path ( +extern const struct submodule *submodule_from_path (struct repository *r,const struct object_id *commit_or_tree,const char *path);There is a recent change to CodingGuidelines that states to not include "extern" in
89a9f2c862("CodingGuidelines: mention "static" and "extern"", 2018-02-08), so consider removing "extern" since you're already changing this file.
Gah!
I have that fixed locally; it will be included in the next version
Quoted 4 lines
Other than that,
Reviewed-by Jonathan Tan <[email protected]>
Thanks!
PATCH 4/5 submodule-config: remove submodule_from_cache
In reply to Stefan BellerThis continues the story of bf12fcdf5e (submodule-config: store the_submodule_cache in the_repository, 2017-06-22).
The previous patch taught submodule_from_path to take a repository into account, such that submodule_from_{path, cache} are the same now. Remove submodule_from_cache, migrating all its callers to submodule_from_path.
--- repository.c | 2 +- submodule-config.c | 9 --------- submodule-config.h | 3 --- submodule.c | 4 ++-- 4 files changed, 3 insertions(+), 15 deletions(-)
repository.c +1 -1
@@ -167,7 +167,7 @@ int repo_submodule_init(struct repository *submodule,struct strbuf worktree =STRBUF_INIT ;int ret =0 ; - sub =submodule_from_cache (superproject, &null_oid, path); + sub =submodule_from_path (superproject, &null_oid, path);if (!sub) { ret =-1 ; goto out;
submodule-config.c +0 -9
@@ -635,15 +635,6 @@ const struct submodule *submodule_from_path(struct repository *r,return config_from (r->submodule_cache, treeish_name, path, lookup_path); } -const struct submodule *submodule_from_cache (struct repository *repo, -const struct object_id *treeish_name, -const char *key) -{ -gitmodules_read_check (repo); -return config_from (repo->submodule_cache, treeish_name, - key, lookup_path); -} -void submodule_free (struct repository *r) {if (r->submodule_cache)
submodule-config.h +0 -3
@@ -43,9 +43,6 @@ extern const struct submodule *submodule_from_name(struct repository *r, const struct object_id *commit_or_tree, const char *name);extern const struct submodule *submodule_from_path (struct repository *r,const struct object_id *commit_or_tree,const char *path); -extern const struct submodule *submodule_from_cache (struct repository *repo, -const struct object_id *treeish_name, -const char *key);extern void submodule_free (struct repository *r);#endif /* SUBMODULE_CONFIG_H */
submodule.c +2 -2
@@ -230,7 +230,7 @@ int is_submodule_active(struct repository *repo, const char *path)const struct string_list *sl;const struct submodule *module; - module =submodule_from_cache (repo, &null_oid, path); + module =submodule_from_path (repo, &null_oid, path);/* early return if there isn't a path->module mapping */ if (!module)
@@ -1235,7 +1235,7 @@ static int get_next_submodule(struct child_process *cp,if (!S_ISGITLINK (ce->ce_mode))continue ; - submodule =submodule_from_cache (spf->r, &null_oid, ce->name); + submodule =submodule_from_path (spf->r, &null_oid, ce->name);if (!submodule) {const char *name =default_name_or_path (ce->name);if (name) {
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Re: [PATCH 4/5] submodule-config: remove submodule_from_cache
Quoted 9 lines · On Tue, 27 Mar 2018 14:39:17 -0700 Stefan Beller <[email protected]> wrote:
This continues the story of
bf12fcdf5e(submodule-config: storethe_submodule_cacheinthe_repository, 2017-06-22).The previous patch taught
submodule_from_pathto take a repository into account, such that submodule_from_{path, cache} are the same now. Removesubmodule_from_cache, migrating all its callers tosubmodule_from_path.Signed-off-by Stefan Beller <[email protected]>
Obviously correct, since submodule_from_{path,cache} are word-for-word identical (other than parameter names).
PATCH 5/5 submodule: fixup nested submodules after moving the submodule
In reply to Stefan Bellerconnect_work_tree_and_git_dir is used to connect a submodule worktree with its git directory and vice versa after events that require a reconnection such as moving around the working tree. As submodules can have nested submoduled themselves, we'd also want to fix the nested submodules when asked to. Add an option to recurse into the nested submodules and connect them as well.
As submodules are identified by their name (which determines their git directory in relation to their superprojects git directory) internally and by their path in the working tree of the superproject, we need to make sure that the mapping of name <-> path is kept intact. We can do that in the git-mv command by writing out the gitmodules file and first and then force a reload of the submodule config machinery.
--- builtin/mv.c | 6 ++-- builtin/submodule--helper.c | 3 +- dir.c | 70 +++++++++++++++++++++++++++++++++++-- dir.h | 12 ++++++- submodule.c | 6 ++-- t/t7001-mv.sh | 2 +- 6 files changed, 87 insertions(+), 12 deletions(-)
builtin/mv.c +4 -2
@@ -276,10 +276,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)die_errno (_ ("renaming '%s' failed" ), src); }if (submodule_gitfile[i]) { -if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) -connect_work_tree_and_git_dir (dst, submodule_gitfile[i]);if (!update_path_in_gitmodules (src, dst)) gitmodules_modified =1 ; +if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) +connect_work_tree_and_git_dir (dst, + submodule_gitfile[i], +1 ); }if (mode ==WORKING_DIRECTORY )
builtin/submodule--helper.c +1 -2
@@ -1259,8 +1259,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)strbuf_reset (&sb); } -/* Connect module worktree and git dir */ -connect_work_tree_and_git_dir (path, sm_gitdir); +connect_work_tree_and_git_dir (path, sm_gitdir,0 ); p =git_pathdup_submodule (path,"config" );if (!p)
dir.c +67 -3
92 lines not on this page · raw message
dir.h +11 -1
@@ -359,7 +359,17 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned longvoid write_untracked_extension (struct strbuf *out,struct untracked_cache *untracked);void add_untracked_cache (struct index_state *istate);void remove_untracked_cache (struct index_state *istate); -extern void connect_work_tree_and_git_dir (const char *work_tree,const char *git_dir); + +/* +* Connect a worktree to a git directory by creating (or overwriting) a +* '.git' file containing the location of the git directory. In the git +* directory set the core.worktree setting to indicate where the worktree is. +* When `recurse_into_nested` is set, recurse into any nested submodules, +* connecting them as well. +*/ +extern void connect_work_tree_and_git_dir (const char *work_tree, +const char *git_dir, +int recurse_into_nested);extern void relocate_gitdir (const char *path,const char *old_git_dir,const char *new_git_dir);
submodule.c +3 -3
@@ -1624,7 +1624,7 @@ int submodule_move_head(const char *path, }else {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,0 );free (gitdir);/* make sure the index is clean as well */
@@ -1634,7 +1634,7 @@ int submodule_move_head(const char *path,if (old_head && (flags &SUBMODULE_MOVE_HEAD_FORCE )) {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,1 );free (gitdir); } }
@@ -1947,7 +1947,7 @@ void absorb_git_dir_into_superproject(const char *prefix,if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path);connect_work_tree_and_git_dir (path, -git_path ("modules/%s" , sub->name)); +git_path ("modules/%s" , sub->name),0 ); }else {/* Is it already absorbed into the superprojects git dir? */ char *real_sub_git_dir =real_pathdup (sub_git_dir,1 );
t/t7001-mv.sh +1 -1
@@ -497,7 +497,7 @@ test_expect_success 'moving a submodule in nested directories' ' test_cmp expect actual ' -test_expect_failure 'moving nested submodules' ' +test_expect_success 'moving nested submodules' ' git commit -am "cleanup commit" && mkdir sub_nested_nested && (cd sub_nested_nested &&
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Re: [PATCH 5/5] submodule: fixup nested submodules after moving the submodule
Quoted 71 lines · Stefan Beller
connect_work_tree_and_git_diris used to connect a submodule worktree with its git directory and vice versa after events that require a reconnection such as moving around the working tree. As submodules can have nested submoduled themselves, we'd also want to fix the nested submodules when asked to. Add an option to recurse into the nested submodules and connect them as well.As submodules are identified by their name (which determines their git directory in relation to their superprojects git directory) internally and by their path in the working tree of the superproject, we need to make sure that the mapping of name <-> path is kept intact. We can do that in the git-mv command by writing out the gitmodules file and first and then force a reload of the submodule config machinery.
Signed-off-by Stefan Beller <[email protected]>--- builtin/mv.c | 6 ++-- builtin/submodule--helper.c | 3 +- dir.c | 70 +++++++++++++++++++++++++++++++++++-- dir.h | 12 ++++++- submodule.c | 6 ++-- t/t7001-mv.sh | 2 +- 6 files changed, 87 insertions(+), 12 deletions(-)builtin/mv.c +4
-2@@ -276,10 +276,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)die_errno (_ ("renaming '%s' failed" ), src); }if (submodule_gitfile[i]) { -if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) -connect_work_tree_and_git_dir (dst, submodule_gitfile[i]);if (!update_path_in_gitmodules (src, dst)) gitmodules_modified =1 ; +if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) +connect_work_tree_and_git_dir (dst, + submodule_gitfile[i], +1 ); }if (mode ==WORKING_DIRECTORY )builtin/submodule--helper.c +1
-2@@ -1259,8 +1259,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)strbuf_reset (&sb); } -/* Connect module worktree and git dir */ -connect_work_tree_and_git_dir (path, sm_gitdir); +connect_work_tree_and_git_dir (path, sm_gitdir,0 ); p =git_pathdup_submodule (path,"config" );if (!p)dir.c +1
-0@@ -19,6 +19,7 @@#include "varint.h" #include "ewah/ewok.h" #include "fsmonitor.h" +#include "submodule-config.h" /* * Tells read_directory_recursive how a file or directory should be treated. @@ -3010,8 +3011,67 @@ void untracked_cache_add_to_index(struct index_state *istate,untracked_cache_invalidate_path (istate, path,1 );
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3019
Quoted 10 lines
dir.c +6
-2} -/* Update gitfile and core.worktree setting to connect work tree and git dir */ -void connect_work_tree_and_git_dir (const char *work_tree_,const char *git_dir_) +static void connect_wt_gitdir_in_nested (const char *sub_worktree, +const char *sub_gitdir, +struct repository *superproject) +{ +int i; +struct repository subrepo;
You never clear this struct which means it leaks the memory it points to.
Quoted 7 lines
dir.c +7
-0+struct strbuf sub_wt =STRBUF_INIT ; +struct strbuf sub_gd =STRBUF_INIT ; +const struct submodule *sub; +const char *super_worktree, + *sub_path;/* path inside the superproject */ + +/* subrepo got moved, so superproject has outdated information */
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3036
Quoted 10 lines
dir.c +10
-0+submodule_free (superproject); + + super_worktree =real_pathdup (superproject->worktree,1 ); + + sub_path = sub_worktree +strlen (super_worktree) +1 ; + +if (repo_submodule_init (&subrepo, superproject, sub_path)) +return ; + +repo_read_index (&subrepo);
You may want to check the return value to see if reading the index was successful.
Quoted 129 lines
dir.c +43
-1+ +for (i =0 ; i < subrepo.index->cache_nr; i++) { +const struct cache_entry *ce = subrepo.index->cache[i]; + +if (!S_ISGITLINK (ce->ce_mode)) +continue ; + +while (i +1 < subrepo.index->cache_nr && + !strcmp (ce->name, subrepo.index->cache[i +1 ]->name)) +/* +* Skip entries with the same name in different stages +* to make sure an entry is returned only once. +*/ + i++; + + sub =submodule_from_path (&subrepo, &null_oid, ce->name); +if (!sub) +/* submodule not checked out? */ +continue ; + +strbuf_reset (&sub_wt); +strbuf_addf (&sub_wt,"%s/%s/.git" , sub_worktree, sub->path); + +strbuf_reset (&sub_gd); +strbuf_addf (&sub_gd,"%s/modules/%s" , sub_gitdir, sub->name); + +strbuf_setlen (&sub_wt, sub_wt.len -strlen ("/.git" )); + +if (is_submodule_active (&subrepo, ce->name)) { +connect_work_tree_and_git_dir (sub_wt.buf, sub_gd.buf,0 ); +connect_wt_gitdir_in_nested (sub_wt.buf, sub_gd.buf, &subrepo); + } + } +} + +void connect_work_tree_and_git_dir (const char *work_tree_, +const char *git_dir_, +int recurse_into_nested) {struct strbuf gitfile_sb =STRBUF_INIT ;struct strbuf cfg_sb =STRBUF_INIT ;@@ -3041,6 +3101,10 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)strbuf_release (&gitfile_sb);strbuf_release (&cfg_sb);strbuf_release (&rel_path); + +if (recurse_into_nested) +connect_wt_gitdir_in_nested (work_tree, git_dir, the_repository); +free (work_tree);free (git_dir); }@@ -3054,5 +3118,5 @@ void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_die_errno (_ ("could not migrate git directory from '%s' to '%s'" ), old_git_dir, new_git_dir); -connect_work_tree_and_git_dir (path, new_git_dir); +connect_work_tree_and_git_dir (path, new_git_dir,0 ); }dir.h +11
-1@@ -359,7 +359,17 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned longvoid write_untracked_extension (struct strbuf *out,struct untracked_cache *untracked);void add_untracked_cache (struct index_state *istate);void remove_untracked_cache (struct index_state *istate); -extern void connect_work_tree_and_git_dir (const char *work_tree,const char *git_dir); + +/* +* Connect a worktree to a git directory by creating (or overwriting) a +* '.git' file containing the location of the git directory. In the git +* directory set the core.worktree setting to indicate where the worktree is. +* When `recurse_into_nested` is set, recurse into any nested submodules, +* connecting them as well. +*/ +extern void connect_work_tree_and_git_dir (const char *work_tree, +const char *git_dir, +int recurse_into_nested);extern void relocate_gitdir (const char *path,const char *old_git_dir,const char *new_git_dir);submodule.c +3
-3@@ -1624,7 +1624,7 @@ int submodule_move_head(const char *path, }else {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,0 );free (gitdir);/* make sure the index is clean as well */ @@ -1634,7 +1634,7 @@ int submodule_move_head(const char *path,if (old_head && (flags &SUBMODULE_MOVE_HEAD_FORCE )) {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,1 );free (gitdir); } }@@ -1947,7 +1947,7 @@ void absorb_git_dir_into_superproject(const char *prefix,if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path);connect_work_tree_and_git_dir (path, -git_path ("modules/%s" , sub->name)); +git_path ("modules/%s" , sub->name),0 ); }else {/* Is it already absorbed into the superprojects git dir? */ char *real_sub_git_dir =real_pathdup (sub_git_dir,1 );t/t7001-mv.sh +1
-1@@ -497,7 +497,7 @@ test_expect_success 'moving a submodule in nested directories' ' test_cmp expect actual ' -test_expect_failure 'moving nested submodules' ' +test_expect_success 'moving nested submodules' ' git commit -am "cleanup commit" && mkdir sub_nested_nested && (cd sub_nested_nested &&Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Signature · 1 line
Brandon Williams
Re: [PATCH 5/5] submodule: fixup nested submodules after moving the submodule
In reply to Stefan BellerQuoted 4 lines · On Tue, 27 Mar 2018 14:39:18 -0700 Stefan Beller <[email protected]> wrote:
connect_work_tree_and_git_diris used to connect a submodule worktree with its git directory and vice versa after events that require a reconnection such as moving around the working tree. As submodules can have nested submoduled themselves, we'd also want to fix the nested submodules when
s/submoduled/submodules
Quoted 5 lines
asked to. Add an option to recurse into the nested submodules and connect them as well.
As submodules are identified by their name (which determines their git directory in relation to their superprojects git directory) internally
s/superprojects/superproject's/
Quoted 3 lines
and by their path in the working tree of the superproject, we need to make sure that the mapping of name <-> path is kept intact. We can do that in the git-mv command by writing out the gitmodules file and first
s/and //
Quoted 1 line
and then force a reload of the submodule config machinery.
s/force/forcing/
Quoted 2 lines
Signed-off-by Stefan Beller <[email protected]>
[snip]
Quoted 13 lines
dir.c +13
-0+static void connect_wt_gitdir_in_nested (const char *sub_worktree, +const char *sub_gitdir, +struct repository *superproject) +{ +int i; +struct repository subrepo; +struct strbuf sub_wt =STRBUF_INIT ; +struct strbuf sub_gd =STRBUF_INIT ; +const struct submodule *sub; +const char *super_worktree, + *sub_path;/* path inside the superproject */ + +/* subrepo got moved, so superproject has outdated information */
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3036
Quoted 10 lines
dir.c +10
-0+submodule_free (superproject); + + super_worktree =real_pathdup (superproject->worktree,1 ); + + sub_path = sub_worktree +strlen (super_worktree) +1 ; + +if (repo_submodule_init (&subrepo, superproject, sub_path)) +return ; + +repo_read_index (&subrepo);
From the name of this function and its usage in connect_work_tree_and_git_dir(), I expected this function to just iterate through all the files in its workdir (which it is doing in the "for" loop below) and connect any nested submodules. Why does it need access to its superproject? (I would think that repo_init() would be sufficient here instead of repo_submodule_init().)
Quoted 21 lines
dir.c +21
-0+ +for (i =0 ; i < subrepo.index->cache_nr; i++) { +const struct cache_entry *ce = subrepo.index->cache[i]; + +if (!S_ISGITLINK (ce->ce_mode)) +continue ; + +while (i +1 < subrepo.index->cache_nr && + !strcmp (ce->name, subrepo.index->cache[i +1 ]->name)) +/* +* Skip entries with the same name in different stages +* to make sure an entry is returned only once. +*/ + i++; + + sub =submodule_from_path (&subrepo, &null_oid, ce->name); +if (!sub) +/* submodule not checked out? */ +continue ; + +strbuf_reset (&sub_wt);
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3067
Quoted 10 lines
dir.c +10
-0+strbuf_addf (&sub_wt,"%s/%s/.git" , sub_worktree, sub->path); + +strbuf_reset (&sub_gd); +strbuf_addf (&sub_gd,"%s/modules/%s" , sub_gitdir, sub->name); + +strbuf_setlen (&sub_wt, sub_wt.len -strlen ("/.git" )); + +if (is_submodule_active (&subrepo, ce->name)) { +connect_work_tree_and_git_dir (sub_wt.buf, sub_gd.buf,0 ); +connect_wt_gitdir_in_nested (sub_wt.buf, sub_gd.buf, &subrepo);
The modifications of sub_wt and sub_gd should probably go here, since they are not used unless this "if" block is executed.
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3074
Quoted 3 lines
dir.c +3
-0+void connect_work_tree_and_git_dir (const char *work_tree_, +const char *git_dir_, +int recurse_into_nested)
How is this function expected to be used? From what I see:
- if recurse_into_nested is 0, this works regardless of whether the work_tree_ and git_dir_ is directly or indirectly a submodule of the_repository
- if recurse_into_nested is 1, work_tree_ and git_dir_ must be directly a submodule of the_repository (since it is referenced directly)
This seems confusing - is this expected?
Re: [PATCH 5/5] submodule: fixup nested submodules after moving the submodule
Quoted 7 lines · Jonathan Tan
s/submoduled/submodules
s/superprojects/superproject's/
s/and //
s/force/forcing/
All wording fixed.
Quoted 5 lines
Quoted 5 lines
dir.c +5
-0+ sub_path = sub_worktree +strlen (super_worktree) +1 ; + +if (repo_submodule_init (&subrepo, superproject, sub_path)) +return ; +
Quoted 8 lines
Quoted 1 line
+
repo_read_index(&subrepo);From the name of this function and its usage in
connect_work_tree_and_git_dir(), I expected this function to just iterate through all the files in its workdir (which it is doing in the "for" loop below) and connect any nested submodules. Why does it need access to its superproject? (I would think thatrepo_init()would be sufficient here instead ofrepo_submodule_init().)
Testing validates your thinking (for now).
If we ever want to have good error reporting (see bmwills hint to check for index corruption), we may want to have all the repos constructed as submodules from the_repository, as then the error messages might be better (e.g. in the future we could display the
"submodule nesting stack").
I'll remove the superproject argument for now.
Quoted 7 lines
Quoted 7 lines
dir.c +7
-0+ +strbuf_reset (&sub_wt); +strbuf_addf (&sub_wt,"%s/%s/.git" , sub_worktree, sub->path); + +strbuf_reset (&sub_gd); +strbuf_addf (&sub_gd,"%s/modules/%s" , sub_gitdir, sub->name); +
Quoted 8 lines
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3067
Quoted 5 lines
dir.c +5
-0+strbuf_setlen (&sub_wt, sub_wt.len -strlen ("/.git" )); + +if (is_submodule_active (&subrepo, ce->name)) { +connect_work_tree_and_git_dir (sub_wt.buf, sub_gd.buf,0 ); +connect_wt_gitdir_in_nested (sub_wt.buf, sub_gd.buf, &subrepo);The modifications of
sub_wtandsub_gdshould probably go here, since they are not used unless this "if" block is executed.
Thanks! I also cut out the setlen call by giving the correct format string. (The code presented here is not very old, I just fired it off as soon as the test passed)
Quoted 5 lines
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3074
Quoted 4 lines
dir.c +3
-0+void connect_work_tree_and_git_dir (const char *work_tree_, +const char *git_dir_, +int recurse_into_nested)
Quoted 8 lines
How is this function expected to be used? From what I see:
- ifrecurse_into_nestedis 0, this works regardless of whether thework_tree_andgit_dir_is directly or indirectly a submodule ofthe_repository
- ifrecurse_into_nestedis 1,work_tree_andgit_dir_must be directly a submodule ofthe_repository(since it is referenced directly)This seems confusing - is this expected?
In the next revision of the series connect_wt_gitdir_in_nested will no longer have a third argument for the repo, as we use repo_init.
That eases the handling a bit here.
v2 cover Moving submodules with nested submodules
v2:
* addressed memleaks and messy code in patch 5
* removed the extern keyword where applicable
* extended the commit message, stating we want to rename submodule_free in the future.
* picked up Jonathans patch and added it as a nice finish of the series. I did not see the need or aesthetic desire to put that patch earlier in the series.
Thanks,
Stefan
v1:
This fixes the bug reported in [1] ("Bug: moving submodules that have submodules inside them causes a fatal error in git status")
[1] https://public-inbox.org/git/[email protected]/
Thanks,
Stefan
Jonathan Tan (1):
grep: remove "repo" arg from non-supporting funcs
Stefan Beller (5):
submodule.h: drop declaration of connect_work_tree_and_git_dir
submodule-config: allow submodule_free to handle arbitrary
repositories
submodule-config: add repository argument to submodule_from_{name,
path}
submodule-config: remove submodule_from_cache
submodule: fixup nested submodules after moving the submodule
.../technical/api-submodule-config.txt | 2 +- builtin/grep.c | 14 ++--- builtin/mv.c | 6 +- builtin/submodule--helper.c | 17 +++-- dir.c | 63 ++++++++++++++++++- dir.h | 12 +++- repository.c | 2 +- submodule-config.c | 29 ++++----- submodule-config.h | 15 +++-- submodule.c | 40 ++++++------ submodule.h | 1 - t/helper/test-submodule-config.c | 8 ++- t/t7001-mv.sh | 2 +- unpack-trees.c | 2 +- 14 files changed, 137 insertions(+), 76 deletions(-)
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v2 1/6 submodule.h: drop declaration of connect_work_tree_and_git_dir
The function connect_work_tree_and_git_dir is declared in both submodule.h and dir.h, such that one of them is redundant. As the function is implemented in dir.c, drop the declaration from submodule.h
--- submodule.h | 1 - 1 file changed, 1 deletion(-)
submodule.h +0 -1
@@ -105,7 +105,6 @@ extern int push_unpushed_submodules(struct oid_array *commits, const char **refspec, int refspec_nr, const struct string_list *push_options, int dry_run); -extern void connect_work_tree_and_git_dir (const char *work_tree,const char *git_dir);/* * Given a submodule path (as in the index), return the repository * path of that submodule in 'buf'. Return -1 on error or when the
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v2 2/6 submodule-config: allow submodule_free to handle arbitrary repositories
In reply to Stefan BellerAt some point we may want to rename the function so that it describes what it actually does as 'submodule_free' doesn't quite describe that this clears a repository's submodule cache. But that's beyond the scope of this series.
While at it remove the extern key word from its declaration.
--- Documentation/technical/api-submodule-config.txt | 2 +- builtin/grep.c | 2 +- submodule-config.c | 6 +++--- submodule-config.h | 2 +- t/helper/test-submodule-config.c | 2 +- unpack-trees.c | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-)
Documentation/technical/api-submodule-config.txt +1 -1
@@ -38,7 +38,7 @@ Data Structures Functions --------- -`void submodule_free()`:: +`void submodule_free(struct repository *r)`:: Use these to free the internally cached values.
builtin/grep.c +1 -1
@@ -651,7 +651,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,/* load the gitmodules file for this rev */ if (recurse_submodules) { -submodule_free (); +submodule_free (repo);gitmodules_config_oid (&real_obj->oid); } if (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].path,
submodule-config.c +3 -3
@@ -642,8 +642,8 @@ const struct submodule *submodule_from_cache(struct repository *repo, key, lookup_path); } -void submodule_free (void ) +void submodule_free (struct repository *r) { -if (the_repository->submodule_cache) -submodule_cache_clear (the_repository->submodule_cache); +if (r->submodule_cache) +submodule_cache_clear (r->submodule_cache); }
submodule-config.h +1 -1
@@ -46,6 +46,6 @@ extern const struct submodule *submodule_from_path(extern const struct submodule *submodule_from_cache (struct repository *repo,const struct object_id *treeish_name,const char *key); -extern void submodule_free (void ); +void submodule_free (struct repository *r);#endif /* SUBMODULE_CONFIG_H */
t/helper/test-submodule-config.c +1 -1
@@ -64,7 +64,7 @@ int cmd_main(int argc, const char **argv) arg +=2 ; } -submodule_free (); +submodule_free (the_repository);return 0 ; }
unpack-trees.c +1 -1
@@ -290,7 +290,7 @@ static void load_gitmodules_file(struct index_state *index,if (!state && ce->ce_flags &CE_WT_REMOVE ) {repo_read_gitmodules (the_repository); }else if (state && (ce->ce_flags &CE_UPDATE )) { -submodule_free (); +submodule_free (the_repository);checkout_entry (ce, state,NULL );repo_read_gitmodules (the_repository); }
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v2 3/6 submodule-config: add repository argument to submodule_from_{name, path}
In reply to Stefan BellerThis enables submodule_from_{name, path} to handle arbitrary repositories. All callers just pass in the_repository, a later patch will pass in other repos.
While at it remove the extern key word from the declarations.
--- builtin/submodule--helper.c | 14 +++++++------- submodule-config.c | 14 ++++++++------ submodule-config.h | 10 ++++++---- submodule.c | 30 ++++++++++++++++-------------- t/helper/test-submodule-config.c | 6 ++++-- 5 files changed, 41 insertions(+), 33 deletions(-)
builtin/submodule--helper.c +7 -7
@@ -454,7 +454,7 @@ static void init_submodule(const char *path, const char *prefix, displaypath =get_submodule_displaypath (path, prefix); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub) die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -621,7 +621,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,struct rev_info rev;int diff_files_result; -if (!submodule_from_path (&null_oid, path)) +if (!submodule_from_path (the_repository, &null_oid, path))die (_ ("no submodule mapping found in .gitmodules for path '%s'" ), path);
@@ -741,7 +741,7 @@ static int module_name(int argc, const char **argv, const char *prefix)if (argc !=2 )usage (_ ("git submodule--helper name <path>" )); - sub =submodule_from_path (&null_oid, argv[1 ]); + sub =submodule_from_path (the_repository, &null_oid, argv[1 ]);if (!sub) die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -772,7 +772,7 @@ static void sync_submodule(const char *path, const char *prefix,if (!is_submodule_active (the_repository, path))return ; - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (sub && sub->url) { if (starts_with_dot_dot_slash(sub->url) ||
@@ -925,7 +925,7 @@ static void deinit_submodule(const char *path, const char *prefix,struct strbuf sb_config =STRBUF_INIT ;char *sub_git_dir =xstrfmt ("%s/.git" , path); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub || !sub->name) goto cleanup;
@@ -1367,7 +1367,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, goto cleanup; } - sub =submodule_from_path (&null_oid, ce->name); + sub =submodule_from_path (the_repository, &null_oid, ce->name);if (suc->recursive_prefix) displaypath = relative_path(suc->recursive_prefix,
@@ -1650,7 +1650,7 @@ static const char *remote_submodule_branch(const char *path)const char *branch =NULL ;char *key; - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)return NULL ;
submodule-config.c +8 -6
@@ -619,18 +619,20 @@ static void gitmodules_read_check(struct repository *repo)repo_read_gitmodules (repo); } -const struct submodule *submodule_from_name (const struct object_id *treeish_name, +const struct submodule *submodule_from_name (struct repository *r, +const struct object_id *treeish_name,const char *name) { -gitmodules_read_check (the_repository); -return config_from (the_repository->submodule_cache, treeish_name, name, lookup_name); +gitmodules_read_check (r); +return config_from (r->submodule_cache, treeish_name, name, lookup_name); } -const struct submodule *submodule_from_path (const struct object_id *treeish_name, +const struct submodule *submodule_from_path (struct repository *r, +const struct object_id *treeish_name,const char *path) { -gitmodules_read_check (the_repository); -return config_from (the_repository->submodule_cache, treeish_name, path, lookup_path); +gitmodules_read_check (r); +return config_from (r->submodule_cache, treeish_name, path, lookup_path); } const struct submodule *submodule_from_cache(struct repository *repo,
submodule-config.h +6 -4
@@ -39,10 +39,12 @@ extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg)extern int parse_push_recurse_submodules_arg (const char *opt,const char *arg);extern void repo_read_gitmodules (struct repository *repo);extern void gitmodules_config_oid (const struct object_id *commit_oid); -extern const struct submodule *submodule_from_name ( -const struct object_id *commit_or_tree,const char *name); -extern const struct submodule *submodule_from_path ( -const struct object_id *commit_or_tree,const char *path); +const struct submodule *submodule_from_name (struct repository *r, +const struct object_id *commit_or_tree, +const char *name); +const struct submodule *submodule_from_path (struct repository *r, +const struct object_id *commit_or_tree, +const char *path);extern const struct submodule *submodule_from_cache (struct repository *repo,const struct object_id *treeish_name,const char *key);
submodule.c +16 -14
@@ -95,7 +95,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)if (is_gitmodules_unmerged (&the_index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); - submodule =submodule_from_path (&null_oid, oldpath); + submodule =submodule_from_path (the_repository, &null_oid, oldpath);if (!submodule || !submodule->name) {warning (_ ("Could not find section in .gitmodules where path=%s" ), oldpath);return -1 ;
@@ -129,7 +129,7 @@ int remove_path_from_gitmodules(const char *path)if (is_gitmodules_unmerged (&the_index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); - submodule =submodule_from_path (&null_oid, path); + submodule =submodule_from_path (the_repository, &null_oid, path);if (!submodule || !submodule->name) {warning (_ ("Could not find section in .gitmodules where path=%s" ), path);return -1 ;
@@ -173,7 +173,8 @@ static int add_submodule_odb(const char *path)void set_diffopt_flags_from_submodule_config (struct diff_options *diffopt,const char *path) { -const struct submodule *submodule =submodule_from_path (&null_oid, path); +const struct submodule *submodule =submodule_from_path (the_repository, + &null_oid, path);if (submodule) {const char *ignore;char *key;
@@ -673,7 +674,7 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)if (!should_update_submodules ())return NULL ; -return submodule_from_path (&null_oid, ce->name); +return submodule_from_path (the_repository, &null_oid, ce->name); } static struct oid_array *submodule_commits(struct string_list *submodules,
@@ -730,13 +731,14 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,if (!S_ISGITLINK (p->two->mode))continue ; - submodule =submodule_from_path (commit_oid, p->two->path); + submodule =submodule_from_path (the_repository, + commit_oid, p->two->path);if (submodule) name = submodule->name;else { name =default_name_or_path (p->two->path);/* make sure name does not collide with existing one */ - submodule =submodule_from_name (commit_oid, name); + submodule =submodule_from_name (the_repository, commit_oid, name);if (submodule) {warning ("Submodule in commit %s at path: " "'%s' collides with a submodule named "
@@ -944,7 +946,7 @@ int find_unpushed_submodules(struct oid_array *commits,const struct submodule *submodule;const char *path =NULL ; - submodule =submodule_from_name (&null_oid, name->string); + submodule =submodule_from_name (the_repository, &null_oid, name->string);if (submodule) path = submodule->path;else
@@ -1112,7 +1114,7 @@ static void calculate_changed_submodule_paths(void)const struct string_list_item *name;/* No need to check if there are no submodules configured */ -if (!submodule_from_path (NULL ,NULL )) +if (!submodule_from_path (the_repository,NULL ,NULL ))return ;argv_array_push (&argv,"--" );/* argv[0] program name */
@@ -1133,7 +1135,7 @@ static void calculate_changed_submodule_paths(void)const struct submodule *submodule;const char *path =NULL ; - submodule =submodule_from_name (&null_oid, name->string); + submodule =submodule_from_name (the_repository, &null_oid, name->string);if (submodule) path = submodule->path;else
@@ -1161,7 +1163,7 @@ int submodule_touches_in_range(struct object_id *excl_oid,int ret;/* No need to check if there are no submodules configured */ -if (!submodule_from_path (NULL ,NULL )) +if (!submodule_from_path (the_repository,NULL ,NULL ))return 0 ;argv_array_push (&args,"--" );/* args[0] program name */
@@ -1603,7 +1605,7 @@ int submodule_move_head(const char *path,if (old_head && !is_submodule_populated_gently (path, error_code_ptr))return 0 ; - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)die ("BUG: could not get submodule information for '%s'" , path);
@@ -1885,7 +1887,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix, real_old_git_dir =real_pathdup (old_git_dir,1 ); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path);
@@ -1941,7 +1943,7 @@ void absorb_git_dir_into_superproject(const char *prefix,* superproject did not rewrite the git file links yet, * fix it now. */ - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path); connect_work_tree_and_git_dir(path,
@@ -2087,7 +2089,7 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)strbuf_addstr (buf, git_dir); }if (!is_git_directory (buf->buf)) { - sub =submodule_from_path (&null_oid, submodule); + sub =submodule_from_path (the_repository, &null_oid, submodule);if (!sub) { ret =-1 ; goto cleanup;
t/helper/test-submodule-config.c +4 -2
@@ -48,9 +48,11 @@ int cmd_main(int argc, const char **argv)die_usage (argc, argv,"Commit not found." );if (lookup_name) { - submodule =submodule_from_name (&commit_oid, path_or_name); + submodule =submodule_from_name (the_repository, + &commit_oid, path_or_name); }else - submodule =submodule_from_path (&commit_oid, path_or_name); + submodule =submodule_from_path (the_repository, + &commit_oid, path_or_name);if (!submodule)die_usage (argc, argv,"Submodule not found." );
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v2 4/6 submodule-config: remove submodule_from_cache
In reply to Stefan BellerThis continues the story of bf12fcdf5e (submodule-config: store the_submodule_cache in the_repository, 2017-06-22).
The previous patch taught submodule_from_path to take a repository into account, such that submodule_from_{path, cache} are the same now. Remove submodule_from_cache, migrating all its callers to submodule_from_path.
--- repository.c | 2 +- submodule-config.c | 9 --------- submodule-config.h | 3 --- submodule.c | 4 ++-- 4 files changed, 3 insertions(+), 15 deletions(-)
repository.c +1 -1
@@ -167,7 +167,7 @@ int repo_submodule_init(struct repository *submodule,struct strbuf worktree =STRBUF_INIT ;int ret =0 ; - sub =submodule_from_cache (superproject, &null_oid, path); + sub =submodule_from_path (superproject, &null_oid, path);if (!sub) { ret =-1 ; goto out;
submodule-config.c +0 -9
@@ -635,15 +635,6 @@ const struct submodule *submodule_from_path(struct repository *r,return config_from (r->submodule_cache, treeish_name, path, lookup_path); } -const struct submodule *submodule_from_cache (struct repository *repo, -const struct object_id *treeish_name, -const char *key) -{ -gitmodules_read_check (repo); -return config_from (repo->submodule_cache, treeish_name, - key, lookup_path); -} -void submodule_free (struct repository *r) {if (r->submodule_cache)
submodule-config.h +0 -3
@@ -45,9 +45,6 @@ const struct submodule *submodule_from_name(struct repository *r,const struct submodule *submodule_from_path (struct repository *r,const struct object_id *commit_or_tree,const char *path); -extern const struct submodule *submodule_from_cache (struct repository *repo, -const struct object_id *treeish_name, -const char *key);void submodule_free (struct repository *r);#endif /* SUBMODULE_CONFIG_H */
submodule.c +2 -2
@@ -230,7 +230,7 @@ int is_submodule_active(struct repository *repo, const char *path)const struct string_list *sl;const struct submodule *module; - module =submodule_from_cache (repo, &null_oid, path); + module =submodule_from_path (repo, &null_oid, path);/* early return if there isn't a path->module mapping */ if (!module)
@@ -1235,7 +1235,7 @@ static int get_next_submodule(struct child_process *cp,if (!S_ISGITLINK (ce->ce_mode))continue ; - submodule =submodule_from_cache (spf->r, &null_oid, ce->name); + submodule =submodule_from_path (spf->r, &null_oid, ce->name);if (!submodule) {const char *name =default_name_or_path (ce->name);if (name) {
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v2 5/6 submodule: fixup nested submodules after moving the submodule
In reply to Stefan Bellerconnect_work_tree_and_git_dir is used to connect a submodule worktree with its git directory and vice versa after events that require a reconnection such as moving around the working tree. As submodules can have nested submodules themselves, we'd also want to fix the nested submodules when asked to. Add an option to recurse into the nested submodules and connect them as well.
As submodules are identified by their name (which determines their git directory in relation to their superproject's git directory) internally and by their path in the working tree of the superproject, we need to make sure that the mapping of name <-> path is kept intact. We can do that in the git-mv command by writing out the gitmodules file first and then forcing a reload of the submodule config machinery.
--- builtin/mv.c | 6 ++-- builtin/submodule--helper.c | 3 +- dir.c | 63 +++++++++++++++++++++++++++++++++++-- dir.h | 12 ++++++- submodule.c | 6 ++-- t/t7001-mv.sh | 2 +- 6 files changed, 80 insertions(+), 12 deletions(-)
builtin/mv.c +4 -2
@@ -276,10 +276,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)die_errno (_ ("renaming '%s' failed" ), src); }if (submodule_gitfile[i]) { -if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) -connect_work_tree_and_git_dir (dst, submodule_gitfile[i]);if (!update_path_in_gitmodules (src, dst)) gitmodules_modified =1 ; +if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) +connect_work_tree_and_git_dir (dst, + submodule_gitfile[i], +1 ); }if (mode ==WORKING_DIRECTORY )
builtin/submodule--helper.c +1 -2
@@ -1259,8 +1259,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)strbuf_reset (&sb); } -/* Connect module worktree and git dir */ -connect_work_tree_and_git_dir (path, sm_gitdir); +connect_work_tree_and_git_dir (path, sm_gitdir,0 ); p =git_pathdup_submodule (path,"config" );if (!p)
dir.c +60 -3
85 lines not on this page · raw message
dir.h +11 -1
@@ -359,7 +359,17 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned longvoid write_untracked_extension (struct strbuf *out,struct untracked_cache *untracked);void add_untracked_cache (struct index_state *istate);void remove_untracked_cache (struct index_state *istate); -extern void connect_work_tree_and_git_dir (const char *work_tree,const char *git_dir); + +/* +* Connect a worktree to a git directory by creating (or overwriting) a +* '.git' file containing the location of the git directory. In the git +* directory set the core.worktree setting to indicate where the worktree is. +* When `recurse_into_nested` is set, recurse into any nested submodules, +* connecting them as well. +*/ +extern void connect_work_tree_and_git_dir (const char *work_tree, +const char *git_dir, +int recurse_into_nested);extern void relocate_gitdir (const char *path,const char *old_git_dir,const char *new_git_dir);
submodule.c +3 -3
@@ -1624,7 +1624,7 @@ int submodule_move_head(const char *path, }else {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,0 );free (gitdir);/* make sure the index is clean as well */
@@ -1634,7 +1634,7 @@ int submodule_move_head(const char *path,if (old_head && (flags &SUBMODULE_MOVE_HEAD_FORCE )) {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,1 );free (gitdir); } }
@@ -1947,7 +1947,7 @@ void absorb_git_dir_into_superproject(const char *prefix,if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path);connect_work_tree_and_git_dir (path, -git_path ("modules/%s" , sub->name)); +git_path ("modules/%s" , sub->name),0 ); }else {/* Is it already absorbed into the superprojects git dir? */ char *real_sub_git_dir =real_pathdup (sub_git_dir,1 );
t/t7001-mv.sh +1 -1
@@ -497,7 +497,7 @@ test_expect_success 'moving a submodule in nested directories' ' test_cmp expect actual ' -test_expect_failure 'moving nested submodules' ' +test_expect_success 'moving nested submodules' ' git commit -am "cleanup commit" && mkdir sub_nested_nested && (cd sub_nested_nested &&
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Re: [PATCH 5/6] submodule: fixup nested submodules after moving the submodule
Quoted 77 lines · Stefan Beller
connect_work_tree_and_git_diris used to connect a submodule worktree with its git directory and vice versa after events that require a reconnection such as moving around the working tree. As submodules can have nested submodules themselves, we'd also want to fix the nested submodules when asked to. Add an option to recurse into the nested submodules and connect them as well.As submodules are identified by their name (which determines their git directory in relation to their superproject's git directory) internally and by their path in the working tree of the superproject, we need to make sure that the mapping of name <-> path is kept intact. We can do that in the git-mv command by writing out the gitmodules file first and then forcing a reload of the submodule config machinery.
Signed-off-by Stefan Beller <[email protected]>--- builtin/mv.c | 6 ++-- builtin/submodule--helper.c | 3 +- dir.c | 63 +++++++++++++++++++++++++++++++++++-- dir.h | 12 ++++++- submodule.c | 6 ++-- t/t7001-mv.sh | 2 +- 6 files changed, 80 insertions(+), 12 deletions(-)builtin/mv.c +4
-2@@ -276,10 +276,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)die_errno (_ ("renaming '%s' failed" ), src); }if (submodule_gitfile[i]) { -if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) -connect_work_tree_and_git_dir (dst, submodule_gitfile[i]);if (!update_path_in_gitmodules (src, dst)) gitmodules_modified =1 ; +if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) +connect_work_tree_and_git_dir (dst, + submodule_gitfile[i], +1 ); }if (mode ==WORKING_DIRECTORY )builtin/submodule--helper.c +1
-2@@ -1259,8 +1259,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)strbuf_reset (&sb); } -/* Connect module worktree and git dir */ -connect_work_tree_and_git_dir (path, sm_gitdir); +connect_work_tree_and_git_dir (path, sm_gitdir,0 ); p =git_pathdup_submodule (path,"config" );if (!p)dir.c +3
-2@@ -19,6 +19,7 @@#include "varint.h" #include "ewah/ewok.h" #include "fsmonitor.h" +#include "submodule-config.h" /* * Tells read_directory_recursive how a file or directory should be treated. @@ -3010,8 +3011,60 @@ void untracked_cache_add_to_index(struct index_state *istate,untracked_cache_invalidate_path (istate, path,1 ); } -/* Update gitfile and core.worktree setting to connect work tree and git dir */ -void connect_work_tree_and_git_dir (const char *work_tree_,const char *git_dir_) +static void connect_wt_gitdir_in_nested (const char *sub_worktree, +const char *sub_gitdir)
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3025
Quoted 10 lines
dir.c +10
-0+{ +int i; +struct repository subrepo; +struct strbuf sub_wt =STRBUF_INIT ; +struct strbuf sub_gd =STRBUF_INIT ; + +const struct submodule *sub; + +if (repo_init (&subrepo, sub_gitdir, sub_worktree)) +return ;
Note that in Duy's object-store series he made this function private (IIRC) so this will result in some clash of the two series.
Quoted 133 lines
dir.c +47
-1+ +if (repo_read_index (&subrepo) <0 ) +die ("index file corrupt in repo %s" , subrepo.gitdir); + +for (i =0 ; i < subrepo.index->cache_nr; i++) { +const struct cache_entry *ce = subrepo.index->cache[i]; + +if (!S_ISGITLINK (ce->ce_mode)) +continue ; + +while (i +1 < subrepo.index->cache_nr && + !strcmp (ce->name, subrepo.index->cache[i +1 ]->name)) +/* +* Skip entries with the same name in different stages +* to make sure an entry is returned only once. +*/ + i++; + + sub =submodule_from_path (&subrepo, &null_oid, ce->name); +if (!sub) +/* submodule not checked out? */ +continue ; + +if (is_submodule_active (&subrepo, ce->name)) { +strbuf_addf (&sub_wt,"%s/%s" , sub_worktree, sub->path); +strbuf_addf (&sub_gd,"%s/modules/%s" , sub_gitdir, sub->name); + +connect_work_tree_and_git_dir (sub_wt.buf, sub_gd.buf,0 ); +connect_wt_gitdir_in_nested (sub_wt.buf, sub_gd.buf); + +strbuf_reset (&sub_wt); +strbuf_reset (&sub_gd); + } + } +strbuf_release (&sub_wt); +strbuf_release (&sub_gd); +repo_clear (&subrepo); +} + +void connect_work_tree_and_git_dir (const char *work_tree_, +const char *git_dir_, +int recurse_into_nested) {struct strbuf gitfile_sb =STRBUF_INIT ;struct strbuf cfg_sb =STRBUF_INIT ;@@ -3041,6 +3094,10 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)strbuf_release (&gitfile_sb);strbuf_release (&cfg_sb);strbuf_release (&rel_path); + +if (recurse_into_nested) +connect_wt_gitdir_in_nested (work_tree, git_dir); +free (work_tree);free (git_dir); }@@ -3054,5 +3111,5 @@ void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_die_errno (_ ("could not migrate git directory from '%s' to '%s'" ), old_git_dir, new_git_dir); -connect_work_tree_and_git_dir (path, new_git_dir); +connect_work_tree_and_git_dir (path, new_git_dir,0 ); }dir.h +11
-1@@ -359,7 +359,17 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned longvoid write_untracked_extension (struct strbuf *out,struct untracked_cache *untracked);void add_untracked_cache (struct index_state *istate);void remove_untracked_cache (struct index_state *istate); -extern void connect_work_tree_and_git_dir (const char *work_tree,const char *git_dir); + +/* +* Connect a worktree to a git directory by creating (or overwriting) a +* '.git' file containing the location of the git directory. In the git +* directory set the core.worktree setting to indicate where the worktree is. +* When `recurse_into_nested` is set, recurse into any nested submodules, +* connecting them as well. +*/ +extern void connect_work_tree_and_git_dir (const char *work_tree, +const char *git_dir, +int recurse_into_nested);extern void relocate_gitdir (const char *path,const char *old_git_dir,const char *new_git_dir);submodule.c +3
-3@@ -1624,7 +1624,7 @@ int submodule_move_head(const char *path, }else {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,0 );free (gitdir);/* make sure the index is clean as well */ @@ -1634,7 +1634,7 @@ int submodule_move_head(const char *path,if (old_head && (flags &SUBMODULE_MOVE_HEAD_FORCE )) {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,1 );free (gitdir); } }@@ -1947,7 +1947,7 @@ void absorb_git_dir_into_superproject(const char *prefix,if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path);connect_work_tree_and_git_dir (path, -git_path ("modules/%s" , sub->name)); +git_path ("modules/%s" , sub->name),0 ); }else {/* Is it already absorbed into the superprojects git dir? */ char *real_sub_git_dir =real_pathdup (sub_git_dir,1 );t/t7001-mv.sh +1
-1@@ -497,7 +497,7 @@ test_expect_success 'moving a submodule in nested directories' ' test_cmp expect actual ' -test_expect_failure 'moving nested submodules' ' +test_expect_success 'moving nested submodules' ' git commit -am "cleanup commit" && mkdir sub_nested_nested && (cd sub_nested_nested &&Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Signature · 1 line
Brandon Williams
Re: [PATCH 5/6] submodule: fixup nested submodules after moving the submodule
Quoted 6 lines
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3025
Quoted 2 lines
dir.c +2
-0+if (repo_init (&subrepo, sub_gitdir, sub_worktree)) +return ;Note that in Duy's object-store series he made this function private (IIRC) so this will result in some clash of the two series.
Yes, that is the case.
I wonder if I'd rather revert to v1 where we only use the submodule repo init, or if we revert b2f0eceecf (repository:
initialize the_repository in main(), 2018-03-03) partially to have repo_init available.
I would think the approach with submodule init is a bit cleaner though has some more lines of code, using just repo_init seems easier, but we really have no use case for a separate repo_init unless they are submodules. And here we ought to check for the repo being a submodule.
Not yet sure which path to take.
Re: [PATCH 5/6] submodule: fixup nested submodules after moving the submodule
In reply to Stefan Bellerdir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3025
Quoted 12 lines · On Wed, 28 Mar 2018 10:24:48 -0700 Stefan Beller <[email protected]> wrote:
dir.c +12
-0+static void connect_wt_gitdir_in_nested (const char *sub_worktree, +const char *sub_gitdir) +{ +int i; +struct repository subrepo; +struct strbuf sub_wt =STRBUF_INIT ; +struct strbuf sub_gd =STRBUF_INIT ; + +const struct submodule *sub; + +if (repo_init (&subrepo, sub_gitdir, sub_worktree)) +return ;
If repo_init() fails, is it because the working tree doesn't exist on disk, so we don't need to perform any connections on submodules? I think a comment should be added to describe this.
Quoted 14 lines
dir.c +14
-0+ +if (repo_read_index (&subrepo) <0 ) +die ("index file corrupt in repo %s" , subrepo.gitdir); + +for (i =0 ; i < subrepo.index->cache_nr; i++) { +const struct cache_entry *ce = subrepo.index->cache[i]; + +if (!S_ISGITLINK (ce->ce_mode)) +continue ; + +while (i +1 < subrepo.index->cache_nr && + !strcmp (ce->name, subrepo.index->cache[i +1 ]->name)) +/* +* Skip entries with the same name in different stages
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3049
Quoted 10 lines
dir.c +10
-0+* to make sure an entry is returned only once. +*/ + i++; + + sub =submodule_from_path (&subrepo, &null_oid, ce->name); +if (!sub) +/* submodule not checked out? */ +continue ; + +if (is_submodule_active (&subrepo, ce->name)) {
Optional: This could be combined with the previous "if" block, so that the following lines don't need to be indented.
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3054
Quoted 5 lines
dir.c +5
-0+strbuf_addf (&sub_wt,"%s/%s" , sub_worktree, sub->path); +strbuf_addf (&sub_gd,"%s/modules/%s" , sub_gitdir, sub->name); + +connect_work_tree_and_git_dir (sub_wt.buf, sub_gd.buf,0 ); +connect_wt_gitdir_in_nested (sub_wt.buf, sub_gd.buf);
What's the difference between having this call to connect_wt_gitdir_in_nested() and just passing 1 instead of 0 to connect_work_tree_and_git_dir()? I see that the latter uses absolute paths, but that would seem to have the same effect. (If not, I think a comment is warranted.)
dir.c void untracked_cache_add_to_index(struct index_state *istate, · line 3057
Quoted 3 lines
dir.c +3
-0+ +strbuf_reset (&sub_wt); +strbuf_reset (&sub_gd);
I think we normally write the resets before the strbuf_addf(), so that it's clearer that sub_wt and sub_gd are meant to start from scratch in every iteration.
Overall, this version of the patch is clearer - thanks.
PATCH v2 6/6 grep: remove "repo" arg from non-supporting funcs
In reply to Stefan BellerFrom: Jonathan Tan <[email protected]>
As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct repository'", 2017-08-02), many functions in builtin/grep.c were converted to also take "struct repository *" arguments. Among them were grep_object() and grep_objects().
However, at least grep_objects() was converted incompletely - it calls gitmodules_config_oid(), which references the_repository.
But it turns out that the conversion was extraneous anyway - there has been no user-visible effect - because grep_objects() is never invoked except with the_repository. This is because grepping through objects cannot be done recursively into submodules.
Revert the changes to grep_objects() and grep_object() (which conversion is also extraneous) to show that both these functions do not support repositories other than the_repository.
--- builtin/grep.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
builtin/grep.c +6 -8
@@ -601,8 +601,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, }static int grep_object (struct grep_opt *opt,const struct pathspec *pathspec, -struct object *obj,const char *name,const char *path, -struct repository *repo) +struct object *obj,const char *name,const char *path) {if (obj->type ==OBJ_BLOB )return grep_oid (opt, &obj->oid, name,0 , path);
@@ -629,7 +628,7 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, }init_tree_desc (&tree, data, size); hit =grep_tree (opt, pathspec, &tree, &base, base.len, - obj->type ==OBJ_COMMIT , repo); + obj->type ==OBJ_COMMIT , the_repository);strbuf_release (&base);free (data);return hit;
@@ -638,7 +637,6 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, }static int grep_objects (struct grep_opt *opt,const struct pathspec *pathspec, -struct repository *repo,const struct object_array *list) {unsigned int i;
@@ -651,11 +649,11 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,/* load the gitmodules file for this rev */ if (recurse_submodules) { -submodule_free (repo); +submodule_free (the_repository);gitmodules_config_oid (&real_obj->oid); } -if (grep_object (opt, pathspec, real_obj, list->objects[i].name, list->objects[i].path, - repo)) { +if (grep_object (opt, pathspec, real_obj, list->objects[i].name, + list->objects[i].path)) { hit =1 ;if (opt->status_only)break ;
@@ -1107,7 +1105,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)if (cached)die (_ ("both --cached and trees are given." )); - hit =grep_objects (&opt, &pathspec, the_repository, &list); + hit =grep_objects (&opt, &pathspec, &list); }if (num_threads)
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Quoted 3 lines · On Wed, 28 Mar 2018 10:24:43 -0700 Stefan Beller <[email protected]> wrote:
* picked up Jonathans patch and added it as a nice finish of the series. I did not see the need or aesthetic desire to put that patch earlier in the series.
Thanks for picking up my patch. The aesthetic desire is to avoid what's currently happening in PATCH 2/6 version 2, where we still have the potentially confusing submodule_free() -> submodule_free(repo) conversion, which is also later redone in PATCH 6/6 version 2 (submodule_free(repo) -> submodule_free(the_repository)). But I'll leave the final decision to you.
Other than that, besides PATCH 5/6 (which I have already commented on), everything looks good.
v3 cover Moving submodules with nested submodules
v3:
* reordered patches to have Jonathans patch before submodule_free
* addressed Jonathans comments on patch 5.
* rebased on origin/sb/object-store to resolve a visibility conflict about repo_init being exposed outside of repository.c
v2:
* addressed memleaks and messy code in patch 5
* removed the extern keyword where applicable
* extended the commit message, stating we want to rename submodule_free in the future.
* picked up Jonathans patch and added it as a nice finish of the series. I did not see the need or aesthetic desire to put that patch earlier in the series.
Thanks,
Stefan
v1:
This fixes the bug reported in [1] ("Bug: moving submodules that have submodules inside them causes a fatal error in git status")
[1] https://public-inbox.org/git/[email protected]/
Thanks,
Stefan
Jonathan Tan (1):
grep: remove "repo" arg from non-supporting funcs
Stefan Beller (5):
submodule.h: drop declaration of connect_work_tree_and_git_dir
submodule-config: allow submodule_free to handle arbitrary
repositories
submodule-config: add repository argument to submodule_from_{name,
path}
submodule-config: remove submodule_from_cache
submodule: fixup nested submodules after moving the submodule
.../technical/api-submodule-config.txt | 2 +- builtin/grep.c | 14 ++--- builtin/mv.c | 6 +- builtin/submodule--helper.c | 17 +++--- dir.c | 60 ++++++++++++++++++- dir.h | 12 +++- repository.c | 8 +-- repository.h | 3 + submodule-config.c | 29 ++++----- submodule-config.h | 15 +++-- submodule.c | 40 +++++++------ submodule.h | 1 - t/helper/test-submodule-config.c | 8 ++- t/t7001-mv.sh | 2 +- unpack-trees.c | 2 +- 15 files changed, 140 insertions(+), 79 deletions(-)
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v3 1/6 submodule.h: drop declaration of connect_work_tree_and_git_dir
The function connect_work_tree_and_git_dir is declared in both submodule.h and dir.h, such that one of them is redundant. As the function is implemented in dir.c, drop the declaration from submodule.h
--- submodule.h | 1 - 1 file changed, 1 deletion(-)
submodule.h +0 -1
@@ -105,7 +105,6 @@ extern int push_unpushed_submodules(struct oid_array *commits, const char **refspec, int refspec_nr, const struct string_list *push_options, int dry_run); -extern void connect_work_tree_and_git_dir (const char *work_tree,const char *git_dir);/* * Given a submodule path (as in the index), return the repository * path of that submodule in 'buf'. Return -1 on error or when the
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v3 2/6 grep: remove "repo" arg from non-supporting funcs
In reply to Stefan BellerFrom: Jonathan Tan <[email protected]>
As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct repository'", 2017-08-02), many functions in builtin/grep.c were converted to also take "struct repository *" arguments. Among them were grep_object() and grep_objects().
However, at least grep_objects() was converted incompletely - it calls gitmodules_config_oid(), which references the_repository.
But it turns out that the conversion was extraneous anyway - there has been no user-visible effect - because grep_objects() is never invoked except with the_repository. This is because grepping through objects cannot be done recursively into submodules.
Revert the changes to grep_objects() and grep_object() (which conversion is also extraneous) to show that both these functions do not support repositories other than the_repository.
--- builtin/grep.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
builtin/grep.c +5 -7
@@ -595,8 +595,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, }static int grep_object (struct grep_opt *opt,const struct pathspec *pathspec, -struct object *obj,const char *name,const char *path, -struct repository *repo) +struct object *obj,const char *name,const char *path) {if (obj->type ==OBJ_BLOB )return grep_oid (opt, &obj->oid, name,0 , path);
@@ -623,7 +622,7 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, }init_tree_desc (&tree, data, size); hit =grep_tree (opt, pathspec, &tree, &base, base.len, - obj->type ==OBJ_COMMIT , repo); + obj->type ==OBJ_COMMIT , the_repository);strbuf_release (&base);free (data);return hit;
@@ -632,7 +631,6 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, }static int grep_objects (struct grep_opt *opt,const struct pathspec *pathspec, -struct repository *repo,const struct object_array *list) {unsigned int i;
@@ -648,8 +646,8 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,submodule_free ();gitmodules_config_oid (&real_obj->oid); } -if (grep_object (opt, pathspec, real_obj, list->objects[i].name, list->objects[i].path, - repo)) { +if (grep_object (opt, pathspec, real_obj, list->objects[i].name, + list->objects[i].path)) { hit =1 ;if (opt->status_only)break ;
@@ -1098,7 +1096,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)if (cached)die (_ ("both --cached and trees are given." )); - hit =grep_objects (&opt, &pathspec, the_repository, &list); + hit =grep_objects (&opt, &pathspec, &list); }if (num_threads)
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v3 3/6 submodule-config: allow submodule_free to handle arbitrary repositories
In reply to Stefan BellerAt some point we may want to rename the function so that it describes what it actually does as 'submodule_free' doesn't quite describe that this clears a repository's submodule cache. But that's beyond the scope of this series.
While at it remove the extern key word from its declaration.
--- Documentation/technical/api-submodule-config.txt | 2 +- builtin/grep.c | 2 +- submodule-config.c | 6 +++--- submodule-config.h | 2 +- t/helper/test-submodule-config.c | 2 +- unpack-trees.c | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-)
Documentation/technical/api-submodule-config.txt +1 -1
@@ -38,7 +38,7 @@ Data Structures Functions --------- -`void submodule_free()`:: +`void submodule_free(struct repository *r)`:: Use these to free the internally cached values.
builtin/grep.c +1 -1
@@ -643,7 +643,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,/* load the gitmodules file for this rev */ if (recurse_submodules) { -submodule_free (); +submodule_free (the_repository);gitmodules_config_oid (&real_obj->oid); } if (grep_object(opt, pathspec, real_obj, list->objects[i].name,
submodule-config.c +3 -3
@@ -642,8 +642,8 @@ const struct submodule *submodule_from_cache(struct repository *repo, key, lookup_path); } -void submodule_free (void ) +void submodule_free (struct repository *r) { -if (the_repository->submodule_cache) -submodule_cache_clear (the_repository->submodule_cache); +if (r->submodule_cache) +submodule_cache_clear (r->submodule_cache); }
submodule-config.h +1 -1
@@ -46,6 +46,6 @@ extern const struct submodule *submodule_from_path(extern const struct submodule *submodule_from_cache (struct repository *repo,const struct object_id *treeish_name,const char *key); -extern void submodule_free (void ); +void submodule_free (struct repository *r);#endif /* SUBMODULE_CONFIG_H */
t/helper/test-submodule-config.c +1 -1
@@ -64,7 +64,7 @@ int cmd_main(int argc, const char **argv) arg +=2 ; } -submodule_free (); +submodule_free (the_repository);return 0 ; }
unpack-trees.c +1 -1
@@ -290,7 +290,7 @@ static void load_gitmodules_file(struct index_state *index,if (!state && ce->ce_flags &CE_WT_REMOVE ) {repo_read_gitmodules (the_repository); }else if (state && (ce->ce_flags &CE_UPDATE )) { -submodule_free (); +submodule_free (the_repository);checkout_entry (ce, state,NULL );repo_read_gitmodules (the_repository); }
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v3 4/6 submodule-config: add repository argument to submodule_from_{name, path}
In reply to Stefan BellerThis enables submodule_from_{name, path} to handle arbitrary repositories. All callers just pass in the_repository, a later patch will pass in other repos.
While at it remove the extern key word from the declarations.
--- builtin/submodule--helper.c | 14 +++++++------- submodule-config.c | 14 ++++++++------ submodule-config.h | 10 ++++++---- submodule.c | 30 ++++++++++++++++-------------- t/helper/test-submodule-config.c | 6 ++++-- 5 files changed, 41 insertions(+), 33 deletions(-)
builtin/submodule--helper.c +7 -7
@@ -455,7 +455,7 @@ static void init_submodule(const char *path, const char *prefix, displaypath =get_submodule_displaypath (path, prefix); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub) die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -622,7 +622,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,struct rev_info rev;int diff_files_result; -if (!submodule_from_path (&null_oid, path)) +if (!submodule_from_path (the_repository, &null_oid, path))die (_ ("no submodule mapping found in .gitmodules for path '%s'" ), path);
@@ -742,7 +742,7 @@ static int module_name(int argc, const char **argv, const char *prefix)if (argc !=2 )usage (_ ("git submodule--helper name <path>" )); - sub =submodule_from_path (&null_oid, argv[1 ]); + sub =submodule_from_path (the_repository, &null_oid, argv[1 ]);if (!sub) die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -773,7 +773,7 @@ static void sync_submodule(const char *path, const char *prefix,if (!is_submodule_active (the_repository, path))return ; - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (sub && sub->url) { if (starts_with_dot_dot_slash(sub->url) ||
@@ -926,7 +926,7 @@ static void deinit_submodule(const char *path, const char *prefix,struct strbuf sb_config =STRBUF_INIT ;char *sub_git_dir =xstrfmt ("%s/.git" , path); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub || !sub->name) goto cleanup;
@@ -1368,7 +1368,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, goto cleanup; } - sub =submodule_from_path (&null_oid, ce->name); + sub =submodule_from_path (the_repository, &null_oid, ce->name);if (suc->recursive_prefix) displaypath = relative_path(suc->recursive_prefix,
@@ -1651,7 +1651,7 @@ static const char *remote_submodule_branch(const char *path)const char *branch =NULL ;char *key; - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)return NULL ;
submodule-config.c +8 -6
@@ -619,18 +619,20 @@ static void gitmodules_read_check(struct repository *repo)repo_read_gitmodules (repo); } -const struct submodule *submodule_from_name (const struct object_id *treeish_name, +const struct submodule *submodule_from_name (struct repository *r, +const struct object_id *treeish_name,const char *name) { -gitmodules_read_check (the_repository); -return config_from (the_repository->submodule_cache, treeish_name, name, lookup_name); +gitmodules_read_check (r); +return config_from (r->submodule_cache, treeish_name, name, lookup_name); } -const struct submodule *submodule_from_path (const struct object_id *treeish_name, +const struct submodule *submodule_from_path (struct repository *r, +const struct object_id *treeish_name,const char *path) { -gitmodules_read_check (the_repository); -return config_from (the_repository->submodule_cache, treeish_name, path, lookup_path); +gitmodules_read_check (r); +return config_from (r->submodule_cache, treeish_name, path, lookup_path); } const struct submodule *submodule_from_cache(struct repository *repo,
submodule-config.h +6 -4
@@ -39,10 +39,12 @@ extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg)extern int parse_push_recurse_submodules_arg (const char *opt,const char *arg);extern void repo_read_gitmodules (struct repository *repo);extern void gitmodules_config_oid (const struct object_id *commit_oid); -extern const struct submodule *submodule_from_name ( -const struct object_id *commit_or_tree,const char *name); -extern const struct submodule *submodule_from_path ( -const struct object_id *commit_or_tree,const char *path); +const struct submodule *submodule_from_name (struct repository *r, +const struct object_id *commit_or_tree, +const char *name); +const struct submodule *submodule_from_path (struct repository *r, +const struct object_id *commit_or_tree, +const char *path);extern const struct submodule *submodule_from_cache (struct repository *repo,const struct object_id *treeish_name,const char *key);
submodule.c +16 -14
@@ -96,7 +96,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)if (is_gitmodules_unmerged (&the_index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); - submodule =submodule_from_path (&null_oid, oldpath); + submodule =submodule_from_path (the_repository, &null_oid, oldpath);if (!submodule || !submodule->name) {warning (_ ("Could not find section in .gitmodules where path=%s" ), oldpath);return -1 ;
@@ -130,7 +130,7 @@ int remove_path_from_gitmodules(const char *path)if (is_gitmodules_unmerged (&the_index))die (_ ("Cannot change unmerged .gitmodules, resolve merge conflicts first" )); - submodule =submodule_from_path (&null_oid, path); + submodule =submodule_from_path (the_repository, &null_oid, path);if (!submodule || !submodule->name) {warning (_ ("Could not find section in .gitmodules where path=%s" ), path);return -1 ;
@@ -174,7 +174,8 @@ static int add_submodule_odb(const char *path)void set_diffopt_flags_from_submodule_config (struct diff_options *diffopt,const char *path) { -const struct submodule *submodule =submodule_from_path (&null_oid, path); +const struct submodule *submodule =submodule_from_path (the_repository, + &null_oid, path);if (submodule) {const char *ignore;char *key;
@@ -674,7 +675,7 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)if (!should_update_submodules ())return NULL ; -return submodule_from_path (&null_oid, ce->name); +return submodule_from_path (the_repository, &null_oid, ce->name); } static struct oid_array *submodule_commits(struct string_list *submodules,
@@ -731,13 +732,14 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,if (!S_ISGITLINK (p->two->mode))continue ; - submodule =submodule_from_path (commit_oid, p->two->path); + submodule =submodule_from_path (the_repository, + commit_oid, p->two->path);if (submodule) name = submodule->name;else { name =default_name_or_path (p->two->path);/* make sure name does not collide with existing one */ - submodule =submodule_from_name (commit_oid, name); + submodule =submodule_from_name (the_repository, commit_oid, name);if (submodule) {warning ("Submodule in commit %s at path: " "'%s' collides with a submodule named "
@@ -945,7 +947,7 @@ int find_unpushed_submodules(struct oid_array *commits,const struct submodule *submodule;const char *path =NULL ; - submodule =submodule_from_name (&null_oid, name->string); + submodule =submodule_from_name (the_repository, &null_oid, name->string);if (submodule) path = submodule->path;else
@@ -1113,7 +1115,7 @@ static void calculate_changed_submodule_paths(void)const struct string_list_item *name;/* No need to check if there are no submodules configured */ -if (!submodule_from_path (NULL ,NULL )) +if (!submodule_from_path (the_repository,NULL ,NULL ))return ;argv_array_push (&argv,"--" );/* argv[0] program name */
@@ -1134,7 +1136,7 @@ static void calculate_changed_submodule_paths(void)const struct submodule *submodule;const char *path =NULL ; - submodule =submodule_from_name (&null_oid, name->string); + submodule =submodule_from_name (the_repository, &null_oid, name->string);if (submodule) path = submodule->path;else
@@ -1162,7 +1164,7 @@ int submodule_touches_in_range(struct object_id *excl_oid,int ret;/* No need to check if there are no submodules configured */ -if (!submodule_from_path (NULL ,NULL )) +if (!submodule_from_path (the_repository,NULL ,NULL ))return 0 ;argv_array_push (&args,"--" );/* args[0] program name */
@@ -1604,7 +1606,7 @@ int submodule_move_head(const char *path,if (old && !is_submodule_populated_gently (path, error_code_ptr))return 0 ; - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)die ("BUG: could not get submodule information for '%s'" , path);
@@ -1886,7 +1888,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix, real_old_git_dir =real_pathdup (old_git_dir,1 ); - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path);
@@ -1942,7 +1944,7 @@ void absorb_git_dir_into_superproject(const char *prefix,* superproject did not rewrite the git file links yet, * fix it now. */ - sub =submodule_from_path (&null_oid, path); + sub =submodule_from_path (the_repository, &null_oid, path);if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path); connect_work_tree_and_git_dir(path,
@@ -2088,7 +2090,7 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)strbuf_addstr (buf, git_dir); }if (!is_git_directory (buf->buf)) { - sub =submodule_from_path (&null_oid, submodule); + sub =submodule_from_path (the_repository, &null_oid, submodule);if (!sub) { ret =-1 ; goto cleanup;
t/helper/test-submodule-config.c +4 -2
@@ -48,9 +48,11 @@ int cmd_main(int argc, const char **argv)die_usage (argc, argv,"Commit not found." );if (lookup_name) { - submodule =submodule_from_name (&commit_oid, path_or_name); + submodule =submodule_from_name (the_repository, + &commit_oid, path_or_name); }else - submodule =submodule_from_path (&commit_oid, path_or_name); + submodule =submodule_from_path (the_repository, + &commit_oid, path_or_name);if (!submodule)die_usage (argc, argv,"Submodule not found." );
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v3 5/6 submodule-config: remove submodule_from_cache
In reply to Stefan BellerThis continues the story of bf12fcdf5e (submodule-config: store the_submodule_cache in the_repository, 2017-06-22).
The previous patch taught submodule_from_path to take a repository into account, such that submodule_from_{path, cache} are the same now. Remove submodule_from_cache, migrating all its callers to submodule_from_path.
--- repository.c | 2 +- submodule-config.c | 9 --------- submodule-config.h | 3 --- submodule.c | 4 ++-- 4 files changed, 3 insertions(+), 15 deletions(-)
repository.c +1 -1
@@ -176,7 +176,7 @@ int repo_submodule_init(struct repository *submodule,struct strbuf worktree =STRBUF_INIT ;int ret =0 ; - sub =submodule_from_cache (superproject, &null_oid, path); + sub =submodule_from_path (superproject, &null_oid, path);if (!sub) { ret =-1 ; goto out;
submodule-config.c +0 -9
@@ -635,15 +635,6 @@ const struct submodule *submodule_from_path(struct repository *r,return config_from (r->submodule_cache, treeish_name, path, lookup_path); } -const struct submodule *submodule_from_cache (struct repository *repo, -const struct object_id *treeish_name, -const char *key) -{ -gitmodules_read_check (repo); -return config_from (repo->submodule_cache, treeish_name, - key, lookup_path); -} -void submodule_free (struct repository *r) {if (r->submodule_cache)
submodule-config.h +0 -3
@@ -45,9 +45,6 @@ const struct submodule *submodule_from_name(struct repository *r,const struct submodule *submodule_from_path (struct repository *r,const struct object_id *commit_or_tree,const char *path); -extern const struct submodule *submodule_from_cache (struct repository *repo, -const struct object_id *treeish_name, -const char *key);void submodule_free (struct repository *r);#endif /* SUBMODULE_CONFIG_H */
submodule.c +2 -2
@@ -231,7 +231,7 @@ int is_submodule_active(struct repository *repo, const char *path)const struct string_list *sl;const struct submodule *module; - module =submodule_from_cache (repo, &null_oid, path); + module =submodule_from_path (repo, &null_oid, path);/* early return if there isn't a path->module mapping */ if (!module)
@@ -1236,7 +1236,7 @@ static int get_next_submodule(struct child_process *cp,if (!S_ISGITLINK (ce->ce_mode))continue ; - submodule =submodule_from_cache (spf->r, &null_oid, ce->name); + submodule =submodule_from_path (spf->r, &null_oid, ce->name);if (!submodule) {const char *name =default_name_or_path (ce->name);if (name) {
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
PATCH v3 6/6 submodule: fixup nested submodules after moving the submodule
In reply to Stefan Bellerconnect_work_tree_and_git_dir is used to connect a submodule worktree with its git directory and vice versa after events that require a reconnection such as moving around the working tree. As submodules can have nested submodules themselves, we'd also want to fix the nested submodules when asked to. Add an option to recurse into the nested submodules and connect them as well.
As submodules are identified by their name (which determines their git directory in relation to their superproject's git directory) internally and by their path in the working tree of the superproject, we need to make sure that the mapping of name <-> path is kept intact. We can do that in the git-mv command by writing out the gitmodules file first and then forcing a reload of the submodule config machinery.
--- builtin/mv.c | 6 ++-- builtin/submodule--helper.c | 3 +- dir.c | 60 +++++++++++++++++++++++++++++++++++-- dir.h | 12 +++++++- repository.c | 6 ++-- repository.h | 3 ++ submodule.c | 6 ++-- t/t7001-mv.sh | 2 +- 8 files changed, 83 insertions(+), 15 deletions(-)
builtin/mv.c +4 -2
@@ -275,10 +275,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)die_errno (_ ("renaming '%s' failed" ), src); }if (submodule_gitfile[i]) { -if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) -connect_work_tree_and_git_dir (dst, submodule_gitfile[i]);if (!update_path_in_gitmodules (src, dst)) gitmodules_modified =1 ; +if (submodule_gitfile[i] !=SUBMODULE_WITH_GITDIR ) +connect_work_tree_and_git_dir (dst, + submodule_gitfile[i], +1 ); }if (mode ==WORKING_DIRECTORY )
builtin/submodule--helper.c +1 -2
@@ -1260,8 +1260,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)strbuf_reset (&sb); } -/* Connect module worktree and git dir */ -connect_work_tree_and_git_dir (path, sm_gitdir); +connect_work_tree_and_git_dir (path, sm_gitdir,0 ); p =git_pathdup_submodule (path,"config" );if (!p)
dir.c +57 -3
82 lines not on this page · raw message
dir.h +11 -1
@@ -359,7 +359,17 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned longvoid write_untracked_extension (struct strbuf *out,struct untracked_cache *untracked);void add_untracked_cache (struct index_state *istate);void remove_untracked_cache (struct index_state *istate); -extern void connect_work_tree_and_git_dir (const char *work_tree,const char *git_dir); + +/* +* Connect a worktree to a git directory by creating (or overwriting) a +* '.git' file containing the location of the git directory. In the git +* directory set the core.worktree setting to indicate where the worktree is. +* When `recurse_into_nested` is set, recurse into any nested submodules, +* connecting them as well. +*/ +extern void connect_work_tree_and_git_dir (const char *work_tree, +const char *git_dir, +int recurse_into_nested);extern void relocate_gitdir (const char *path,const char *old_git_dir,const char *new_git_dir);
repository.c +3 -3
@@ -135,9 +135,9 @@ static int read_and_verify_repository_format(struct repository_format *format,* Initialize 'repo' based on the provided 'gitdir'. * Return 0 upon success and a non-zero value upon failure. */ -static int repo_init (struct repository *repo, -const char *gitdir, -const char *worktree) +int repo_init (struct repository *repo, +const char *gitdir, +const char *worktree) {struct repository_format format;memset (repo,0 ,sizeof (*repo));
repository.h +3 -0
@@ -97,6 +97,9 @@ extern void repo_set_gitdir(struct repository *repo,extern void repo_set_worktree (struct repository *repo,const char *path);extern void repo_set_hash_algo (struct repository *repo,int algo);extern void initialize_the_repository (void ); +extern int repo_init (struct repository *r, +const char *gitdir, +const char *worktree);extern int repo_submodule_init (struct repository *submodule,struct repository *superproject,const char *path);
submodule.c +3 -3
@@ -1625,7 +1625,7 @@ int submodule_move_head(const char *path, }else {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,0 );free (gitdir);/* make sure the index is clean as well */
@@ -1635,7 +1635,7 @@ int submodule_move_head(const char *path,if (old && (flags &SUBMODULE_MOVE_HEAD_FORCE )) {char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, gitdir); +connect_work_tree_and_git_dir (path, gitdir,1 );free (gitdir); } }
@@ -1948,7 +1948,7 @@ void absorb_git_dir_into_superproject(const char *prefix,if (!sub)die (_ ("could not lookup name for submodule '%s'" ), path);connect_work_tree_and_git_dir (path, -git_path ("modules/%s" , sub->name)); +git_path ("modules/%s" , sub->name),0 ); }else {/* Is it already absorbed into the superprojects git dir? */ char *real_sub_git_dir =real_pathdup (sub_git_dir,1 );
t/t7001-mv.sh +1 -1
@@ -491,7 +491,7 @@ test_expect_success 'moving a submodule in nested directories' ' test_cmp expect actual ' -test_expect_failure 'moving nested submodules' ' +test_expect_success 'moving nested submodules' ' git commit -am "cleanup commit" && mkdir sub_nested_nested && (cd sub_nested_nested &&
Signature · 1 line
2.17.0.rc1.321.gba9d0f2565-goog
Quoted 5 lines · On Wed, 28 Mar 2018 15:35:25 -0700 Stefan Beller <[email protected]> wrote:
v3:
* reordered patches to have Jonathans patch beforesubmodule_free
* addressed Jonathans comments on patch 5.
* rebased on origin/sb/object-store to resolve a visibility conflict aboutrepo_initbeing exposed outside ofrepository.c
All 6 patches look good to me, thanks.