From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 19 Aug 2016 20:24:20 GMT
Subject: Re: [PATCH v8 6/8] submodule: refactor show_submodule_summary with helper function
Message-ID: <xmqqk2fc1p8r.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160819000031.24854-7-jacob.e.keller@intel.com>

Jacob Keller <jacob.e.keller@intel.com> writes:

> @@ -290,12 +289,6 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,
>  	add_pending_object(rev, &left->object, path);
>  	add_pending_object(rev, &right->object, path);
>  	merge_bases = get_merge_bases(left, right);
> -	if (merge_bases) {
> -		if (merge_bases->item == left)
> -			*fast_forward = 1;
> -		else if (merge_bases->item == right)
> -			*fast_backward = 1;
> -	}
>  	for (list = merge_bases; list; list = list->next) {
>  		list->item->object.flags |= UNINTERESTING;
>  		add_pending_object(rev, &list->item->object,

Not a new issue with this patch, but I wonder if this commit_list is
leaking here.

> +	/*
> +	 * Warn about missing commits in the submodule project, but only if
> +	 * they aren't null.
> +	 */
> +	if ((!is_null_oid(one) && !*left) ||
> +	     (!is_null_oid(two) && !*right))
> +		message = "(commits not present)";
> +
> +	merge_bases = get_merge_bases(*left, *right);
> +	if (merge_bases) {
> +		if (merge_bases->item == *left)
> +			fast_forward = 1;
> +		else if (merge_bases->item == *right)
> +			fast_backward = 1;
> +	}

And probably merge_bases also leaks here.

It is not cheap to compute merge bases, but show_submodule_summary()
makes two calls to get_merge_bases(), one in show_submodule_header()
and then another inside prepare_submodule_summary() to compute
exactly the same set of merge bases.  We somehow need to reduce it
to just one.