On Thu, Dec 30 2021, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
> +static int do_remerge_diff(struct rev_info *opt,
> + struct commit_list *parents,
> + struct object_id *oid,
> + struct commit *commit)
> +{
> + struct merge_options o;
> + struct commit_list *bases;
> + struct merge_result res = {0};
> + struct pretty_print_context ctx = {0};
> + struct commit *parent1 = parents->item;
> + struct commit *parent2 = parents->next->item;
> + struct strbuf parent1_desc = STRBUF_INIT;
> + struct strbuf parent2_desc = STRBUF_INIT;
> +
> + /* Setup merge options */
> + init_merge_options(&o, the_repository);
> + o.show_rename_progress = 0;
> +
> + ctx.abbrev = DEFAULT_ABBREV;
> + format_commit_message(parent1, "%h (%s)", &parent1_desc, &ctx);
> + format_commit_message(parent2, "%h (%s)", &parent2_desc, &ctx);
> + o.branch1 = parent1_desc.buf;
> + o.branch2 = parent2_desc.buf;
> +
> + /* Parse the relevant commits and get the merge bases */
> + parse_commit_or_die(parent1);
> + parse_commit_or_die(parent2);
> + bases = get_merge_bases(parent1, parent2);
There's existing leaks all over the place here unrelated to this new
code, so this is no big deal, but I noticed that get_merge_bases() here
leaks.
Shouldn't it call free_commit_list() like e.g. diff_get_merge_base()
which invokes get_merge_bases() does on the return value?
> +test_description='remerge-diff handling'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup basic merges' '
> + test_write_lines 1 2 3 4 5 6 7 8 9 >numbers &&
> + git add numbers &&
> + git commit -m base &&
> +
> + git branch feature_a &&
> + git branch feature_b &&
> + git branch feature_c &&
> +
> + git branch ab_resolution &&
> + git branch bc_resolution &&
> +
> + git checkout feature_a &&
> + test_write_lines 1 2 three 4 5 6 7 eight 9 >numbers &&
> + git commit -a -m change_a &&
> +
> + git checkout feature_b &&
> + test_write_lines 1 2 tres 4 5 6 7 8 9 >numbers &&
> + git commit -a -m change_b &&
> +
> + git checkout feature_c &&
> + test_write_lines 1 2 3 4 5 6 7 8 9 10 >numbers &&
> + git commit -a -m change_c &&
> +
> + git checkout bc_resolution &&
> + git merge --ff-only feature_b &&
> + # no conflict
> + git merge feature_c &&
> +
> + git checkout ab_resolution &&
> + git merge --ff-only feature_a &&
> + # conflicts!
> + test_must_fail git merge feature_b &&
> + # Resolve conflict...and make another change elsewhere
> + test_write_lines 1 2 drei 4 5 6 7 acht 9 >numbers &&
> + git add numbers &&
Just a matter of taste, but FWIW some of the custom
test_write_lines/commit here could nowadays use test_commit with
--printf: 47c88d16ba6 (test-lib functions: add --printf option to
test_commit, 2021-05-10)
I don't think it's worth the churn to change it here, just an FYI.
