git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Yonatan Roth <yroth@paloaltonetworks.com>,
	david asraf <dasraf9@gmail.com>,
	Emily Shaffer <nasamuffin@google.com>,
	Ramsay Jones <ramsay@ramsayjones.plus.com>,
	Ben Knoble <ben.knoble@gmail.com>
Subject: Re: [PATCH v3 10/12] usage: allow dying without writing an error message
Date: Thu, 5 Jun 2025 15:02:39 +0200	[thread overview]
Message-ID: <aEGVbz7fWmm3yf1L@pks.im> (raw)
In-Reply-To: <xmqqmsaoq244.fsf@gitster.g>

On Tue, Jun 03, 2025 at 08:24:59AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Sometimes code wants to die in a situation where it already has written
> > an error message. To use the same error code as `die()` we have to open
> > code the code with a call to `exit(128)` in such cases, which is easy to
> > get wrong and leaves magical numbers all over our codebase.
> > ...
> >  	if (!patch_format) {
> >  		fprintf_ln(stderr, _("Patch format detection failed."));
> > -		exit(128);
> > +		die(NULL);
> 
> It is somewhat surprising that the compiler would not complain with
> "Hey, a NULL string as printf format string???" given its decl.
> 
> NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
> 
> As long as we are sure that compilers we care about are OK with
> this, it is a very nice ergonomics enhancement.

Hard to say. The documentation of -Wformat doesn't explicitly mention
what is expected to happen in that case. I tried compiling with
-Wformat=2, and that didn't generate any warnings for those calls to
`die(NULL)`. It did generate a bunch of unrelated warnings though.

I haven't seen any issues in our CI, either.

So I'm inclined to just go with this variant for now. If we ever see
that it does cause problems with some compiler it's trivial to just
`s/die(NULL)/die_silent()` or something similar.

Patrick

  reply	other threads:[~2025-06-05 13:02 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-05-27 14:04 [PATCH 00/11] builtin/maintenance: fix ref lock races when detaching Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 01/11] builtin/gc: use designated field initializers for maintenance tasks Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 02/11] builtin/gc: drop redundant local variable Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 03/11] builtin/maintenance: centralize configuration of explicit tasks Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 04/11] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible Patrick Steinhardt
2025-05-27 16:43   ` Ramsay Jones
2025-05-28  7:02     ` Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 05/11] builtin/maintenance: stop modifying global array of tasks Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 06/11] builtin/maintenance: extract function to run tasks Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 07/11] builtin/maintenance: fix typedef for function pointers Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 08/11] builtin/maintenance: let tasks do maintenance before and after detach Patrick Steinhardt
2025-05-27 17:01   ` Ramsay Jones
2025-05-28  7:02     ` Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 09/11] builtin/maintenance: fix locking race when packing refs and reflogs Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()` Patrick Steinhardt
2025-05-27 14:04 ` [PATCH 11/11] builtin/maintenance: fix locking race when handling "gc" task Patrick Steinhardt
2025-05-30 15:08 ` [PATCH v2 00/12] builtin/maintenance: fix ref lock races when detaching Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 01/12] builtin/gc: use designated field initializers for maintenance tasks Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 02/12] builtin/gc: drop redundant local variable Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 03/12] builtin/maintenance: centralize configuration of explicit tasks Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 04/12] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 05/12] builtin/maintenance: stop modifying global array of tasks Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 06/12] builtin/maintenance: extract function to run tasks Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 07/12] builtin/maintenance: fix typedef for function pointers Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 08/12] builtin/maintenance: let tasks do maintenance before and after detach Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 09/12] builtin/maintenance: fix locking race when packing refs and reflogs Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 10/12] usage: allow dying without writing an error message Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 11/12] builtin/gc: avoid global state in `gc_before_repack()` Patrick Steinhardt
2025-05-30 15:08   ` [PATCH v2 12/12] builtin/maintenance: fix locking race when handling "gc" task Patrick Steinhardt
2025-06-02  7:17 ` [PATCH v3 00/12] builtin/maintenance: fix ref lock races when detaching Patrick Steinhardt
2025-06-02  7:17   ` [PATCH v3 01/12] builtin/gc: use designated field initializers for maintenance tasks Patrick Steinhardt
2025-06-02 10:35     ` Karthik Nayak
2025-06-02  7:17   ` [PATCH v3 02/12] builtin/gc: drop redundant local variable Patrick Steinhardt
2025-06-02  7:17   ` [PATCH v3 03/12] builtin/maintenance: centralize configuration of explicit tasks Patrick Steinhardt
2025-06-02  7:17   ` [PATCH v3 04/12] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible Patrick Steinhardt
2025-06-02  7:17   ` [PATCH v3 05/12] builtin/maintenance: stop modifying global array of tasks Patrick Steinhardt
2025-06-02  7:17   ` [PATCH v3 06/12] builtin/maintenance: extract function to run tasks Patrick Steinhardt
2025-06-02  7:17   ` [PATCH v3 07/12] builtin/maintenance: fix typedef for function pointers Patrick Steinhardt
2025-06-02  7:17   ` [PATCH v3 08/12] builtin/maintenance: split into foreground and background tasks Patrick Steinhardt
2025-06-02  7:17   ` [PATCH v3 09/12] builtin/maintenance: fix locking race when packing refs and reflogs Patrick Steinhardt
2025-06-02 10:03     ` Kristoffer Haugsbakk
2025-06-03  6:34       ` Patrick Steinhardt
2025-06-03  7:08         ` Kristoffer Haugsbakk
2025-06-02  7:17   ` [PATCH v3 10/12] usage: allow dying without writing an error message Patrick Steinhardt
2025-06-03  8:31     ` Karthik Nayak
2025-06-03  8:33       ` Kristoffer Haugsbakk
2025-06-03  9:04         ` Karthik Nayak
2025-06-03 10:46           ` Patrick Steinhardt
2025-06-03 13:45             ` Kristoffer Haugsbakk
2025-06-03 15:24     ` Junio C Hamano
2025-06-05 13:02       ` Patrick Steinhardt [this message]
2025-06-02  7:17   ` [PATCH v3 11/12] builtin/gc: avoid global state in `gc_before_repack()` Patrick Steinhardt
2025-06-02  7:17   ` [PATCH v3 12/12] builtin/maintenance: fix locking race when handling "gc" task Patrick Steinhardt
2025-06-03 14:01 ` [PATCH v4 00/12] builtin/maintenance: fix ref lock races when detaching Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 01/12] builtin/gc: use designated field initializers for maintenance tasks Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 02/12] builtin/gc: drop redundant local variable Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 03/12] builtin/maintenance: centralize configuration of explicit tasks Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 04/12] builtin/maintenance: mark "--task=" and "--schedule=" as incompatible Patrick Steinhardt
2025-06-05 15:46     ` Derrick Stolee
2025-06-03 14:01   ` [PATCH v4 05/12] builtin/maintenance: stop modifying global array of tasks Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 06/12] builtin/maintenance: extract function to run tasks Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 07/12] builtin/maintenance: fix typedef for function pointers Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 08/12] builtin/maintenance: split into foreground and background tasks Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 09/12] builtin/maintenance: fix locking race with refs and reflogs tasks Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 10/12] usage: allow dying without writing an error message Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 11/12] builtin/gc: avoid global state in `gc_before_repack()` Patrick Steinhardt
2025-06-03 14:01   ` [PATCH v4 12/12] builtin/maintenance: fix locking race when handling "gc" task Patrick Steinhardt
2025-06-03 15:31   ` [PATCH v4 00/12] builtin/maintenance: fix ref lock races when detaching Kristoffer Haugsbakk
2025-06-04  0:35   ` Ben Knoble
2025-06-05 15:53   ` Derrick Stolee
2025-06-06  5:23     ` Patrick Steinhardt

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=aEGVbz7fWmm3yf1L@pks.im \
    --to=ps@pks.im \
    --cc=ben.knoble@gmail.com \
    --cc=dasraf9@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=nasamuffin@google.com \
    --cc=ramsay@ramsayjones.plus.com \
    --cc=yroth@paloaltonetworks.com \
    /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).