Re: [RFC PATCH v2 0/4] developer: support pedantic

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
	git@vger.kernel.org, phillip.wood@dunelm.org.uk,
	gitster@pobox.com, mackyle@gmail.com, sunshine@sunshineco.com
Subject: Re: [RFC PATCH v2 0/4] developer: support pedantic
Date: Wed, 01 Sep 2021 13:27:41 +0200	[thread overview]
Message-ID: <878s0gwmvq.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YS9RieTeJSFmd6M7@coredump.intra.peff.net>

[I should have included this in my just-sent [1], but forgot]

On Wed, Sep 01 2021, Jeff King wrote:

> On Wed, Sep 01, 2021 at 02:19:37AM -0700, Carlo Marcelo Arenas Belón wrote:
>
>> first patch was suggested[1] by Peff, so hopefully my commit message
>> and his assumed SoB are still worth not mixing it with patch 2 (which
>> has a slight different but related focus and touches the same files)
>> but since it is no longer a single patch, lets go wild.
>
> My SoB is fine there (though really Ævar did the actual thinking; I just
> deleted a lot of lines in vim :) ).
>
> Patch 2 looks good to me, though I kind of wonder if it is even worth
> having an option to turn it off.
>
>> patches 3 and 4 are optional and mostly for RFC, so that a solution
>> to any possible issue that the retiring of USE_PARENS_AROUND_GETTEXT_N
>> are addressed.
>
> IMHO the issue it is trying to find is not worth the inevitable problems
> that hacky perl parsing of C will cause (both false positives and
> negatives). Not a statement on your perl code, but just based on
> previous experience.
>
> So I'd probably take the first two patches, and leave the others.

Agreed. Per the rationale in my version of the commit messsage for
Carlo's 1/4 at [1] I don't think this was ever worth it.

I.e. it wasn't even an N_()-specific issue to begin with, but just a
migration from usage() (takes a string) to usage_with_options() (takes
an array of strings).

I just submitted a related series at [2] to fix the alignment of
continued strings containing "\n" in parse-options.c, which is the
reason we need to support "\n"-continued strings at all in
parse-options.c.

So I think (per [1]) that we should just remove
USE_PARENS_AROUND_GETTEXT_N, and that the 3/4 here isn't needed at all
(aside from concerns about parsing C with Perl).

But in the future we needed any assertion for this sort of thing at all
it would be better built on top of my [2]. I.e. parse-options.c could do
some basic sanity checking on the usage array it takes, we'd then end up
detecting the issue USE_PARENS_AROUND_GETTEXT_N was trying to address,
and more (such as the alignment problems I fixed in 1/2 of my [2]).

1. https://lore.kernel.org/git/patch-1.1-d24f1df5d49-20210901T112248Z-avarab@gmail.com
2. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com