| Message ID | patch-v4-6.9-17c1218e74c-20230202T131155Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f7b2ff95163247a3ec437f0ce78bb27cdac68b75 |
| Headers | show |
| Series | config API: make "multi" safe, fix numerous segfaults | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Before this, all these added tests would pass with an exit code of 0. > > We could preserve the comment added in 6c62f015520, but now that we're > directly using the documented repo_config_get_value_multi() value it's > just narrating something that should be obvious from the API use, so > let's drop it. [...] > diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c > index fd0e7739e6a..224164addb3 100644 > --- a/builtin/for-each-repo.c > +++ b/builtin/for-each-repo.c > @@ -32,6 +32,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix) > static const char *config_key = NULL; > int i, result = 0; > const struct string_list *values; > + int err; > > const struct option options[] = { > OPT_STRING(0, "config", &config_key, N_("config"), > @@ -45,11 +46,11 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix) > if (!config_key) > die(_("missing --config=<config>")); > > - /* > - * Do nothing on an empty list, which is equivalent to the case > - * where the config variable does not exist at all. > - */ > - if (repo_config_get_value_multi(the_repository, config_key, &values)) > + err = repo_config_get_value_multi(the_repository, config_key, &values); > + if (err < 0) > + usage_msg_optf(_("got bad config --config=%s"), > + for_each_repo_usage, options, config_key); > + else if (err) > return 0; Compared to v3, this change was moved from the previous patch to this one. > diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh > index 3648d439a87..6b51e00da0e 100755 > --- a/t/t0068-for-each-repo.sh > +++ b/t/t0068-for-each-repo.sh > @@ -40,4 +40,10 @@ test_expect_success 'do nothing on empty config' ' > git for-each-repo --config=bogus.config -- help --no-such-option > ' > > +test_expect_success 'error on bad config keys' ' > + test_expect_code 129 git for-each-repo --config=a && > + test_expect_code 129 git for-each-repo --config=a.b. && > + test_expect_code 129 git for-each-repo --config="'\''.b" > +' > + > test_done And this was moved from patch 1. Both make a lot of sense in this patch, I think this version reads a bit better.
diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c index fd0e7739e6a..224164addb3 100644 --- a/builtin/for-each-repo.c +++ b/builtin/for-each-repo.c @@ -32,6 +32,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix) static const char *config_key = NULL; int i, result = 0; const struct string_list *values; + int err; const struct option options[] = { OPT_STRING(0, "config", &config_key, N_("config"), @@ -45,11 +46,11 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix) if (!config_key) die(_("missing --config=<config>")); - /* - * Do nothing on an empty list, which is equivalent to the case - * where the config variable does not exist at all. - */ - if (repo_config_get_value_multi(the_repository, config_key, &values)) + err = repo_config_get_value_multi(the_repository, config_key, &values); + if (err < 0) + usage_msg_optf(_("got bad config --config=%s"), + for_each_repo_usage, options, config_key); + else if (err) return 0; for (i = 0; !result && i < values->nr; i++) diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh index 3648d439a87..6b51e00da0e 100755 --- a/t/t0068-for-each-repo.sh +++ b/t/t0068-for-each-repo.sh @@ -40,4 +40,10 @@ test_expect_success 'do nothing on empty config' ' git for-each-repo --config=bogus.config -- help --no-such-option ' +test_expect_success 'error on bad config keys' ' + test_expect_code 129 git for-each-repo --config=a && + test_expect_code 129 git for-each-repo --config=a.b. && + test_expect_code 129 git for-each-repo --config="'\''.b" +' + test_done
As noted in 6c62f015520 (for-each-repo: do nothing on empty config, 2021-01-08) this command wants to ignore a non-existing config key, but let's not conflate that with bad config. Before this, all these added tests would pass with an exit code of 0. We could preserve the comment added in 6c62f015520, but now that we're directly using the documented repo_config_get_value_multi() value it's just narrating something that should be obvious from the API use, so let's drop it. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/for-each-repo.c | 11 ++++++----- t/t0068-for-each-repo.sh | 6 ++++++ 2 files changed, 12 insertions(+), 5 deletions(-)