Re: [PATCH v7 3/4] advice: revamp advise API
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 2, 2020, 21:03 UTC
- Message-ID
- <xmqqblpebhae.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <a4673803eaf881f7caa25d97e432c0ff0d065060.1583179320.git.gitgitgadget@gmail.com>
"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> +static struct {
> + const char *key;
> + int enabled;
> +} advice_setting[] = {
> + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 },It would be nicer to future developers to flip the polarity, as we do not have to write 1 all over the place, especially if we plan to extend the structure over time and to use designated initializers for only certain fields:
static struct {
const char *key;
int disabled;
} advice_setting[] = {
[ADDVICE_ADD_EMBEDDED_REPO] = { .key = "addEmbeddedRepo" },Show 12 quoted lines
> @@ -149,6 +218,13 @@ int git_default_advice_config(const char *var, const char *value)
> if (strcasecmp(k, advice_config[i].name))
> continue;
> *advice_config[i].preference = git_config_bool(var, value);
> + break;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
> + if (strcasecmp(k, advice_setting[i].key))
> + continue;
> + advice_setting[i].enabled = git_config_bool(var, value);
> return 0;Turning this into "break;" would make it similar to the loop before this one, and will allow other people to add more code after this loop later.
Show 12 quoted lines
> +int cmd__advise_if_enabled(int argc, const char **argv)
> +{
> + if (!argv[1])
> + die("usage: %s <advice>", argv[0]);
> +
> + setup_git_directory();
> + git_config(git_default_config, NULL);
> +
> + /*
> + Any advice type can be used for testing, but NESTED_TAG was selected
> + here and in t0018 where this command is being executed.
> + */Style (will fix up locally).
Thanks. I think this is reasonable with or without the suggested fixes.