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

From: Jeff King <peff@peff.net>
Date: Oct 12, 2018, 19:09 UTC
Message-ID: <20181012190920.GD4917@sigill.intra.peff.net>
In-Reply-To: <xmqqzhvli8kl.fsf@gitster-ct.c.googlers.com>

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

Show 12 quoted lines

> > @@ -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