merge-ort: clean up after failed merge

From: Johannes Schindelin <[email protected]>

In 9fefce68dc8 (merge-ort: basic outline for merge_switch_to_result(), 2020-12-13), we added functionality to lay down the result of a merge on disk. But we forgot to release the data structures in case unpack_trees() failed to run properly.

This was pointed out by the linux-leaks job in our CI runs.

---
merge-ort: clean up after failed merge

I was investigating why seen's CI runs fail, and came up with this fix.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1307%2Fdscho%2Fmerge-ort-impl-leakfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1307/dscho/merge-ort-impl-leakfix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1307

 merge-ort.c | 2 ++
 1 file changed, 2 insertions(+)

index ee7fbe71404..61b9e90018b 100644
@@ -1002,6 +1002,7 @@ void merge_switch_to_result(struct merge_options *opt,
 		if (checkout(opt, head, result->tree)) {
 			/* failure to function */
 			result->clean = -1;
+			merge_finalize(opt, result);
 			return;
 		}
 

@@ -1010,6 +1011,7 @@ void merge_switch_to_result(struct merge_options *opt,
 						    &opti->conflicted)) {
 			/* failure to function */
 			result->clean = -1;
+			merge_finalize(opt, result);
 			return;
 		}
 	}

base-commit: 9fefce68dc85d96781090f86c067d83f7c50b617

gitgitgadget

Good catch. Can you rebase on to a slightly newer commit? I think we also need a trace2_region_leave() call in each block as well, which is only clear if you look at newer versions.

I was investigating why seen's CI runs fail, and came up with this fix.

Changes since v1:

* Rebased onto en/merge-ort-perf.
* Now we're not only cleaning up the merge data structure, but also leaving the Trace2 region when returning early from merge_switch_to_result().

Johannes Schindelin (2):
merge-ort: clean up after failed merge
merge-ort: do leave Trace2 region even if checkout fails

 merge-ort.c | 5 +++++
 1 file changed, 5 insertions(+)

base-commit: 557ac0350d9efa1f59c708779ca3fb3aee121131
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1307%2Fdscho%2Fmerge-ort-impl-leakfix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1307/dscho/merge-ort-impl-leakfix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1307

Range-diff vs v1:

1: 128f77f7f34 ! 1: 082c7ffa41f merge-ort: clean up after failed merge

     @@ merge-ort.c: void merge_switch_to_result(struct merge_options *opt,
      +			merge_finalize(opt, result);
       			return;
       		}
     - 
     + 		trace2_region_leave("merge", "checkout", opt->repo);
      @@ merge-ort.c: void merge_switch_to_result(struct merge_options *opt,
       						    &opti->conflicted)) {
       			/* failure to function */
     @@ merge-ort.c: void merge_switch_to_result(struct merge_options *opt,
      +			merge_finalize(opt, result);
       			return;
       		}
     - 	}
     + 		trace2_region_leave("merge", "record_conflicted", opt->repo);

-: ----------- > 2: d2e1af0f922 merge-ort: do leave Trace2 region even if checkout fails

gitgitgadget

From: Johannes Schindelin <[email protected]>

In 9fefce68dc8 (merge-ort: basic outline for merge_switch_to_result(), 2020-12-13), we added functionality to lay down the result of a merge on disk. But we forgot to release the data structures in case unpack_trees() failed to run properly.

This was pointed out by the linux-leaks job in our CI runs.

---
 merge-ort.c | 2 ++
 1 file changed, 2 insertions(+)

index 931b91438cf..e820e45a8e8 100644
@@ -3157,6 +3157,7 @@ void merge_switch_to_result(struct merge_options *opt,
 		if (checkout(opt, head, result->tree)) {
 			/* failure to function */
 			result->clean = -1;
+			merge_finalize(opt, result);
 			return;
 		}
 		trace2_region_leave("merge", "checkout", opt->repo);

@@ -3167,6 +3168,7 @@ void merge_switch_to_result(struct merge_options *opt,
 						    &opti->conflicted)) {
 			/* failure to function */
 			result->clean = -1;
+			merge_finalize(opt, result);
 			return;
 		}
 		trace2_region_leave("merge", "record_conflicted", opt->repo);

gitgitgadget

From: Johannes Schindelin <[email protected]>

In 557ac0350d9 (merge-ort: begin performance work; instrument with trace2_region_* calls, 2021-01-23), we added Trace2 instrumentation, but in the error path that returns early, we forgot to tell Trace2 that we're leaving the region. Let's fix that.

---
 merge-ort.c | 3 +++
 1 file changed, 3 insertions(+)

index e820e45a8e8..eb0296902ad 100644
@@ -3158,6 +3158,7 @@ void merge_switch_to_result(struct merge_options *opt,
 			/* failure to function */
 			result->clean = -1;
 			merge_finalize(opt, result);
+			trace2_region_leave("merge", "checkout", opt->repo);
 			return;
 		}
 		trace2_region_leave("merge", "checkout", opt->repo);

@@ -3169,6 +3170,8 @@ void merge_switch_to_result(struct merge_options *opt,
 			/* failure to function */
 			result->clean = -1;
 			merge_finalize(opt, result);
+			trace2_region_leave("merge", "record_conflicted",
+					    opt->repo);
 			return;
 		}
 		trace2_region_leave("merge", "record_conflicted", opt->repo);

gitgitgadget

Thanks, series looks good to me:

Thanks, both.

The new "leave" calls immediately next to the existing ones that look identical appear duplicated, but correcting the logic first in an obvious way like the posted patch, leaving any clean-up to later, is a prudent thing to do.

Queued.