Add a nascent WIP facility to specify via the options parsing that
we'd e.g. like to grab the --status option for commit.status from the
commit.status config key.
This is all very proof-of-concept, and uses the ugly hack of s/const
// for the options struct because I'm now keeping state in it, as
noted in one of the TODO comments that should be moved.
But even so this works, passes all tests, gets rid of 3 lines in
commit.c, and has the promise of getting rid of a *lot* more manual
option parsing in favor of declaring config keys via OPT_*
everywhere. See my
<cacbzzx4fksu6nujpz_3gz45eq+kdjj5g2sajtripe1wbaa3...@mail.gmail.com>
for more details.
Signed-off-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
---
On Sun, Mar 19, 2017 at 2:43 PM, Ævar Arnfjörð Bjarmason <ava...@gmail.com>
wrote:
> I don't know if this is what Duy has in mind, but the facility I've
> described is purely an internal code reorganization issue. I.e. us
> not having to write custom code for each bultin every time we want to
> take an option from the command line || config.
Here's an implementation of this I hacked up this evening. This is
very WIP as noted in the commit message / TODO comments, but it works!
I thought I'd send it to the list for comments on the general approach
before taking it much further.
builtin/commit.c | 7 ++-----
parse-options-cb.c | 21 +++++++++++++++++++++
parse-options.c | 40 ++++++++++++++++++++++++++++++++++++----
parse-options.h | 16 +++++++++++++---
4 files changed, 72 insertions(+), 12 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc513..a7c9e4128f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1500,10 +1500,6 @@ static int git_commit_config(const char *k, const char
*v, void *cb)
if (!strcmp(k, "commit.template"))
return git_config_pathname(&template_file, k, v);
- if (!strcmp(k, "commit.status")) {
- include_status = git_config_bool(k, v);
- return 0;
- }
if (!strcmp(k, "commit.cleanup"))
return git_config_string(&cleanup_arg, k, v);
if (!strcmp(k, "commit.gpgsign")) {
@@ -1596,7 +1592,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
OPT_FILENAME('t', "template", &template_file, N_("use specified
template file")),
OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how
to strip spaces and #comments from message")),
- OPT_BOOL(0, "status", &include_status, N_("include status in
commit message template")),
+ OPT_BOOL_C(0, "status", &include_status, N_("include status in
commit message template"),
+ "commit.status", parse_opt_confkey_bool),
{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) ""
},
/* end commit message options */
diff --git a/parse-options-cb.c b/parse-options-cb.c
index b7d8f7dcb2..2383d9bbe0 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -236,3 +236,24 @@ int parse_opt_passthru_argv(const struct option *opt,
const char *arg, int unset
return 0;
}
+
+/* Does it suck that I have to use the cache interface to the config
+ * here? Should we somehow unroll this whole thing so
+ * parse_options_step loops over the config values, and maybe
+ * populates opt->conf_key (which we'd need to add) for all the
+ * options that need it?
+ *
+ * I.e. should we make this more complex because this one-shot
+ * interface is expensive, or is it just fine?
+*/
+
+int parse_opt_confkey_bool(const struct option *opt, const char *arg, int
unset) {
+ const char *value;
+
+ if (git_config_get_value(opt->conf_key, &value))
+ return 0;
+
+ *(int *)opt->value = git_config_bool(opt->conf_key, value);
+
+ return 0;
+}
diff --git a/parse-options.c b/parse-options.c
index 4fbe924a5d..f9aba088d8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -235,12 +235,13 @@ static int parse_short_opt(struct parse_opt_ctx_t *p,
const struct option *optio
}
static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
- const struct option *options)
+ struct option *options)
{
const struct option *all_opts = options;
const char *arg_end = strchrnul(arg, '=');
const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
int abbrev_flags = 0, ambiguous_flags = 0;
+ int ret;
for (; options->type != OPTION_END; options++) {
const char *rest, *long_name = options->long_name;
@@ -313,7 +314,17 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const
char *arg,
continue;
p->opt = rest + 1;
}
- return get_value(p, options, all_opts, flags ^ opt_flags);
+ if (!(ret = get_value(p, options, all_opts, flags ^
opt_flags))) {
+ /* TODO: Keep some different state on the side
+ * with info about what options we've
+ * retrieved via the CLI for use in the loop
+ * in parse_options_step, instead of making
+ * the 'options' non-const
+ */
+ if (options->flags & PARSE_OPT_CONFIGURABLE)
+ options->flags |= PARSE_OPT_VIA_CLI;
+ }
+ return ret;
}
if (ambiguous_option)
@@ -429,7 +440,7 @@ static int usage_with_options_internal(struct
parse_opt_ctx_t *,
const struct option *, int, int);
int parse_options_step(struct parse_opt_ctx_t *ctx,
- const struct option *options,
+ struct option *options,
const char * const usagestr[])
{
int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
@@ -514,6 +525,27 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
ctx->out[ctx->cpidx++] = ctx->argv[0];
ctx->opt = NULL;
}
+
+ /* The loop above is driven by the argument vector, so we need
+ * to make a second pass and find those options that are
+ * configurable, and haven't been set via the command-line */
+ for (; options->type != OPTION_END; options++) {
+ if (!(options->flags & PARSE_OPT_CONFIGURABLE))
+ continue;
+
+ if (options->flags & PARSE_OPT_VIA_CLI)
+ continue;
+
+ /* TODO: Maybe factor the handling of OPTION_CALLBACK
+ * in get_value() into a function.
+ *
+ * Do we also need to save away the state from the
+ * loop above to handle unset? I think not, I think
+ * we're always unset here by definition, right?
+ */
+ return (*options->conf_callback)(options, NULL, 1) ? (-1) : 0;
+ }
+
return PARSE_OPT_DONE;
show_usage_error:
@@ -530,7 +562,7 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
}
int parse_options(int argc, const char **argv, const char *prefix,
- const struct option *options, const char * const usagestr[],
+ struct option *options, const char * const usagestr[],
int flags)
{
struct parse_opt_ctx_t ctx;
diff --git a/parse-options.h b/parse-options.h
index dcd8a0926c..14abf21467 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -38,7 +38,9 @@ enum parse_opt_option_flags {
PARSE_OPT_LASTARG_DEFAULT = 16,
PARSE_OPT_NODASH = 32,
PARSE_OPT_LITERAL_ARGHELP = 64,
- PARSE_OPT_SHELL_EVAL = 256
+ PARSE_OPT_SHELL_EVAL = 256,
+ PARSE_OPT_CONFIGURABLE = 512,
+ PARSE_OPT_VIA_CLI = 1024
};
struct option;
@@ -110,6 +112,9 @@ struct option {
int flags;
parse_opt_cb *callback;
intptr_t defval;
+
+ const char *conf_key;
+ parse_opt_cb *conf_callback;
};
#define OPT_END() { OPTION_END }
@@ -124,7 +129,11 @@ struct option {
(h), PARSE_OPT_NOARG }
#define OPT_SET_INT(s, l, v, h, i) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG, NULL, (i) }
+#define OPT_SET_INT_C(s, l, v, h, i, ck, cb) \
+ { OPTION_SET_INT, (s), (l), (v), NULL, \
+ (h), PARSE_OPT_NOARG |
PARSE_OPT_CONFIGURABLE, NULL, (i), ck, cb }
#define OPT_BOOL(s, l, v, h) OPT_SET_INT(s, l, v, h, 1)
+#define OPT_BOOL_C(s, l, v, h, ck, cb) OPT_SET_INT_C(s, l, v, h, 1, ck, cb)
#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
NULL, 1}
#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
@@ -166,7 +175,7 @@ struct option {
* Returns the number of arguments left in argv[].
*/
extern int parse_options(int argc, const char **argv, const char *prefix,
- const struct option *options,
+ struct option *options,
const char * const usagestr[], int flags);
extern NORETURN void usage_with_options(const char * const *usagestr,
@@ -210,7 +219,7 @@ extern void parse_options_start(struct parse_opt_ctx_t *ctx,
const struct option *options, int flags);
extern int parse_options_step(struct parse_opt_ctx_t *ctx,
- const struct option *options,
+ struct option *options,
const char * const usagestr[]);
extern int parse_options_end(struct parse_opt_ctx_t *ctx);
@@ -231,6 +240,7 @@ extern int parse_opt_noop_cb(const struct option *, const
char *, int);
extern int parse_opt_unknown_cb(const struct option *, const char *, int);
extern int parse_opt_passthru(const struct option *, const char *, int);
extern int parse_opt_passthru_argv(const struct option *, const char *, int);
+extern int parse_opt_confkey_bool(const struct option *, const char *, int);
#define OPT__VERBOSE(var, h) OPT_COUNTUP('v', "verbose", (var), (h))
#define OPT__QUIET(var, h) OPT_COUNTUP('q', "quiet", (var), (h))
--
2.11.0
