mbox series

[v2,0/2] merge-ort: clean up after failed merge

Message ID pull.1307.v2.git.1659114727.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series merge-ort: clean up after failed merge | expand

Message

Philippe Blain via GitGitGadget July 29, 2022, 5:12 p.m. UTC
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

Comments

Elijah Newren July 30, 2022, 12:50 a.m. UTC | #1
On Fri, Jul 29, 2022 at 10:12 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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(+)

Thanks, series looks good to me:

Reviewed-by: Elijah Newren <newren@gmail.com>
Junio C Hamano July 31, 2022, 6:44 p.m. UTC | #2
Elijah Newren <newren@gmail.com> writes:

> On Fri, Jul 29, 2022 at 10:12 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> 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(+)
>
> Thanks, series looks good to me:
>
> Reviewed-by: Elijah Newren <newren@gmail.com>

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.