On Thu, Feb 2, 2023 at 9:10 AM Kousik Sanagavarapu <five231003@xxxxxxxxx> wrote:
>
> Instead of manually doing an in-place list reversal, use the helper
> function reverse_commit_list(), hence improving code readability.
>
> Signed-off-by: Kousik Sanagavarapu <five231003@xxxxxxxxx>
> ---
>
> This patch addresses the issue #1156(Use reverse_commit_list() in more
> places) on gitgitgadget. I also would like to submit this patch as the microproject for
> GSoC 2023.
Looks like this was a suggestion from me, so I'm to blame. But
looking at it again, I'm not sure builtin/merge.c is actually a good
candidate because...
> builtin/merge.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 5900b81729..4503dbfeb3 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -736,7 +736,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
> struct commit *result;
> struct commit_list *reversed = NULL;
> struct merge_options o;
> - struct commit_list *j;
>
> if (remoteheads->next) {
> error(_("Not handling anything other than two heads merge."));
> @@ -757,8 +756,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
> o.branch1 = head_arg;
> o.branch2 = merge_remote_util(remoteheads->item)->name;
>
> - for (j = common; j; j = j->next)
> - commit_list_insert(j->item, &reversed);
This is not an in-place list reversal; it's creating a new list that
is reversed. "common" can continue to be used separately from
"reversed" after this point. (And the new list, "reversed" is
consumed by merge_recursive()/merge_ort_recursive(), so there's
nothing to free.)
> + reversed = reverse_commit_list(common);
This is an in-place list reversal. If there's only one merge strategy
being attempted, this may work, since "common" probably won't be used
afterwards.
However, if we have multiple merge strategies that expect to be passed
the common commits -- a situation we probably don't have a testcase
for in the testsuite -- then I think this code change will result in a
use-after-free error.
> hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
> if (!strcmp(strategy, "ort"))
> --
> 2.25.1
