From: Patrick Steinhardt <ps@pks.im>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org,
"Carlos Andrés Ramírez Cataño" <antaigroupltda@gmail.com>
Subject: Re: [PATCH 0/7] fix segfaults with implicit-bool config
Date: Tue, 12 Dec 2023 05:10:01 +0100 [thread overview]
Message-ID: <ZXfdGYUyJz4bnkQ0@tanuki> (raw)
In-Reply-To: <20231212005228.GB376323@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1338 bytes --]
On Mon, Dec 11, 2023 at 07:52:28PM -0500, Jeff King wrote:
> On Thu, Dec 07, 2023 at 09:14:36AM +0100, Patrick Steinhardt wrote:
>
> > Thanks for working on this topic! I've left a couple of comments, most
> > of which are about whether we should retain previous behaviour where we
> > generate a warning instead of raising an error for unknown values.
>
> Thanks for taking a look. I see what you're saying about the warnings,
> but IMHO it's not worth the extra complexity. Returning early means the
> existing code can proceed without worrying about NULLs. Though I suppose
> we could have a "warn_error_nonbool()" which issues a warning and
> returns 0.
>
> Still, the "return config_error_nonbool()" pattern is pretty
> well-established and used for most options. I would go so far as to say
> the ones that warn for invalid values are the odd ones out, and probably
> should be returning errors as well (though doing so now may not be worth
> the trouble and risk of annoyance).
>
> And certainly there should be no regressions in this series; every case
> is currently a segfault, so returning an error is a strict improvement.
> I'd just as soon stay strict there, as it's easier to loosen later if we
> choose than to tighten.
Fair enough, I'm perfectly fine with this reasoning. Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
prev parent reply other threads:[~2023-12-12 4:10 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-07 7:10 [PATCH 0/7] fix segfaults with implicit-bool config Jeff King
2023-12-07 7:11 ` [PATCH 1/7] config: handle NULL value when parsing non-bools Jeff King
2023-12-07 8:14 ` Patrick Steinhardt
2023-12-12 0:58 ` Jeff King
2023-12-07 7:11 ` [PATCH 2/7] setup: handle NULL value when parsing extensions Jeff King
2023-12-07 7:11 ` [PATCH 3/7] trace2: handle NULL values in tr2_sysenv config callback Jeff King
2023-12-07 7:11 ` [PATCH 4/7] help: handle NULL value for alias.* config Jeff King
2023-12-07 7:11 ` [PATCH 5/7] submodule: handle NULL value when parsing submodule.*.branch Jeff King
2023-12-07 8:14 ` Patrick Steinhardt
2023-12-12 0:46 ` Jeff King
2023-12-07 7:11 ` [PATCH 6/7] trailer: handle NULL value when parsing trailer-specific config Jeff King
2023-12-07 8:14 ` Patrick Steinhardt
2023-12-07 7:11 ` [PATCH 7/7] fsck: handle NULL value when parsing message config Jeff King
2023-12-07 8:15 ` Patrick Steinhardt
2023-12-07 8:14 ` [PATCH 0/7] fix segfaults with implicit-bool config Patrick Steinhardt
2023-12-12 0:52 ` Jeff King
2023-12-12 4:10 ` Patrick Steinhardt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZXfdGYUyJz4bnkQ0@tanuki \
--to=ps@pks.im \
--cc=antaigroupltda@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).