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

[Glen Choo <chooglen@google.com>]

Taylor Blau <me@ttaylorr.com> writes:

> On Thu, Jun 30, 2022 at 06:13:59PM +0000, Glen Choo via GitGitGadget wrote:
>> [1]: https://lore.kernel.org/git/kl6lsfqpygsj.fsf@chooglen-macbookpro.roam.corp.google.com
>> [2]: https://lore.kernel.org/git/5b969c5e-e802-c447-ad25-6acc0b784582@github.com
>>
>> Signed-off-by: Glen Choo <chooglen@google.com>
>> ---
>>  Documentation/config.txt           |  2 ++
>>  Documentation/config/discovery.txt | 23 ++++++++++++
>>  setup.c                            | 57 +++++++++++++++++++++++++++++-
>>  t/t0035-discovery-bare.sh          | 52 +++++++++++++++++++++++++++
>>  4 files changed, 133 insertions(+), 1 deletion(-)
>>  create mode 100644 Documentation/config/discovery.txt
>>  create mode 100755 t/t0035-discovery-bare.sh
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index e284b042f22..9a5e1329772 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -409,6 +409,8 @@ include::config/diff.txt[]
>>
>>  include::config/difftool.txt[]
>>
>> +include::config/discovery.txt[]
>> +
>>  include::config/extensions.txt[]
>>
>>  include::config/fastimport.txt[]
>> diff --git a/Documentation/config/discovery.txt b/Documentation/config/discovery.txt
>> new file mode 100644
>> index 00000000000..bbcf89bb0b5
>> --- /dev/null
>> +++ b/Documentation/config/discovery.txt
>> @@ -0,0 +1,23 @@
>> +discovery.bare::
>> +	Specifies whether Git will work with a bare repository that it
>> +	found during repository discovery. If the repository is
>
> Is it clear from the context what "discovery" means here? It's probably
> easier to describe what it isn't, which you kind of do in the next
> sentence. But it may be clearer to say something like:
>
>     Specifies whether Git will recognize bare repositories that aren't
>     specified via the top-level `--git-dir` command-line option, or the
>     `GIT_DIR` environment variable (see linkgit:git[1]).

Hm that's a good point and the suggestion is very well-worded. In
addition to what you have, I think we should make reference to
"discovery" _somewhere_ in here since the option is named
`discovery.bare`, and this seems like a good teaching opportunity.

>> +This defaults to `always`, but this default may change in the future.
>
> I think the default being subject to change is par for the course. It's
> probably easy enough to just say "Defaults to 'always'" and leave it at
> that.

Makes sense.

>> +static enum discovery_bare_allowed get_discovery_bare(void)
>> +{
>> +	enum discovery_bare_allowed result = DISCOVERY_BARE_ALWAYS;
>> +	git_protected_config(discovery_bare_cb, &result);
>> +	return result;
>> +}
>> +
>> +static const char *discovery_bare_allowed_to_string(
>> +	enum discovery_bare_allowed discovery_bare_allowed)
>> +{
>> +	switch (discovery_bare_allowed) {
>> +	case DISCOVERY_BARE_NEVER:
>> +		return "never";
>> +	case DISCOVERY_BARE_ALWAYS:
>> +		return "always";
>
>> +	default:
>> +		BUG("invalid discovery_bare_allowed %d",
>> +		    discovery_bare_allowed);
>
> Should we have a default case here since the case arms above are
> exhaustive?

Ah, this "default:" was suggested by Stolee in
https://lore.kernel.org/git/7b37f3b7-58c5-1ac5-46eb-d995dc3cc33b@github.com

  This case should be a "default:" in case somehow an arbitrary integer
  value was placed in the variable. [...]

I'm not sure where we stand on this kind of defensiveness. It's not
really necessary, but I suppose having a "default:" won't hurt here,
especially if it BUG()-s instead of silently passing.

>> +	}
>> +	return NULL;
>> +}
>> +
>>  enum discovery_result {
>>  	GIT_DIR_NONE = 0,
>>  	GIT_DIR_EXPLICIT,
>> @@ -1151,7 +1195,8 @@ enum discovery_result {
>>  	GIT_DIR_HIT_CEILING = -1,
>>  	GIT_DIR_HIT_MOUNT_POINT = -2,
>>  	GIT_DIR_INVALID_GITFILE = -3,
>> -	GIT_DIR_INVALID_OWNERSHIP = -4
>> +	GIT_DIR_INVALID_OWNERSHIP = -4,
>> +	GIT_DIR_DISALLOWED_BARE = -5,
>>  };
>>
>>  /*
>> @@ -1248,6 +1293,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>>  		}
>>
>>  		if (is_git_directory(dir->buf)) {
>> +			if (!get_discovery_bare())
>
> Relying on NEVER being the zero value here seems fragile to me. Should
> we check that `if (get_discovery_bare() == DISCOVERY_BARE_NEVER)` to be
> more explicit here?

This was also originally suggested by Stolee in 
https://lore.kernel.org/git/7b37f3b7-58c5-1ac5-46eb-d995dc3cc33b@github.com

  With (some changes to return the enum), we can [...] let the caller
  treat the response as a simple boolean.

but.. your suggestion does seem less fragile. It won't really matter
when we add a third enum and replace the "if" with a "switch", but it
does matter if we ever muck around with the integer values of
DISCOVER_BARE_*.

>> +				return GIT_DIR_DISALLOWED_BARE;
>>  			if (!ensure_valid_ownership(dir->buf))
>>  				return GIT_DIR_INVALID_OWNERSHIP;
>>  			strbuf_addstr(gitdir, ".");
>> @@ -1394,6 +1441,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
>>  		}
>>  		*nongit_ok = 1;
>>  		break;
>> +	case GIT_DIR_DISALLOWED_BARE:
>> +		if (!nongit_ok) {
>> +			die(_("cannot use bare repository '%s' (discovery.bare is '%s')"),
>> +			    dir.buf,
>> +			    discovery_bare_allowed_to_string(get_discovery_bare()));
>> +		}
>> +		*nongit_ok = 1;
>> +		break;
>>  	case GIT_DIR_NONE:
>>  		/*
>>  		 * As a safeguard against setup_git_directory_gently_1 returning
>
> Thanks,
> Taylor