Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
