Re: [PATCH v3 3/5] setup.c: create `discovery.bare`

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

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +enum discovery_bare_config {
> +	DISCOVERY_BARE_UNKNOWN = -1,
> +	DISCOVERY_BARE_NEVER = 0,
> +	DISCOVERY_BARE_ALWAYS,
> +};
> +static enum discovery_bare_config discovery_bare_config =
> +	DISCOVERY_BARE_UNKNOWN;

Can discovery_bare come from anywhere other than config?

I am wondering if both the variable and the type should be called
"discovery_bare_allowed" instead.  That it comes from the config is
not the more important part.  That it determines if it is allowed
is.

> +static int check_bare_repo_allowed(void)
> +{
> +	if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN) {
> +		discovery_bare_config = DISCOVERY_BARE_ALWAYS;
> +		git_protected_config(discovery_bare_cb, NULL);
> +	}

OK, so the thing is initialized to "unknown", and the first time we
want to use the value of it, we read from the file (or default to
"always").  Makes sense.

And then ...

> +	switch (discovery_bare_config) {
> +	case DISCOVERY_BARE_NEVER:
> +		return 0;
> +	case DISCOVERY_BARE_ALWAYS:
> +		return 1;
> +	case DISCOVERY_BARE_UNKNOWN:
> +		BUG("invalid discovery_bare_config %d", discovery_bare_config);

... this is being defensive; we know discovery_bare_cb() won't give
UNKNOWN, but we want to make sure.

> +	}
> +	return 0;
> +}
> +
> +static const char *discovery_bare_config_to_string(void)
> +{

But this one feels strangely asymmetrical, as there is no inherent
reason why one must be called before the other.  I would expect it
to either

 * take a parameter of type "enum discovery_bare" and return
   "never", "always", or "unset", without calling any BUG().

or

 * have the same "we lazily figure out the discovery_bare_config
   variable on demand" logic.

As both of these functions are file-scope static, we can live with
it, though.

> +	switch (discovery_bare_config) {
> +	case DISCOVERY_BARE_NEVER:
> +		return "never";
> +	case DISCOVERY_BARE_ALWAYS:
> +		return "always";
> +	case DISCOVERY_BARE_UNKNOWN:
> +		BUG("invalid discovery_bare_config %d", discovery_bare_config);
> +	}
> +	return NULL;
> +}