Re: [PATCH 1/3] usage.c: add BUG() function

From: Jeff King <peff@peff.net>
Date: May 13, 2017, 03:55 UTC
Message-ID: <20170513035503.cubqhzcvhifp54yg@sigill.intra.peff.net>
In-Reply-To: <20170513032850.zeru4cm2l7i23rkc@sigill.intra.peff.net>

On Fri, May 12, 2017 at 11:28:50PM -0400, Jeff King wrote:

Show 13 quoted lines

> +static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params)
> +{
> +	char prefix[256];
> +
> +	/* truncation via snprintf is OK here */
> +	if (file)
> +		snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
> +	else
> +		snprintf(prefix, sizeof(prefix), "BUG: ");
> +
> +	vreportf(prefix, fmt, params);
> +	abort();
> +}

I used vreportf() here to match die(). But the two things that function does are:

  1. Respect error_handle. But after bw/forking-and-threading is merged,
     nobody will ever set error_handle (and I just sent a patch to drop
     it entirely).

  2. Quotes non-printable characters. I don't know how useful this is.
     Most of the assertion messages are pretty vanilla (because anything
     that relies on user input probably should be a regular die, not an
     assertion failure). But a few of them do actually print arbitrary
     strings in a reasonable way (e.g., a BUG() which is handling user
     string which was supposed to be vetted by an earlier function is
     still a reasonable assertion, but it's useful to show the string).

So an alternative would be just:

  fprintf(stderr, "BUG: ");
  if (file)
	fprintf(stderr, "%s:%d ", file, line);
  vfprintf(stderr, fmt, params);
  fputc('\n', stderr);

which is perhaps a bit simpler (not much in lines of code, but there's no extra buffer to reason about). But given the discussion in (2) above, it's probably worth continuing to use vreportf.

-Peff