Re: [PATCH v2 0/6] usage API: add and use a bug() + BUG_if_bug()

[Josh Steadmon <steadmon@google.com>]

On 2022.05.31 18:58, Ævar Arnfjörð Bjarmason wrote:
> This series adds a bug() (lower-case) function to go along with
> BUG(). As seen in 3-5/6 this makes it much easier to handle the cases
> such as parse-options.c where we'd like to call BUG(), but would like
> to first exhaustively accumulate the N issues we spot before doing so,
> and not merely BUG() out on the first one.
> 
> Changes since v1
> (https://lore.kernel.org/git/cover-0.5-00000000000-20220521T170939Z-avarab@gmail.com/):
> 
>  * Move the exit() wrapper to common-main.c, I tried to add a
>    "common-exit.c" or just rename "common-main.c" to "common.c", but
>    due to how the CMake build system declares it those changes would
>    result in a lot of churn, so for now just adding it to
>    common-main.c makes more sense.
> 
>  * Typo/grammar fixes in commit messages, as pointed out in review.
> 
>  * The BUG_if_bug() function is now optional, and the docs have been
>    updated to reflect that.
> 
>  * The BUG_if_bug() function now takes a va_args like BUG() to
>    indicate what the problem was.
> 
>  * Updated 3/6 to note that the exit(128) code changes with a
>    migration to BUG().
> 
>  * Fix logic error in 4/6: We now "break" after calling bug(), to
>    behave as the previous code did.
> 
>  * Fix logic error in 5/6, which now makes use of the new argument(s)
>    to BUG_if_bug().
> 
>  * There was some suggestion of ejecting 6/6, but I've instead
>    replaced it with the implementation Glen suggested in
>    http://lore.kernel.org/git/kl6lk0a6mzp0.fsf@chooglen-macbookpro.roam.corp.google.com
> 
> Ævar Arnfjörð Bjarmason (6):
>   common-main.o: move non-trace2 exit() behavior out of trace2.c
>   usage.c: add a non-fatal bug() function to go with BUG()
>   parse-options.c: use new bug() API for optbug()
>   parse-options.c: use optbug() instead of BUG() "opts" check
>   receive-pack: use bug() and BUG_if_bug()
>   cache-tree.c: use bug() and BUG_if_bug()
> 
>  .../technical/api-error-handling.txt          | 19 +++++-
>  Documentation/technical/api-trace2.txt        |  4 +-
>  builtin/receive-pack.c                        | 16 ++---
>  cache-tree.c                                  |  8 +--
>  common-main.c                                 | 30 ++++++++-
>  git-compat-util.h                             | 24 ++++++-
>  parse-options.c                               | 67 ++++++++++---------
>  t/helper/test-trace2.c                        | 22 +++++-
>  t/t0210-trace2-normal.sh                      | 52 ++++++++++++++
>  trace2.c                                      |  8 +--
>  trace2.h                                      |  8 +--
>  usage.c                                       | 30 +++++++--
>  12 files changed, 217 insertions(+), 71 deletions(-)

Thanks for the updates! I have a minor question about 4/6, but
regardless of if/how that's addressed, I think this version looks good.

Reviewed-by: Josh Steadmon <steadmon@google.com>