Re: [PATCH v6 6/6] list-objects-filter: implement filter tree:0

On Mon, Aug 20, 2018 at 11:38 AM Stefan Beller [off-list ref] wrote:

On Mon, Aug 20, 2018 at 6:18 AM Matthew DeVore [off-list ref] wrote:

quoted

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

Ah I see. Plumbing commands are not translated. Makes perfect sense now.

I think BUG() would never go with translated strings.

quoted

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

At the risk of going on a tangent, I assumed this was because enums
are really ints, and the "default" is there in case the enum somehow
got assigned to an int without a corresponding value. Either because
of a cast from an int that was out-of-range, or new values that were
obtained from arithmetic or bitwise operations on the declared enum
values, which created undeclared values.

I agree that keeping the defaults and having a BUG() is reasonable.

quoted

        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!

Thank you for your feedback :)

Thanks,
Stefan