Re: [PATCH 5/5] cache-tree.c: use bug() and BUG_if_bug()

From: Glen Choo <chooglen@google.com>
Date: May 27, 2022, 21:29 UTC
Message-ID: <kl6lk0a6mzp0.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-5.5-bb5a53f3b73-20220521T170939Z-avarab@gmail.com>

Capturing the discussion from Review Club :)

Ævar Arnfjörð Bjarmason         <avarab@gmail.com> writes:

Show 5 quoted lines

> This gets the same job done with less code, this changes the output a
> bit, but since we're emitting BUG output let's say it's OK to prefix
> every line with the "unmerged index entry" message, instead of
> optimizing for readability. doing it this way gets rid of any state
> management in the loop itself in favor of BUG_if_bug().

[...]

Show 25 quoted lines

>  cache-tree.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 6752f69d515..9e96097500d 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -692,14 +692,13 @@ struct tree* write_in_core_index_as_tree(struct repository *repo) {
>  	ret = write_index_as_tree_internal(&o, index_state, was_valid, 0, NULL);
>  	if (ret == WRITE_TREE_UNMERGED_INDEX) {
>  		int i;
> -		fprintf(stderr, "BUG: There are unmerged index entries:\n");
>  		for (i = 0; i < index_state->cache_nr; i++) {
>  			const struct cache_entry *ce = index_state->cache[i];
>  			if (ce_stage(ce))
> -				fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
> -					(int)ce_namelen(ce), ce->name);
> +				bug("unmerged index entry on in-memory index write: %d %.*s",
> +				    ce_stage(ce), (int)ce_namelen(ce), ce->name);
>  		}
> -		BUG("unmerged index entries when writing inmemory index");
> +		BUG_if_bug();
>  	}
>  
>  	return lookup_tree(repo, &index_state->cache_tree->oid);

Once we hit this `if` block, we are already confident that we want to BUG() out no matter what, so I think this could be equivalently rewritten as:

  -		fprintf(stderr, "BUG: There are unmerged index entries:\n");
  +		bug("There are unmerged index entries:");
   		for (i = 0; i < index_state->cache_nr; i++) {
   			const struct cache_entry *ce = index_state->cache[i];
   			if (ce_stage(ce))
  -				fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
  -					(int)ce_namelen(ce), ce->name);
  +				bug("%d %.*s\n", ce_stage(ce), (int)ce_namelen(ce), ce->name);
   		}
   		BUG("unmerged index entries when writing inmemory index");
   	}
   
   	return lookup_tree(repo, &index_state->cache_tree->oid);

And just musing here, like Junio mentioned upthread [1], BUG_if_bug() no longer protects us the way BUG() does. This technically isn't an issue here since we're confident we'll call bug() at least once, but the reader has to do a bit more work to convince themselves that we will never hit the return statement later on.

So in this case, we want to be aware of the previous bug() calls, but don't really want the 'conditional dying' behavior of BUG_if_bug(). Maybe BUG() could learn about previous bug() messages, and we insist that bug() be followed up by BUG_if_bug() or BUG().

[1] https://lore.kernel.org/git/xmqqk0a95nni.fsf@gitster.g