From 34224e14d6b50cb04430188332362e6a0327e5ed Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:14 -0700 Subject: [PATCH 1/7] refs: plumb repo into ref stores In preparation for the next 2 patches that adds (partial) support for arbitrary repositories to ref iterators, plumb a repository into all ref stores. There are no changes to program logic. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs.c | 29 ++++++++++++++++++++++------- refs/files-backend.c | 6 ++++-- refs/packed-backend.c | 4 +++- refs/packed-backend.h | 4 +++- refs/refs-internal.h | 10 ++++++++-- 5 files changed, 40 insertions(+), 13 deletions(-) diff --git a/refs.c b/refs.c index 2be0d0f057..9c4e388153 100644 --- a/refs.c +++ b/refs.c @@ -1873,7 +1873,8 @@ static struct ref_store *lookup_ref_store_map(struct hashmap *map, * Create, record, and return a ref_store instance for the specified * gitdir. */ -static struct ref_store *ref_store_init(const char *gitdir, +static struct ref_store *ref_store_init(struct repository *repo, + const char *gitdir, unsigned int flags) { const char *be_name = "files"; @@ -1883,7 +1884,7 @@ static struct ref_store *ref_store_init(const char *gitdir, if (!be) BUG("reference backend %s is unknown", be_name); - refs = be->init(gitdir, flags); + refs = be->init(repo, gitdir, flags); return refs; } @@ -1895,7 +1896,7 @@ struct ref_store *get_main_ref_store(struct repository *r) if (!r->gitdir) BUG("attempting to get main_ref_store outside of repository"); - r->refs_private = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS); + r->refs_private = ref_store_init(r, r->gitdir, REF_STORE_ALL_CAPS); r->refs_private = maybe_debug_wrap_ref_store(r->gitdir, r->refs_private); return r->refs_private; } @@ -1925,6 +1926,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule) struct ref_store *refs; char *to_free = NULL; size_t len; + struct repository *subrepo; if (!submodule) return NULL; @@ -1950,8 +1952,19 @@ struct ref_store *get_submodule_ref_store(const char *submodule) if (submodule_to_gitdir(&submodule_sb, submodule)) goto done; - /* assume that add_submodule_odb() has been called */ - refs = ref_store_init(submodule_sb.buf, + subrepo = xmalloc(sizeof(*subrepo)); + /* + * NEEDSWORK: Make get_submodule_ref_store() work with arbitrary + * superprojects other than the_repository. This probably should be + * done by making it take a struct repository * parameter instead of a + * submodule path. + */ + if (repo_submodule_init(subrepo, the_repository, submodule, + null_oid())) { + free(subrepo); + goto done; + } + refs = ref_store_init(subrepo, submodule_sb.buf, REF_STORE_READ | REF_STORE_ODB); register_ref_store_map(&submodule_ref_stores, "submodule", refs, submodule); @@ -1977,10 +1990,12 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt) return refs; if (wt->id) - refs = ref_store_init(git_common_path("worktrees/%s", wt->id), + refs = ref_store_init(the_repository, + git_common_path("worktrees/%s", wt->id), REF_STORE_ALL_CAPS); else - refs = ref_store_init(get_git_common_dir(), + refs = ref_store_init(the_repository, + get_git_common_dir(), REF_STORE_ALL_CAPS); if (refs) diff --git a/refs/files-backend.c b/refs/files-backend.c index 1148c0cf09..6a481e968f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -79,13 +79,15 @@ static void clear_loose_ref_cache(struct files_ref_store *refs) * Create a new submodule ref cache and add it to the internal * set of caches. */ -static struct ref_store *files_ref_store_create(const char *gitdir, +static struct ref_store *files_ref_store_create(struct repository *repo, + const char *gitdir, unsigned int flags) { struct files_ref_store *refs = xcalloc(1, sizeof(*refs)); struct ref_store *ref_store = (struct ref_store *)refs; struct strbuf sb = STRBUF_INIT; + ref_store->repo = repo; ref_store->gitdir = xstrdup(gitdir); base_ref_store_init(ref_store, &refs_be_files); refs->store_flags = flags; @@ -93,7 +95,7 @@ static struct ref_store *files_ref_store_create(const char *gitdir, get_common_dir_noenv(&sb, gitdir); refs->gitcommondir = strbuf_detach(&sb, NULL); strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir); - refs->packed_ref_store = packed_ref_store_create(sb.buf, flags); + refs->packed_ref_store = packed_ref_store_create(repo, sb.buf, flags); strbuf_release(&sb); chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir); diff --git a/refs/packed-backend.c b/refs/packed-backend.c index f8aa97d799..ea3493b24e 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -193,13 +193,15 @@ static int release_snapshot(struct snapshot *snapshot) } } -struct ref_store *packed_ref_store_create(const char *path, +struct ref_store *packed_ref_store_create(struct repository *repo, + const char *path, unsigned int store_flags) { struct packed_ref_store *refs = xcalloc(1, sizeof(*refs)); struct ref_store *ref_store = (struct ref_store *)refs; base_ref_store_init(ref_store, &refs_be_packed); + ref_store->repo = repo; ref_store->gitdir = xstrdup(path); refs->store_flags = store_flags; diff --git a/refs/packed-backend.h b/refs/packed-backend.h index a01a0aff9c..f61a73ec25 100644 --- a/refs/packed-backend.h +++ b/refs/packed-backend.h @@ -1,6 +1,7 @@ #ifndef REFS_PACKED_BACKEND_H #define REFS_PACKED_BACKEND_H +struct repository; struct ref_transaction; /* @@ -12,7 +13,8 @@ struct ref_transaction; * even among packed refs. */ -struct ref_store *packed_ref_store_create(const char *path, +struct ref_store *packed_ref_store_create(struct repository *repo, + const char *path, unsigned int store_flags); /* diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 96911fb26e..d28440c9cc 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -539,7 +539,8 @@ struct ref_store; * should call base_ref_store_init() to initialize the shared part of * the ref_store and to record the ref_store for later lookup. */ -typedef struct ref_store *ref_store_init_fn(const char *gitdir, +typedef struct ref_store *ref_store_init_fn(struct repository *repo, + const char *gitdir, unsigned int flags); typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err); @@ -697,7 +698,12 @@ struct ref_store { /* The backend describing this ref_store's storage scheme: */ const struct ref_storage_be *be; - /* The gitdir that this ref_store applies to: */ + struct repository *repo; + + /* + * The gitdir that this ref_store applies to. Note that this is not + * necessarily repo->gitdir if the repo has multiple worktrees. + */ char *gitdir; }; From 9bc45a28026eaea42011e8c1884aa2d9dc30f947 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:15 -0700 Subject: [PATCH 2/7] refs: teach arbitrary repo support to iterators Note that should_pack_ref() is called when writing refs, which is only supported for the_repository, hence the_repository is hardcoded there. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs.c | 3 ++- refs/files-backend.c | 5 ++++- refs/packed-backend.c | 6 ++++-- refs/refs-internal.h | 1 + 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index 9c4e388153..c07aeff6f4 100644 --- a/refs.c +++ b/refs.c @@ -255,12 +255,13 @@ int refname_is_safe(const char *refname) * does not exist, emit a warning and return false. */ int ref_resolves_to_object(const char *refname, + struct repository *repo, const struct object_id *oid, unsigned int flags) { if (flags & REF_ISBROKEN) return 0; - if (!has_object_file(oid)) { + if (!repo_has_object_file(repo, oid)) { error(_("%s does not point to a valid object!"), refname); return 0; } diff --git a/refs/files-backend.c b/refs/files-backend.c index 6a481e968f..3f213d24b0 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -732,6 +732,7 @@ struct files_ref_iterator { struct ref_iterator base; struct ref_iterator *iter0; + struct repository *repo; unsigned int flags; }; @@ -753,6 +754,7 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator) if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && !ref_resolves_to_object(iter->iter0->refname, + iter->repo, iter->iter0->oid, iter->iter0->flags)) continue; @@ -855,6 +857,7 @@ static struct ref_iterator *files_ref_iterator_begin( base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable, overlay_iter->ordered); iter->iter0 = overlay_iter; + iter->repo = ref_store->repo; iter->flags = flags; return ref_iterator; @@ -1139,7 +1142,7 @@ static int should_pack_ref(const char *refname, return 0; /* Do not pack broken refs: */ - if (!ref_resolves_to_object(refname, oid, ref_flags)) + if (!ref_resolves_to_object(refname, the_repository, oid, ref_flags)) return 0; return 1; diff --git a/refs/packed-backend.c b/refs/packed-backend.c index ea3493b24e..63f78bbaea 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -778,6 +778,7 @@ struct packed_ref_iterator { struct object_id oid, peeled; struct strbuf refname_buf; + struct repository *repo; unsigned int flags; }; @@ -866,8 +867,8 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator) continue; if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && - !ref_resolves_to_object(iter->base.refname, &iter->oid, - iter->flags)) + !ref_resolves_to_object(iter->base.refname, iter->repo, + &iter->oid, iter->flags)) continue; return ITER_OK; @@ -956,6 +957,7 @@ static struct ref_iterator *packed_ref_iterator_begin( iter->base.oid = &iter->oid; + iter->repo = ref_store->repo; iter->flags = flags; if (prefix && *prefix) diff --git a/refs/refs-internal.h b/refs/refs-internal.h index d28440c9cc..500d77864d 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -66,6 +66,7 @@ int refname_is_safe(const char *refname); * referred-to object does not exist, emit a warning and return false. */ int ref_resolves_to_object(const char *refname, + struct repository *repo, const struct object_id *oid, unsigned int flags); From 8788195c8846be949e6cd4bd19c8dc5e6371ffd3 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:16 -0700 Subject: [PATCH 3/7] refs: peeling non-the_repository iterators is BUG There is currently no support for peeling the current ref of an iterator iterating over a non-the_repository ref store, and none is needed. Thus, for now, BUG() if that happens. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs/files-backend.c | 5 +++-- refs/packed-backend.c | 3 +++ refs/ref-cache.c | 10 ++++++++++ refs/ref-cache.h | 1 + 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 3f213d24b0..8ee6ac2103 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -833,7 +833,7 @@ static struct ref_iterator *files_ref_iterator_begin( */ loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), - prefix, 1); + prefix, ref_store->repo, 1); /* * The packed-refs file might contain broken references, for @@ -1165,7 +1165,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err); - iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0); + iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, + the_repository, 0); while ((ok = ref_iterator_advance(iter)) == ITER_OK) { /* * If the loose reference can be packed, add an entry diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 63f78bbaea..2161218719 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -886,6 +886,9 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator, struct packed_ref_iterator *iter = (struct packed_ref_iterator *)ref_iterator; + if (iter->repo != the_repository) + BUG("peeling for non-the_repository is not supported"); + if ((iter->base.flags & REF_KNOWS_PEELED)) { oidcpy(peeled, &iter->peeled); return is_null_oid(&iter->peeled) ? -1 : 0; diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 49d732f6db..97a6ac349e 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -435,6 +435,8 @@ struct cache_ref_iterator { * on from there.) */ struct cache_ref_iterator_level *levels; + + struct repository *repo; }; static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) @@ -491,6 +493,11 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator, struct object_id *peeled) { + struct cache_ref_iterator *iter = + (struct cache_ref_iterator *)ref_iterator; + + if (iter->repo != the_repository) + BUG("peeling for non-the_repository is not supported"); return peel_object(ref_iterator->oid, peeled) ? -1 : 0; } @@ -513,6 +520,7 @@ static struct ref_iterator_vtable cache_ref_iterator_vtable = { struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, const char *prefix, + struct repository *repo, int prime_dir) { struct ref_dir *dir; @@ -547,5 +555,7 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, level->prefix_state = PREFIX_CONTAINS_DIR; } + iter->repo = repo; + return ref_iterator; } diff --git a/refs/ref-cache.h b/refs/ref-cache.h index 3bfb89d2b3..7877bf86ed 100644 --- a/refs/ref-cache.h +++ b/refs/ref-cache.h @@ -238,6 +238,7 @@ struct ref_entry *find_ref_entry(struct ref_dir *dir, const char *refname); */ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, const char *prefix, + struct repository *repo, int prime_dir); #endif /* REFS_REF_CACHE_H */ From 155b517d5c8701f3b8bef78fe59d8fe33adbee96 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:17 -0700 Subject: [PATCH 4/7] merge-{ort,recursive}: remove add_submodule_odb() After the parent commit and some of its ancestors, the only place commits are being accessed through alternates is in the user-facing message formatting code. Fix those, and remove the add_submodule_odb() calls. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- merge-ort.c | 18 ++++------------- merge-recursive.c | 41 +++++++++++++++++++------------------- strbuf.c | 12 ++++++++--- strbuf.h | 6 ++++-- t/t6437-submodule-merge.sh | 3 +++ 5 files changed, 40 insertions(+), 40 deletions(-) diff --git a/merge-ort.c b/merge-ort.c index b88475475d..fbc5c204c1 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -609,6 +609,7 @@ static int err(struct merge_options *opt, const char *err, ...) static void format_commit(struct strbuf *sb, int indent, + struct repository *repo, struct commit *commit) { struct merge_remote_desc *desc; @@ -622,7 +623,7 @@ static void format_commit(struct strbuf *sb, return; } - format_commit_message(commit, "%h %s", sb, &ctx); + repo_format_commit_message(repo, commit, "%h %s", sb, &ctx); strbuf_addch(sb, '\n'); } @@ -1578,17 +1579,6 @@ static int merge_submodule(struct merge_options *opt, if (is_null_oid(b)) return 0; - /* - * NEEDSWORK: Remove this when all submodule object accesses are - * through explicitly specified repositores. - */ - if (add_submodule_odb(path)) { - path_msg(opt, path, 0, - _("Failed to merge submodule %s (not checked out)"), - path); - return 0; - } - if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) { path_msg(opt, path, 0, _("Failed to merge submodule %s (not checked out)"), @@ -1653,7 +1643,7 @@ static int merge_submodule(struct merge_options *opt, break; case 1: - format_commit(&sb, 4, + format_commit(&sb, 4, &subrepo, (struct commit *)merges.objects[0].item); path_msg(opt, path, 0, _("Failed to merge submodule %s, but a possible merge " @@ -1670,7 +1660,7 @@ static int merge_submodule(struct merge_options *opt, break; default: for (i = 0; i < merges.nr; i++) - format_commit(&sb, 4, + format_commit(&sb, 4, &subrepo, (struct commit *)merges.objects[i].item); path_msg(opt, path, 0, _("Failed to merge submodule %s, but multiple " diff --git a/merge-recursive.c b/merge-recursive.c index 5a2d8a60c0..80594153f1 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -334,7 +334,9 @@ static void output(struct merge_options *opt, int v, const char *fmt, ...) flush_output(opt); } -static void output_commit_title(struct merge_options *opt, struct commit *commit) +static void repo_output_commit_title(struct merge_options *opt, + struct repository *repo, + struct commit *commit) { struct merge_remote_desc *desc; @@ -343,23 +345,29 @@ static void output_commit_title(struct merge_options *opt, struct commit *commit if (desc) strbuf_addf(&opt->obuf, "virtual %s\n", desc->name); else { - strbuf_add_unique_abbrev(&opt->obuf, &commit->object.oid, - DEFAULT_ABBREV); + strbuf_repo_add_unique_abbrev(&opt->obuf, repo, + &commit->object.oid, + DEFAULT_ABBREV); strbuf_addch(&opt->obuf, ' '); - if (parse_commit(commit) != 0) + if (repo_parse_commit(repo, commit) != 0) strbuf_addstr(&opt->obuf, _("(bad commit)\n")); else { const char *title; - const char *msg = get_commit_buffer(commit, NULL); + const char *msg = repo_get_commit_buffer(repo, commit, NULL); int len = find_commit_subject(msg, &title); if (len) strbuf_addf(&opt->obuf, "%.*s\n", len, title); - unuse_commit_buffer(commit, msg); + repo_unuse_commit_buffer(repo, commit, msg); } } flush_output(opt); } +static void output_commit_title(struct merge_options *opt, struct commit *commit) +{ + repo_output_commit_title(opt, the_repository, commit); +} + static int add_cacheinfo(struct merge_options *opt, const struct diff_filespec *blob, const char *path, int stage, int refresh, int options) @@ -1149,14 +1157,14 @@ static int find_first_merges(struct repository *repo, return result->nr; } -static void print_commit(struct commit *commit) +static void print_commit(struct repository *repo, struct commit *commit) { struct strbuf sb = STRBUF_INIT; struct pretty_print_context ctx = {0}; ctx.date_mode.type = DATE_NORMAL; /* FIXME: Merge this with output_commit_title() */ assert(!merge_remote_util(commit)); - format_commit_message(commit, " %h: %m %s", &sb, &ctx); + repo_format_commit_message(repo, commit, " %h: %m %s", &sb, &ctx); fprintf(stderr, "%s\n", sb.buf); strbuf_release(&sb); } @@ -1196,15 +1204,6 @@ static int merge_submodule(struct merge_options *opt, if (is_null_oid(b)) return 0; - /* - * NEEDSWORK: Remove this when all submodule object accesses are - * through explicitly specified repositores. - */ - if (add_submodule_odb(path)) { - output(opt, 1, _("Failed to merge submodule %s (not checked out)"), path); - return 0; - } - if (repo_submodule_init(&subrepo, opt->repo, path, null_oid())) { output(opt, 1, _("Failed to merge submodule %s (not checked out)"), path); return 0; @@ -1229,7 +1228,7 @@ static int merge_submodule(struct merge_options *opt, oidcpy(result, b); if (show(opt, 3)) { output(opt, 3, _("Fast-forwarding submodule %s to the following commit:"), path); - output_commit_title(opt, commit_b); + repo_output_commit_title(opt, &subrepo, commit_b); } else if (show(opt, 2)) output(opt, 2, _("Fast-forwarding submodule %s"), path); else @@ -1242,7 +1241,7 @@ static int merge_submodule(struct merge_options *opt, oidcpy(result, a); if (show(opt, 3)) { output(opt, 3, _("Fast-forwarding submodule %s to the following commit:"), path); - output_commit_title(opt, commit_a); + repo_output_commit_title(opt, &subrepo, commit_a); } else if (show(opt, 2)) output(opt, 2, _("Fast-forwarding submodule %s"), path); else @@ -1274,7 +1273,7 @@ static int merge_submodule(struct merge_options *opt, case 1: output(opt, 1, _("Failed to merge submodule %s (not fast-forward)"), path); output(opt, 2, _("Found a possible merge resolution for the submodule:\n")); - print_commit((struct commit *) merges.objects[0].item); + print_commit(&subrepo, (struct commit *) merges.objects[0].item); output(opt, 2, _( "If this is correct simply add it to the index " "for example\n" @@ -1287,7 +1286,7 @@ static int merge_submodule(struct merge_options *opt, default: output(opt, 1, _("Failed to merge submodule %s (multiple merges found)"), path); for (i = 0; i < merges.nr; i++) - print_commit((struct commit *) merges.objects[i].item); + print_commit(&subrepo, (struct commit *) merges.objects[i].item); } object_array_clear(&merges); diff --git a/strbuf.c b/strbuf.c index c8a5789694..b22e981655 100644 --- a/strbuf.c +++ b/strbuf.c @@ -1059,15 +1059,21 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, strbuf_setlen(sb, sb->len + len); } -void strbuf_add_unique_abbrev(struct strbuf *sb, const struct object_id *oid, - int abbrev_len) +void strbuf_repo_add_unique_abbrev(struct strbuf *sb, struct repository *repo, + const struct object_id *oid, int abbrev_len) { int r; strbuf_grow(sb, GIT_MAX_HEXSZ + 1); - r = find_unique_abbrev_r(sb->buf + sb->len, oid, abbrev_len); + r = repo_find_unique_abbrev_r(repo, sb->buf + sb->len, oid, abbrev_len); strbuf_setlen(sb, sb->len + r); } +void strbuf_add_unique_abbrev(struct strbuf *sb, const struct object_id *oid, + int abbrev_len) +{ + strbuf_repo_add_unique_abbrev(sb, the_repository, oid, abbrev_len); +} + /* * Returns the length of a line, without trailing spaces. * diff --git a/strbuf.h b/strbuf.h index 5b1113abf8..2d9e01c16f 100644 --- a/strbuf.h +++ b/strbuf.h @@ -634,8 +634,10 @@ void strbuf_list_free(struct strbuf **list); * Add the abbreviation, as generated by find_unique_abbrev, of `sha1` to * the strbuf `sb`. */ -void strbuf_add_unique_abbrev(struct strbuf *sb, - const struct object_id *oid, +struct repository; +void strbuf_repo_add_unique_abbrev(struct strbuf *sb, struct repository *repo, + const struct object_id *oid, int abbrev_len); +void strbuf_add_unique_abbrev(struct strbuf *sb, const struct object_id *oid, int abbrev_len); /** diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh index e5e89c2045..178413c22f 100755 --- a/t/t6437-submodule-merge.sh +++ b/t/t6437-submodule-merge.sh @@ -5,6 +5,9 @@ test_description='merging with submodules' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 +export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB + . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh From eef71904ff68e90f2db37aa9e25fe82c3fccf2a0 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:18 -0700 Subject: [PATCH 5/7] object-file: only register submodule ODB if needed In a35e03dee0 ("submodule: lazily add submodule ODBs as alternates", 2021-09-08), Git was taught to add all known submodule ODBs as alternates when attempting to read an object that doesn't exist, as a fallback for when a submodule object is read as if it were in the_repository. However, this behavior wasn't restricted to happen only when reading from the_repository. Fix this. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- object-file.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/object-file.c b/object-file.c index be4f94ecf3..0a1835fe30 100644 --- a/object-file.c +++ b/object-file.c @@ -1614,7 +1614,14 @@ static int do_oid_object_info_extended(struct repository *r, break; } - if (register_all_submodule_odb_as_alternates()) + /* + * If r is the_repository, this might be an attempt at + * accessing a submodule object as if it were in the_repository + * (having called add_submodule_odb() on that submodule's ODB). + * If any such ODBs exist, register them and try again. + */ + if (r == the_repository && + register_all_submodule_odb_as_alternates()) /* We added some alternates; retry */ continue; From 13a2f620b27c0fa29de026de5d74a2434c565ece Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:19 -0700 Subject: [PATCH 6/7] submodule: pass repo to check_has_commit() Pass the repo explicitly when calling check_has_commit() to avoid relying on add_submodule_odb(). With this commit and the parent commit, the last remaining tests no longer rely on add_submodule_odb(), so mark these tests accordingly. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- submodule.c | 16 +++++++++++++--- t/t5526-fetch-submodules.sh | 3 +++ t/t5531-deep-submodule-push.sh | 3 +++ t/t5545-push-options.sh | 3 +++ t/t5572-pull-submodule.sh | 3 +++ t/t7418-submodule-sparse-gitmodules.sh | 3 +++ 6 files changed, 28 insertions(+), 3 deletions(-) diff --git a/submodule.c b/submodule.c index 62beb8fd5f..4bf552b0e5 100644 --- a/submodule.c +++ b/submodule.c @@ -928,23 +928,33 @@ struct has_commit_data { static int check_has_commit(const struct object_id *oid, void *data) { struct has_commit_data *cb = data; + struct repository subrepo; + enum object_type type; - enum object_type type = oid_object_info(cb->repo, oid, NULL); + if (repo_submodule_init(&subrepo, cb->repo, cb->path, null_oid())) { + cb->result = 0; + goto cleanup; + } + + type = oid_object_info(&subrepo, oid, NULL); switch (type) { case OBJ_COMMIT: - return 0; + goto cleanup; case OBJ_BAD: /* * Object is missing or invalid. If invalid, an error message * has already been printed. */ cb->result = 0; - return 0; + goto cleanup; default: die(_("submodule entry '%s' (%s) is a %s, not a commit"), cb->path, oid_to_hex(oid), type_name(type)); } +cleanup: + repo_clear(&subrepo); + return 0; } static int submodule_has_commits(struct repository *r, diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index ed11569d8d..2dc75b80db 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -6,6 +6,9 @@ test_description='Recursive "git fetch" for submodules' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 +export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB + . ./test-lib.sh pwd=$(pwd) diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh index d573ca496a..3f58b515ce 100755 --- a/t/t5531-deep-submodule-push.sh +++ b/t/t5531-deep-submodule-push.sh @@ -5,6 +5,9 @@ test_description='test push with submodules' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 +export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB + . ./test-lib.sh test_expect_success setup ' diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh index 58c7add7ee..214228349a 100755 --- a/t/t5545-push-options.sh +++ b/t/t5545-push-options.sh @@ -5,6 +5,9 @@ test_description='pushing to a repository using push options' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 +export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB + . ./test-lib.sh mk_repo_pair () { diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh index 4f92a116e1..fa6b4cca65 100755 --- a/t/t5572-pull-submodule.sh +++ b/t/t5572-pull-submodule.sh @@ -2,6 +2,9 @@ test_description='pull can handle submodules' +GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 +export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB + . ./test-lib.sh . "$TEST_DIRECTORY"/lib-submodule-update.sh diff --git a/t/t7418-submodule-sparse-gitmodules.sh b/t/t7418-submodule-sparse-gitmodules.sh index 3f7f271883..f87e524d6d 100755 --- a/t/t7418-submodule-sparse-gitmodules.sh +++ b/t/t7418-submodule-sparse-gitmodules.sh @@ -12,6 +12,9 @@ The test setup uses a sparse checkout, however the same scenario can be set up also by committing .gitmodules and then just removing it from the filesystem. ' +GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 +export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB + . ./test-lib.sh test_expect_success 'sparse checkout setup which hides .gitmodules' ' From 71ef66d7403c05a6fe8ec118431332a8919b52a2 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Fri, 8 Oct 2021 14:08:20 -0700 Subject: [PATCH 7/7] submodule: trace adding submodule ODB as alternate Submodule ODBs are never added as alternates during the execution of the test suite, but there may be a rare interaction that the test suite does not have coverage of. Add a trace message when this happens, so that users who trace their commands can notice such occurrences. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- submodule.c | 2 ++ t/README | 7 ++----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/submodule.c b/submodule.c index 4bf552b0e5..61575e5a56 100644 --- a/submodule.c +++ b/submodule.c @@ -201,6 +201,8 @@ int register_all_submodule_odb_as_alternates(void) add_to_alternates_memory(added_submodule_odb_paths.items[i].string); if (ret) { string_list_clear(&added_submodule_odb_paths, 0); + trace2_data_intmax("submodule", the_repository, + "register_all_submodule_odb_as_alternates/registered", ret); if (git_env_bool("GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB", 0)) BUG("register_all_submodule_odb_as_alternates() called"); } diff --git a/t/README b/t/README index 51065d0800..b677caaf68 100644 --- a/t/README +++ b/t/README @@ -456,11 +456,8 @@ GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=, when true, makes registering submodule ODBs as alternates a fatal action. Support for this environment variable can be removed once the migration to explicitly providing repositories when accessing submodule objects is -complete (in which case we might want to replace this with a trace2 -call so that users can make it visible if accessing submodule objects -without an explicit repository still happens) or needs to be abandoned -for whatever reason (in which case the migrated codepaths still retain -their performance benefits). +complete or needs to be abandoned for whatever reason (in which case the +migrated codepaths still retain their performance benefits). Naming Tests ------------