Re: [RFC PATCH 1/2] implement fetching of moved submodules
From: Stefan Beller <hidden>
Date: 2017-08-17 17:20:20
On Thu, Aug 17, 2017 at 3:53 AM, Heiko Voigt [off-list ref] wrote:
We store the changed submodules paths to calculate which submodule needs fetching. This does not work for moved submodules since their paths do not stay the same in case of a moved submodules. In case of new submodules we do not have a path in the current checkout, since they just appeared in this fetch. It is more general to collect the submodule names for changes instead of their paths to include the above cases. With the change described above we implement 'on-demand' fetching of changes in moved submodules.
This sounds as if this would also enable fetching new submodules eventually?
Note: This does only work when repositories have a .gitmodules file. In other words: It breaks if we do not get a name for a repository. IIRC, consensus was that this is a requirement to get nice submodule handling these days?
I think that should have been the consensus since ~1.7.8 (since the submodules git dir can live inside the superprojects <gitdir>/module/<name>). A gitlink entry without corresponding .gitmodules entry is just a gitlink. If we happen to have a repository at that path of the gitlink, we can be nice and pretend like it is a functional submodule, but it really is not. It's just another repo inside the superproject that happen to live at the path of a gitlink.
Signed-off-by: Heiko Voigt <redacted> --- I updated the leftover code from my series implementing recursive fetch for moved submodules[1] to the current master. This breaks t5531 and t5545 because they do not use a .gitmodules file. I also have some code leftover that does fallback on paths in case no submodule names can be found. But I do not really like it. The question here is how far do we support not using .gitmodules. Is it e.g. reasonable to say: "For --recurse-submodules=on-demand you need a .gitmodules file?"
I would not intentionally break users here, but any new functionality can safely assume (a) we have a proper .gitmodules entry or (b) it is not a submodule, so do nothing/be extra careful. For example in recursive diff sort of makes sense to also handle non-submodule gitlinks, but fetch is harder to tell. (just last night I was rereading https://public-inbox.org/git/CAJo=hJvnAPNAdDcAAwAvU9C4RVeQdoS3Ev9WTguHx4fD0V_nOg@mail.gmail.com/ which I think is a super cute application of gitlinks. If you happen to checkout such a tree, you don't want to fetch all of the fake submodules)
[1] https://public-inbox.org/git/f5baa2acc09531a16f4f693eebbe60706bb8ed1e.1361751905.git.hvoigt@hvoigt.net/
Oha, that is from way back in the time. :)
quoted hunk ↗ jump to hunk
submodule.c | 92 +++++++++++++++++++++++++-------------------- t/t5526-fetch-submodules.sh | 35 +++++++++++++++++ 2 files changed, 86 insertions(+), 41 deletions(-)diff --git a/submodule.c b/submodule.c index 27de65a..3ed78ac 100644 --- a/submodule.c +++ b/submodule.c@@ -23,7 +23,7 @@ static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND; static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF; static int parallel_jobs = 1; -static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP; +static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP; static int initialized_fetch_ref_tips; static struct oid_array ref_tips_before_fetch; static struct oid_array ref_tips_after_fetch;@@ -742,11 +742,11 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce) } static struct oid_array *submodule_commits(struct string_list *submodules, - const char *path) + const char *name) { struct string_list_item *item; - item = string_list_insert(submodules, path); + item = string_list_insert(submodules, name); if (item->util) return (struct oid_array *) item->util;@@ -755,39 +755,34 @@ static struct oid_array *submodule_commits(struct string_list *submodules, return (struct oid_array *) item->util; } +struct collect_changed_submodules_cb_data { + struct string_list *changed;
Here a comment would be helpful or a more concise variable name. (What is changed?)
+ const struct object_id *commit_oid;
+};
+
static void collect_changed_submodules_cb(struct diff_queue_struct *q,
struct diff_options *options,
void *data)
{
+ struct collect_changed_submodules_cb_data *me = data;
+ struct string_list *changed = me->changed;
+ const struct object_id *commit_oid = me->commit_oid;
int i;
- struct string_list *changed = data;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
struct oid_array *commits;
+ const struct submodule *submodule;(optional style nit, personal opinion, feel free to ignore) I personally prefer to not name variables exactly as their type. Also most (all) of the struct submodule uses used 'sub' as the variable name, maybe keep it consistent?
quoted hunk ↗ jump to hunk
+ if (!S_ISGITLINK(p->two->mode)) continue; - if (S_ISGITLINK(p->one->mode)) { - /* - * NEEDSWORK: We should honor the name configured in - * the .gitmodules file of the commit we are examining - * here to be able to correctly follow submodules - * being moved around. - */ - commits = submodule_commits(changed, p->two->path); - oid_array_append(commits, &p->two->oid); - } else { - /* Submodule is new or was moved here */ - /* - * NEEDSWORK: When the .git directories of submodules - * live inside the superprojects .git directory some - * day we should fetch new submodules directly into - * that location too when config or options request - * that so they can be checked out from there. - */ + submodule = submodule_from_path(commit_oid, p->two->path); + if (!submodule) continue; - } + + commits = submodule_commits(changed, submodule->name); + oid_array_append(commits, &p->two->oid); } }@@ -810,11 +805,14 @@ static void collect_changed_submodules(struct string_list *changed, while ((commit = get_revision(&rev))) { struct rev_info diff_rev; + struct collect_changed_submodules_cb_data data; + data.changed = changed; + data.commit_oid = &commit->object.oid; init_revisions(&diff_rev, NULL); diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK; diff_rev.diffopt.format_callback = collect_changed_submodules_cb; - diff_rev.diffopt.format_callback_data = changed; + diff_rev.diffopt.format_callback_data = &data; diff_tree_combined_merge(commit, 1, &diff_rev); }@@ -871,6 +869,7 @@ static int submodule_has_commits(const char *path, struct oid_array *commits) oid_array_for_each_unique(commits, check_has_commit, &has_commit); if (has_commit) { +
stray new line
quoted hunk ↗ jump to hunk
/* * Even if the submodule is checked out and the commit is * present, make sure it exists in the submodule's object store@@ -945,7 +944,7 @@ int find_unpushed_submodules(struct oid_array *commits, const char *remotes_name, struct string_list *needs_pushing) { struct string_list submodules = STRING_LIST_INIT_DUP; - struct string_list_item *submodule; + struct string_list_item *name; struct argv_array argv = ARGV_ARRAY_INIT; /* argv.argv[0] will be ignored by setup_revisions */@@ -956,12 +955,16 @@ int find_unpushed_submodules(struct oid_array *commits, collect_changed_submodules(&submodules, &argv); - for_each_string_list_item(submodule, &submodules) { - struct oid_array *commits = submodule->util; - const char *path = submodule->string; + for_each_string_list_item(name, &submodules) { + struct oid_array *commits = name->util; + const struct submodule *submodule; + + submodule = submodule_from_name(&null_oid, name->string); + if (!submodule) + continue; - if (submodule_needs_pushing(path, commits)) - string_list_insert(needs_pushing, path); + if (submodule_needs_pushing(submodule->path, commits)) + string_list_insert(needs_pushing, submodule->path);
eventually we can also migrate to name here as well. In a later patch.
quoted hunk ↗ jump to hunk
} free_submodules_oids(&submodules);@@ -1104,7 +1107,7 @@ static void calculate_changed_submodule_paths(void) { struct argv_array argv = ARGV_ARRAY_INIT; struct string_list changed_submodules = STRING_LIST_INIT_DUP; - const struct string_list_item *item; + const struct string_list_item *name; /* No need to check if there are no submodules configured */ if (!submodule_from_path(NULL, NULL))@@ -1119,16 +1122,20 @@ static void calculate_changed_submodule_paths(void) /* * Collect all submodules (whether checked out or not) for which new - * commits have been recorded upstream in "changed_submodule_paths". + * commits have been recorded upstream in "changed_submodule_names". */ collect_changed_submodules(&changed_submodules, &argv); - for_each_string_list_item(item, &changed_submodules) { - struct oid_array *commits = item->util; - const char *path = item->string; + for_each_string_list_item(name, &changed_submodules) { + struct oid_array *commits = name->util; + const struct submodule *submodule; + + submodule = submodule_from_name(&null_oid, name->string); + if (!submodule) + continue; - if (!submodule_has_commits(path, commits)) - string_list_append(&changed_submodule_paths, path); + if (!submodule_has_commits(submodule->path, commits)) + string_list_append(&changed_submodule_names, name->string); } free_submodules_oids(&changed_submodules);@@ -1206,7 +1213,8 @@ static int get_next_submodule(struct child_process *cp, continue; if (submodule->fetch_recurse == RECURSE_SUBMODULES_ON_DEMAND) { - if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name)) + if (!unsorted_string_list_lookup(&changed_submodule_names, + submodule->name)) continue; default_argv = "on-demand"; }@@ -1215,13 +1223,15 @@ static int get_next_submodule(struct child_process *cp, gitmodules_is_unmerged) continue; if (config_fetch_recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) { - if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name)) + if (!unsorted_string_list_lookup(&changed_submodule_names, + submodule->name)) continue; default_argv = "on-demand"; } } } else if (spf->command_line_option == RECURSE_SUBMODULES_ON_DEMAND) { - if (!unsorted_string_list_lookup(&changed_submodule_paths, ce->name)) + if (!unsorted_string_list_lookup(&changed_submodule_names, + submodule->name)) continue; default_argv = "on-demand"; }@@ -1315,7 +1325,7 @@ int fetch_populated_submodules(const struct argv_array *options, argv_array_clear(&spf.args); out: - string_list_clear(&changed_submodule_paths, 1); + string_list_clear(&changed_submodule_names, 1); return spf.result; }diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index 162baf1..ce788e9 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh@@ -530,4 +530,39 @@ test_expect_success 'fetching submodule into a broken repository' ' test_must_fail git -C dst fetch --recurse-submodules ' +test_expect_success "fetch new commits when submodule got renamed" ' + git clone . downstream_rename && + ( + cd downstream_rename && + git submodule update --init && +# NEEDSWORK: we omitted --recursive for the submodule update here since +# that does not work. See test 7001 for mv "moving nested submodules" +# for details. Once that is fixed we should add the --recursive option +# here. + git checkout -b rename && + git mv submodule submodule_renamed && + ( + cd submodule_renamed && + git checkout -b rename_sub && + echo a >a && + git add a && + git commit -ma && + git push origin rename_sub && + git rev-parse HEAD >../../expect + ) && + git add submodule_renamed && + git commit -m "update renamed submodule" && + git push origin rename + ) && + ( + cd downstream && + git fetch --recurse-submodules=on-demand && + ( + cd submodule && + git rev-parse origin/rename_sub >../../actual + ) + ) && + test_cmp expect actual +' + test_done --2.0.0.274.g6b2cd91