Re: [PATCH 2/2] log: do not shorten decoration names too early

From: Jeff King <peff@peff.net>
Date: May 14, 2015, 22:33 UTC
Message-ID: <20150514223313.GA21149@peff.net>
In-Reply-To: <xmqq7fsaiyoy.fsf@gitster.dls.corp.google.com>

On Thu, May 14, 2015 at 03:25:33PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> @@ -90,6 +97,8 @@ static MAYBE_UNUSED elemtype *slabname## _at(struct slabname *s,	\
>  									\
>  	if (s->slab_count <= nth_slab) {				\
>  		int i;							\
> +		if (!add_if_missing)					\
> +			return NULL;					\
>  		s->slab = xrealloc(s->slab,				\
>  				   (nth_slab + 1) * sizeof(*s->slab));	\
>  		stat_ ##slabname## realloc++;				\

This skips extending the list of slabs if we would go past the nth slab. But we don't fill in each slab in the first place. I.e., we may have 10 slabs, but only s->slab[10] is non-NULL.

A few lines below this, we xcalloc() it if necessary. I think that needs to respect add_if_missing, as well.

Show 8 quoted lines

>  void unuse_commit_buffer(const struct commit *commit, const void *buffer)
>  {
> -	struct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);
> -	if (v->buffer != buffer)
> +	struct commit_buffer *v = buffer_slab_peek(&buffer_slab, commit);
> +	if (v && v->buffer != buffer)
>  		free((void *)buffer);
>  }

I think you want:

  if (!v || v->buffer != buffer)

here. IOW, we free it only if it is not our cached buffer, and it cannot be if we do not have a cached buffer. It may be easier to read by flipping the logic:

  if (v && v->buffer == buffer)
	return; /* it is saved in the cache */
  free((void *)buffer);

Or some variation on that.

-Peff