git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Philip Oakley <philipoakley@iee.email>,
	Phillip Wood <phillip.wood123@gmail.com>
Subject: Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages
Date: Thu, 1 Apr 2021 09:31:26 -0700	[thread overview]
Message-ID: <CABPp-BFLyYTboiGkP=sc7S+stRZvozAqnJPnza+M0q2nakvD2Q@mail.gmail.com> (raw)
In-Reply-To: <xmqqmtuj8ank.fsf@gitster.g>

On Wed, Mar 31, 2021 at 11:01 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > ... would you two prefer
> > that I add a new affirm() function that is ALSO compiled out in
> > production?
>
> The reason I do not think affirm would fly is because it shares the
> most problematic trait with assert(): it can be comipled out.
>
> There was at least one bug we found and fixed in a piece of code
> that came later in a codepath that had an assert() in it.  The buggy
> code (IIRC, it was added way after the assert() was written in an
> unrelated patch) was unknowingly depending on a side-effect of an
> innocuous function call (i.e. assert(func_tion(args) == expected))
> made in the assert condition, and the bug did not reproduce in the
> developer's environment.

Ah, now that's a much different argument than claiming assert is
useless.  I had to debug one of those once about a decade ago, in a
different project, put in by some other developer, and yeah it's
painful.

If that causes assert() to be frowned upon, then I can understand
based on this reason.  If so, perhaps a BUG_ON() function would be
useful?  The "if (...) BUG()" construct causes line coverage problems
and I find it an onerous way to signal to future code readers "hey,
just as a reminder, these things should be true at this point".  If
you need to signal something more than that or explain why the things
being false is a problem or what might cause it, then the "if (...)
BUG()" construct is useful and there are certainly many places for
that, but it just doesn't fit all the cases.

  reply	other threads:[~2021-04-01 17:51 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-26  7:16 [PATCH] " Elijah Newren via GitGitGadget
2021-03-26 12:27 ` Philip Oakley
2021-03-26 15:12   ` Elijah Newren
2021-03-28  1:30     ` Junio C Hamano
2021-03-29  9:23 ` Phillip Wood
2021-03-29 20:52   ` Junio C Hamano
2021-03-29 21:25   ` Elijah Newren
2021-03-30  2:09 ` [PATCH v2] " Elijah Newren via GitGitGadget
2021-03-30 10:13   ` Johannes Schindelin
2021-03-30 18:47     ` Junio C Hamano
2021-03-30 20:16       ` Elijah Newren
2021-03-31 17:36         ` Ævar Arnfjörð Bjarmason
2021-03-31 17:52           ` Elijah Newren
2021-03-31 18:01         ` Junio C Hamano
2021-04-01 16:31           ` Elijah Newren [this message]
2021-03-30 19:37     ` Elijah Newren
2021-03-31 13:48       ` unifying sequencer's options persisting, was " Johannes Schindelin
2021-04-02 11:28         ` Phillip Wood
2021-04-02 13:10           ` Phillip Wood
2021-04-02 21:01           ` Junio C Hamano
2021-04-02 22:18             ` Elijah Newren
2021-04-02 22:27               ` Junio C Hamano
2021-04-08  2:40                 ` Johannes Schindelin
2021-04-08 17:45                   ` Junio C Hamano
2021-04-08 19:58                   ` Christian Couder
2021-04-09 13:53                     ` Johannes Schindelin
2021-03-31  6:52   ` [PATCH v3] " Elijah Newren via GitGitGadget
2021-03-31 14:38     ` Johannes Schindelin

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BFLyYTboiGkP=sc7S+stRZvozAqnJPnza+M0q2nakvD2Q@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=philipoakley@iee.email \
    --cc=phillip.wood123@gmail.com \
    --subject='Re: [PATCH v2] sequencer: fix edit handling for cherry-pick and revert messages' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).