Re: [PATCH 3/3] ref-filter: free item->value and item->value->s

[Jeff King <peff@peff.net>]

On Thu, Oct 11, 2018 at 10:19:22AM +0900, Junio C Hamano wrote:

> > @@ -1373,36 +1379,31 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> >  		}
> >  	} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
> >  		if (stat_tracking_info(branch, &num_ours, &num_theirs,
> > -				       NULL, AHEAD_BEHIND_FULL) < 0)
> > +				       NULL, AHEAD_BEHIND_FULL) < 0) {
> > +			*s = xstrdup("");
> >  			return;
> 
> It is a bit sad that we need to sprinkle xstrdup() all over the
> place, but I do not offhand think of a better alternative to ensure
> that it is safe to blindly free the .s field.

I think the root of the problem is that the current code needs
_something_ in the "s" field to know that the value has already been
filled in.

If there were a separate flag for "filled", then this could just be
NULL (and the later code that builds the output would have to realize
to replace that with an empty string).

I think in the long run, we'd ideally have one of two code structures:

  - a single pass, without iterating over the used atoms list
    repeatedly. E.g., the way oid_object_info() takes a struct
    representing the set of items that the caller is interested in, and
    then fills it in as it digs for information.

  - individual atoms could write directly to an output strbuf, avoiding
    the extra allocation and copy altogether (that would help these
    cases that are just copying an empty string, but also the ones that
    really are allocating a piece of data and end up copying it.

I'm OK with this approach in the meantime, though. There's a fair bit of
refactoring to get to either of those end-states, and this clears up the
confusing memory ownership issues. And I don't think for-each-ref is
_too_ sensitive to a few extra mallocs (as compared to say, cat-file's
formatter, which is often run once per object).

I didn't dig into the valgrind errors you saw, but I'd guess they are
the result of this patch missing one of these cases (if not a string
literal like "", perhaps a const pointer into another heap string).

-Peff