Taylor Blau <me@xxxxxxxxxxxx> writes: > On Thu, Jun 30, 2022 at 06:13:59PM +0000, Glen Choo via GitGitGadget wrote: >> [1]: https://lore.kernel.org/git/kl6lsfqpygsj.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx >> [2]: https://lore.kernel.org/git/5b969c5e-e802-c447-ad25-6acc0b784582@xxxxxxxxxx >> >> Signed-off-by: Glen Choo <chooglen@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx 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@xxxxxxxxxx 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
