The merge-base logic vs missing commit objects (follow-up) #1686
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `merge_submodule()` function returns an integer that indicates whether the merge was clean (returning 1) or unclean (returning 0). Like the version in `merge-ort.c`, the version in `merge-recursive.c` does not report any errors (such as repository corruption) by returning -1 as of time of writing, even if the callers in `merge-ort.c` are prepared for exactly such errors. However, we want to teach (both variants of) the `merge_submodule()` function that trick: to report errors by returning -1. Therefore, prepare the caller in `merge-recursive.c` to handle that scenario. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In 24876eb (commit-reach(repo_in_merge_bases_many): report missing commits, 2024-02-28), I taught `merge_submodule()` to handle errors reported by `repo_in_merge_bases_many()`. However, those errors were not passed through to the callers. That was unintentional, and this commit remedies that. Note that `find_first_merges()` can now also return -1 (because it passes through that return value from `repo_in_merge_bases()`), and this commit also adds the forgotten handling for that scenario. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
/submit |
|
Submitted as pull.1686.git.1709993397.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Sat, Mar 9, 2024 at 6:10 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> Jeff King reported that Coverity pointed out a problem in the patch series
> "The merge-base logic vs missing commit objects" (which made it into the
> next branch already): The return value of merge_submodules() is assigned to
> an unsigned, single-bit variable, which as a consequence is not able to hold
> a negative value indicating a non-recoverable error.
>
> I looked into this issue and am happy to report that there are no other
> instances of the same issue in that patch series. The first patch in this
> here patch series addresses that issue.
>
> While looking into this issue I also noticed that the merge_submodule()
> function did not even return negative values! This was an oversight on my
> part (which I attribute with a large amount of self-compassion to my utter
> lack of enthusiasm for submodules as a Git feature), and the second patch in
> this here patch series addresses that.
>
> This is a follow-up for
> https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,
> based on the js/merge-base-with-missing-commit branch.
This series looks good to me; thanks. |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Jeff King reported that Coverity pointed out a problem in the patch series
> "The merge-base logic vs missing commit objects" (which made it into the
> next branch already): The return value of merge_submodules() is assigned to
> an unsigned, single-bit variable, which as a consequence is not able to hold
> a negative value indicating a non-recoverable error.
>
> I looked into this issue and am happy to report that there are no other
> instances of the same issue in that patch series. The first patch in this
> here patch series addresses that issue.
>
> While looking into this issue I also noticed that the merge_submodule()
> function did not even return negative values! This was an oversight on my
> part (which I attribute with a large amount of self-compassion to my utter
> lack of enthusiasm for submodules as a Git feature), and the second patch in
> this here patch series addresses that.
>
> This is a follow-up for
> https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,
> based on the js/merge-base-with-missing-commit branch.
Thanks.
>
> Johannes Schindelin (2):
> merge-recursive: prepare for `merge_submodule()` to report errors
> merge-ort/merge-recursive: do report errors in `merge_submodule()`
>
> merge-ort.c | 5 +++++
> merge-recursive.c | 21 +++++++++++++++------
> 2 files changed, 20 insertions(+), 6 deletions(-)
>
>
> base-commit: caaf1a2942c25c1f1a15818b718c9f641e52beef
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1686%2Fdscho%2Fmerge-base-and-missing-objects-followup-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1686/dscho/merge-base-and-missing-objects-followup-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1686 |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1819,6 +1819,7 @@ static int merge_submodule(struct merge_options *opt, | |||
| _("Failed to merge submodule %s " | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 24876ebf68b (commit-reach(repo_in_merge_bases_many): report missing
> commits, 2024-02-28), I taught `merge_submodule()` to handle errors
> reported by `repo_in_merge_bases_many()`.
>
> However, those errors were not passed through to the callers. That was
> unintentional, and this commit remedies that.
>
> Note that `find_first_merges()` can now also return -1 (because it
> passes through that return value from `repo_in_merge_bases()`), and this
> commit also adds the forgotten handling for that scenario.
Good clean-up. But this "oops, we did not check for errors" makes
me wonder if we are better off adopting "by default we assume an
error, until we are sure we are good" pattern, i.e.
func()
{
int ret = -1; /* assume worst */
do stuff;
if (...) {
error(_("this is bad"));
goto cleanup;
}
do stuff;
if (...) {
error(_("this is bad, too"));
goto cleanup;
}
/* ok we are happy */
ret = 0;
cleanup:
release resources;
return ret;
}
The patch to both functions do make it appear that they are good
candidates for application of the pattern to me.
Thanks.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> merge-ort.c | 5 +++++
> merge-recursive.c | 8 ++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 033c4348e2d..5d36c04f509 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -1819,6 +1819,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2 > 0)
> @@ -1829,6 +1830,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> goto cleanup;
> }
> if (!ret2) {
> @@ -1848,6 +1850,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2 > 0) {
> @@ -1866,6 +1869,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2 > 0) {
> @@ -1899,6 +1903,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> break;
> case 0:
> path_msg(opt, CONFLICT_SUBMODULE_FAILED_TO_MERGE, 0,
> diff --git a/merge-recursive.c b/merge-recursive.c
> index f3132a9ecae..fc772c2b113 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1246,12 +1246,14 @@ static int merge_submodule(struct merge_options *opt,
> ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_a);
> if (ret2 < 0) {
> output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2 > 0)
> ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_b);
> if (ret2 < 0) {
> output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> goto cleanup;
> }
> if (!ret2) {
> @@ -1263,6 +1265,7 @@ static int merge_submodule(struct merge_options *opt,
> ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);
> if (ret2 < 0) {
> output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2) {
> @@ -1281,6 +1284,7 @@ static int merge_submodule(struct merge_options *opt,
> ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
> if (ret2 < 0) {
> output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2) {
> @@ -1312,6 +1316,10 @@ static int merge_submodule(struct merge_options *opt,
> parent_count = find_first_merges(&subrepo, &merges, path,
> commit_a, commit_b);
> switch (parent_count) {
> + case -1:
> + output(opt, 1,_("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> + break;
> case 0:
> output(opt, 1, _("Failed to merge submodule %s (merge following commits not found)"), path);
> break;There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Sat, 9 Mar 2024, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In 24876ebf68b (commit-reach(repo_in_merge_bases_many): report missing
> > commits, 2024-02-28), I taught `merge_submodule()` to handle errors
> > reported by `repo_in_merge_bases_many()`.
> >
> > However, those errors were not passed through to the callers. That was
> > unintentional, and this commit remedies that.
> >
> > Note that `find_first_merges()` can now also return -1 (because it
> > passes through that return value from `repo_in_merge_bases()`), and this
> > commit also adds the forgotten handling for that scenario.
>
> Good clean-up. But this "oops, we did not check for errors" makes
> me wonder if we are better off adopting "by default we assume an
> error, until we are sure we are good" pattern, i.e.
>
> func()
> {
> int ret = -1; /* assume worst */
>
> do stuff;
> if (...) {
> error(_("this is bad"));
> goto cleanup;
> }
> do stuff;
> if (...) {
> error(_("this is bad, too"));
> goto cleanup;
> }
>
> /* ok we are happy */
> ret = 0;
>
> cleanup:
> release resources;
> return ret;
> }
>
> The patch to both functions do make it appear that they are good
> candidates for application of the pattern to me.
This is a very fitting pattern here, and it is in fact used already! It is
used with `ret = 0`, though, to indicate unclean merges.
This makes sense, as most of the specifically-handled cases have that
outcome. By my counting, 5 of the handled cases result in ret = -1, i.e.
non-recoverable errors, but 8 of the cases result in ret = 0, i.e. unclean
merges, whereas only 2 cases return 1, i.e. clean merges.
Those numbers are for the `merge_submodule()` variant in `merge-ort.c`. In
`merge-recursive.c`, by my counting there are 10 instead of 8 `ret = 0`
cases, the other two numbers are the same.
Ciao,
JohannesThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> By my counting, 5 of the handled cases result in ret = -1, i.e.
> non-recoverable errors, but 8 of the cases result in ret = 0,
> i.e. unclean merges, whereas only 2 cases return 1, i.e. clean
> merges.
Sounds good. Thanks.
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Elijah Newren <newren@gmail.com> writes:
>> This is a follow-up for
>> https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,
>> based on the js/merge-base-with-missing-commit branch.
>
> This series looks good to me; thanks.
Thanks, both. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@0c2d15f. |
|
This patch series was integrated into next via git@caa7a7b. |
|
This patch series was integrated into seen via git@7745f92. |
|
This patch series was integrated into master via git@7745f92. |
|
This patch series was integrated into next via git@7745f92. |
|
Closed via 7745f92. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jeff King reported that Coverity pointed out a problem in the patch series "The merge-base logic vs missing commit objects" (which made it into the
nextbranch already): The return value ofmerge_submodules()is assigned to an unsigned, single-bit variable, which as a consequence is not able to hold a negative value indicating a non-recoverable error.I looked into this issue and am happy to report that there are no other instances of the same issue in that patch series. The first patch in this here patch series addresses that issue.
While looking into this issue I also noticed that the
merge_submodule()function did not even return negative values! This was an oversight on my part (which I attribute with a large amount of self-compassion to my utter lack of enthusiasm for submodules as a Git feature), and the second patch in this here patch series addresses that.This is a follow-up for https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/, based on the
js/merge-base-with-missing-commitbranch.cc: Patrick Steinhardt ps@pks.im
cc: Dirk Gouders dirk@gouders.net
cc: Jeff King peff@peff.net
cc: Elijah Newren newren@gmail.com