git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4 2/2] merge-ort: return early when failing to write a blob

From
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Date
Sep 26, 2022, 21:55 UTC
Message-ID
<087207ae0b0932fcec9aa25e97bbe9227eff81cb.1664229348.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.1362.v4.git.1664229348.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the previous commit, we fixed a segmentation fault when a tree object could not be written.

However, before the tree object is written, `merge-ort` wants to write out a blob object (except in cases where the merge results in a blob that already exists in the database). And this can fail, too, but we ignore that write failure so far.

Since we will always write out a new tree object in addition to the blob (and if the blob did not exist in the database yet, we can be certain that the tree object did not exist yet), the merge will _still_ fail at that point, but it does unnecessary work by continuing after the blob could not be written.

Let's pay close attention and error out early if the blob could not be written. This reduces the error output of t4301.25 ("merge-ort fails gracefully in a read-only repository") from:

	error: insufficient permission for adding an object to repository database ./objects
	error: error: Unable to add numbers to database
	error: insufficient permission for adding an object to repository database ./objects
	error: error: Unable to add greeting to database
	error: insufficient permission for adding an object to repository database ./objects
	fatal: failure to merge
to:
	error: insufficient permission for adding an object to repository database ./objects
	error: error: Unable to add numbers to database
	fatal: failure to merge
Note: This patch adjusts two variable declarations from `unsigned` to
`int` because their purpose is to hold the return value of
`handle_content_merge()`, which is of type `int`. The existing users of
those variables are only interested whether that variable is zero or
non-zero, therefore this type change does not affect the existing code.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 merge-ort.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index f3bdce1041a..e5f41cce481 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2807,6 +2807,8 @@ static int process_renames(struct merge_options *opt,
 							   pathnames,
 							   1 + 2 * opt->priv->call_depth,
 							   &merged);
+			if (clean_merge < 0)
+				return -1;
 			if (!clean_merge &&
 			    merged.mode == side1->stages[1].mode &&
 			    oideq(&merged.oid, &side1->stages[1].oid))
@@ -2916,7 +2918,7 @@ static int process_renames(struct merge_options *opt,
 			struct version_info merged;
 
 			struct conflict_info *base, *side1, *side2;
-			unsigned clean;
+			int clean;
 
 			pathnames[0] = oldpath;
 			pathnames[other_source_index] = oldpath;
@@ -2937,6 +2939,8 @@ static int process_renames(struct merge_options *opt,
 						     pathnames,
 						     1 + 2 * opt->priv->call_depth,
 						     &merged);
+			if (clean < 0)
+				return -1;
 
 			memcpy(&newinfo->stages[target_index], &merged,
 			       sizeof(merged));
@@ -3806,10 +3810,10 @@ static int write_completed_directory(struct merge_options *opt,
 }
 
 /* Per entry merge function */
-static void process_entry(struct merge_options *opt,
-			  const char *path,
-			  struct conflict_info *ci,
-			  struct directory_versions *dir_metadata)
+static int process_entry(struct merge_options *opt,
+			 const char *path,
+			 struct conflict_info *ci,
+			 struct directory_versions *dir_metadata)
 {
 	int df_file_index = 0;
 
@@ -3823,7 +3827,7 @@ static void process_entry(struct merge_options *opt,
 		record_entry_for_tree(dir_metadata, path, &ci->merged);
 		if (ci->filemask == 0)
 			/* nothing else to handle */
-			return;
+			return 0;
 		assert(ci->df_conflict);
 	}
 
@@ -3870,7 +3874,7 @@ static void process_entry(struct merge_options *opt,
 		 */
 		if (ci->filemask == 1) {
 			ci->filemask = 0;
-			return;
+			return 0;
 		}
 
 		/*
@@ -4065,7 +4069,7 @@ static void process_entry(struct merge_options *opt,
 	} else if (ci->filemask >= 6) {
 		/* Need a two-way or three-way content merge */
 		struct version_info merged_file;
-		unsigned clean_merge;
+		int clean_merge;
 		struct version_info *o = &ci->stages[0];
 		struct version_info *a = &ci->stages[1];
 		struct version_info *b = &ci->stages[2];
@@ -4074,6 +4078,8 @@ static void process_entry(struct merge_options *opt,
 						   ci->pathnames,
 						   opt->priv->call_depth * 2,
 						   &merged_file);
+		if (clean_merge < 0)
+			return -1;
 		ci->merged.clean = clean_merge &&
 				   !ci->df_conflict && !ci->path_conflict;
 		ci->merged.result.mode = merged_file.mode;
@@ -4169,6 +4175,7 @@ static void process_entry(struct merge_options *opt,
 
 	/* Record metadata for ci->merged in dir_metadata */
 	record_entry_for_tree(dir_metadata, path, &ci->merged);
+	return 0;
 }
 
 static void prefetch_for_content_merges(struct merge_options *opt,
@@ -4285,7 +4292,10 @@ static int process_entries(struct merge_options *opt,
 			record_entry_for_tree(&dir_metadata, path, mi);
 		else {
 			struct conflict_info *ci = (struct conflict_info *)mi;
-			process_entry(opt, path, ci, &dir_metadata);
+			if (process_entry(opt, path, ci, &dir_metadata) < 0) {
+				ret = -1;
+				goto cleanup;
+			};
 		}
 	}
 	trace2_region_leave("merge", "processing", opt->repo);

-- 
gitgitgadget
Previous: Johannes Schindelin via GitGitGadgetNext: Junio C Hamano
Message 18 of 29 in “merge-tree: fix segmentation fault in read-only repositories”
  1. merge-tree: fix segmentation fault in read-only repositoriesJohannes Schindelin via GitGitGadget, Sep 21, 2022
  2. Taylor BlauSep 21, 2022
  3. Johannes SchindelinSep 21, 2022
  4. Junio C HamanoSep 21, 2022
  5. Johannes SchindelinSep 21, 2022
  6. merge-tree: fix segmentation fault in read-only repositoriesJohannes Schindelin via GitGitGadget, Sep 21, 2022
  7. Junio C HamanoSep 21, 2022
  8. Johannes SchindelinSep 22, 2022
  9. Elijah NewrenSep 21, 2022
  10. Junio C HamanoSep 21, 2022
  11. Johannes SchindelinSep 22, 2022
  12. Johannes SchindelinSep 22, 2022
  13. Elijah NewrenSep 23, 2022
  14. merge-ort: fix segmentation fault in read-only repositoriesJohannes Schindelin via GitGitGadget, Sep 22, 2022
  15. Elijah NewrenSep 23, 2022
  16. Johannes SchindelinSep 23, 2022
  17. 0/2 merge-tree: fix segmentation fault in read-only repositoriesJohannes Schindelin via GitGitGadget, Sep 26, 2022
  18. 2/2 merge-ort: return early when failing to write a blobJohannes Schindelin via GitGitGadget, Sep 26, 2022
  19. Junio C HamanoSep 26, 2022
  20. Elijah NewrenSep 27, 2022
  21. Junio C HamanoSep 27, 2022
  22. 1/2 merge-ort: fix segmentation fault in read-only repositoriesJohannes Schindelin via GitGitGadget, Sep 26, 2022
  23. Elijah NewrenSep 27, 2022
  24. 0/2 merge-tree: fix segmentation fault in read-only repositoriesJohannes Schindelin via GitGitGadget, Sep 28, 2022
  25. 1/2 merge-ort: fix segmentation fault in read-only repositoriesJohannes Schindelin via GitGitGadget, Sep 28, 2022
  26. 2/2 merge-ort: return early when failing to write a blobJohannes Schindelin via GitGitGadget, Sep 28, 2022
  27. Junio C HamanoSep 28, 2022
  28. Elijah NewrenSep 29, 2022
  29. Junio C HamanoSep 29, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.