Christian Couder <christian.couder@xxxxxxxxx> writes:
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index baf0090fc8..55d08c686d 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -39,6 +39,7 @@
> #include "promisor-remote.h"
> #include "pack-mtimes.h"
> #include "parse-options.h"
> +#include "missing.h"
>
> /*
> * Objects we are going to pack are collected in the `to_pack` structure.
> @@ -250,11 +251,6 @@ static unsigned long window_memory_limit = 0;
>
> static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;
>
> -enum missing_action {
> - MA_ERROR = 0, /* fail if any missing objects are encountered */
> - MA_ALLOW_ANY, /* silently allow ALL missing objects */
> - MA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */
> -};
Interesting. This used to be private to this file, shared the same
name and most of the values with the one used in rev-list, but not
identical (i.e. the new "missing" API knows about MA_PRINT but this
side has been unaware of that value).
> @@ -3826,33 +3822,39 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
> show_object(obj, name, data);
> }
>
> +static show_object_fn show_object_fn_from_action(enum missing_action action)
> +{
> + switch (action) {
> + case MA_ERROR:
> + return show_object;
> + case MA_ALLOW_ANY:
> + return show_object__ma_allow_any;
> + case MA_ALLOW_PROMISOR:
> + return show_object__ma_allow_promisor;
> + default:
> + BUG("invalid missing action %d", action);
As this is BUG() to catch programming error, ("%d" % action) is OK;
if this were end-user facint, we would also want to pass the "arg"
string the caller had only for error reporting.
> 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?
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);
something like
int parse_missing_action_value_for_packing(const char *arg)
{
int res = parse_missing_action_value(arg);
if (res < 0)
return res;
switch (res) {
case MA_ERROR:
case MA_ALLOW_ANY:
case MA_ALLOW_PROMISOR:
return res;
default:
return -2 - res;
}
}
here, and also in the other place [3/3] adds. This thin wrapper
returns:
0 <= res : MA_FOO values that are OK for packing
-1 = res : parse_missing_action_value() failed
-1 > res : (2 - res) is the MA_FOO which is unsuitable for packing
to allow the caller to recover which value the user gave us that is
unsuitable for packing, if it wanted to.
> + 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?
Other than that, the intention of the code is very clear.
Will queue. Thanks.
