git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Ævar Arnfjörð Bjarmason  <avarab@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>,
	Duy Nguyen <pclouds@gmail.com>,
	Brandon Williams <bmwill@google.com>,
	Ævar Arnfjörð Bjarmason  <avarab@gmail.com>
Subject: [PATCH/RFC] parse-options: add facility to make options configurable
Date: Fri, 24 Mar 2017 23:10:13 +0000
Message-ID: <20170324231013.23346-1-avarab@gmail.com> (raw)
In-Reply-To: <CACBZZX4FksU6NujPZ_3GZ45EQ+KdJj5G2sajtRipE1wbaA3URA@mail.gmail.com>

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+KdJj5G2sajtRipE1wbaA3URA@mail.gmail.com>
for more details.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sun, Mar 19, 2017 at 2:43 PM, Ævar Arnfjörð Bjarmason <avarab@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


  parent reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-19  9:57 Add configuration options for some commonly used command-line options (Was: [RFH] GSoC 2015 application) Duy Nguyen
2017-03-19 10:15 ` Add configuration options for some commonly used command-line options Matthieu Moy
2017-03-19 13:18   ` brian m. carlson
2017-03-19 13:43     ` Ævar Arnfjörð Bjarmason
2017-03-20 10:56       ` Duy Nguyen
2017-03-20 17:32         ` Brandon Williams
2017-03-20 18:18           ` Jeff King
2017-03-31 19:44             ` Brandon McCaig
2017-03-20 18:56           ` Junio C Hamano
2017-03-20 19:14             ` Jeff King
2017-03-20 21:57             ` Ævar Arnfjörð Bjarmason
2017-03-24 23:10       ` Ævar Arnfjörð Bjarmason [this message]
2017-03-25 16:47         ` [PATCH/RFC] parse-options: add facility to make options configurable Ævar Arnfjörð Bjarmason
2017-03-25 21:31           ` Jeff King
2017-03-25 22:32             ` Ævar Arnfjörð Bjarmason
2017-03-28  5:17               ` Jeff King
2017-03-28 13:13                 ` [PATCH/RFC v2] WIP configurable options facility Ævar Arnfjörð Bjarmason
2017-03-25 21:28         ` [PATCH/RFC] parse-options: add facility to make options configurable brian m. carlson
2017-03-20 10:42     ` Add configuration options for some commonly used command-line options Duy Nguyen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170324231013.23346-1-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox