| Message ID | f947cf221c0b5320d0b7438b88a0d94a5bd3a70b.1654038754.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f1dfbd9ee010e5cdf0d931d16b4b2892b33331e5 |
| Headers | show |
| Series | remote.c: reject 0-length branch names | expand |
On Tue, May 31 2022, Glen Choo via GitGitGadget wrote: > From: Glen Choo <chooglen@google.com> > > Branch names can't be empty, so config keys with an empty branch name, > e.g. "branch..remote", are silently ignored. > > Since these config keys will never be useful, make it a fatal error when > remote.c finds a key that starts with "branch." and has an empty > subsection. Perhaps this is fine, but I think this commit message (and I checked the CL too) really needs to work a bit harder to convince us that this is safe to do. Are we confident that this is just bizarro config that nobody would have had in practice? In that case I think it's fine to start dying on it. But as I understand we previously just ignored this, then if there's any doubt about that perhaps we should start with a warning? Or are we really confident that this is an edge case not worth worrying about in that way, and that we can go straight to die()?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Tue, May 31 2022, Glen Choo via GitGitGadget wrote: > >> From: Glen Choo <chooglen@google.com> >> >> Branch names can't be empty, so config keys with an empty branch name, >> e.g. "branch..remote", are silently ignored. >> >> Since these config keys will never be useful, make it a fatal error when >> remote.c finds a key that starts with "branch." and has an empty >> subsection. > > Perhaps this is fine, but I think this commit message (and I checked the > CL too) really needs to work a bit harder to convince us that this is > safe to do. Fair. > Are we confident that this is just bizarro config that nobody would have > had in practice? In that case I think it's fine to start dying on it. > > But as I understand we previously just ignored this, then if there's any > doubt about that perhaps we should start with a warning? > > Or are we really confident that this is an edge case not worth worrying > about in that way, and that we can go straight to die()? The case I want to make is even stronger than that - this is an edge case that _we_ shouldn't worry about, and we should tell the _user_ that their config is bogus. It truly makes no sense because `branch..remote` fits the schema of `branch.<name>.remote` where <name> is "", but "" isn't a valid branch name (and it never has been AFAIK). So such a key would never be useful to Git, and it would be extremely hacky for a non-Git tool to use such a key. I'm not sure how a user would generate such a key in the wild (e.g. [1]). Maybe it was a typo, but more worryingly (I don't have evidence for this, but it could happen), it might be misbehavior from `git [branch|config]` that we never noticed because the bogus keys have flown under the radar. If there really is a bug elsewhere, erroring out when we see such keys might also alert us to the bug. Perhaps I need to capture all of this in the commit message? [1] https://lore.kernel.org/git/24f547-6285e280-59-40303580@48243747/
Glen Choo <chooglen@google.com> writes: > It truly makes no sense because `branch..remote` fits the schema of > `branch.<name>.remote` where <name> is "", but "" isn't a valid branch > name (and it never has been AFAIK). So such a key would never be useful > to Git, and it would be extremely hacky for a non-Git tool to use such > a key. Yup, we might want to reserve a bogus key or two that can never be a branch name to allow us express "this configuration is in effect for all branches" (e.g. "branch.*.rebase = never"), but the natural such name would be "*" and does not have to be an empty string.
On Wed, Jun 01, 2022 at 09:55:57AM -0700, Glen Choo wrote: > > Are we confident that this is just bizarro config that nobody would have > > had in practice? In that case I think it's fine to start dying on it. > > > > But as I understand we previously just ignored this, then if there's any > > doubt about that perhaps we should start with a warning? > > > > Or are we really confident that this is an edge case not worth worrying > > about in that way, and that we can go straight to die()? > > The case I want to make is even stronger than that - this is an edge > case that _we_ shouldn't worry about, and we should tell the _user_ that > their config is bogus. It's a tradeoff, isn't it? We don't know how the user ended up with this config, what they were trying to do, nor how common it is. Clearly the config makes no sense and is broken, but by alerting the user, we are: - maybe doing some good, because now they know that whatever they were trying to do didn't work, and can clean up the broken config - maybe doing some bad, because it was not (and is not) hurting anything to have config that nobody bothers to do anything with. But if we die, now the user is presented with a situation that they know nothing about, and must resolve it before continuing with their unrelated work. -Peff
diff --git a/remote.c b/remote.c index cf7015ae8ab..a3888dd789c 100644 --- a/remote.c +++ b/remote.c @@ -352,8 +352,12 @@ static int handle_config(const char *key, const char *value, void *cb) struct remote_state *remote_state = cb; if (parse_config_key(key, "branch", &name, &namelen, &subkey) >= 0) { + /* There is no subsection. */ if (!name) return 0; + /* There is a subsection, but it is empty. */ + if (!namelen) + return -1; branch = make_branch(remote_state, name, namelen); if (!strcmp(subkey, "remote")) { return git_config_string(&branch->remote_name, key, value); diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index a05268952e9..e99c31f8c35 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -598,13 +598,23 @@ test_expect_success 'branch.*.pushremote config order is irrelevant' ' check_push_result two_repo $the_commit heads/main ' -test_expect_success 'push ignores empty branch name entries' ' +test_expect_success 'push rejects empty branch name entries' ' mk_test one_repo heads/main && test_config remote.one.url one_repo && test_config branch..remote one && test_config branch..merge refs/heads/ && test_config branch.main.remote one && test_config branch.main.merge refs/heads/main && + test_must_fail git push 2>err && + grep "bad config variable .branch\.\." err +' + +test_expect_success 'push ignores "branch." config without subsection' ' + mk_test one_repo heads/main && + test_config remote.one.url one_repo && + test_config branch.autoSetupMerge true && + test_config branch.main.remote one && + test_config branch.main.merge refs/heads/main && git push '