From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Fri, 20 Sep 2019 18:13:02 GMT
Subject: Re: [PATCH 08/15] name-rev: pull out deref handling from the recursion
Message-ID: <20190920181302.GA26402@szeder.dev>
In-Reply-To: <5bf3b958-8c95-ae3d-1ae5-df3d57272d8c@web.de>

> > @@ -280,12 +269,16 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
> >  	if (o && o->type == OBJ_COMMIT) {
> >  		struct commit *commit = (struct commit *)o;
> >  		int from_tag = starts_with(path, "refs/tags/");
> > +		const char *tip_name;
> 
> This should not be const because you allocate the buffer it points to
> right here in the function, in each execution path.

Marking it as const indicates that this function doesn't modify the
buffer where the pointer points at.

> >
> >  		if (taggerdate == TIME_MAX)
> >  			taggerdate = commit->date;
> >  		path = name_ref_abbrev(path, can_abbreviate_output);
> > -		name_rev(commit, xstrdup(path), taggerdate, 0, 0,
> > -			 from_tag, deref);
> > +		if (deref)
> > +			tip_name = xstrfmt("%s^0", path);
> > +		else
> > +			tip_name = xstrdup(path);
> > +		name_rev(commit, tip_name, taggerdate, 0, 0, from_tag);
> 
> tip_name should be free(3)'d here.  Except we can't do that because
> name_rev() sometimes stores that pointer in a commit slab.  Ugh.
> 
> If the (re)introduced leak doesn't impact performance and memory
> usage too much then duplicating tip_name again in name_rev() or
> rather your new create_or_update_name() would likely make the
> lifetimes of those string buffers easier to manage.

Yeah, the easiest would be when each 'struct rev_name' in the commit
slab would have its own 'tip_name' string, but that would result in
a lot of duplicated strings and increased memory usage.