Re: [PATCH v3 1/9] show, log: provide a --remerge-diff capability
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 19, 2022, 15:49 UTC
- Message-ID
- <220119.86pmonn2mb.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <d57ae218cf9eaee0b66db299ee1bba9b488b69b1.1640907369.git.gitgitgadget@gmail.com>
On Thu, Dec 30 2021, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
Show 28 quoted lines
> +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?
Show 40 quoted lines
> +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.