Re: [PATCH v2 2/3] pack-objects: use the missing action API

[Christian Couder <christian.couder@gmail.com>]

On Wed, May 15, 2024 at 6:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> >  static int option_parse_missing_action(const struct option *opt UNUSED,
> >                                      const char *arg, int unset)
> >  {
> > +     int res;
> > +
> >       assert(arg);
> >       assert(!unset);
> >
> > +     res = parse_missing_action_value(arg);
> > +     if (res < 0 || (res != MA_ERROR &&
> > +                     res != MA_ALLOW_ANY &&
> > +                     res != MA_ALLOW_PROMISOR))
> > +             die(_("invalid value for '%s': '%s'"), "--missing", arg);
>
> What is our expectation for how <missing.h> API would evolve over
> time?  I think it is a given that it will always be a superset of
> the need of rev-list and the need of pack-objects, but if we were
> to add a new value of MA_FOO, do we expect that all of the new ones
> are not handled by pack-objects,  Some but not all?  Or none of the
> new ones are handled by pack-objects?

I don't think the <missing.h> API would evolve much over time. At
least I don't think we have plans to make it evolve. Perhaps other
options similar to MA_PRINT could be added though, maybe MA_TRACE or
MA_LOG. Maybe such a new option could be handled by pack-object, maybe
not. I think for now it's better to be flexible and not guess too
much.

> Regardless of the answer to that question, I think a simple helper
> is warranted here, which will also help the [3/3] which adds exactly
> the same code to upload-pack.c:upload_pack_config(), so that the
> callers can do
>
>         res = parse_missing_action_value_for_packing(arg);
>         if (res < 0)
>                 die(_("invalid value for '%s': '%s'"), "--missing", arg);

Ok I implemented this in the v3 I just sent.

> > +     if (res != MA_ERROR)
> >               fetch_if_missing = 0;
> > +     arg_missing_action = res;
> > +     fn_show_object = show_object_fn_from_action(arg_missing_action);
> >
> > -     die(_("invalid value for '%s': '%s'"), "--missing", arg);
> >       return 0;
> >  }
>
> Hmph, wouldn't a small array of show_object_fn suffice, making the
> whole thing more like:
>
>         static show_object_fn const fn[] = {
>                 [MA_ERROR] = show_object,
>                 [MA_ALLOW_ANY] = show_object__ma_allow_any,
>                 [MA_ALLOW_PROMISOR] = show_object__ma_allow_promisor,
>         };
>
>         res = parse_missing_action_value_for_packing(arg);
>         if (res < 0 || ARRAY_SIZE[fn] <= res)
>                 die(_("invalid value for '%s': '%s'"), "--missing", arg);
>         fn_show_object = fn[res];
>         return 0;
>
> without the need for show_object_fn_from_action() helper function?

Ok with the small array. I implemented it in v3 too.

Thanks for the suggestions.