Subject: Re: [PATCH 08/15] name-rev: pull out deref handling from the recursion

From: SZEDER Gábor <[email-masked]>
Date: Fri, 20 Sep 2019 19:42:13 +0200

On Fri, Sep 20, 2019 at 11:21:53AM -0400, Derrick Stolee wrote:
> On 9/19/2019 5:47 PM, SZEDER Gábor wrote:
> > The 'if (deref) { ... }' condition near the beginning of the recursive
> > name_rev() function can only ever be true in the first invocation,
> > because the 'deref' parameter is always 0 in the subsequent recursive
> > invocations.
> > 
> > Extract this condition from the recursion into name_rev()'s caller and
> > drop the function's 'deref' parameter.  This makes eliminating the
> > recursion a bit easier to follow, and it will be moved back into
> > name_rev() after the recursion is elminated.

s/elminated/eliminated/

> > Furthermore, drop the condition that die()s when both 'deref' and
> > 'generation' are non-null (which should have been a BUG() to begin
> > with).
> 
> These changes seem sensible. I look forward to seeing how deref is
> reintroduced.
> 
> > Note that this change reintroduces the memory leak that was plugged in
> > in commit 5308224633 (name-rev: avoid leaking memory in the `deref`
> > case, 2017-05-04), but a later patch in this series will plug it in
> > again.
> 
> The memory leak is now for "tip_name" correct? Just tracking to make
> sure it gets plugged later.

Yes, it's 'tip_name' (the one returned by xstrfmt()).

> > -	if (deref) {
> > -		tip_name = to_free = xstrfmt("%s^0", tip_name);

> > +		if (deref)
> > +			tip_name = xstrfmt("%s^0", path);
> > +		else
> > +			tip_name = xstrdup(path);