Re: [PATCH v6 5/5] setup.c: create `discovery.bare`
- From
Glen Choo <chooglen@google.com>
- Date
- Jul 7, 2022, 19:55 UTC
- Message-ID
- <kl6lh73svgt5.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <Yr5OTq7s2qxxqsiY@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
Show 44 quoted lines
> 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.
Show 5 quoted lines
>> +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.
Show 22 quoted lines
>> +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.
Show 26 quoted lines
>> + }
>> + 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_*.
Show 22 quoted lines
>> + 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