[PATCH 0/3] Add a static analysis job to prevent assertions with side effects

We have several hundred assert() invocations in our code base. Some have suggested that we should add a recommendation in our CodingGuidelines to avoid their use, because there is a risk that someone might include something with a side-effect in their assertion, which can lead to a very difficult to debug problem. However, CodingGuidelines are going to be less effective at preventing that foot-gun than a CI job which can warn of assertions that possibly have side-effects. So, let's add a CI job instead.

While it is difficult to perfectly determine whether any expression has side effects, a simple compiler/linker hack can prove that all but 9 of our several hundred assert() calls are indeed free from them. While I believe the remaining 9 are also free of side effects, it's easier to just convert those 9 to a new macro (which will not be compiled out when NDEBUG is defined), and instruct any future assertion writers to likewise switch to that alternative macro if they have a slightly more involved assert() invocation.

See https://github.com/newren/git/actions/runs/13845548634/job/38743076293#step:4:1938 for an example of it running in CI and reporting possibly problematic assertions (sample output also included in the commit message of the middle commit in this series if you don't have access to view the link; I'm not sure what the rules on that are).

Elijah Newren (3):
  git-compat-util: introduce BUG_IF_NOT() macro
  ci: add build checking for side-effects in assert() calls
  treewide: replace assert() with BUG_IF_NOT() in special cases

 Makefile                      |  4 ++++
 ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++
 ci/run-static-analysis.sh     |  2 ++
 diffcore-rename.c             |  2 +-
 git-compat-util.h             |  7 +++++++
 merge-ort.c                   |  4 ++--
 merge-recursive.c             |  2 +-
 object-file.c                 |  2 +-
 parallel-checkout.c           |  2 +-
 scalar.c                      |  4 ++--
 sequencer.c                   |  2 +-
 11 files changed, 40 insertions(+), 9 deletions(-)
 create mode 100755 ci/check-unsafe-assertions.sh

base-commit: 4b68faf6b93311254efad80e554780e372deb42f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1881%2Fnewren%2Fassertion-side-effects-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1881/newren/assertion-side-effects-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1881