Re: [PATCH 2/3] commit: add function to unparse a commit and its parents

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 25, 2009, 09:46 UTC
Message-ID: <alpine.DEB.1.00.0905251140320.4288@intel-tinevez-2-302>
In-Reply-To: <4A1A621E.10703@viscovery.net>

Hi,

On Mon, 25 May 2009, Johannes Sixt wrote:

Show 23 quoted lines

> Christian Couder schrieb:
> > +static void unparse_commit_list(struct commit_list *list)
> > +{
> > +	for (; list; list = list->next)
> > +		unparse_commit(list->item);
> > +}
> > +
> > +void unparse_commit(struct commit *item)
> > +{
> > +	item->object.flags = 0;
> > +	item->object.used = 0;
> > +	if (item->object.parsed) {
> > +		item->object.parsed = 0;
> > +		if (item->parents) {
> > +			unparse_commit_list(item->parents);
> > +			free_commit_list(item->parents);
> > +			item->parents = NULL;
> > +		}
> > +	}
> > +}
> 
> I see a recursion here. Could this not overflow the stack if there is a
> long ancestry chain?

You mean tail recursion, i.e. something like

void unparse_commit(struct commit *item)
{
	item->object.flags = 0;
	item->object.used = 0;
	while (item->object.parsed) {
		struct commit *first;

		item->object.parsed = 0;
		if (!item->parents)
			break;
		if (item->parents->next)
			unparse_commit_list(item->parents->next);
		first = item->parents->item;
		free_commit_list(item->parents);
		item->parents = NULL;
		item = first;
	}
}

However, I am a bit concerned that this function is dangerous, as it just assumes that there is no reference to the commits left, which assumption is _very_ easy to break by mistake.

Ciao, Dscho