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

From: Matthew DeVore <matvore@google.com>
Date: Aug 20, 2018, 23:20 UTC
Message-ID: <CAMfpvh+7NoOGAmypGSCAtxH7DxHTvVB8_C3OkWNC14+c3MBO4A@mail.gmail.com>
In-Reply-To: <CAGZ79kZfNeWnZEnX4Z0hMShxZZEsB2jYJ67JgE7F_d-3ymHi+A@mail.gmail.com>

On Mon, Aug 20, 2018 at 11:38 AM Stefan Beller <sbeller@google.com> wrote:

Show 19 quoted lines

>
> On Mon, Aug 20, 2018 at 6:18 AM Matthew DeVore <matvore@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).

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

Show 53 quoted lines

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

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.

Show 19 quoted lines

>
> 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!

Thank you for your feedback :)

>
> Thanks,
> Stefan