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

[Glen Choo <chooglen@google.com>]

Capturing the discussion from Review Club :)

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

> 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().

[...]

>  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