From: Jeff King <peff@peff.net>
Date: Tue, 05 Jul 2016 20:28:20 GMT
Subject: Re: [PATCH] revert: clarify seemingly bogus OPT_END repetition
Message-ID: <20160705202820.GA14496@sigill.intra.peff.net>
In-Reply-To: <5b424bad41ca027b39eea4b1fa9d87df0a489e0f.1467719888.git.johannes.schindelin@gmx.de>

On Tue, Jul 05, 2016 at 01:58:51PM +0200, Johannes Schindelin wrote:

> This developer stumbled over repeated OPT_END entries and was *so
> close* (almost touches his thumb with his index finger) to collapse
> them into a single one. Only inspecting the file's history with
> `git log -p -SOPT_END` clarified why they are there.

Wow, that's really ugly, and confused me, too.

I've been trying to move us away from this kind of manually-computed
array size, simply because it's error-prone and often not obviously
correct[1].

I wonder if parse_options_concat should simply allocate a new list
(after computing the total required size). I guess this is the only
caller, though, so perhaps it's not the end of the world. In the
meantime, your patch is certainly an improvement.

By the way, I notice that the error message when concat fails is just:

  if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
	die(_("program error"));

Should this become:

        die("BUG: not enough room to concatenate options");

as part of your BUG cleanups elsewhere?

-Peff

[1] At least the concat interface takes ARRAY_SIZE(), so that we will
    catch an error, rather than silently causing memory corruption. A
    much more likely error is to forget OPT_END(), which will cause
    parse_options to read past the end of the array.