Re: [PATCH 3/6] pretty: prepare notes message at a centralized place

From: Jeff King <peff@peff.net>
Date: Oct 18, 2012, 07:49 UTC
Message-ID: <20121018074921.GC9999@sigill.intra.peff.net>
In-Reply-To: <1350539128-21577-4-git-send-email-gitster@pobox.com>

On Wed, Oct 17, 2012 at 10:45:25PM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> +	if (opt->show_notes) {
> +		int raw;
> +		struct strbuf notebuf = STRBUF_INIT;
> +
> +		raw = (opt->commit_format == CMIT_FMT_USERFORMAT);
> +		format_display_notes(commit->object.sha1, &notebuf,
> +				     get_log_output_encoding(), raw);
> +		ctx.notes_message = notebuf.len
> +			? strbuf_detach(&notebuf, NULL)
> +			: xcalloc(1, 1);
> +	}

This last line seems like it is caused by a bug in the strbuf API. Detaching an empty string will sometimes get you NULL and sometimes not. For example, this:

  struct strbuf foo = STRBUF_INIT;
  strbuf_detach(&foo, NULL);

will return NULL. But this:

  struct strbuf foo = STRBUF_INIT;
  strbuf_addstr(&foo, "bar");
  strbuf_reset(&foo);
  strbuf_detach(&foo, NULL);

will get you a zero-length string. Which just seems insane to me. The whole point of strbuf_detach is that you do not have to care about the internal representation. It should probably always return a newly allocated zero-length string.

Looking through a few uses of strbuf_detach, it looks like callers assume they will always get a pointer from strbuf_detach, and we are saved by implementation details. For example, sha1_file_to_archive might have an empty file, but the fact that strbuf_attach always allocates a byte means that the detach will never return NULL. Similarly, argv_array_pushf would never want to turn an empty string into an accidental NULL; it is saved by the fact that strbuf_vaddf will always preemptively allocate 64 bytes.

It's possible that switching it would create bugs elsewhere (there are over 100 uses of strbuf_detach, so maybe somebody really does want this NULL behavior), but I tend to think it is just as likely to be fixing undiscovered bugs.

-Peff