Thanks for being thorough, I find it really helpful.
For brevity, I won't reply to comments that I think are obviously good,
so you can assume I'll incorproate anything that isn't commented on.
Derrick Stolee <derrickstolee@xxxxxxxxxx> writes:
> On 5/13/2022 7:37 PM, Glen Choo via GitGitGadget wrote:
>> From: Glen Choo <chooglen@xxxxxxxxxx>
>>
>> +This config setting is only respected when specified in a system or global
>> +config, not when it is specified in a repository config or via the command
>> +line option `-c discovery.bare=<value>`.
>
> We are sprinkling config options that have these same restrictions throughout
> the config documentation. It might be time to define a term like "protected
> config" at the top of git-config.txt and then refer to that from these other
> locations.
Agree, and I think defining the term will be useful in future on-list
discussions.
>> +static int check_bare_repo_allowed(void)
>> +{
>> + if (discovery_bare_config == DISCOVERY_BARE_UNKNOWN) {
>> + read_very_early_config(discovery_bare_cb, NULL);
>
> This will add the third place where we use read_very_early_config(),
> adding to the existing calls in tr2_sysenv_load() and
> ensure_valid_ownership(). If I understand it correctly, that means
> that every Git execution in a bare repository will now parse the
> system and global config three times.
>
> This doesn't count the check for uploadpack.packobjectshook in
> upload-pack.c that uses current_config_scope() to restrict its
> value to the system and global config.
>
> We are probably at the point where we need to instead create a
> configset that stores this "protected config" and allow us to
> lookup config keys directly from that configset instead of
> iterating through these config files repeatedly.
Looking at all of the read_very_early_config() calls,
- check_bare_repo_allowed() can use git_configset_get_string()
- ensure_valid_ownership() can use git_configset_get_value_multi()
- tr2_sysenv_load() reads every value with the "trace2." prefix. AFAICT
configsets only support exact key lookups and I don't see an easy way
teach configsets to support prefix lookups.
(I didn't look too closely at uploadpack.packobjectshook because I don't
know enough about config scopes to comment.)
So using a configset, we'll still need to read the config files at least
twice. That's better than thrice, but it doesn't cover the
tr2_sysenv_load() use case, and we'll run into this yet again if add
function that reads all config values with a given prefix.
An hacky alternative that covers all of these use cases would be to read
all protected config in a single pass, e.g.
static struct protected_config {
struct safe_directory_data safe_directory_data;
const char *discovery_bare;
struct string_list tr2_sysenv;
};
static int protected_config_cb()
{
/* Parse EVERYTHING that belongs in protected_config. */
}
but protected_config_cb() would have to parse too many unrelated things
for my liking.
So I'll use the configset for the cases where the key is known, and
perhaps we'll punt on tr2_sysenv_load().
>> + }
>> + switch (discovery_bare_config) {
>> + case DISCOVERY_BARE_NEVER:
>> + return 0;
>> + case DISCOVERY_BARE_ALWAYS:
>> + return 1;
>> + default:
>> + BUG("invalid discovery_bare_config %d", discovery_bare_config);
>> + }
>
> You return -1 in discovery_bare_cb when the key matches, but
> the value is not understood. Should we check the return value
> of read_very_early_config(), too?
This comment doesn't apply because unlike most other config reading
functions, read_very_early_config() and read_early_config() die when the
callback returns -1.
I'm not sure why this is the case though, and maybe you think there is
value in having a non-die()-ing variant, e.g.
read_very_early_config_gently()?
>> };
>>
>> /*
>> @@ -1239,6 +1293,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>> }
>>
>> if (is_git_directory(dir->buf)) {
>> + if (!check_bare_repo_allowed())
>> + return GIT_DIR_DISALLOWED_BARE;
>
> Won't this fail if someone runs a Git command inside of a .git/
> directory for a non-bare repository? I just want to be sure that
> we hit this error instead:
>
> fatal: this operation must be run in a work tree
>
> I see that this error is tested in t0008-ignores.sh, but that's
> with the default "always" value. It would be good to explicitly
> check that this is the right error when using the "never" config.
Yes, it will fail if run inside of a .git/ directory. "never" prevents
you from working from inside .git/ unless you set GIT_DIR.
IIRC, we don't show "fatal: this operation must be run in a work
tree" for every Git command, e.g. "git log" works just fine. It makes
sense to show this warning when the CWD supports 'some, but not all' Git
commands, but I don't think this is valuable if we forbid *all* Git
commands.
Instead of trying to make "never" accomodate this use case, perhaps what
we want is a "dotgit-only" option that allows a bare repository if it is
below a .git/ directory. Since we forbid .git in the index, this seems
somewhat safe, but I hadn't proposed this sooner because I don't know if
we need it yet, and I'm certain that there are less secure edge cases
that need to be thought through.
