diff mbox series

[v2,3/3] treewide: replace assert() with BUG_IF_NOT() in special cases

Message ID 20c763f295105bda9a701b9bf5b9aa47af5bf1e1.1742107322.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Add a static analysis job to prevent assertions with side effects | expand

Commit Message

Elijah Newren March 16, 2025, 6:42 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

When the compiler/linker cannot verify that an assert() invocation is
free of side effects for us (e.g. because the assertion includes some
kind of function call), replace the use of assert() with BUG_IF_NOT().

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 diffcore-rename.c   | 2 +-
 merge-ort.c         | 4 ++--
 merge-recursive.c   | 2 +-
 object-file.c       | 2 +-
 parallel-checkout.c | 2 +-
 scalar.c            | 4 ++--
 sequencer.c         | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

Comments

Taylor Blau March 17, 2025, 10:33 p.m. UTC | #1 
On Sun, Mar 16, 2025 at 06:42:02AM +0000, Elijah Newren via GitGitGadget wrote:
> When the compiler/linker cannot verify that an assert() invocation is
> free of side effects for us (e.g. because the assertion includes some
> kind of function call), replace the use of assert() with BUG_IF_NOT().

Nice. I guess since this is split out into its own patch, we wouldn't be
able to cleanly run CI on the previous commit, but I think that's fine,
since we don't treat CI as being nearly as precious as being able to
'make' anywhere in history.

You didn't realign any multi-line assert() statements, but I actually
think that's preferable in this case to demonstrate that the patch does
not change the behavior of these assertions.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 91b77993c78..1a945945fab 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -1406,7 +1406,7 @@  void diffcore_rename_extended(struct diff_options *options,
 
 	trace2_region_enter("diff", "setup", options->repo);
 	info.setup = 0;
-	assert(!dir_rename_count || strmap_empty(dir_rename_count));
+	BUG_IF_NOT(!dir_rename_count || strmap_empty(dir_rename_count));
 	want_copies = (detect_rename == DIFF_DETECT_COPY);
 	if (dirs_removed && (break_idx || want_copies))
 		BUG("dirs_removed incompatible with break/copy detection");
diff --git a/merge-ort.c b/merge-ort.c
index 46e78c3ffa6..3db7a911f81 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -791,7 +791,7 @@  static void path_msg(struct merge_options *opt,
 	struct strbuf tmp = STRBUF_INIT;
 
 	/* Sanity checks */
-	assert(omittable_hint ==
+	BUG_IF_NOT(omittable_hint ==
 	       (!starts_with(type_short_descriptions[type], "CONFLICT") &&
 		!starts_with(type_short_descriptions[type], "ERROR")) ||
 	       type == CONFLICT_DIR_RENAME_SUGGESTED);
@@ -1642,7 +1642,7 @@  static int handle_deferred_entries(struct merge_options *opt,
 			ci = strmap_get(&opt->priv->paths, path);
 			VERIFY_CI(ci);
 
-			assert(renames->deferred[side].trivial_merges_okay &&
+			BUG_IF_NOT(renames->deferred[side].trivial_merges_okay &&
 			       !strset_contains(&renames->deferred[side].target_dirs,
 						path));
 			resolve_trivial_directory_merge(ci, side);
diff --git a/merge-recursive.c b/merge-recursive.c
index 884ccf99a58..ab888689ae4 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1197,7 +1197,7 @@  static void print_commit(struct repository *repo, struct commit *commit)
 	struct pretty_print_context ctx = {0};
 	ctx.date_mode.type = DATE_NORMAL;
 	/* FIXME: Merge this with output_commit_title() */
-	assert(!merge_remote_util(commit));
+	BUG_IF_NOT(!merge_remote_util(commit));
 	repo_format_commit_message(repo, commit, " %h: %m %s", &sb, &ctx);
 	fprintf(stderr, "%s\n", sb.buf);
 	strbuf_release(&sb);
diff --git a/object-file.c b/object-file.c
index 726e41a0475..8ef4813eb63 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2706,7 +2706,7 @@  static int index_stream_convert_blob(struct index_state *istate,
 	struct strbuf sbuf = STRBUF_INIT;
 
 	assert(path);
-	assert(would_convert_to_git_filter_fd(istate, path));
+	BUG_IF_NOT(would_convert_to_git_filter_fd(istate, path));
 
 	convert_to_git_filter_fd(istate, path, fd, &sbuf,
 				 get_conv_flags(flags));
diff --git a/parallel-checkout.c b/parallel-checkout.c
index 7cc6b305281..4d2fa6d7374 100644
--- a/parallel-checkout.c
+++ b/parallel-checkout.c
@@ -277,7 +277,7 @@  static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,
 	ssize_t wrote;
 
 	/* Sanity check */
-	assert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));
+	BUG_IF_NOT(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));
 
 	filter = get_stream_filter_ca(&pc_item->ca, &pc_item->ce->oid);
 	if (filter) {
diff --git a/scalar.c b/scalar.c
index da42b4be0cc..173286110ea 100644
--- a/scalar.c
+++ b/scalar.c
@@ -241,7 +241,7 @@  static int add_or_remove_enlistment(int add)
 
 static int start_fsmonitor_daemon(void)
 {
-	assert(have_fsmonitor_support());
+	BUG_IF_NOT(have_fsmonitor_support());
 
 	if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
 		return run_git("fsmonitor--daemon", "start", NULL);
@@ -251,7 +251,7 @@  static int start_fsmonitor_daemon(void)
 
 static int stop_fsmonitor_daemon(void)
 {
-	assert(have_fsmonitor_support());
+	BUG_IF_NOT(have_fsmonitor_support());
 
 	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
 		return run_git("fsmonitor--daemon", "stop", NULL);
diff --git a/sequencer.c b/sequencer.c
index ad0ab75c8d4..98a7657b398 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4965,7 +4965,7 @@  static int pick_commits(struct repository *r,
 
 	ctx->reflog_message = sequencer_reflog_action(opts);
 	if (opts->allow_ff)
-		assert(!(opts->signoff || opts->no_commit ||
+		BUG_IF_NOT(!(opts->signoff || opts->no_commit ||
 			 opts->record_origin || should_edit(opts) ||
 			 opts->committer_date_is_author_date ||
 			 opts->ignore_date));