| Message ID | 20240515132543.851987-3-christian.couder@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | upload-pack: support a missing-action | expand |
Christian Couder <christian.couder@gmail.com> 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.
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.
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 */ -}; static enum missing_action arg_missing_action; static show_object_fn fn_show_object; @@ -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); + } +} + static int option_parse_missing_action(const struct option *opt UNUSED, const char *arg, int unset) { + int res; + assert(arg); assert(!unset); - if (!strcmp(arg, "error")) { - arg_missing_action = MA_ERROR; - fn_show_object = show_object; - return 0; - } + 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); - if (!strcmp(arg, "allow-any")) { - arg_missing_action = MA_ALLOW_ANY; + if (res != MA_ERROR) fetch_if_missing = 0; - fn_show_object = show_object__ma_allow_any; - return 0; - } - - if (!strcmp(arg, "allow-promisor")) { - arg_missing_action = MA_ALLOW_PROMISOR; - fetch_if_missing = 0; - fn_show_object = show_object__ma_allow_promisor; - return 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; }
Both `git rev-list` and `git pack-objects` support a `--missing=<missing-action>` option. Previous commits created an API in "missing.{c,h}" to help supporting that option, but only `git rev-list` has been using that API so far. Let's make `git pack-objects` use it too. This involves creating a new show_object_fn_from_action() function to set the `fn_show_object` variable independently from parsing the missing action, which is now performed by the parse_missing_action_value() API function. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- builtin/pack-objects.c | 48 ++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 23 deletions(-)