Re: [PATCH] setup: Only allow extenions.objectFormat to be specified once

[Junio C Hamano <gitster@pobox.com>]

"Eric W. Biederman" <ebiederm@gmail.com> writes:

> Today there is no sanity checking of what happens when
> extensions.objectFormat is specified multiple times.  Catch confused git
> configurations by only allowing this option to be specified once.

Hmph.  I am not sure if this is worth doing, and especially only for
"objectformat".  Do we intend to apply different rules other than
"you can give it only once" to other extensions, and if so where
will these rules be catalogued?  I do not see particular harm to let
them follow the usual "last one wins".

If the patch were about trying to make sure that extensions, which
are inherentaly per-repository, appear only in $GIT_DIR/config and
complain if the code gets confused and tried to read them from the
system or global configuration files, I would understand, and
strongly support such an effort, ithough.

The real sanity we want to enforce is that what is reported by
running "git config extensions.objectformat" must match the object
format that is used in refs and object database.  Manually futzing
the configuration file and adding an entry with a contradictory
value certainly is one way to break that sanity, and this patch may
catch such a breakage, but once we start worrying about manually
futzing the configuration file, the check added here would easily
miss if the futzing is done by replacing instead of adding, so I am
not sure if this extra code is worth its bits.

But perhaps I am missing something and not seeing why it is worth
insisting on "last one is the first one" for this particular one.

Thanks.

> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  setup.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/setup.c b/setup.c
> index 18927a847b86..ef9f79b8885e 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -580,6 +580,7 @@ static enum extension_result handle_extension(const char *var,
>  	if (!strcmp(ext, "noop-v1")) {
>  		return EXTENSION_OK;
>  	} else if (!strcmp(ext, "objectformat")) {
> +		struct string_list_item *item;
>  		int format;
>  
>  		if (!value)
> @@ -588,6 +589,13 @@ static enum extension_result handle_extension(const char *var,
>  		if (format == GIT_HASH_UNKNOWN)
>  			return error(_("invalid value for '%s': '%s'"),
>  				     "extensions.objectformat", value);
> +		/* Only support objectFormat being specified once. */
> +		for_each_string_list_item(item, &data->v1_only_extensions) {
> +			if (!strcmp(item->string, "objectformat"))
> +				return error(_("'%s' already specified as '%s'"),
> +					"extensions.objectformat",
> +					hash_algos[data->hash_algo].name);
> +		}
>  		data->hash_algo = format;
>  		return EXTENSION_OK;
>  	}