On Tue, Feb 13, 2024 at 08:41:38AM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@xxxxxx> [snip] > diff --git a/http-push.c b/http-push.c > index a704f490fdb..85fa2f457d4 100644 > --- a/http-push.c > +++ b/http-push.c > @@ -1576,8 +1576,11 @@ static int verify_merge_base(struct object_id *head_oid, struct ref *remote) > struct commit *head = lookup_commit_or_die(head_oid, "HEAD"); > struct commit *branch = lookup_commit_or_die(&remote->old_oid, > remote->name); > + int i = repo_in_merge_bases(the_repository, branch, head); > > - return repo_in_merge_bases(the_repository, branch, head); > + if (i < 0) > + exit(128); > + return i; Nit: it's a bit unusual that we use `i` here instead of `ret`. Not worth a reroll on its own though. > } > > static int delete_remote_branch(const char *pattern, int force) > diff --git a/merge-ort.c b/merge-ort.c > index 6491070d965..64e76afe89f 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -544,6 +544,7 @@ enum conflict_and_info_types { > CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE, > CONFLICT_SUBMODULE_MAY_HAVE_REWINDS, > CONFLICT_SUBMODULE_NULL_MERGE_BASE, > + CONFLICT_SUBMODULE_CORRUPT, > > /* Keep this entry _last_ in the list */ > NB_CONFLICT_TYPES, > @@ -596,7 +597,9 @@ static const char *type_short_descriptions[] = { > [CONFLICT_SUBMODULE_MAY_HAVE_REWINDS] = > "CONFLICT (submodule may have rewinds)", > [CONFLICT_SUBMODULE_NULL_MERGE_BASE] = > - "CONFLICT (submodule lacks merge base)" > + "CONFLICT (submodule lacks merge base)", > + [CONFLICT_SUBMODULE_CORRUPT] = > + "CONFLICT (submodule corrupt)" > }; > > struct logical_conflict_info { > @@ -1710,7 +1713,11 @@ static int find_first_merges(struct repository *repo, > die("revision walk setup failed"); > while ((commit = get_revision(&revs)) != NULL) { > struct object *o = &(commit->object); > - if (repo_in_merge_bases(repo, b, commit)) > + int ret = repo_in_merge_bases(repo, b, commit); > + > + if (ret < 0) > + return ret; This is leaking both `merges` and `revs`. > + if (ret > 0) > add_object_array(o, NULL, &merges); > } > reset_revision_walk(); > @@ -1725,9 +1732,14 @@ static int find_first_merges(struct repository *repo, > contains_another = 0; > for (j = 0; j < merges.nr; j++) { > struct commit *m2 = (struct commit *) merges.objects[j].item; > - if (i != j && repo_in_merge_bases(repo, m2, m1)) { > - contains_another = 1; > - break; > + if (i != j) { > + int ret = repo_in_merge_bases(repo, m2, m1); > + if (ret < 0) > + return ret; > + if (ret > 0) { > + contains_another = 1; > + break; > + } > } > } > > @@ -1749,7 +1761,7 @@ static int merge_submodule(struct merge_options *opt, > { > struct repository subrepo; > struct strbuf sb = STRBUF_INIT; > - int ret = 0; > + int ret = 0, ret2; > struct commit *commit_o, *commit_a, *commit_b; > int parent_count; > struct object_array merges; > @@ -1796,8 +1808,26 @@ static int merge_submodule(struct merge_options *opt, > } > > /* check whether both changes are forward */ > - if (!repo_in_merge_bases(&subrepo, commit_o, commit_a) || > - !repo_in_merge_bases(&subrepo, commit_o, commit_b)) { > + ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_a); > + if (ret2 < 1) { > + path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, > + path, NULL, NULL, NULL, > + _("Failed to merge submodule %s " > + "(repository corrupt)"), > + path); > + goto cleanup; > + } Is `if (ret2 < 1)` intentional? I doubt it is, because it would also trigger for `ret2 == 0`, which we try to explicitly handle in the next line. So the following condition `if (!ret2)` cannot ever be true. > + if (!ret2) > + ret2 = repo_in_merge_bases(&subrepo, commit_o, commit_b); > + if (ret2 < 1) { > + path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, > + path, NULL, NULL, NULL, > + _("Failed to merge submodule %s " > + "(repository corrupt)"), > + path); > + goto cleanup; > + } Same here. > + if (!ret2) { > path_msg(opt, CONFLICT_SUBMODULE_MAY_HAVE_REWINDS, 0, > path, NULL, NULL, NULL, > _("Failed to merge submodule %s " > @@ -1807,7 +1837,16 @@ static int merge_submodule(struct merge_options *opt, > } > > /* Case #1: a is contained in b or vice versa */ > - if (repo_in_merge_bases(&subrepo, commit_a, commit_b)) { > + ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b); > + if (ret2 < 0) { > + path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, > + path, NULL, NULL, NULL, > + _("Failed to merge submodule %s " > + "(repository corrupt)"), > + path); > + goto cleanup; > + } > + if (ret2 > 0) { > oidcpy(result, b); > path_msg(opt, INFO_SUBMODULE_FAST_FORWARDING, 1, > path, NULL, NULL, NULL, > @@ -1816,7 +1855,16 @@ static int merge_submodule(struct merge_options *opt, > ret = 1; > goto cleanup; > } > - if (repo_in_merge_bases(&subrepo, commit_b, commit_a)) { > + ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a); > + if (ret2 < 0) { > + path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, > + path, NULL, NULL, NULL, > + _("Failed to merge submodule %s " > + "(repository corrupt)"), > + path); > + goto cleanup; > + } > + if (ret2 > 0) { > oidcpy(result, a); > path_msg(opt, INFO_SUBMODULE_FAST_FORWARDING, 1, > path, NULL, NULL, NULL, > @@ -1841,6 +1889,13 @@ static int merge_submodule(struct merge_options *opt, > parent_count = find_first_merges(&subrepo, path, commit_a, commit_b, > &merges); > switch (parent_count) { > + case -1: > + path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, > + path, NULL, NULL, NULL, > + _("Failed to merge submodule %s " > + "(repository corrupt)"), > + path); > + break; I was wondering whether it is safe to `break` here because we do end up calling `object_array_clear()` on `merges` which we didn't initialize in this function. But `find_first_merges()` always initializes the array with zeroes, so this is fine. > case 0: > path_msg(opt, CONFLICT_SUBMODULE_FAILED_TO_MERGE, 0, > path, NULL, NULL, NULL, > diff --git a/merge-recursive.c b/merge-recursive.c > index e3beb0801b1..e3fe7803cbe 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -1144,7 +1144,10 @@ static int find_first_merges(struct repository *repo, > die("revision walk setup failed"); > while ((commit = get_revision(&revs)) != NULL) { > struct object *o = &(commit->object); > - if (repo_in_merge_bases(repo, b, commit)) > + int ret = repo_in_merge_bases(repo, b, commit); > + if (ret < 0) > + return ret; We also leak `merges` and `revs` here. Patrick > + if (ret) > add_object_array(o, NULL, &merges); > } > reset_revision_walk(); > @@ -1159,9 +1162,14 @@ static int find_first_merges(struct repository *repo, > contains_another = 0; > for (j = 0; j < merges.nr; j++) { > struct commit *m2 = (struct commit *) merges.objects[j].item; > - if (i != j && repo_in_merge_bases(repo, m2, m1)) { > - contains_another = 1; > - break; > + if (i != j) { > + int ret = repo_in_merge_bases(repo, m2, m1); > + if (ret < 0) > + return ret; > + if (ret > 0) { > + contains_another = 1; > + break; > + } > } > } > > @@ -1197,7 +1205,7 @@ static int merge_submodule(struct merge_options *opt, > const struct object_id *b) > { > struct repository subrepo; > - int ret = 0; > + int ret = 0, ret2; > struct commit *commit_base, *commit_a, *commit_b; > int parent_count; > struct object_array merges; > @@ -1234,14 +1242,29 @@ static int merge_submodule(struct merge_options *opt, > } > > /* check whether both changes are forward */ > - if (!repo_in_merge_bases(&subrepo, commit_base, commit_a) || > - !repo_in_merge_bases(&subrepo, commit_base, commit_b)) { > + ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_a); > + if (ret2 < 0) { > + output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path); > + goto cleanup; > + } > + if (ret2) > + ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_b); > + if (ret2 < 0) { > + output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path); > + goto cleanup; > + } > + if (!ret2) { > output(opt, 1, _("Failed to merge submodule %s (commits don't follow merge-base)"), path); > goto cleanup; > } > > /* Case #1: a is contained in b or vice versa */ > - if (repo_in_merge_bases(&subrepo, commit_a, commit_b)) { > + ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b); > + if (ret2 < 0) { > + output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path); > + goto cleanup; > + } > + if (ret2) { > oidcpy(result, b); > if (show(opt, 3)) { > output(opt, 3, _("Fast-forwarding submodule %s to the following commit:"), path); > @@ -1254,7 +1277,12 @@ static int merge_submodule(struct merge_options *opt, > ret = 1; > goto cleanup; > } > - if (repo_in_merge_bases(&subrepo, commit_b, commit_a)) { > + ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a); > + if (ret2 < 0) { > + output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path); > + goto cleanup; > + } > + if (ret2) { > oidcpy(result, a); > if (show(opt, 3)) { > output(opt, 3, _("Fast-forwarding submodule %s to the following commit:"), path); > @@ -1402,6 +1430,8 @@ static int merge_mode_and_contents(struct merge_options *opt, > &o->oid, > &a->oid, > &b->oid); > + if (result->clean < 0) > + return -1; > } else if (S_ISLNK(a->mode)) { > switch (opt->recursive_variant) { > case MERGE_VARIANT_NORMAL: > diff --git a/shallow.c b/shallow.c > index ac728cdd778..cf4b95114b7 100644 > --- a/shallow.c > +++ b/shallow.c > @@ -795,12 +795,16 @@ static void post_assign_shallow(struct shallow_info *info, > if (!*bitmap) > continue; > for (j = 0; j < bitmap_nr; j++) > - if (bitmap[0][j] && > - /* Step 7, reachability test at commit level */ > - !repo_in_merge_bases_many(the_repository, c, ca.nr, ca.commits)) { > - update_refstatus(ref_status, info->ref->nr, *bitmap); > - dst++; > - break; > + if (bitmap[0][j]) { > + /* Step 7, reachability test at commit level */ > + int ret = repo_in_merge_bases_many(the_repository, c, ca.nr, ca.commits); > + if (ret < 0) > + exit(128); > + if (!ret) { > + update_refstatus(ref_status, info->ref->nr, *bitmap); > + dst++; > + break; > + } > } > } > info->nr_ours = dst; > @@ -829,6 +833,8 @@ int delayed_reachability_test(struct shallow_info *si, int c) > commit, > si->nr_commits, > si->commits); > + if (si->reachable[c] < 0) > + exit(128); > si->need_reachability_test[c] = 0; > } > return si->reachable[c]; > -- > gitgitgadget > >
Attachment:
signature.asc
Description: PGP signature