Re: [PATCH 1/5] usage.c: add a non-fatal bug() function to go with BUG()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, May 26 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I don't think we should do it like that and keep it a BUG() not to call
>> BUG_if_bug() when we hit exit(), because e.g. in the case of
>> parse-options.c once we have the a bad "struct options" we don't want to
>> continue, as we might segfault, or have other bad behavior etc. So we'd
>> like to BUG() out as soon as possible.
>
> Oh, there is no question about that.  When we detect that the
> program is in an inconsistent and unexpected state, we would want to
> bug out instead of continuing at some point, and that is why we would
> want to have BUG_if_bug(), or exit_if_called_bug(), as I called in
> the message you are reponding to.
>
> What I am getting at is that the code often or usually calls
> BUG_if_bug() is not a reason to require it to be called, especially
> if there are conditional calls to bug() near the end of the program.
> Imagine a program, after finishing to respond to the end-user
> request, before the end of the program, performing some self sanity
> checks with a series of "if (condition) bug()", and there is no more
> thing that needs to be done other than exiting after such check.  I
> would have added such a call to sanity_check_refcnt() at the end of
> "git blame", for example, if the facility existed.

I'm re-rolling this and FWIW came up with this on top of the re-roll,
but didn't include it. It could also call the find_alignment() and
output(), but for this it seemed just leaving it at the bug() calls was
sufficient:

diff --git a/blame.c b/blame.c
index da1052ac94b..84c112f76bd 100644
--- a/blame.c
+++ b/blame.c
@@ -1155,21 +1155,15 @@ static int compare_commits_by_reverse_commit_date(const void *a,
  */
 static void sanity_check_refcnt(struct blame_scoreboard *sb)
 {
-	int baa = 0;
 	struct blame_entry *ent;
 
-	for (ent = sb->ent; ent; ent = ent->next) {
+	for (ent = sb->ent; ent; ent = ent->next)
 		/* Nobody should have zero or negative refcnt */
-		if (ent->suspect->refcnt <= 0) {
-			fprintf(stderr, "%s in %s has negative refcnt %d\n",
-				ent->suspect->path,
-				oid_to_hex(&ent->suspect->commit->object.oid),
-				ent->suspect->refcnt);
-			baa = 1;
-		}
-	}
-	if (baa)
-		sb->on_sanity_fail(sb, baa);
+		if (ent->suspect->refcnt <= 0)
+			bug("%s in %s has negative refcnt %d",
+			    ent->suspect->path,
+			    oid_to_hex(&ent->suspect->commit->object.oid),
+			    ent->suspect->refcnt);
 }
 
 /*
diff --git a/blame.h b/blame.h
index 38bde535b3d..f110bf3c40e 100644
--- a/blame.h
+++ b/blame.h
@@ -154,7 +154,6 @@ struct blame_scoreboard {
 	int debug;
 
 	/* callbacks */
-	void(*on_sanity_fail)(struct blame_scoreboard *, int);
 	void(*found_guilty_entry)(struct blame_entry *, void *);
 
 	void *found_guilty_entry_data;
diff --git a/builtin/blame.c b/builtin/blame.c
index e33372c56b0..70f31e94d38 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -655,14 +655,6 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)
 		abbrev = auto_abbrev + 1;
 }
 
-static void sanity_check_on_fail(struct blame_scoreboard *sb, int baa)
-{
-	int opt = OUTPUT_SHOW_SCORE | OUTPUT_SHOW_NUMBER | OUTPUT_SHOW_NAME;
-	find_alignment(sb, &opt);
-	output(sb, opt);
-	die("Baa %d!", baa);
-}
-
 static unsigned parse_score(const char *arg)
 {
 	char *end;
@@ -1151,7 +1143,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		sb.copy_score = blame_copy_score;
 
 	sb.debug = DEBUG_BLAME;
-	sb.on_sanity_fail = &sanity_check_on_fail;
 
 	sb.show_root = show_root;
 	sb.xdl_opts = xdl_opts;