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

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2024, 16:46 UTC
Message-ID: <xmqqo797xbt9.fsf@gitster.g>
In-Reply-To: <20240515132543.851987-3-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 21 quoted lines

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

Show 15 quoted lines

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

Show 13 quoted lines

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

Show 8 quoted lines

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