Re: [PATCH] reduce_heads: fix memory leaks

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 2, 2017, 03:11 UTC
Message-ID: <xmqq7ev9s7bp.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171101090326.8091-1-martin.agren@gmail.com>

Martin Ågren <martin.agren@gmail.com> writes:

Show 12 quoted lines

> 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?

Show 15 quoted lines

> 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.

Show 26 quoted lines

> 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.