From: Patrick Steinhardt <ps@pks.im>
To: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 02/12] Let `repo_in_merge_bases()` report missing commits
Date: Thu, 15 Feb 2024 10:33:15 +0100 [thread overview]
Message-ID: <Zc3aWzeRXFm1E2O_@tanuki> (raw)
In-Reply-To: <5f0af7fc0b95f9539ba7f740cb805ff1638d14cc.1707813709.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 11211 bytes --]
On Tue, Feb 13, 2024 at 08:41:38AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
[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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2024-02-15 9:33 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-13 8:41 [PATCH 00/12] The merge-base logic vs missing commit objects Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 01/12] paint_down_to_common: plug a memory leak Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 02/12] Let `repo_in_merge_bases()` report missing commits Johannes Schindelin via GitGitGadget
2024-02-15 9:33 ` Patrick Steinhardt [this message]
2024-02-13 8:41 ` [PATCH 03/12] Prepare `repo_in_merge_bases_many()` to optionally expect " Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 04/12] Prepare `paint_down_to_common()` for handling shallow commits Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 05/12] commit-reach: start reporting errors in `paint_down_to_common()` Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 06/12] merge_bases_many(): pass on errors from `paint_down_to_common()` Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 07/12] get_merge_bases_many_0(): pass on errors from `merge_bases_many()` Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 08/12] repo_get_merge_bases(): " Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 09/12] get_octopus_merge_bases(): " Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 10/12] repo_get_merge_bases_many(): " Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 11/12] repo_get_merge_bases_many_dirty(): " Johannes Schindelin via GitGitGadget
2024-02-13 8:41 ` [PATCH 12/12] paint_down_to_common(): special-case shallow/partial clones Johannes Schindelin via GitGitGadget
2024-02-13 18:37 ` [PATCH 00/12] The merge-base logic vs missing commit objects Junio C Hamano
2024-02-22 13:21 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 01/11] paint_down_to_common: plug two memory leaks Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 02/11] Prepare `repo_in_merge_bases_many()` to optionally expect missing commits Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 03/11] Start reporting missing commits in `repo_in_merge_bases_many()` Johannes Schindelin via GitGitGadget
2024-02-24 0:33 ` Junio C Hamano
2024-02-26 9:34 ` Johannes Schindelin
2024-02-26 20:01 ` Junio C Hamano
2024-03-01 6:56 ` Jeff King
2024-02-22 13:21 ` [PATCH v2 04/11] Prepare `paint_down_to_common()` for handling shallow commits Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 05/11] commit-reach: start reporting errors in `paint_down_to_common()` Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 06/11] merge_bases_many(): pass on errors from `paint_down_to_common()` Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 07/11] get_merge_bases_many_0(): pass on errors from `merge_bases_many()` Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 08/11] repo_get_merge_bases(): " Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 09/11] get_octopus_merge_bases(): " Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 10/11] repo_get_merge_bases_many(): " Johannes Schindelin via GitGitGadget
2024-02-22 13:21 ` [PATCH v2 11/11] repo_get_merge_bases_many_dirty(): " Johannes Schindelin via GitGitGadget
2024-02-27 13:28 ` [PATCH v3 00/11] The merge-base logic vs missing commit objects Johannes Schindelin via GitGitGadget
2024-02-27 13:28 ` [PATCH v3 01/11] paint_down_to_common: plug two memory leaks Johannes Schindelin via GitGitGadget
2024-02-27 13:28 ` [PATCH v3 02/11] Prepare `repo_in_merge_bases_many()` to optionally expect missing commits Johannes Schindelin via GitGitGadget
2024-02-27 13:28 ` [PATCH v3 03/11] Start reporting missing commits in `repo_in_merge_bases_many()` Johannes Schindelin via GitGitGadget
2024-02-27 13:28 ` [PATCH v3 04/11] Prepare `paint_down_to_common()` for handling shallow commits Johannes Schindelin via GitGitGadget
2024-02-27 14:46 ` Dirk Gouders
2024-02-27 15:00 ` Johannes Schindelin
2024-02-27 18:08 ` Junio C Hamano
2024-02-27 18:10 ` Junio C Hamano
2024-02-27 19:07 ` Dirk Gouders
2024-02-27 13:28 ` [PATCH v3 05/11] commit-reach: start reporting errors in `paint_down_to_common()` Johannes Schindelin via GitGitGadget
2024-02-27 14:56 ` Dirk Gouders
2024-02-27 15:08 ` Johannes Schindelin
2024-02-27 18:24 ` Junio C Hamano
2024-02-27 13:28 ` [PATCH v3 06/11] merge_bases_many(): pass on errors from `paint_down_to_common()` Johannes Schindelin via GitGitGadget
2024-02-27 18:29 ` Junio C Hamano
2024-02-27 13:28 ` [PATCH v3 07/11] get_merge_bases_many_0(): pass on errors from `merge_bases_many()` Johannes Schindelin via GitGitGadget
2024-02-27 13:28 ` [PATCH v3 08/11] repo_get_merge_bases(): " Johannes Schindelin via GitGitGadget
2024-02-27 13:28 ` [PATCH v3 09/11] get_octopus_merge_bases(): " Johannes Schindelin via GitGitGadget
2024-02-27 13:28 ` [PATCH v3 10/11] repo_get_merge_bases_many(): " Johannes Schindelin via GitGitGadget
2024-02-27 13:28 ` [PATCH v3 11/11] repo_get_merge_bases_many_dirty(): " Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 00/11] The merge-base logic vs missing commit objects Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 01/11] commit-reach(paint_down_to_common): plug two memory leaks Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 02/11] commit-reach(repo_in_merge_bases_many): optionally expect missing commits Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 03/11] commit-reach(repo_in_merge_bases_many): report " Johannes Schindelin via GitGitGadget
2024-03-01 6:58 ` Jeff King
2024-03-01 16:18 ` Junio C Hamano
2024-02-28 9:44 ` [PATCH v4 04/11] commit-reach(paint_down_to_common): prepare for handling shallow commits Johannes Schindelin via GitGitGadget
2024-02-28 20:24 ` Junio C Hamano
2024-02-28 20:59 ` Dirk Gouders
2024-02-29 9:54 ` Johannes Schindelin
2024-02-29 9:53 ` Johannes Schindelin
2024-02-28 9:44 ` [PATCH v4 05/11] commit-reach(paint_down_to_common): start reporting errors Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 06/11] commit-reach(merge_bases_many): pass on "missing commits" errors Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 07/11] commit-reach(get_merge_bases_many_0): " Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 08/11] commit-reach(repo_get_merge_bases): " Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 09/11] commit-reach(get_octopus_merge_bases): " Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 10/11] commit-reach(repo_get_merge_bases_many): " Johannes Schindelin via GitGitGadget
2024-02-28 9:44 ` [PATCH v4 11/11] commit-reach(repo_get_merge_bases_many_dirty): pass on errors Johannes Schindelin via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zc3aWzeRXFm1E2O_@tanuki \
--to=ps@pks.im \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johannes.schindelin@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.