"Kevin Backhouse via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Kevin Backhouse <kevinbackhouse@xxxxxxxxxx>
>
> To reproduce (with an ASAN build):
>
> ```
> mkdir test
> cd test
> git init
> echo x > x.txt
> git add .
> git commit -m "WIP"
> git checkout -b dev
> echo y > x.txt
> git add .
> git commit -m "WIP"
> git checkout main
> echo z > x.txt
> git add .
> git commit -m "WIP"
> echo a > x.txt
> git add .
> git merge dev
> ```
We'd rather not to see the above in the proposed log message; can't
we add (a variation of) it to our test suite?
> The fix is to call free_commit_list(merge_bases) when an error occurs.
We usually have the description of what the problem is and give an
analysis on why/how it happens, before presenting a solution. Write
it more like:
The caller of merge_ort_recursive() expects the commit list
passed in as the merge_bases parameter to be fully consumed by
the function and does not free it when the function returns. In
normal cases, the commit list does get consumed, but when the
function returns early upon encountering an error, it forgets to
clean it up.
Fix this by freeing the list in the code paths for error returns.
> merge-ort-wrappers.c | 4 +++-
> merge-ort.c | 4 +++-
These two places and their fixes seem OK, but I have to wonder if
these are complete fixes.
> diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c
> index 4acedf3c338..aeb56c9970c 100644
> --- a/merge-ort-wrappers.c
> +++ b/merge-ort-wrappers.c
> @@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,
> struct tree *head = repo_get_commit_tree(opt->repo, side1);
> struct merge_result tmp;
>
> - if (unclean(opt, head))
> + if (unclean(opt, head)) {
> + free_commit_list(merge_bases);
> return -1;
> + }
>
> memset(&tmp, 0, sizeof(tmp));
> merge_incore_recursive(opt, merge_bases, side1, side2, &tmp);
The function before this hunk appears to have very similar code
structure. Does it need the same fix, or if not why not?
> diff --git a/merge-ort.c b/merge-ort.c
> index 8631c997002..a0eb91fb011 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,
> opt->branch1 = "Temporary merge branch 1";
> opt->branch2 = "Temporary merge branch 2";
> merge_ort_internal(opt, NULL, prev, next, result);
> - if (result->clean < 0)
> + if (result->clean < 0) {
> + free_commit_list(merge_bases);
> return;
> + }
Before this function, there is a comment that this came from another
function and it seems to still have a very similar code structure.
Does the other function need the same fix, or if not why not?
Thanks.
