| Message ID | patch-5.5-bb5a53f3b73-20220521T170939Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usage API: add and use a bug() + BUG_if_bug() | expand |
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
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);
Change "BUG" output originally added in a97e4075a16 (Keep rename/rename conflicts of intermediate merges while doing recursive merge, 2007-03-31), and later made to say it was a "BUG" in 19c6a4f8369 (merge-recursive: do not return NULL only to cause segfault, 2010-01-21) to use the new bug() function. 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(). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- cache-tree.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)