{"thread":{"id":"63117","subject":"[PATCH 0/3] Add a static analysis job to prevent assertions with side effects","startedAt":"2025-03-14T00:20:56Z","lastAt":"2025-03-19T22:27:59Z","messageCount":30,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","brian m. carlson","Elijah Newren","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"514243","messageId":"pull.1881.git.1741911652.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":null,"subject":"[PATCH 0/3] Add a static analysis job to prevent assertions with side effects","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-14T00:20:49Z","receivedAt":"2025-03-14T00:20:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We have several hundred assert() invocations in our code base. Some have\nsuggested that we should add a recommendation in our CodingGuidelines to\navoid their use, because there is a risk that someone might include\nsomething with a side-effect in their assertion, which can lead to a very\ndifficult to debug problem. However, CodingGuidelines are going to be less\neffective at preventing that foot-gun than a CI job which can warn of\nassertions that possibly have side-effects. So, let's add a CI job instead.\n\nWhile it is difficult to perfectly determine whether any expression has side\neffects, a simple compiler/linker hack can prove that all but 9 of our\nseveral hundred assert() calls are indeed free from them. While I believe\nthe remaining 9 are also free of side effects, it's easier to just convert\nthose 9 to a new macro (which will not be compiled out when NDEBUG is\ndefined), and instruct any future assertion writers to likewise switch to\nthat alternative macro if they have a slightly more involved assert()\ninvocation.\n\nSee\nhttps://github.com/newren/git/actions/runs/13845548634/job/38743076293#step:4:1938\nfor an example of it running in CI and reporting possibly problematic\nassertions (sample output also included in the commit message of the middle\ncommit in this series if you don't have access to view the link; I'm not\nsure what the rules on that are).\n\nElijah Newren (3):\n git-compat-util: introduce BUG_IF_NOT() macro\n ci: add build checking for side-effects in assert() calls\n treewide: replace assert() with BUG_IF_NOT() in special cases\n\n Makefile | 4 ++++\n ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++\n ci/run-static-analysis.sh | 2 ++\n diffcore-rename.c | 2 +-\n git-compat-util.h | 7 +++++++\n merge-ort.c | 4 ++--\n merge-recursive.c | 2 +-\n object-file.c | 2 +-\n parallel-checkout.c | 2 +-\n scalar.c | 4 ++--\n sequencer.c | 2 +-\n 11 files changed, 40 insertions(+), 9 deletions(-)\n create mode 100755 ci/check-unsafe-assertions.sh\n\n\nbase-commit: 4b68faf6b93311254efad80e554780e372deb42f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1881%2Fnewren%2Fassertion-side-effects-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1881/newren/assertion-side-effects-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1881\n-- \ngitgitgadget\n"},{"id":"514244","messageId":"109060ccb8665c73aa0c4f73e3cbbddcd135bde4.1741911652.git.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.git.1741911652.gitgitgadget@gmail.com","subject":"[PATCH 1/3] git-compat-util: introduce BUG_IF_NOT() macro","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-14T00:20:50Z","receivedAt":"2025-03-14T00:20:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCreate a BUG_IF_NOT() macro which is similar to assert(), but will not be\ncompiled out when NDEBUG is defined, and is thus safe to use even if its\nargument has side-effects.\n\nWe will use this new macro in a subsequent commit to convert a few\nexisting assert() invocations to BUG_IF_NOT(). In particular, we'll\nconvert the handful of invocations which cannot be proven to be free of\nside effects with a simple compiler/linker hack.\n\nSigned-off-by: Elijah Newren \n---\n git-compat-util.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e123288e8f1..c3415ad7e0a 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1460,6 +1460,7 @@ extern int bug_called_must_BUG;\n __attribute__((format (printf, 3, 4))) NORETURN\n void BUG_fl(const char *file, int line, const char *fmt, ...);\n #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)\n+#define BUG_IF_NOT(a) if (!(a)) BUG(\"Assertion `\" #a \"' failed.\")\n __attribute__((format (printf, 3, 4)))\n void bug_fl(const char *file, int line, const char *fmt, ...);\n #define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)\n-- \ngitgitgadget\n\n"},{"id":"514245","messageId":"80dcc2ba3aa0ef72abe18f8525d571ea39ac6382.1741911652.git.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.git.1741911652.gitgitgadget@gmail.com","subject":"[PATCH 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-14T00:20:51Z","receivedAt":"2025-03-14T00:20:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIt is a big no-no to have side-effects in an assertion, because if the\nassert() is compiled out, you don't get that side-effect, leading to the\ncode behaving differently. That can be a large headache to debug.\n\nWe have roughly 566 assert() calls in our codebase (my grep might have\npicked up things that aren't actually assert() calls, but most appeared\nto be). All but 9 of them can be determined by gcc to be free of side\neffects with a clever redefine of assert(). The current 9 appear to be\nfree of side effects to me as well, but are too complicated for a\ncompiler/linker to figure that since each assertion involves some kind\nof function call. Add a CI job which will find and report these\npossibly problematic assertions, and have the job suggest to the user\nthat they replace these with BUG_IF_NOT() calls.\n\nExample output from running:\n\n```\nERROR: The compiler could not verify the following assert()\n calls are free of side-effects. Please replace with\n BUG_IF_NOT() calls.\n/home/newren/floss/git/diffcore-rename.c:1409\n\tassert(!dir_rename_count || strmap_empty(dir_rename_count));\n/home/newren/floss/git/merge-ort.c:1645\n\t\t\tassert(renames->deferred[side].trivial_merges_okay &&\n\t\t\t !strset_contains(&renames->deferred[side].target_dirs,\n\t\t\t\t\t\tpath));\n/home/newren/floss/git/merge-ort.c:794\n\tassert(omittable_hint ==\n\t (!starts_with(type_short_descriptions[type], \"CONFLICT\") &&\n\t\t!starts_with(type_short_descriptions[type], \"ERROR\")) ||\n\t type == CONFLICT_DIR_RENAME_SUGGESTED);\n/home/newren/floss/git/merge-recursive.c:1200\n\tassert(!merge_remote_util(commit));\n/home/newren/floss/git/object-file.c:2709\n\tassert(would_convert_to_git_filter_fd(istate, path));\n/home/newren/floss/git/parallel-checkout.c:280\n\tassert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n/home/newren/floss/git/scalar.c:244\n\tassert(have_fsmonitor_support());\n/home/newren/floss/git/scalar.c:254\n\tassert(have_fsmonitor_support());\n/home/newren/floss/git/sequencer.c:4968\n\t\tassert(!(opts->signoff || opts->no_commit ||\n\t\t\t opts->record_origin || should_edit(opts) ||\n\t\t\t opts->committer_date_is_author_date ||\n\t\t\t opts->ignore_date));\n```\n\nNote that if there are possibly problematic assertions, not necessarily\nall of them will be shown in a single run, because the compiler errors\nmay include something like \"ld: ... more undefined references to\n`not_supposed_to_survive' follow\" instead of listing each individually.\nBut in such cases, once you clean up a few that are shown in your first\nrun, subsequent runs will show (some of) the ones that remain, allowing\nyou to iteratively remove them all.\n\nSigned-off-by: Elijah Newren \n---\n Makefile | 4 ++++\n ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++\n ci/run-static-analysis.sh | 2 ++\n git-compat-util.h | 6 ++++++\n 4 files changed, 30 insertions(+)\n create mode 100755 ci/check-unsafe-assertions.sh\n\ndiff --git a/Makefile b/Makefile\nindex 7315507381e..57774912f18 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2261,6 +2261,10 @@ ifdef WITH_BREAKING_CHANGES\n \tBASIC_CFLAGS += -DWITH_BREAKING_CHANGES\n endif\n \n+ifdef CHECK_ASSERTION_SIDE_EFFECTS\n+\tBASIC_CFLAGS += -DCHECK_ASSERTION_SIDE_EFFECTS\n+endif\n+\n ifdef INCLUDE_LIBGIT_RS\n \t# Enable symbol hiding in contrib/libgit-sys/libgitpub.a without making\n \t# us rebuild the whole tree every time we run a Rust build.\ndiff --git a/ci/check-unsafe-assertions.sh b/ci/check-unsafe-assertions.sh\nnew file mode 100755\nindex 00000000000..d66091efd22\n--- /dev/null\n+++ b/ci/check-unsafe-assertions.sh\n@@ -0,0 +1,18 @@\n+#!/bin/sh\n+\n+make CHECK_ASSERTION_SIDE_EFFECTS=1 >compiler_output 2>compiler_error\n+if test $? != 0\n+then\n+ echo \"ERROR: The compiler could not verify the following assert()\" >&2\n+ echo \" calls are free of side-effects. Please replace with\" >&2\n+ echo \" BUG_IF_NOT() calls.\" >&2\n+ grep undefined.reference.to..not_supposed_to_survive compiler_error \\\n+ | sed -e s/:[^:]*$// | sort | uniq | tr ':' ' ' \\\n+ | while read f l\n+ do\n+\tprintf \"${f}:${l}\\n \"\n+\tawk -v start=\"$l\" 'NR >= start { print; if (/\\);/) exit }' $f\n+ done\n+ exit 1\n+fi\n+rm compiler_output compiler_error\ndiff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh\nindex 0d51e5ce0e7..ae714e020ae 100755\n--- a/ci/run-static-analysis.sh\n+++ b/ci/run-static-analysis.sh\n@@ -31,4 +31,6 @@ exit 1\n \n make check-pot\n \n+${0%/*}/check-unsafe-assertions.sh\n+\n save_good_tree\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c3415ad7e0a..0aefd763751 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1584,4 +1584,10 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)\n \t((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))\n #endif /* !__GNUC__ */\n \n+#ifdef CHECK_ASSERTION_SIDE_EFFECTS\n+#undef assert\n+extern int not_supposed_to_survive;\n+#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n+#endif /* CHECK_ASSERTION_SIDE_EFFECTS */\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"514246","messageId":"4c668039bb72e7e86ce08dc7b8e6c7f09673cee4.1741911652.git.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.git.1741911652.gitgitgadget@gmail.com","subject":"[PATCH 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-14T00:20:52Z","receivedAt":"2025-03-14T00:20:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen the compiler/linker cannot verify that an assert() invocation is\nfree of side effects for us (e.g. because the assertion includes some\nkind of function call), replace the use of assert() with BUG_IF_NOT().\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 2 +-\n merge-ort.c | 4 ++--\n merge-recursive.c | 2 +-\n object-file.c | 2 +-\n parallel-checkout.c | 2 +-\n scalar.c | 4 ++--\n sequencer.c | 2 +-\n 7 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 91b77993c78..1a945945fab 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -1406,7 +1406,7 @@ void diffcore_rename_extended(struct diff_options *options,\n \n \ttrace2_region_enter(\"diff\", \"setup\", options->repo);\n \tinfo.setup = 0;\n-\tassert(!dir_rename_count || strmap_empty(dir_rename_count));\n+\tBUG_IF_NOT(!dir_rename_count || strmap_empty(dir_rename_count));\n \twant_copies = (detect_rename == DIFF_DETECT_COPY);\n \tif (dirs_removed && (break_idx || want_copies))\n \t\tBUG(\"dirs_removed incompatible with break/copy detection\");\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 46e78c3ffa6..3db7a911f81 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -791,7 +791,7 @@ static void path_msg(struct merge_options *opt,\n \tstruct strbuf tmp = STRBUF_INIT;\n \n \t/* Sanity checks */\n-\tassert(omittable_hint ==\n+\tBUG_IF_NOT(omittable_hint ==\n \t (!starts_with(type_short_descriptions[type], \"CONFLICT\") &&\n \t\t!starts_with(type_short_descriptions[type], \"ERROR\")) ||\n \t type == CONFLICT_DIR_RENAME_SUGGESTED);\n@@ -1642,7 +1642,7 @@ static int handle_deferred_entries(struct merge_options *opt,\n \t\t\tci = strmap_get(&opt->priv->paths, path);\n \t\t\tVERIFY_CI(ci);\n \n-\t\t\tassert(renames->deferred[side].trivial_merges_okay &&\n+\t\t\tBUG_IF_NOT(renames->deferred[side].trivial_merges_okay &&\n \t\t\t !strset_contains(&renames->deferred[side].target_dirs,\n \t\t\t\t\t\tpath));\n \t\t\tresolve_trivial_directory_merge(ci, side);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 884ccf99a58..ab888689ae4 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1197,7 +1197,7 @@ static void print_commit(struct repository *repo, struct commit *commit)\n \tstruct pretty_print_context ctx = {0};\n \tctx.date_mode.type = DATE_NORMAL;\n \t/* FIXME: Merge this with output_commit_title() */\n-\tassert(!merge_remote_util(commit));\n+\tBUG_IF_NOT(!merge_remote_util(commit));\n \trepo_format_commit_message(repo, commit, \" %h: %m %s\", &sb, &ctx);\n \tfprintf(stderr, \"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\ndiff --git a/object-file.c b/object-file.c\nindex 726e41a0475..8ef4813eb63 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2706,7 +2706,7 @@ static int index_stream_convert_blob(struct index_state *istate,\n \tstruct strbuf sbuf = STRBUF_INIT;\n \n \tassert(path);\n-\tassert(would_convert_to_git_filter_fd(istate, path));\n+\tBUG_IF_NOT(would_convert_to_git_filter_fd(istate, path));\n \n \tconvert_to_git_filter_fd(istate, path, fd, &sbuf,\n \t\t\t\t get_conv_flags(flags));\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 7cc6b305281..4d2fa6d7374 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -277,7 +277,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,\n \tssize_t wrote;\n \n \t/* Sanity check */\n-\tassert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n+\tBUG_IF_NOT(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n \n \tfilter = get_stream_filter_ca(&pc_item->ca, &pc_item->ce->oid);\n \tif (filter) {\ndiff --git a/scalar.c b/scalar.c\nindex da42b4be0cc..173286110ea 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -241,7 +241,7 @@ static int add_or_remove_enlistment(int add)\n \n static int start_fsmonitor_daemon(void)\n {\n-\tassert(have_fsmonitor_support());\n+\tBUG_IF_NOT(have_fsmonitor_support());\n \n \tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n \t\treturn run_git(\"fsmonitor--daemon\", \"start\", NULL);\n@@ -251,7 +251,7 @@ static int start_fsmonitor_daemon(void)\n \n static int stop_fsmonitor_daemon(void)\n {\n-\tassert(have_fsmonitor_support());\n+\tBUG_IF_NOT(have_fsmonitor_support());\n \n \tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n \t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\ndiff --git a/sequencer.c b/sequencer.c\nindex ad0ab75c8d4..98a7657b398 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4965,7 +4965,7 @@ static int pick_commits(struct repository *r,\n \n \tctx->reflog_message = sequencer_reflog_action(opts);\n \tif (opts->allow_ff)\n-\t\tassert(!(opts->signoff || opts->no_commit ||\n+\t\tBUG_IF_NOT(!(opts->signoff || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n \t\t\t opts->committer_date_is_author_date ||\n \t\t\t opts->ignore_date));\n-- \ngitgitgadget\n"},{"id":"514248","messageId":"xmqqy0x8l8tu.fsf@gitster.g","threadId":"63117","inReplyTo":"80dcc2ba3aa0ef72abe18f8525d571ea39ac6382.1741911652.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-14T01:06:21Z","receivedAt":"2025-03-14T01:06:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> +make CHECK_ASSERTION_SIDE_EFFECTS=1 >compiler_output 2>compiler_error\n> +if test $? != 0\n> +then\n> + echo \"ERROR: The compiler could not verify the following assert()\" >&2\n> + echo \" calls are free of side-effects. Please replace with\" >&2\n> + echo \" BUG_IF_NOT() calls.\" >&2\n> + grep undefined.reference.to..not_supposed_to_survive compiler_error \\\n> + | sed -e s/:[^:]*$// | sort | uniq | tr ':' ' ' \\\n> + | while read f l\n\nA few style guides:\n\n - doing multiple echo into the same descriptor is easier to read if\n you have redirection near the beginning, i.e.\n\n\techo >&2 \"message one\"\n\techo >&2 \"message two that may way be longer than the previous\"\n ehco >&2 \"message three\"\n\n - multi-line pipelines are easier to follow without backslash by\n ending the previous line with a '|', i.e.\n\n\tgrep ... file |\n\tsed -e ... |\n\twhile read file line\n\tdo\n\t\t...\n\n - I thought our \"one indent one tab\" standard extends to shell\n scripts as well?\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index c3415ad7e0a..0aefd763751 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1584,4 +1584,10 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)\n> \t((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))\n> #endif /* !__GNUC__ */\n> \n> +#ifdef CHECK_ASSERTION_SIDE_EFFECTS\n> +#undef assert\n> +extern int not_supposed_to_survive;\n> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n> +#endif /* CHECK_ASSERTION_SIDE_EFFECTS */\n\nCute. As this checking assert is in void context, the optimizing\ncompiler knows that the entire thing can be optimized away ONLY IF\nit can somehow prove that (expr) has no side effect. And if it does\nnot optimize it away, you will hit an error from the linker, saying\nthat the undefined variable is being used.\n\nThis requires a fairly good optimizing compiler that can peek into\n(as in \"inline\") what is in expr to notice, so it cannot be free of\nfalse positive, but at least when the optimization works as expected,\nit is provably (modulo optimizer bugs) side-effect free.\n\nIs this something we can use in our project? I am just double\nchecking.\n\nThanks.\n\n"},{"id":"514249","messageId":"Z9OD8ihB-tiCdBmp@tapette.crustytoothpaste.net","threadId":"63117","inReplyTo":"xmqqy0x8l8tu.fsf@gitster.g","subject":"Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-03-14T01:18:42Z","receivedAt":"2025-03-14T01:18:45Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-03-14 at 01:06:21, Junio C Hamano wrote:\n> \"Elijah Newren via GitGitGadget\" writes:\n> \n> > diff --git a/git-compat-util.h b/git-compat-util.h\n> > index c3415ad7e0a..0aefd763751 100644\n> > --- a/git-compat-util.h\n> > +++ b/git-compat-util.h\n> > @@ -1584,4 +1584,10 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)\n> > \t((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))\n> > #endif /* !__GNUC__ */\n> > \n> > +#ifdef CHECK_ASSERTION_SIDE_EFFECTS\n> > +#undef assert\n> > +extern int not_supposed_to_survive;\n> > +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n> > +#endif /* CHECK_ASSERTION_SIDE_EFFECTS */\n> \n> Cute. As this checking assert is in void context, the optimizing\n> compiler knows that the entire thing can be optimized away ONLY IF\n> it can somehow prove that (expr) has no side effect. And if it does\n> not optimize it away, you will hit an error from the linker, saying\n> that the undefined variable is being used.\n\nI agree this is very clever.\n\n> This requires a fairly good optimizing compiler that can peek into\n> (as in \"inline\") what is in expr to notice, so it cannot be free of\n> false positive, but at least when the optimization works as expected,\n> it is provably (modulo optimizer bugs) side-effect free.\n> \n> Is this something we can use in our project? I am just double\n> checking.\n\nI believe it's valid in C99. Certainly some compiler might be bad at\noptimizing, or a user may have compiled with -O0, but this is run in CI,\nwhere we have known good compilers and can control the optimization\nflags. I doubt GCC, Clang, or MSVC will have problems here, and since\nthis is not on by default, users using something less capable (the Tiny\nC Compiler, maybe?) or a vendor compiler won't even see it.\n\nWas there some other case that you were concerned about?\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"514250","messageId":"xmqqtt7wl85l.fsf@gitster.g","threadId":"63117","inReplyTo":"Z9OD8ihB-tiCdBmp@tapette.crustytoothpaste.net","subject":"Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-14T01:20:54Z","receivedAt":"2025-03-14T01:20:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n>> Is this something we can use in our project? I am just double\n>> checking.\n>\n> I believe it's valid in C99. Certainly some compiler might be bad at\n> optimizing, or a user may have compiled with -O0, but this is run in CI,\n> where we have known good compilers and can control the optimization\n> flags. I doubt GCC, Clang, or MSVC will have problems here, and since\n> this is not on by default, users using something less capable (the Tiny\n> C Compiler, maybe?) or a vendor compiler won't even see it.\n>\n> Was there some other case that you were concerned about?\n\nLicensing, mostly, as clever things we see are not necessarily home\ngrown. I know the patch came with DCO sign-off, but it does not\nhurt to double check.\n"},{"id":"514251","messageId":"CABPp-BHRSnNE0zj9kRjrVm4-NXt33tYBT_iSgZU-5JU9Y2vp3w@mail.gmail.com","threadId":"63117","inReplyTo":"xmqqtt7wl85l.fsf@gitster.g","subject":"Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-14T01:27:50Z","receivedAt":"2025-03-14T01:28:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 13, 2025 at 6:20 PM Junio C Hamano wrote:\n>\n> \"brian m. carlson\" writes:\n>\n> >> Is this something we can use in our project? I am just double\n> >> checking.\n> >\n> > I believe it's valid in C99. Certainly some compiler might be bad at\n> > optimizing, or a user may have compiled with -O0, but this is run in CI,\n> > where we have known good compilers and can control the optimization\n> > flags. I doubt GCC, Clang, or MSVC will have problems here, and since\n> > this is not on by default, users using something less capable (the Tiny\n> > C Compiler, maybe?) or a vendor compiler won't even see it.\n> >\n> > Was there some other case that you were concerned about?\n>\n> Licensing, mostly, as clever things we see are not necessarily home\n> grown. I know the patch came with DCO sign-off, but it does not\n> hurt to double check.\n\nThese two lines:\n\n> +extern int not_supposed_to_survive;\n> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n\n, which serve as the core trick, I had used elsewhere before. Doing\nsome searches, it looks like those likely came from\nhttps://stackoverflow.com/questions/10593492/catching-assert-with-side-effects.\nAnd it appears that StackOverflow is CC-BY-SA-3.0 or -4.0. Doh,\nsorry. Anyone got a clever alternative?\n\nThe rest of the patch was all written by me.\n"},{"id":"514282","messageId":"xmqqa59nh66g.fsf@gitster.g","threadId":"63117","inReplyTo":"CABPp-BHRSnNE0zj9kRjrVm4-NXt33tYBT_iSgZU-5JU9Y2vp3w@mail.gmail.com","subject":"Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-14T17:29:27Z","receivedAt":"2025-03-14T17:29:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> Licensing, mostly, as clever things we see are not necessarily home\n>> grown. I know the patch came with DCO sign-off, but it does not\n>> hurt to double check.\n>\n> These two lines:\n>\n>> +extern int not_supposed_to_survive;\n>> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n>\n> , which serve as the core trick, I had used elsewhere before.\n\nIt may be arguable that it is too small to be copyrightable and\nthere is no other way to express the idea behind that check, but\nin any case ...\n\n> Anyone got a clever alternative?\n\n... as I cannot unsee your patch, I cannot be the one who comes up\nwith a clever alternative, if we are worried about licensing with\nwhat you posted X-<.\n\n"},{"id":"514378","messageId":"CABPp-BF1LgdafW+PHmttuhHL1OOHPTsGpOTVuG_A0SSH5cbfbg@mail.gmail.com","threadId":"63117","inReplyTo":"xmqqa59nh66g.fsf@gitster.g","subject":"Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-16T06:38:38Z","receivedAt":"2025-03-16T06:38:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Mar 14, 2025 at 10:29 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> >> Licensing, mostly, as clever things we see are not necessarily home\n> >> grown. I know the patch came with DCO sign-off, but it does not\n> >> hurt to double check.\n> >\n> > These two lines:\n> >\n> >> +extern int not_supposed_to_survive;\n> >> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n> >\n> > , which serve as the core trick, I had used elsewhere before.\n>\n> It may be arguable that it is too small to be copyrightable and\n> there is no other way to express the idea behind that check, but\n> in any case ...\n\nThat's what I had been assuming, but then you, brian, and Taylor all\npointed out how clever it was making me think otherwise.\n\n> > Anyone got a clever alternative?\n>\n> ... as I cannot unsee your patch, I cannot be the one who comes up\n> with a clever alternative, if we are worried about licensing with\n> what you posted X-<.\n\nTurns out we don't need an alternative. I contacted the author, who\nresponded and placed the two-liner into the public domain with no\nwarranty of any kind. I'll send a re-roll with an updated commit\nmessage.\n"},{"id":"514379","messageId":"pull.1881.v2.git.1742107322.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.git.1741911652.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-16T06:41:59Z","receivedAt":"2025-03-16T06:42:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We have several hundred assert() invocations in our code base. Some have\nsuggested that we should add a recommendation in our CodingGuidelines to\navoid their use, because there is a risk that someone might include\nsomething with a side-effect in their assertion, which can lead to a very\ndifficult to debug problem. However, CodingGuidelines are going to be less\neffective at preventing that foot-gun than a CI job which can warn of\nassertions that possibly have side-effects. So, let's add a CI job instead.\n\nWhile it is difficult to perfectly determine whether any expression has side\neffects, a simple compiler/linker hack can prove that all but 9 of our\nseveral hundred assert() calls are indeed free from them. While I believe\nthe remaining 9 are also free of side effects, it's easier to just convert\nthose 9 to a new macro (which will not be compiled out when NDEBUG is\ndefined), and instruct any future assertion writers to likewise switch to\nthat alternative macro if they have a slightly more involved assert()\ninvocation.\n\nSee\nhttps://github.com/newren/git/actions/runs/13845548634/job/38743076293#step:4:1938\nfor an example of it running in CI and reporting possibly problematic\nassertions (sample output also included in the commit message of the middle\ncommit in this series if you don't have access to view the link; I'm not\nsure what the rules on that are).\n\nElijah Newren (3):\n git-compat-util: introduce BUG_IF_NOT() macro\n ci: add build checking for side-effects in assert() calls\n treewide: replace assert() with BUG_IF_NOT() in special cases\n\n Makefile | 4 ++++\n ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++\n ci/run-static-analysis.sh | 2 ++\n diffcore-rename.c | 2 +-\n git-compat-util.h | 7 +++++++\n merge-ort.c | 4 ++--\n merge-recursive.c | 2 +-\n object-file.c | 2 +-\n parallel-checkout.c | 2 +-\n scalar.c | 4 ++--\n sequencer.c | 2 +-\n 11 files changed, 40 insertions(+), 9 deletions(-)\n create mode 100755 ci/check-unsafe-assertions.sh\n\n\nbase-commit: 4b68faf6b93311254efad80e554780e372deb42f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1881%2Fnewren%2Fassertion-side-effects-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1881/newren/assertion-side-effects-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1881\n\nRange-diff vs v1:\n\n 1: 109060ccb86 = 1: 109060ccb86 git-compat-util: introduce BUG_IF_NOT() macro\n 2: 80dcc2ba3aa ! 2: 58cb8f6a160 ci: add build checking for side-effects in assert() calls\n @@ Commit message\n We have roughly 566 assert() calls in our codebase (my grep might have\n picked up things that aren't actually assert() calls, but most appeared\n to be). All but 9 of them can be determined by gcc to be free of side\n - effects with a clever redefine of assert(). The current 9 appear to be\n - free of side effects to me as well, but are too complicated for a\n - compiler/linker to figure that since each assertion involves some kind\n - of function call. Add a CI job which will find and report these\n - possibly problematic assertions, and have the job suggest to the user\n - that they replace these with BUG_IF_NOT() calls.\n + effects with a clever redefine of assert() provided by Bruno De Fraine\n + (from\n + https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects),\n + who upon request has graciously placed his two-liner into the public\n + domain without warranty of any kind. The current 9 assert() calls\n + flagged by this clever redefinition of assert() appear to me to be free\n + of side effects as well, but are too complicated for a compiler/linker\n + to figure that since each assertion involves some kind of function call.\n + Add a CI job which will find and report these possibly problematic\n + assertions, and have the job suggest to the user that they replace these\n + with BUG_IF_NOT() calls.\n \n Example output from running:\n \n @@ Commit message\n run, subsequent runs will show (some of) the ones that remain, allowing\n you to iteratively remove them all.\n \n + Helped-by: Bruno De Fraine \n Signed-off-by: Elijah Newren \n \n ## Makefile ##\n 3: 4c668039bb7 = 3: 20c763f2951 treewide: replace assert() with BUG_IF_NOT() in special cases\n\n-- \ngitgitgadget\n"},{"id":"514380","messageId":"109060ccb8665c73aa0c4f73e3cbbddcd135bde4.1742107322.git.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.v2.git.1742107322.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-16T06:42:00Z","receivedAt":"2025-03-16T06:42:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCreate a BUG_IF_NOT() macro which is similar to assert(), but will not be\ncompiled out when NDEBUG is defined, and is thus safe to use even if its\nargument has side-effects.\n\nWe will use this new macro in a subsequent commit to convert a few\nexisting assert() invocations to BUG_IF_NOT(). In particular, we'll\nconvert the handful of invocations which cannot be proven to be free of\nside effects with a simple compiler/linker hack.\n\nSigned-off-by: Elijah Newren \n---\n git-compat-util.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e123288e8f1..c3415ad7e0a 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1460,6 +1460,7 @@ extern int bug_called_must_BUG;\n __attribute__((format (printf, 3, 4))) NORETURN\n void BUG_fl(const char *file, int line, const char *fmt, ...);\n #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)\n+#define BUG_IF_NOT(a) if (!(a)) BUG(\"Assertion `\" #a \"' failed.\")\n __attribute__((format (printf, 3, 4)))\n void bug_fl(const char *file, int line, const char *fmt, ...);\n #define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)\n-- \ngitgitgadget\n\n"},{"id":"514381","messageId":"58cb8f6a1609b10d761e86bdad541d1c018cb582.1742107322.git.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.v2.git.1742107322.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-16T06:42:01Z","receivedAt":"2025-03-16T06:42:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIt is a big no-no to have side-effects in an assertion, because if the\nassert() is compiled out, you don't get that side-effect, leading to the\ncode behaving differently. That can be a large headache to debug.\n\nWe have roughly 566 assert() calls in our codebase (my grep might have\npicked up things that aren't actually assert() calls, but most appeared\nto be). All but 9 of them can be determined by gcc to be free of side\neffects with a clever redefine of assert() provided by Bruno De Fraine\n(from\nhttps://stackoverflow.com/questions/10593492/catching-assert-with-side-effects),\nwho upon request has graciously placed his two-liner into the public\ndomain without warranty of any kind. The current 9 assert() calls\nflagged by this clever redefinition of assert() appear to me to be free\nof side effects as well, but are too complicated for a compiler/linker\nto figure that since each assertion involves some kind of function call.\nAdd a CI job which will find and report these possibly problematic\nassertions, and have the job suggest to the user that they replace these\nwith BUG_IF_NOT() calls.\n\nExample output from running:\n\n```\nERROR: The compiler could not verify the following assert()\n calls are free of side-effects. Please replace with\n BUG_IF_NOT() calls.\n/home/newren/floss/git/diffcore-rename.c:1409\n\tassert(!dir_rename_count || strmap_empty(dir_rename_count));\n/home/newren/floss/git/merge-ort.c:1645\n\t\t\tassert(renames->deferred[side].trivial_merges_okay &&\n\t\t\t !strset_contains(&renames->deferred[side].target_dirs,\n\t\t\t\t\t\tpath));\n/home/newren/floss/git/merge-ort.c:794\n\tassert(omittable_hint ==\n\t (!starts_with(type_short_descriptions[type], \"CONFLICT\") &&\n\t\t!starts_with(type_short_descriptions[type], \"ERROR\")) ||\n\t type == CONFLICT_DIR_RENAME_SUGGESTED);\n/home/newren/floss/git/merge-recursive.c:1200\n\tassert(!merge_remote_util(commit));\n/home/newren/floss/git/object-file.c:2709\n\tassert(would_convert_to_git_filter_fd(istate, path));\n/home/newren/floss/git/parallel-checkout.c:280\n\tassert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n/home/newren/floss/git/scalar.c:244\n\tassert(have_fsmonitor_support());\n/home/newren/floss/git/scalar.c:254\n\tassert(have_fsmonitor_support());\n/home/newren/floss/git/sequencer.c:4968\n\t\tassert(!(opts->signoff || opts->no_commit ||\n\t\t\t opts->record_origin || should_edit(opts) ||\n\t\t\t opts->committer_date_is_author_date ||\n\t\t\t opts->ignore_date));\n```\n\nNote that if there are possibly problematic assertions, not necessarily\nall of them will be shown in a single run, because the compiler errors\nmay include something like \"ld: ... more undefined references to\n`not_supposed_to_survive' follow\" instead of listing each individually.\nBut in such cases, once you clean up a few that are shown in your first\nrun, subsequent runs will show (some of) the ones that remain, allowing\nyou to iteratively remove them all.\n\nHelped-by: Bruno De Fraine \nSigned-off-by: Elijah Newren \n---\n Makefile | 4 ++++\n ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++\n ci/run-static-analysis.sh | 2 ++\n git-compat-util.h | 6 ++++++\n 4 files changed, 30 insertions(+)\n create mode 100755 ci/check-unsafe-assertions.sh\n\ndiff --git a/Makefile b/Makefile\nindex 7315507381e..57774912f18 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2261,6 +2261,10 @@ ifdef WITH_BREAKING_CHANGES\n \tBASIC_CFLAGS += -DWITH_BREAKING_CHANGES\n endif\n \n+ifdef CHECK_ASSERTION_SIDE_EFFECTS\n+\tBASIC_CFLAGS += -DCHECK_ASSERTION_SIDE_EFFECTS\n+endif\n+\n ifdef INCLUDE_LIBGIT_RS\n \t# Enable symbol hiding in contrib/libgit-sys/libgitpub.a without making\n \t# us rebuild the whole tree every time we run a Rust build.\ndiff --git a/ci/check-unsafe-assertions.sh b/ci/check-unsafe-assertions.sh\nnew file mode 100755\nindex 00000000000..d66091efd22\n--- /dev/null\n+++ b/ci/check-unsafe-assertions.sh\n@@ -0,0 +1,18 @@\n+#!/bin/sh\n+\n+make CHECK_ASSERTION_SIDE_EFFECTS=1 >compiler_output 2>compiler_error\n+if test $? != 0\n+then\n+ echo \"ERROR: The compiler could not verify the following assert()\" >&2\n+ echo \" calls are free of side-effects. Please replace with\" >&2\n+ echo \" BUG_IF_NOT() calls.\" >&2\n+ grep undefined.reference.to..not_supposed_to_survive compiler_error \\\n+ | sed -e s/:[^:]*$// | sort | uniq | tr ':' ' ' \\\n+ | while read f l\n+ do\n+\tprintf \"${f}:${l}\\n \"\n+\tawk -v start=\"$l\" 'NR >= start { print; if (/\\);/) exit }' $f\n+ done\n+ exit 1\n+fi\n+rm compiler_output compiler_error\ndiff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh\nindex 0d51e5ce0e7..ae714e020ae 100755\n--- a/ci/run-static-analysis.sh\n+++ b/ci/run-static-analysis.sh\n@@ -31,4 +31,6 @@ exit 1\n \n make check-pot\n \n+${0%/*}/check-unsafe-assertions.sh\n+\n save_good_tree\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c3415ad7e0a..0aefd763751 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1584,4 +1584,10 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)\n \t((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))\n #endif /* !__GNUC__ */\n \n+#ifdef CHECK_ASSERTION_SIDE_EFFECTS\n+#undef assert\n+extern int not_supposed_to_survive;\n+#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n+#endif /* CHECK_ASSERTION_SIDE_EFFECTS */\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"514382","messageId":"20c763f295105bda9a701b9bf5b9aa47af5bf1e1.1742107322.git.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.v2.git.1742107322.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-16T06:42:02Z","receivedAt":"2025-03-16T06:42:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen the compiler/linker cannot verify that an assert() invocation is\nfree of side effects for us (e.g. because the assertion includes some\nkind of function call), replace the use of assert() with BUG_IF_NOT().\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 2 +-\n merge-ort.c | 4 ++--\n merge-recursive.c | 2 +-\n object-file.c | 2 +-\n parallel-checkout.c | 2 +-\n scalar.c | 4 ++--\n sequencer.c | 2 +-\n 7 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 91b77993c78..1a945945fab 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -1406,7 +1406,7 @@ void diffcore_rename_extended(struct diff_options *options,\n \n \ttrace2_region_enter(\"diff\", \"setup\", options->repo);\n \tinfo.setup = 0;\n-\tassert(!dir_rename_count || strmap_empty(dir_rename_count));\n+\tBUG_IF_NOT(!dir_rename_count || strmap_empty(dir_rename_count));\n \twant_copies = (detect_rename == DIFF_DETECT_COPY);\n \tif (dirs_removed && (break_idx || want_copies))\n \t\tBUG(\"dirs_removed incompatible with break/copy detection\");\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 46e78c3ffa6..3db7a911f81 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -791,7 +791,7 @@ static void path_msg(struct merge_options *opt,\n \tstruct strbuf tmp = STRBUF_INIT;\n \n \t/* Sanity checks */\n-\tassert(omittable_hint ==\n+\tBUG_IF_NOT(omittable_hint ==\n \t (!starts_with(type_short_descriptions[type], \"CONFLICT\") &&\n \t\t!starts_with(type_short_descriptions[type], \"ERROR\")) ||\n \t type == CONFLICT_DIR_RENAME_SUGGESTED);\n@@ -1642,7 +1642,7 @@ static int handle_deferred_entries(struct merge_options *opt,\n \t\t\tci = strmap_get(&opt->priv->paths, path);\n \t\t\tVERIFY_CI(ci);\n \n-\t\t\tassert(renames->deferred[side].trivial_merges_okay &&\n+\t\t\tBUG_IF_NOT(renames->deferred[side].trivial_merges_okay &&\n \t\t\t !strset_contains(&renames->deferred[side].target_dirs,\n \t\t\t\t\t\tpath));\n \t\t\tresolve_trivial_directory_merge(ci, side);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 884ccf99a58..ab888689ae4 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1197,7 +1197,7 @@ static void print_commit(struct repository *repo, struct commit *commit)\n \tstruct pretty_print_context ctx = {0};\n \tctx.date_mode.type = DATE_NORMAL;\n \t/* FIXME: Merge this with output_commit_title() */\n-\tassert(!merge_remote_util(commit));\n+\tBUG_IF_NOT(!merge_remote_util(commit));\n \trepo_format_commit_message(repo, commit, \" %h: %m %s\", &sb, &ctx);\n \tfprintf(stderr, \"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\ndiff --git a/object-file.c b/object-file.c\nindex 726e41a0475..8ef4813eb63 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2706,7 +2706,7 @@ static int index_stream_convert_blob(struct index_state *istate,\n \tstruct strbuf sbuf = STRBUF_INIT;\n \n \tassert(path);\n-\tassert(would_convert_to_git_filter_fd(istate, path));\n+\tBUG_IF_NOT(would_convert_to_git_filter_fd(istate, path));\n \n \tconvert_to_git_filter_fd(istate, path, fd, &sbuf,\n \t\t\t\t get_conv_flags(flags));\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 7cc6b305281..4d2fa6d7374 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -277,7 +277,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,\n \tssize_t wrote;\n \n \t/* Sanity check */\n-\tassert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n+\tBUG_IF_NOT(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n \n \tfilter = get_stream_filter_ca(&pc_item->ca, &pc_item->ce->oid);\n \tif (filter) {\ndiff --git a/scalar.c b/scalar.c\nindex da42b4be0cc..173286110ea 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -241,7 +241,7 @@ static int add_or_remove_enlistment(int add)\n \n static int start_fsmonitor_daemon(void)\n {\n-\tassert(have_fsmonitor_support());\n+\tBUG_IF_NOT(have_fsmonitor_support());\n \n \tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n \t\treturn run_git(\"fsmonitor--daemon\", \"start\", NULL);\n@@ -251,7 +251,7 @@ static int start_fsmonitor_daemon(void)\n \n static int stop_fsmonitor_daemon(void)\n {\n-\tassert(have_fsmonitor_support());\n+\tBUG_IF_NOT(have_fsmonitor_support());\n \n \tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n \t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\ndiff --git a/sequencer.c b/sequencer.c\nindex ad0ab75c8d4..98a7657b398 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4965,7 +4965,7 @@ static int pick_commits(struct repository *r,\n \n \tctx->reflog_message = sequencer_reflog_action(opts);\n \tif (opts->allow_ff)\n-\t\tassert(!(opts->signoff || opts->no_commit ||\n+\t\tBUG_IF_NOT(!(opts->signoff || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n \t\t\t opts->committer_date_is_author_date ||\n \t\t\t opts->ignore_date));\n-- \ngitgitgadget\n"},{"id":"514410","messageId":"CABPp-BGA+mSbj_n4k8QX3nB6rxox04E-KH6sdBWLUbGex+K5Bw@mail.gmail.com","threadId":"63117","inReplyTo":"CABPp-BF1LgdafW+PHmttuhHL1OOHPTsGpOTVuG_A0SSH5cbfbg@mail.gmail.com","subject":"Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-17T15:45:54Z","receivedAt":"2025-03-17T15:46:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Mar 15, 2025 at 11:38 PM Elijah Newren wrote:\n>\n> On Fri, Mar 14, 2025 at 10:29 AM Junio C Hamano wrote:\n> >\n> > Elijah Newren writes:\n> >\n> > >> Licensing, mostly, as clever things we see are not necessarily home\n> > >> grown. I know the patch came with DCO sign-off, but it does not\n> > >> hurt to double check.\n> > >\n> > > These two lines:\n> > >\n> > >> +extern int not_supposed_to_survive;\n> > >> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n> > >\n> > > , which serve as the core trick, I had used elsewhere before.\n> >\n> > It may be arguable that it is too small to be copyrightable and\n> > there is no other way to express the idea behind that check, but\n> > in any case ...\n>\n> That's what I had been assuming, but then you, brian, and Taylor all\n> pointed out how clever it was making me think otherwise.\n>\n> > > Anyone got a clever alternative?\n> >\n> > ... as I cannot unsee your patch, I cannot be the one who comes up\n> > with a clever alternative, if we are worried about licensing with\n> > what you posted X-<.\n>\n> Turns out we don't need an alternative. I contacted the author, who\n> responded and placed the two-liner into the public domain with no\n> warranty of any kind. I'll send a re-roll with an updated commit\n> message.\n\nAnd of course, in my excitement to send a re-roll addressing that\nissue, I totally spaced fixing the style issues you pointed out. I'll\nwait a couple days for any comments and then send a re-roll with those\nfixes.\n"},{"id":"514449","messageId":"xmqqo6xz70oh.fsf@gitster.g","threadId":"63117","inReplyTo":"CABPp-BF1LgdafW+PHmttuhHL1OOHPTsGpOTVuG_A0SSH5cbfbg@mail.gmail.com","subject":"Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-17T22:27:26Z","receivedAt":"2025-03-17T22:27:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> >> +extern int not_supposed_to_survive;\n>> >> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n>> >\n>> > , which serve as the core trick, I had used elsewhere before.\n>>\n>> It may be arguable that it is too small to be copyrightable and\n>> there is no other way to express the idea behind that check, but\n>> in any case ...\n>\n> That's what I had been assuming, but then you, brian, and Taylor all\n> pointed out how clever it was making me think otherwise.\n\nHeh, cleverness lies in the idea, not the expression, and copyright\nis about expression.\n\n> Turns out we don't need an alternative. I contacted the author, who\n> responded and placed the two-liner into the public domain with no\n> warranty of any kind. I'll send a re-roll with an updated commit\n> message.\n\nWonderful. That is the best solution.\n\nThanks.\n"},{"id":"514451","messageId":"Z9iinbyrn4PeFmCl@nand.local","threadId":"63117","inReplyTo":"58cb8f6a1609b10d761e86bdad541d1c018cb582.1742107322.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-17T22:30:53Z","receivedAt":"2025-03-17T22:30:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Mar 16, 2025 at 06:42:01AM +0000, Elijah Newren via GitGitGadget wrote:\n> We have roughly 566 assert() calls in our codebase (my grep might have\n> picked up things that aren't actually assert() calls, but most appeared\n> to be). All but 9 of them can be determined by gcc to be free of side\n> effects with a clever redefine of assert() provided by Bruno De Fraine\n> (from\n> https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects),\n> who upon request has graciously placed his two-liner into the public\n> domain without warranty of any kind. The current 9 assert() calls\n> flagged by this clever redefinition of assert() appear to me to be free\n> of side effects as well, but are too complicated for a compiler/linker\n> to figure that since each assertion involves some kind of function call.\n> Add a CI job which will find and report these possibly problematic\n> assertions, and have the job suggest to the user that they replace these\n> with BUG_IF_NOT() calls.\n\nVery nice, and thank you Bruno for placing your very clever assert() in\nthe public domain :-).\n\nI wonder if it might be useful to explain this in\nDocumentation/CodingGuidelines as a follow-up to this series. I was\nthinking of a scenario where someone either writes a side-effecting\nassert(), or a non-side-effecting one that is too complicated to prove\notherwise.\n\nIf that person runs 'make test' locally, they might not see any\nfailures, but then be surprised when CI fails on the new step. It may be\nworth mentioning that we have such a check, and that we expect all\nassert() statements to be side effect-free, and that developers can\nverify this by ci/check-unsafe-assertions.sh.\n\nBut that may bring us into an assert() versus BUG_IF_NOT() debate, which\nmay be somewhat counterproductive, so I'm just as happy if you did\nnothing here :-).\n\nThanks,\nTaylor\n"},{"id":"514452","messageId":"Z9ijIkCU+Tmt9H8i@nand.local","threadId":"63117","inReplyTo":"20c763f295105bda9a701b9bf5b9aa47af5bf1e1.1742107322.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-17T22:33:06Z","receivedAt":"2025-03-17T22:33:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Mar 16, 2025 at 06:42:02AM +0000, Elijah Newren via GitGitGadget wrote:\n> When the compiler/linker cannot verify that an assert() invocation is\n> free of side effects for us (e.g. because the assertion includes some\n> kind of function call), replace the use of assert() with BUG_IF_NOT().\n\nNice. I guess since this is split out into its own patch, we wouldn't be\nable to cleanly run CI on the previous commit, but I think that's fine,\nsince we don't treat CI as being nearly as precious as being able to\n'make' anywhere in history.\n\nYou didn't realign any multi-line assert() statements, but I actually\nthink that's preferable in this case to demonstrate that the patch does\nnot change the behavior of these assertions.\n\nThanks,\nTaylor\n"},{"id":"514453","messageId":"xmqqjz8n70dt.fsf@gitster.g","threadId":"63117","inReplyTo":"109060ccb8665c73aa0c4f73e3cbbddcd135bde4.1742107322.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-17T22:33:50Z","receivedAt":"2025-03-17T22:33:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> Create a BUG_IF_NOT() macro which is similar to assert(), but will not be\n> compiled out when NDEBUG is defined, and is thus safe to use even if its\n> argument has side-effects.\n\nIf this is meant to be \"similar to\" assert, let's not call it\nBUG_IF_NOT(). The point of BUG() is that the developer can mark the\nproblem with something more than just a conditional, and it feels\nfunny to call a facility that lacks that central feature with a name\nwith BUG in it.\n\nASSERT(), safer_assert(), safe_assert(), sane_assert()? \n\nThe last one is in line with safe_istest() that is used on\nsane_ctype[] and sane_qsort(), with the intention to allow\ndevelopers to write right code more easily than using the plain\nvanilla C.\n\nThanks.\n"},{"id":"514454","messageId":"Z9ijYliQPX7PI/0h@nand.local","threadId":"63117","inReplyTo":"pull.1881.v2.git.1742107322.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-17T22:34:10Z","receivedAt":"2025-03-17T22:34:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Mar 16, 2025 at 06:41:59AM +0000, Elijah Newren via GitGitGadget wrote:\n> Elijah Newren (3):\n> git-compat-util: introduce BUG_IF_NOT() macro\n> ci: add build checking for side-effects in assert() calls\n> treewide: replace assert() with BUG_IF_NOT() in special cases\n\nNice, this version looks great to me. I left a couple of notes\nthroughout, but they range from \"could be done later\" to idle\ncommentary. Thanks for working on this, and I'm sorry to have sent you\ndown such a rabbit hole ;-).\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"514455","messageId":"Z9ij72MASlTLT6+8@nand.local","threadId":"63117","inReplyTo":"xmqqjz8n70dt.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-17T22:36:31Z","receivedAt":"2025-03-17T22:36:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 17, 2025 at 03:33:50PM -0700, Junio C Hamano wrote:\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > From: Elijah Newren \n> >\n> > Create a BUG_IF_NOT() macro which is similar to assert(), but will not be\n> > compiled out when NDEBUG is defined, and is thus safe to use even if its\n> > argument has side-effects.\n>\n> If this is meant to be \"similar to\" assert, let's not call it\n> BUG_IF_NOT(). The point of BUG() is that the developer can mark the\n> problem with something more than just a conditional, and it feels\n> funny to call a facility that lacks that central feature with a name\n> with BUG in it.\n>\n> ASSERT(), safer_assert(), safe_assert(), sane_assert()?\n>\n> The last one is in line with safe_istest() that is used on\n> sane_ctype[] and sane_qsort(), with the intention to allow\n> developers to write right code more easily than using the plain\n> vanilla C.\n\nFor my $.02, I prefer ASSERT() to the other options. It's clear, but\nindicates that it's a macro and thus not the same as assert(3). But I\ndon't have a strong opinion here.\n\nThanks,\nTaylor\n"},{"id":"514456","messageId":"xmqqfrjb707x.fsf@gitster.g","threadId":"63117","inReplyTo":"58cb8f6a1609b10d761e86bdad541d1c018cb582.1742107322.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-17T22:37:22Z","receivedAt":"2025-03-17T22:37:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> ... All but 9 of them can be determined by gcc to be free of side\n> effects with a clever redefine of assert() provided by Bruno De Fraine\n> (from\n> https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects),\n> who upon request has graciously placed his two-liner into the public\n> domain without warranty of any kind.\n\nNice.\n\n> diff --git a/ci/check-unsafe-assertions.sh b/ci/check-unsafe-assertions.sh\n> new file mode 100755\n> index 00000000000..d66091efd22\n> --- /dev/null\n> +++ b/ci/check-unsafe-assertions.sh\n> @@ -0,0 +1,18 @@\n> +#!/bin/sh\n> +\n> +make CHECK_ASSERTION_SIDE_EFFECTS=1 >compiler_output 2>compiler_error\n> +if test $? != 0\n> +then\n> + echo \"ERROR: The compiler could not verify the following assert()\" >&2\n> + echo \" calls are free of side-effects. Please replace with\" >&2\n> + echo \" BUG_IF_NOT() calls.\" >&2\n> + grep undefined.reference.to..not_supposed_to_survive compiler_error \\\n> + | sed -e s/:[^:]*$// | sort | uniq | tr ':' ' ' \\\n> + | while read f l\n\nLet's lose the unsightly backslash by ending each line with \"|\"\ninstead.\n\nAlso let's stick to HT indentation, not whitespaces.\n\nThanks.\n"},{"id":"514660","messageId":"CABPp-BHxYec4LBTrhg+Bq9NrpyGXkt8fBgofuy=D3X2ArM6wiQ@mail.gmail.com","threadId":"63117","inReplyTo":"Z9iinbyrn4PeFmCl@nand.local","subject":"Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-19T16:21:59Z","receivedAt":"2025-03-19T16:22:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Mar 17, 2025 at 3:30 PM Taylor Blau wrote:\n>\n> On Sun, Mar 16, 2025 at 06:42:01AM +0000, Elijah Newren via GitGitGadget wrote:\n> > We have roughly 566 assert() calls in our codebase (my grep might have\n> > picked up things that aren't actually assert() calls, but most appeared\n> > to be). All but 9 of them can be determined by gcc to be free of side\n> > effects with a clever redefine of assert() provided by Bruno De Fraine\n> > (from\n> > https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects),\n> > who upon request has graciously placed his two-liner into the public\n> > domain without warranty of any kind. The current 9 assert() calls\n> > flagged by this clever redefinition of assert() appear to me to be free\n> > of side effects as well, but are too complicated for a compiler/linker\n> > to figure that since each assertion involves some kind of function call.\n> > Add a CI job which will find and report these possibly problematic\n> > assertions, and have the job suggest to the user that they replace these\n> > with BUG_IF_NOT() calls.\n>\n> Very nice, and thank you Bruno for placing your very clever assert() in\n> the public domain :-).\n>\n> I wonder if it might be useful to explain this in\n> Documentation/CodingGuidelines as a follow-up to this series. I was\n> thinking of a scenario where someone either writes a side-effecting\n> assert(), or a non-side-effecting one that is too complicated to prove\n> otherwise.\n>\n> If that person runs 'make test' locally, they might not see any\n> failures, but then be surprised when CI fails on the new step. It may be\n> worth mentioning that we have such a check, and that we expect all\n> assert() statements to be side effect-free, and that developers can\n> verify this by ci/check-unsafe-assertions.sh.\n\nThe same could be said for coccinelle patches, hdr-check, check-pot,\nfuzz tests, asan/ubsan, GIT_TEST_SPLIT_INDEX, pedantic build, osx, vs.\nwindows vs. linux, and perhaps others, which users won't catch on\n'make test' locally but can result in failed CI builds and aren't\nmentioned in CodingGuidelines. I usually think of CodingGuidelines as\nbeing the place for documenting things that can't be tested in an\nautomated fashion, and a brief mention that both cross platform and\nadditional more thorough but non-default tests can go in\nSubmittingPatches.\n\n> But that may bring us into an assert() versus BUG_IF_NOT() debate, which\n> may be somewhat counterproductive, so I'm just as happy if you did\n> nothing here :-).\n\n:-)\n"},{"id":"514662","messageId":"pull.1881.v3.git.1742401378.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.v2.git.1742107322.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] Add a static analysis job to prevent assertions with side effects","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-19T16:22:55Z","receivedAt":"2025-03-19T16:23:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We have several hundred assert() invocations in our code base. Some have\nsuggested that we should add a recommendation in our CodingGuidelines to\navoid their use, because there is a risk that someone might include\nsomething with a side-effect in their assertion, which can lead to a very\ndifficult to debug problem. However, CodingGuidelines are going to be less\neffective at preventing that foot-gun than a CI job which can warn of\nassertions that possibly have side-effects. So, let's add a CI job instead.\n\nWhile it is difficult to perfectly determine whether any expression has side\neffects, a simple compiler/linker hack can prove that all but 9 of our\nseveral hundred assert() calls are indeed free from them. While I believe\nthe remaining 9 are also free of side effects, it's easier to just convert\nthose 9 to a new macro (which will not be compiled out when NDEBUG is\ndefined), and instruct any future assertion writers to likewise switch to\nthat alternative macro if they have a slightly more involved assert()\ninvocation.\n\nSee\nhttps://github.com/newren/git/actions/runs/13845548634/job/38743076293#step:4:1938\nfor an example of it running in CI and reporting possibly problematic\nassertions (sample output also included in the commit message of the middle\ncommit in this series if you don't have access to view the link; I'm not\nsure what the rules on that are).\n\nChanges since v1:\n\n * Tweaked commit message for patch 2 Changes since v2:\n * Rename BUT_IF_NOT() -> ASSERT(). Didn't have a strong opinion on the set\n of alternatives Junio gave, so went with Taylor's small preference. If\n anyone has a strong preference here, I can pick a different alternative.\n * Fixed shell style issues (indentation, multi-line pipes, multiple lines\n with stderr redirects) in patch 2\n\nElijah Newren (3):\n git-compat-util: introduce ASSERT() macro\n ci: add build checking for side-effects in assert() calls\n treewide: replace assert() with ASSERT() in special cases\n\n Makefile | 4 ++++\n ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++\n ci/run-static-analysis.sh | 2 ++\n diffcore-rename.c | 2 +-\n git-compat-util.h | 8 ++++++++\n merge-ort.c | 4 ++--\n merge-recursive.c | 2 +-\n object-file.c | 2 +-\n parallel-checkout.c | 2 +-\n scalar.c | 4 ++--\n sequencer.c | 2 +-\n 11 files changed, 41 insertions(+), 9 deletions(-)\n create mode 100755 ci/check-unsafe-assertions.sh\n\n\nbase-commit: 4b68faf6b93311254efad80e554780e372deb42f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1881%2Fnewren%2Fassertion-side-effects-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1881/newren/assertion-side-effects-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1881\n\nRange-diff vs v2:\n\n 1: 109060ccb86 ! 1: d22ff3e3f97 git-compat-util: introduce BUG_IF_NOT() macro\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - git-compat-util: introduce BUG_IF_NOT() macro\n + git-compat-util: introduce ASSERT() macro\n \n - Create a BUG_IF_NOT() macro which is similar to assert(), but will not be\n + Create a ASSERT() macro which is similar to assert(), but will not be\n compiled out when NDEBUG is defined, and is thus safe to use even if its\n argument has side-effects.\n \n We will use this new macro in a subsequent commit to convert a few\n - existing assert() invocations to BUG_IF_NOT(). In particular, we'll\n + existing assert() invocations to ASSERT(). In particular, we'll\n convert the handful of invocations which cannot be proven to be free of\n side effects with a simple compiler/linker hack.\n \n @@ git-compat-util.h: extern int bug_called_must_BUG;\n __attribute__((format (printf, 3, 4))) NORETURN\n void BUG_fl(const char *file, int line, const char *fmt, ...);\n #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)\n -+#define BUG_IF_NOT(a) if (!(a)) BUG(\"Assertion `\" #a \"' failed.\")\n ++/* ASSERT: like assert(), but won't be compiled out with NDEBUG */\n ++#define ASSERT(a) if (!(a)) BUG(\"Assertion `\" #a \"' failed.\")\n __attribute__((format (printf, 3, 4)))\n void bug_fl(const char *file, int line, const char *fmt, ...);\n #define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)\n 2: 58cb8f6a160 ! 2: 223d8c0ca2a ci: add build checking for side-effects in assert() calls\n @@ Commit message\n to figure that since each assertion involves some kind of function call.\n Add a CI job which will find and report these possibly problematic\n assertions, and have the job suggest to the user that they replace these\n - with BUG_IF_NOT() calls.\n + with ASSERT() calls.\n \n Example output from running:\n \n ```\n ERROR: The compiler could not verify the following assert()\n calls are free of side-effects. Please replace with\n - BUG_IF_NOT() calls.\n + ASSERT() calls.\n /home/newren/floss/git/diffcore-rename.c:1409\n assert(!dir_rename_count || strmap_empty(dir_rename_count));\n /home/newren/floss/git/merge-ort.c:1645\n @@ ci/check-unsafe-assertions.sh (new)\n +make CHECK_ASSERTION_SIDE_EFFECTS=1 >compiler_output 2>compiler_error\n +if test $? != 0\n +then\n -+ echo \"ERROR: The compiler could not verify the following assert()\" >&2\n -+ echo \" calls are free of side-effects. Please replace with\" >&2\n -+ echo \" BUG_IF_NOT() calls.\" >&2\n -+ grep undefined.reference.to..not_supposed_to_survive compiler_error \\\n -+ | sed -e s/:[^:]*$// | sort | uniq | tr ':' ' ' \\\n -+ | while read f l\n -+ do\n -+\tprintf \"${f}:${l}\\n \"\n -+\tawk -v start=\"$l\" 'NR >= start { print; if (/\\);/) exit }' $f\n -+ done\n -+ exit 1\n ++\techo >&2 \"ERROR: The compiler could not verify the following assert()\"\n ++\techo >&2 \" calls are free of side-effects. Please replace with\"\n ++\techo >&2 \" ASSERT() calls.\"\n ++\tgrep undefined.reference.to..not_supposed_to_survive compiler_error |\n ++\t\tsed -e s/:[^:]*$// | sort | uniq | tr ':' ' ' |\n ++\t\twhile read f l\n ++\t\tdo\n ++\t\t\tprintf \"${f}:${l}\\n \"\n ++\t\t\tawk -v start=\"$l\" 'NR >= start { print; if (/\\);/) exit }' $f\n ++\t\tdone\n ++\texit 1\n +fi\n +rm compiler_output compiler_error\n \n 3: 20c763f2951 ! 3: 82b7344e966 treewide: replace assert() with BUG_IF_NOT() in special cases\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - treewide: replace assert() with BUG_IF_NOT() in special cases\n + treewide: replace assert() with ASSERT() in special cases\n \n When the compiler/linker cannot verify that an assert() invocation is\n free of side effects for us (e.g. because the assertion includes some\n - kind of function call), replace the use of assert() with BUG_IF_NOT().\n + kind of function call), replace the use of assert() with ASSERT().\n \n Signed-off-by: Elijah Newren \n \n @@ diffcore-rename.c: void diffcore_rename_extended(struct diff_options *options,\n \ttrace2_region_enter(\"diff\", \"setup\", options->repo);\n \tinfo.setup = 0;\n -\tassert(!dir_rename_count || strmap_empty(dir_rename_count));\n -+\tBUG_IF_NOT(!dir_rename_count || strmap_empty(dir_rename_count));\n ++\tASSERT(!dir_rename_count || strmap_empty(dir_rename_count));\n \twant_copies = (detect_rename == DIFF_DETECT_COPY);\n \tif (dirs_removed && (break_idx || want_copies))\n \t\tBUG(\"dirs_removed incompatible with break/copy detection\");\n @@ merge-ort.c: static void path_msg(struct merge_options *opt,\n \n \t/* Sanity checks */\n -\tassert(omittable_hint ==\n -+\tBUG_IF_NOT(omittable_hint ==\n ++\tASSERT(omittable_hint ==\n \t (!starts_with(type_short_descriptions[type], \"CONFLICT\") &&\n \t\t!starts_with(type_short_descriptions[type], \"ERROR\")) ||\n \t type == CONFLICT_DIR_RENAME_SUGGESTED);\n @@ merge-ort.c: static int handle_deferred_entries(struct merge_options *opt,\n \t\t\tVERIFY_CI(ci);\n \n -\t\t\tassert(renames->deferred[side].trivial_merges_okay &&\n -+\t\t\tBUG_IF_NOT(renames->deferred[side].trivial_merges_okay &&\n ++\t\t\tASSERT(renames->deferred[side].trivial_merges_okay &&\n \t\t\t !strset_contains(&renames->deferred[side].target_dirs,\n \t\t\t\t\t\tpath));\n \t\t\tresolve_trivial_directory_merge(ci, side);\n @@ merge-recursive.c: static void print_commit(struct repository *repo, struct comm\n \tctx.date_mode.type = DATE_NORMAL;\n \t/* FIXME: Merge this with output_commit_title() */\n -\tassert(!merge_remote_util(commit));\n -+\tBUG_IF_NOT(!merge_remote_util(commit));\n ++\tASSERT(!merge_remote_util(commit));\n \trepo_format_commit_message(repo, commit, \" %h: %m %s\", &sb, &ctx);\n \tfprintf(stderr, \"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\n @@ object-file.c: static int index_stream_convert_blob(struct index_state *istate,\n \n \tassert(path);\n -\tassert(would_convert_to_git_filter_fd(istate, path));\n -+\tBUG_IF_NOT(would_convert_to_git_filter_fd(istate, path));\n ++\tASSERT(would_convert_to_git_filter_fd(istate, path));\n \n \tconvert_to_git_filter_fd(istate, path, fd, &sbuf,\n \t\t\t\t get_conv_flags(flags));\n @@ parallel-checkout.c: static int write_pc_item_to_fd(struct parallel_checkout_ite\n \n \t/* Sanity check */\n -\tassert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n -+\tBUG_IF_NOT(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n ++\tASSERT(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n \n \tfilter = get_stream_filter_ca(&pc_item->ca, &pc_item->ce->oid);\n \tif (filter) {\n @@ scalar.c: static int add_or_remove_enlistment(int add)\n static int start_fsmonitor_daemon(void)\n {\n -\tassert(have_fsmonitor_support());\n -+\tBUG_IF_NOT(have_fsmonitor_support());\n ++\tASSERT(have_fsmonitor_support());\n \n \tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n \t\treturn run_git(\"fsmonitor--daemon\", \"start\", NULL);\n @@ scalar.c: static int start_fsmonitor_daemon(void)\n static int stop_fsmonitor_daemon(void)\n {\n -\tassert(have_fsmonitor_support());\n -+\tBUG_IF_NOT(have_fsmonitor_support());\n ++\tASSERT(have_fsmonitor_support());\n \n \tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n \t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\n @@ sequencer.c: static int pick_commits(struct repository *r,\n \tctx->reflog_message = sequencer_reflog_action(opts);\n \tif (opts->allow_ff)\n -\t\tassert(!(opts->signoff || opts->no_commit ||\n -+\t\tBUG_IF_NOT(!(opts->signoff || opts->no_commit ||\n ++\t\tASSERT(!(opts->signoff || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n \t\t\t opts->committer_date_is_author_date ||\n \t\t\t opts->ignore_date));\n\n-- \ngitgitgadget\n"},{"id":"514663","messageId":"d22ff3e3f97dfa1b88e78b81d85d9bc66c6a74d1.1742401378.git.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.v3.git.1742401378.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] git-compat-util: introduce ASSERT() macro","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-19T16:22:56Z","receivedAt":"2025-03-19T16:23:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCreate a ASSERT() macro which is similar to assert(), but will not be\ncompiled out when NDEBUG is defined, and is thus safe to use even if its\nargument has side-effects.\n\nWe will use this new macro in a subsequent commit to convert a few\nexisting assert() invocations to ASSERT(). In particular, we'll\nconvert the handful of invocations which cannot be proven to be free of\nside effects with a simple compiler/linker hack.\n\nSigned-off-by: Elijah Newren \n---\n git-compat-util.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e123288e8f1..d7f3407128c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1460,6 +1460,8 @@ extern int bug_called_must_BUG;\n __attribute__((format (printf, 3, 4))) NORETURN\n void BUG_fl(const char *file, int line, const char *fmt, ...);\n #define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)\n+/* ASSERT: like assert(), but won't be compiled out with NDEBUG */\n+#define ASSERT(a) if (!(a)) BUG(\"Assertion `\" #a \"' failed.\")\n __attribute__((format (printf, 3, 4)))\n void bug_fl(const char *file, int line, const char *fmt, ...);\n #define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)\n-- \ngitgitgadget\n\n"},{"id":"514664","messageId":"223d8c0ca2a9f78ae9e281773037cc5673f0637a.1742401378.git.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.v3.git.1742401378.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-19T16:22:57Z","receivedAt":"2025-03-19T16:23:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIt is a big no-no to have side-effects in an assertion, because if the\nassert() is compiled out, you don't get that side-effect, leading to the\ncode behaving differently. That can be a large headache to debug.\n\nWe have roughly 566 assert() calls in our codebase (my grep might have\npicked up things that aren't actually assert() calls, but most appeared\nto be). All but 9 of them can be determined by gcc to be free of side\neffects with a clever redefine of assert() provided by Bruno De Fraine\n(from\nhttps://stackoverflow.com/questions/10593492/catching-assert-with-side-effects),\nwho upon request has graciously placed his two-liner into the public\ndomain without warranty of any kind. The current 9 assert() calls\nflagged by this clever redefinition of assert() appear to me to be free\nof side effects as well, but are too complicated for a compiler/linker\nto figure that since each assertion involves some kind of function call.\nAdd a CI job which will find and report these possibly problematic\nassertions, and have the job suggest to the user that they replace these\nwith ASSERT() calls.\n\nExample output from running:\n\n```\nERROR: The compiler could not verify the following assert()\n calls are free of side-effects. Please replace with\n ASSERT() calls.\n/home/newren/floss/git/diffcore-rename.c:1409\n\tassert(!dir_rename_count || strmap_empty(dir_rename_count));\n/home/newren/floss/git/merge-ort.c:1645\n\t\t\tassert(renames->deferred[side].trivial_merges_okay &&\n\t\t\t !strset_contains(&renames->deferred[side].target_dirs,\n\t\t\t\t\t\tpath));\n/home/newren/floss/git/merge-ort.c:794\n\tassert(omittable_hint ==\n\t (!starts_with(type_short_descriptions[type], \"CONFLICT\") &&\n\t\t!starts_with(type_short_descriptions[type], \"ERROR\")) ||\n\t type == CONFLICT_DIR_RENAME_SUGGESTED);\n/home/newren/floss/git/merge-recursive.c:1200\n\tassert(!merge_remote_util(commit));\n/home/newren/floss/git/object-file.c:2709\n\tassert(would_convert_to_git_filter_fd(istate, path));\n/home/newren/floss/git/parallel-checkout.c:280\n\tassert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n/home/newren/floss/git/scalar.c:244\n\tassert(have_fsmonitor_support());\n/home/newren/floss/git/scalar.c:254\n\tassert(have_fsmonitor_support());\n/home/newren/floss/git/sequencer.c:4968\n\t\tassert(!(opts->signoff || opts->no_commit ||\n\t\t\t opts->record_origin || should_edit(opts) ||\n\t\t\t opts->committer_date_is_author_date ||\n\t\t\t opts->ignore_date));\n```\n\nNote that if there are possibly problematic assertions, not necessarily\nall of them will be shown in a single run, because the compiler errors\nmay include something like \"ld: ... more undefined references to\n`not_supposed_to_survive' follow\" instead of listing each individually.\nBut in such cases, once you clean up a few that are shown in your first\nrun, subsequent runs will show (some of) the ones that remain, allowing\nyou to iteratively remove them all.\n\nHelped-by: Bruno De Fraine \nSigned-off-by: Elijah Newren \n---\n Makefile | 4 ++++\n ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++\n ci/run-static-analysis.sh | 2 ++\n git-compat-util.h | 6 ++++++\n 4 files changed, 30 insertions(+)\n create mode 100755 ci/check-unsafe-assertions.sh\n\ndiff --git a/Makefile b/Makefile\nindex 7315507381e..57774912f18 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2261,6 +2261,10 @@ ifdef WITH_BREAKING_CHANGES\n \tBASIC_CFLAGS += -DWITH_BREAKING_CHANGES\n endif\n \n+ifdef CHECK_ASSERTION_SIDE_EFFECTS\n+\tBASIC_CFLAGS += -DCHECK_ASSERTION_SIDE_EFFECTS\n+endif\n+\n ifdef INCLUDE_LIBGIT_RS\n \t# Enable symbol hiding in contrib/libgit-sys/libgitpub.a without making\n \t# us rebuild the whole tree every time we run a Rust build.\ndiff --git a/ci/check-unsafe-assertions.sh b/ci/check-unsafe-assertions.sh\nnew file mode 100755\nindex 00000000000..233bd9dfbc4\n--- /dev/null\n+++ b/ci/check-unsafe-assertions.sh\n@@ -0,0 +1,18 @@\n+#!/bin/sh\n+\n+make CHECK_ASSERTION_SIDE_EFFECTS=1 >compiler_output 2>compiler_error\n+if test $? != 0\n+then\n+\techo >&2 \"ERROR: The compiler could not verify the following assert()\"\n+\techo >&2 \" calls are free of side-effects. Please replace with\"\n+\techo >&2 \" ASSERT() calls.\"\n+\tgrep undefined.reference.to..not_supposed_to_survive compiler_error |\n+\t\tsed -e s/:[^:]*$// | sort | uniq | tr ':' ' ' |\n+\t\twhile read f l\n+\t\tdo\n+\t\t\tprintf \"${f}:${l}\\n \"\n+\t\t\tawk -v start=\"$l\" 'NR >= start { print; if (/\\);/) exit }' $f\n+\t\tdone\n+\texit 1\n+fi\n+rm compiler_output compiler_error\ndiff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh\nindex 0d51e5ce0e7..ae714e020ae 100755\n--- a/ci/run-static-analysis.sh\n+++ b/ci/run-static-analysis.sh\n@@ -31,4 +31,6 @@ exit 1\n \n make check-pot\n \n+${0%/*}/check-unsafe-assertions.sh\n+\n save_good_tree\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex d7f3407128c..5891efaeb18 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1585,4 +1585,10 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)\n \t((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))\n #endif /* !__GNUC__ */\n \n+#ifdef CHECK_ASSERTION_SIDE_EFFECTS\n+#undef assert\n+extern int not_supposed_to_survive;\n+#define assert(expr) ((void)(not_supposed_to_survive || (expr)))\n+#endif /* CHECK_ASSERTION_SIDE_EFFECTS */\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"514665","messageId":"82b7344e96683efdac5dfc559a023d81ea0084d0.1742401378.git.gitgitgadget@gmail.com","threadId":"63117","inReplyTo":"pull.1881.v3.git.1742401378.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-19T16:22:58Z","receivedAt":"2025-03-19T16:23:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen the compiler/linker cannot verify that an assert() invocation is\nfree of side effects for us (e.g. because the assertion includes some\nkind of function call), replace the use of assert() with ASSERT().\n\nSigned-off-by: Elijah Newren \n---\n diffcore-rename.c | 2 +-\n merge-ort.c | 4 ++--\n merge-recursive.c | 2 +-\n object-file.c | 2 +-\n parallel-checkout.c | 2 +-\n scalar.c | 4 ++--\n sequencer.c | 2 +-\n 7 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 91b77993c78..624304f0416 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -1406,7 +1406,7 @@ void diffcore_rename_extended(struct diff_options *options,\n \n \ttrace2_region_enter(\"diff\", \"setup\", options->repo);\n \tinfo.setup = 0;\n-\tassert(!dir_rename_count || strmap_empty(dir_rename_count));\n+\tASSERT(!dir_rename_count || strmap_empty(dir_rename_count));\n \twant_copies = (detect_rename == DIFF_DETECT_COPY);\n \tif (dirs_removed && (break_idx || want_copies))\n \t\tBUG(\"dirs_removed incompatible with break/copy detection\");\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 46e78c3ffa6..d484f16cf2b 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -791,7 +791,7 @@ static void path_msg(struct merge_options *opt,\n \tstruct strbuf tmp = STRBUF_INIT;\n \n \t/* Sanity checks */\n-\tassert(omittable_hint ==\n+\tASSERT(omittable_hint ==\n \t (!starts_with(type_short_descriptions[type], \"CONFLICT\") &&\n \t\t!starts_with(type_short_descriptions[type], \"ERROR\")) ||\n \t type == CONFLICT_DIR_RENAME_SUGGESTED);\n@@ -1642,7 +1642,7 @@ static int handle_deferred_entries(struct merge_options *opt,\n \t\t\tci = strmap_get(&opt->priv->paths, path);\n \t\t\tVERIFY_CI(ci);\n \n-\t\t\tassert(renames->deferred[side].trivial_merges_okay &&\n+\t\t\tASSERT(renames->deferred[side].trivial_merges_okay &&\n \t\t\t !strset_contains(&renames->deferred[side].target_dirs,\n \t\t\t\t\t\tpath));\n \t\t\tresolve_trivial_directory_merge(ci, side);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 884ccf99a58..4fbbece922c 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1197,7 +1197,7 @@ static void print_commit(struct repository *repo, struct commit *commit)\n \tstruct pretty_print_context ctx = {0};\n \tctx.date_mode.type = DATE_NORMAL;\n \t/* FIXME: Merge this with output_commit_title() */\n-\tassert(!merge_remote_util(commit));\n+\tASSERT(!merge_remote_util(commit));\n \trepo_format_commit_message(repo, commit, \" %h: %m %s\", &sb, &ctx);\n \tfprintf(stderr, \"%s\\n\", sb.buf);\n \tstrbuf_release(&sb);\ndiff --git a/object-file.c b/object-file.c\nindex 726e41a0475..4fb3cd9dcb9 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2706,7 +2706,7 @@ static int index_stream_convert_blob(struct index_state *istate,\n \tstruct strbuf sbuf = STRBUF_INIT;\n \n \tassert(path);\n-\tassert(would_convert_to_git_filter_fd(istate, path));\n+\tASSERT(would_convert_to_git_filter_fd(istate, path));\n \n \tconvert_to_git_filter_fd(istate, path, fd, &sbuf,\n \t\t\t\t get_conv_flags(flags));\ndiff --git a/parallel-checkout.c b/parallel-checkout.c\nindex 7cc6b305281..57c2dcaa8f6 100644\n--- a/parallel-checkout.c\n+++ b/parallel-checkout.c\n@@ -277,7 +277,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd,\n \tssize_t wrote;\n \n \t/* Sanity check */\n-\tassert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n+\tASSERT(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca));\n \n \tfilter = get_stream_filter_ca(&pc_item->ca, &pc_item->ce->oid);\n \tif (filter) {\ndiff --git a/scalar.c b/scalar.c\nindex da42b4be0cc..d359f08bb8e 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -241,7 +241,7 @@ static int add_or_remove_enlistment(int add)\n \n static int start_fsmonitor_daemon(void)\n {\n-\tassert(have_fsmonitor_support());\n+\tASSERT(have_fsmonitor_support());\n \n \tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n \t\treturn run_git(\"fsmonitor--daemon\", \"start\", NULL);\n@@ -251,7 +251,7 @@ static int start_fsmonitor_daemon(void)\n \n static int stop_fsmonitor_daemon(void)\n {\n-\tassert(have_fsmonitor_support());\n+\tASSERT(have_fsmonitor_support());\n \n \tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n \t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\ndiff --git a/sequencer.c b/sequencer.c\nindex ad0ab75c8d4..c625a39111e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4965,7 +4965,7 @@ static int pick_commits(struct repository *r,\n \n \tctx->reflog_message = sequencer_reflog_action(opts);\n \tif (opts->allow_ff)\n-\t\tassert(!(opts->signoff || opts->no_commit ||\n+\t\tASSERT(!(opts->signoff || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n \t\t\t opts->committer_date_is_author_date ||\n \t\t\t opts->ignore_date));\n-- \ngitgitgadget\n"},{"id":"514688","messageId":"Z9tEilK0OOEBX/JQ@nand.local","threadId":"63117","inReplyTo":"CABPp-BHxYec4LBTrhg+Bq9NrpyGXkt8fBgofuy=D3X2ArM6wiQ@mail.gmail.com","subject":"Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T22:26:18Z","receivedAt":"2025-03-19T22:26:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 19, 2025 at 09:21:59AM -0700, Elijah Newren wrote:\n> > I wonder if it might be useful to explain this in\n> > Documentation/CodingGuidelines as a follow-up to this series. I was\n> > thinking of a scenario where someone either writes a side-effecting\n> > assert(), or a non-side-effecting one that is too complicated to prove\n> > otherwise.\n> >\n> > If that person runs 'make test' locally, they might not see any\n> > failures, but then be surprised when CI fails on the new step. It may be\n> > worth mentioning that we have such a check, and that we expect all\n> > assert() statements to be side effect-free, and that developers can\n> > verify this by ci/check-unsafe-assertions.sh.\n>\n> The same could be said for coccinelle patches, hdr-check, check-pot,\n> fuzz tests, asan/ubsan, GIT_TEST_SPLIT_INDEX, pedantic build, osx, vs.\n> windows vs. linux, and perhaps others, which users won't catch on\n> 'make test' locally but can result in failed CI builds and aren't\n> mentioned in CodingGuidelines. I usually think of CodingGuidelines as\n> being the place for documenting things that can't be tested in an\n> automated fashion, and a brief mention that both cross platform and\n> additional more thorough but non-default tests can go in\n> SubmittingPatches.\n\nFair enough ;-).\n\nThanks,\nTaylor\n"},{"id":"514689","messageId":"Z9tE2rQxT+qQP2mh@nand.local","threadId":"63117","inReplyTo":"82b7344e96683efdac5dfc559a023d81ea0084d0.1742401378.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T22:27:38Z","receivedAt":"2025-03-19T22:27:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 19, 2025 at 04:22:58PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n>\n> When the compiler/linker cannot verify that an assert() invocation is\n> free of side effects for us (e.g. because the assertion includes some\n> kind of function call), replace the use of assert() with ASSERT().\n\nAs a nice side-benefit, since our new ASSERT() macro naturally has the\nsame number of characters as a bog-standard assert(), we don't have to\nrealign any multi-line assertions below ;-).\n\nThanks,\nTaylor\n"},{"id":"514690","messageId":"Z9tE7Bo6uVkXUaEC@nand.local","threadId":"63117","inReplyTo":"pull.1881.v3.git.1742401378.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/3] Add a static analysis job to prevent assertions with side effects","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T22:27:56Z","receivedAt":"2025-03-19T22:27:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 19, 2025 at 04:22:55PM +0000, Elijah Newren via GitGitGadget wrote:\n> Changes since v1:\n>\n> * Tweaked commit message for patch 2 Changes since v2:\n> * Rename BUT_IF_NOT() -> ASSERT(). Didn't have a strong opinion on the set\n> of alternatives Junio gave, so went with Taylor's small preference. If\n> anyone has a strong preference here, I can pick a different alternative.\n> * Fixed shell style issues (indentation, multi-line pipes, multiple lines\n> with stderr redirects) in patch 2\n>\n> Elijah Newren (3):\n> git-compat-util: introduce ASSERT() macro\n> ci: add build checking for side-effects in assert() calls\n> treewide: replace assert() with ASSERT() in special cases\n>\n> Makefile | 4 ++++\n> ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++\n> ci/run-static-analysis.sh | 2 ++\n> diffcore-rename.c | 2 +-\n> git-compat-util.h | 8 ++++++++\n> merge-ort.c | 4 ++--\n> merge-recursive.c | 2 +-\n> object-file.c | 2 +-\n> parallel-checkout.c | 2 +-\n> scalar.c | 4 ++--\n> sequencer.c | 2 +-\n> 11 files changed, 41 insertions(+), 9 deletions(-)\n> create mode 100755 ci/check-unsafe-assertions.sh\n\nThanks, this version LGTM.\n\nThanks,\nTaylor\n"}]}