diff mbox series

[v2,7/7] merge-ort: convert more error() cases to path_msg()

Message ID 500433edf49a4df448b330e4ed9201cfac83cecf.1718766019.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit f19b9165351a4058832bb43560178474c7501925
Headers show
Series Fix and improve some error codepaths in merge-ort | expand

Commit Message

Elijah Newren June 19, 2024, 3 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

merge_submodule() stores errors using path_msg(), whereas other call
sites make use of the error() function.  This is inconsistent, and
moving towards path_msg() seems more friendly for libification efforts
since it will allow the caller to determine whether the error messages
need to be printed.

Note that this deferred handling of error messages changes the error
message in a recursive merge from
  error: failed to execute internal merge
to
  From inner merge:  error: failed to execute internal merge
which provides a little more information about the error which may be
useful.  Since the recursive merge strategy still only shows the older
error, we had to adjust the new testcase introduced a few commits ago to
just search for the older message somewhere in the output.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c           | 53 +++++++++++++++++++++++++++++++++----------
 t/t6406-merge-attr.sh |  2 +-
 2 files changed, 42 insertions(+), 13 deletions(-)

Comments

Jeff King July 2, 2024, 9:33 p.m. UTC | #1 
On Wed, Jun 19, 2024 at 03:00:19AM +0000, Elijah Newren via GitGitGadget wrote:

> +static int read_oid_strbuf(struct merge_options *opt,
> +			   const struct object_id *oid,
> +			   struct strbuf *dst,
> +			   const char *path)
>  {
>  	void *buf;
>  	enum object_type type;
>  	unsigned long size;
>  	buf = repo_read_object_file(the_repository, oid, &type, &size);
> -	if (!buf)
> -		return error(_("cannot read object %s"), oid_to_hex(oid));
> +	if (!buf) {
> +		path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
> +			 path, NULL, NULL, NULL,
> +			 _("error: cannot read object %s"), oid_to_hex(oid));
> +		return -1;
> +	}
>  	if (type != OBJ_BLOB) {
>  		free(buf);
> -		return error(_("object %s is not a blob"), oid_to_hex(oid));
> +		path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
> +			 path, NULL, NULL, NULL,
> +			 _("error: object %s is not a blob"), oid_to_hex(oid));
>  	}
>  	strbuf_attach(dst, buf, size, size + 1);
>  	return 0;

This loses the early return in the "type != OBJ_BLOB" code path. So we
free(buf), but then continue on to the strbuf_attach() call on the
dangling pointer. Should it "return -1" like the earlier conditional?

-Peff
Elijah Newren July 3, 2024, 3:48 p.m. UTC | #2 
On Tue, Jul 2, 2024 at 2:33 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jun 19, 2024 at 03:00:19AM +0000, Elijah Newren via GitGitGadget wrote:
>
> > +static int read_oid_strbuf(struct merge_options *opt,
> > +                        const struct object_id *oid,
> > +                        struct strbuf *dst,
> > +                        const char *path)
> >  {
> >       void *buf;
> >       enum object_type type;
> >       unsigned long size;
> >       buf = repo_read_object_file(the_repository, oid, &type, &size);
> > -     if (!buf)
> > -             return error(_("cannot read object %s"), oid_to_hex(oid));
> > +     if (!buf) {
> > +             path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
> > +                      path, NULL, NULL, NULL,
> > +                      _("error: cannot read object %s"), oid_to_hex(oid));
> > +             return -1;
> > +     }
> >       if (type != OBJ_BLOB) {
> >               free(buf);
> > -             return error(_("object %s is not a blob"), oid_to_hex(oid));
> > +             path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
> > +                      path, NULL, NULL, NULL,
> > +                      _("error: object %s is not a blob"), oid_to_hex(oid));
> >       }
> >       strbuf_attach(dst, buf, size, size + 1);
> >       return 0;
>
> This loses the early return in the "type != OBJ_BLOB" code path. So we
> free(buf), but then continue on to the strbuf_attach() call on the
> dangling pointer. Should it "return -1" like the earlier conditional?
>
> -Peff

Oops!  That's embarrassing.  Thanks for catching; I'll send in a patch
on top since this is already in next.
Junio C Hamano July 3, 2024, 6:35 p.m. UTC | #3 
Elijah Newren <newren@gmail.com> writes:

> Oops!  That's embarrassing.  Thanks for catching; I'll send in a patch
> on top since this is already in next.

Thanks.
Jeff King July 6, 2024, 6:11 a.m. UTC | #4 
On Wed, Jul 03, 2024 at 08:48:59AM -0700, Elijah Newren wrote:

> > >       if (type != OBJ_BLOB) {
> > >               free(buf);
> > > -             return error(_("object %s is not a blob"), oid_to_hex(oid));
> > > +             path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
> > > +                      path, NULL, NULL, NULL,
> > > +                      _("error: object %s is not a blob"), oid_to_hex(oid));
> > >       }
> > >       strbuf_attach(dst, buf, size, size + 1);
> > >       return 0;
> >
> > This loses the early return in the "type != OBJ_BLOB" code path. So we
> > free(buf), but then continue on to the strbuf_attach() call on the
> > dangling pointer. Should it "return -1" like the earlier conditional?
> 
> Oops!  That's embarrassing.  Thanks for catching; I'll send in a patch
> on top since this is already in next.

Yeah, I only caught it because Coverity flagged it when it hit 'next'.
It is quite subtle. :)

-Peff
diff mbox series

Patch

diff --git a/merge-ort.c b/merge-ort.c
index b337e4d74ef..8dfe80f1009 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -558,6 +558,10 @@  enum conflict_and_info_types {
 	 * Keep this group _last_ other than NB_TOTAL_TYPES
 	 */
 	ERROR_SUBMODULE_CORRUPT,
+	ERROR_THREEWAY_CONTENT_MERGE_FAILED,
+	ERROR_OBJECT_WRITE_FAILED,
+	ERROR_OBJECT_READ_FAILED,
+	ERROR_OBJECT_NOT_A_BLOB,
 
 	/* Keep this entry _last_ in the list */
 	NB_TOTAL_TYPES,
@@ -615,6 +619,14 @@  static const char *type_short_descriptions[] = {
 	/* Something is seriously wrong; cannot even perform merge */
 	[ERROR_SUBMODULE_CORRUPT] =
 		"ERROR (submodule corrupt)",
+	[ERROR_THREEWAY_CONTENT_MERGE_FAILED] =
+		"ERROR (three-way content merge failed)",
+	[ERROR_OBJECT_WRITE_FAILED] =
+		"ERROR (object write failed)",
+	[ERROR_OBJECT_READ_FAILED] =
+		"ERROR (object read failed)",
+	[ERROR_OBJECT_NOT_A_BLOB] =
+		"ERROR (object is not a blob)",
 };
 
 struct logical_conflict_info {
@@ -2190,15 +2202,24 @@  static int handle_content_merge(struct merge_options *opt,
 					  pathnames, extra_marker_size,
 					  &result_buf);
 
-		if ((merge_status < 0) || !result_buf.ptr)
-			ret = error(_("failed to execute internal merge"));
+		if ((merge_status < 0) || !result_buf.ptr) {
+			path_msg(opt, ERROR_THREEWAY_CONTENT_MERGE_FAILED, 0,
+				 pathnames[0], pathnames[1], pathnames[2], NULL,
+				 _("error: failed to execute internal merge for %s"),
+				 path);
+			ret = -1;
+		}
 
 		if (!ret &&
 		    write_object_file(result_buf.ptr, result_buf.size,
-				      OBJ_BLOB, &result->oid))
-			ret = error(_("unable to add %s to database"), path);
-
+				      OBJ_BLOB, &result->oid)) {
+			path_msg(opt, ERROR_OBJECT_WRITE_FAILED, 0,
+				 pathnames[0], pathnames[1], pathnames[2], NULL,
+				 _("error: unable to add %s to database"), path);
+			ret = -1;
+		}
 		free(result_buf.ptr);
+
 		if (ret)
 			return -1;
 		if (merge_status > 0)
@@ -3577,18 +3598,26 @@  static int sort_dirs_next_to_their_children(const char *one, const char *two)
 		return c1 - c2;
 }
 
-static int read_oid_strbuf(const struct object_id *oid,
-			   struct strbuf *dst)
+static int read_oid_strbuf(struct merge_options *opt,
+			   const struct object_id *oid,
+			   struct strbuf *dst,
+			   const char *path)
 {
 	void *buf;
 	enum object_type type;
 	unsigned long size;
 	buf = repo_read_object_file(the_repository, oid, &type, &size);
-	if (!buf)
-		return error(_("cannot read object %s"), oid_to_hex(oid));
+	if (!buf) {
+		path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
+			 path, NULL, NULL, NULL,
+			 _("error: cannot read object %s"), oid_to_hex(oid));
+		return -1;
+	}
 	if (type != OBJ_BLOB) {
 		free(buf);
-		return error(_("object %s is not a blob"), oid_to_hex(oid));
+		path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
+			 path, NULL, NULL, NULL,
+			 _("error: object %s is not a blob"), oid_to_hex(oid));
 	}
 	strbuf_attach(dst, buf, size, size + 1);
 	return 0;
@@ -3612,8 +3641,8 @@  static int blob_unchanged(struct merge_options *opt,
 	if (oideq(&base->oid, &side->oid))
 		return 1;
 
-	if (read_oid_strbuf(&base->oid, &basebuf) ||
-	    read_oid_strbuf(&side->oid, &sidebuf))
+	if (read_oid_strbuf(opt, &base->oid, &basebuf, path) ||
+	    read_oid_strbuf(opt, &side->oid, &sidebuf, path))
 		goto error_return;
 	/*
 	 * Note: binary | is used so that both renormalizations are
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index b6db5c2cc36..9bf95249347 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -295,7 +295,7 @@  test_expect_success !WINDOWS 'custom merge driver that is killed with a signal o
 	>./please-abort &&
 	echo "* merge=custom" >.gitattributes &&
 	test_expect_code 2 git merge recursive-a 2>err &&
-	grep "^error: failed to execute internal merge" err &&
+	grep "error: failed to execute internal merge" err &&
 	git ls-files -u >output &&
 	git diff --name-only HEAD >>output &&
 	test_must_be_empty output