Martin Ågren <martin.agren@xxxxxxxxx> writes:
> diff --git a/builtin/merge-base.c b/builtin/merge-base.c
> index 6dbd167d3..b1b7590c4 100644
> --- a/builtin/merge-base.c
> +++ b/builtin/merge-base.c
> @@ -59,6 +59,8 @@ static int handle_independent(int count, const char **args)
> commit_list_insert(get_commit_reference(args[i]), &revs);
>
> result = reduce_heads(revs);
> + free_commit_list(revs);
> +
> if (!result)
> return 1;
The post-context of this hunk continues like so:
while (result) {
printf("%s\n", oid_to_hex(&result->item->object.oid));
result = result->next;
}
return 0;
}
and we end up leaking "result". This function is directly called from
cmd_merge_base() and its value is returned to main(), so leaking it
is not that a grave offence, but that excuse applies equally well to
revs.
I can see you are shooting for minimum change in this patch, but if
we were writing this code in a codebase where reduce_heads_replace()
is already available, I would imagine that we wouldn't use two separate
variables, perhaps?
> diff --git a/commit.c b/commit.c
> index 1e0e63379..cab8d4455 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1090,6 +1090,13 @@ struct commit_list *reduce_heads(struct commit_list *heads)
> return result;
> }
>
> +void reduce_heads_replace(struct commit_list **heads)
> +{
> + struct commit_list *result = reduce_heads(*heads);
> + free_commit_list(*heads);
> + *heads = result;
> +}
> +
Looks good.
> diff --git a/commit.h b/commit.h
> index 6d769590f..99a3fea68 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -313,7 +313,23 @@ extern int interactive_add(int argc, const char **argv, const char *prefix, int
> extern int run_add_interactive(const char *revision, const char *patch_mode,
> const struct pathspec *pathspec);
>
> -struct commit_list *reduce_heads(struct commit_list *heads);
> +/*
> + * Takes a list of commits and returns a new list where those
> + * have been removed that can be reached from other commits in
> + * the list. It is useful for, e.g., reducing the commits
> + * randomly thrown at the git-merge command and removing
> + * redundant commits that the user shouldn't have given to it.
> + *
> + * This function destroys the STALE bit of the commit objects'
> + * flags.
> + */
> +extern struct commit_list *reduce_heads(struct commit_list *heads);
> +
> +/*
> + * Like `reduce_heads()`, except it replaces the list. Use this
> + * instead of `foo = reduce_heads(foo);` to avoid memory leaks.
> + */
> +extern void reduce_heads_replace(struct commit_list **heads);
Looks excellent.
Thanks.
