On Mon, Aug 20, 2018 at 6:18 AM Matthew DeVore <matv...@google.com> wrote:
>
> There were many instances in this file where it seemed like BUG would be
> better, so I created a new commit before this one to switch them over. The
> interdiff is below.
>
> BTW, why are there so many instances of "die" without "_"? I expect all
> errors that may be caused by a user to be localized.
Well, there is the porcelain layer to be consumed by a human user
and the plumbing that is good for scripts. And in scripts you might want
to grep for certain errors and react to that, so a non-localized error
message makes the script possible to run in any localisation.
The BUG is strictly for things that are due to Gits internals,
not for problematic user input. Problematic user input
definitely wants a die(...), and depending on the plumbing/porcelain
layer it may need to be _(translatable).
I think BUG() would never go with translated strings.
> I'm going by the output of this: grep -IrE '\Wdie\([^_]' --exclude-dir=t
>
> diff --git a/list-objects-filter.c b/list-objects-filter.c
> index 8e3caf5bf..09b2b05d5 100644
> --- a/list-objects-filter.c
> +++ b/list-objects-filter.c
> @@ -44,8 +44,7 @@ static enum list_objects_filter_result filter_blobs_none(
>
> switch (filter_situation) {
> default:
> - die("unknown filter_situation");
> - return LOFR_ZERO;
> + BUG("unknown filter_situation: %d", filter_situation);
>
> case LOFS_BEGIN_TREE:
> assert(obj->type == OBJ_TREE);
> @@ -99,8 +98,7 @@ static enum list_objects_filter_result filter_trees_none(
>
> switch (filter_situation) {
> default:
> - die("unknown filter_situation");
> - return LOFR_ZERO;
> + BUG("unknown filter_situation: %d", filter_situation);
>
> case LOFS_BEGIN_TREE:
> case LOFS_BLOB:
> @@ -151,8 +149,7 @@ static enum list_objects_filter_result
> filter_blobs_limit(
>
> switch (filter_situation) {
> default:
> - die("unknown filter_situation");
> - return LOFR_ZERO;
> + BUG("unknown filter_situation: %d", filter_situation);
>
> case LOFS_BEGIN_TREE:
> assert(obj->type == OBJ_TREE);
> @@ -257,8 +254,7 @@ static enum list_objects_filter_result filter_sparse(
>
> switch (filter_situation) {
> default:
> - die("unknown filter_situation");
> - return LOFR_ZERO;
> + BUG("unknown filter_situation: %d", filter_situation);
Up until here we just have replace the die by BUG in the default
case of the state machine switch. (We need the default due to strict
compile flags, but as filter_situation is an enum I thought we would not
as compilers are smart enough to see we got all values of the enum
covered).
I agree that keeping the defaults and having a BUG() is reasonable.
>
> case LOFS_BEGIN_TREE:
> assert(obj->type == OBJ_TREE);
> @@ -439,7 +435,7 @@ void *list_objects_filter__init(
> assert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);
>
> if (filter_options->choice >= LOFC__COUNT)
> - die("invalid list-objects filter choice: %d",
> + BUG("invalid list-objects filter choice: %d",
> filter_options->choice);
This also makes sense, combined with the assert before, this looks like
really defensive code.
I think this patch is a good idea!
Thanks,
Stefan
