All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] Reduce parse-options.o dependencies
@ 2011-08-11  9:15 Dmitry Ivankov
  2011-08-11  9:15 ` [PATCH v3 1/2] parse-options: export opterr, optbug Dmitry Ivankov
  2011-08-11  9:15 ` [PATCH v3 2/2] Reduce parse-options.o dependencies Dmitry Ivankov
  0 siblings, 2 replies; 6+ messages in thread
From: Dmitry Ivankov @ 2011-08-11  9:15 UTC (permalink / raw)
  To: git; +Cc: Jonathan Nieder, David Barr, Dmitry Ivankov

This is a reroll of [1]. The main purpose is to make parse-options.o more
self-contained so that it at least doesn't pull any extlibs. Immediate usage
is for stuff in contrib (svn-fe). Distant application is that some day we could
publish this small library separately.

Mostly no changes since [1], just commit message line wrapping, fixup for a
Makefile typo and rebase on top of master (new parse_opt_string_list moved to
parse-options-cb.c too).

[1] http://thread.gmane.org/gmane.comp.version-control.git/176318/focus=176574

Dmitry Ivankov (2):
  parse-options: export opterr, optbug
  Reduce parse-options.o dependencies

 Makefile           |    3 +-
 abspath.c          |   28 ++++++++++++
 parse-options-cb.c |  125 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 parse-options.c    |  125 +---------------------------------------------------
 parse-options.h    |    2 +
 setup.c            |   28 ------------
 6 files changed, 159 insertions(+), 152 deletions(-)
 create mode 100644 parse-options-cb.c

-- 
1.7.3.4

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v3 1/2] parse-options: export opterr, optbug
  2011-08-11  9:15 [PATCH v3 0/2] Reduce parse-options.o dependencies Dmitry Ivankov
@ 2011-08-11  9:15 ` Dmitry Ivankov
  2011-08-11 10:42   ` Jonathan Nieder
  2011-08-11  9:15 ` [PATCH v3 2/2] Reduce parse-options.o dependencies Dmitry Ivankov
  1 sibling, 1 reply; 6+ messages in thread
From: Dmitry Ivankov @ 2011-08-11  9:15 UTC (permalink / raw)
  To: git; +Cc: Jonathan Nieder, David Barr, Dmitry Ivankov

opterror and optbug functions are used by some of parsing routines
in parse-options.c to report errors and bugs respectively.

Export these functions to allow more custom parsing routines to use
them in a uniform way.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 parse-options.c |    4 ++--
 parse-options.h |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 879ea82..7b061af 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -12,14 +12,14 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
-static int optbug(const struct option *opt, const char *reason)
+int optbug(const struct option *opt, const char *reason)
 {
 	if (opt->long_name)
 		return error("BUG: option '%s' %s", opt->long_name, reason);
 	return error("BUG: switch '%c' %s", opt->short_name, reason);
 }
 
-static int opterror(const struct option *opt, const char *reason, int flags)
+int opterror(const struct option *opt, const char *reason, int flags)
 {
 	if (flags & OPT_SHORT)
 		return error("switch `%c' %s", opt->short_name, reason);
diff --git a/parse-options.h b/parse-options.h
index 05eb09b..59e0b52 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -165,6 +165,8 @@ extern NORETURN void usage_msg_opt(const char *msg,
 				   const char * const *usagestr,
 				   const struct option *options);
 
+extern int optbug(const struct option *opt, const char *reason);
+extern int opterror(const struct option *opt, const char *reason, int flags);
 /*----- incremental advanced APIs -----*/
 
 enum {
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v3 2/2] Reduce parse-options.o dependencies
  2011-08-11  9:15 [PATCH v3 0/2] Reduce parse-options.o dependencies Dmitry Ivankov
  2011-08-11  9:15 ` [PATCH v3 1/2] parse-options: export opterr, optbug Dmitry Ivankov
@ 2011-08-11  9:15 ` Dmitry Ivankov
  2011-08-11 11:04   ` Jonathan Nieder
  1 sibling, 1 reply; 6+ messages in thread
From: Dmitry Ivankov @ 2011-08-11  9:15 UTC (permalink / raw)
  To: git; +Cc: Jonathan Nieder, David Barr, Dmitry Ivankov

Currently parse-options.o pulls quite a big bunch of dependencies.
his complicates it's usage in contrib/ because it pulls external
dependencies and it also increases executables size.

Split off less generic and more internal to git part of
parse-options.c to parse-options-cb.c.

Move prefix_filename function from setup.c to abspath.c. abspath.o
and wrapper.o pull each other, so it's unlikely to increase the
dependencies. It was a dependency of parse-options.o that pulled
many others.

Now parse-options.o pulls just abspath.o, ctype.o, strbuf.o, usage.o,
wrapper.o, libc directly and strlcpy.o indirectly.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 Makefile           |    3 +-
 abspath.c          |   28 ++++++++++++
 parse-options-cb.c |  125 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 parse-options.c    |  121 --------------------------------------------------
 setup.c            |   28 ------------
 5 files changed, 155 insertions(+), 150 deletions(-)
 create mode 100644 parse-options-cb.c

diff --git a/Makefile b/Makefile
index 62ad0c2..7d47bdb 100644
--- a/Makefile
+++ b/Makefile
@@ -642,6 +642,7 @@ LIB_OBJS += pack-revindex.o
 LIB_OBJS += pack-write.o
 LIB_OBJS += pager.o
 LIB_OBJS += parse-options.o
+LIB_OBJS += parse-options-cb.o
 LIB_OBJS += patch-delta.o
 LIB_OBJS += patch-ids.o
 LIB_OBJS += path.o
@@ -2204,7 +2205,7 @@ test-delta$X: diff-delta.o patch-delta.o
 
 test-line-buffer$X: vcs-svn/lib.a
 
-test-parse-options$X: parse-options.o
+test-parse-options$X: parse-options.o parse-options-cb.o
 
 test-string-pool$X: vcs-svn/lib.a
 
diff --git a/abspath.c b/abspath.c
index 37287f8..f04ac18 100644
--- a/abspath.c
+++ b/abspath.c
@@ -139,3 +139,31 @@ const char *absolute_path(const char *path)
 	}
 	return buf;
 }
+
+/*
+ * Unlike prefix_path, this should be used if the named file does
+ * not have to interact with index entry; i.e. name of a random file
+ * on the filesystem.
+ */
+const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
+{
+	static char path[PATH_MAX];
+#ifndef WIN32
+	if (!pfx_len || is_absolute_path(arg))
+		return arg;
+	memcpy(path, pfx, pfx_len);
+	strcpy(path + pfx_len, arg);
+#else
+	char *p;
+	/* don't add prefix to absolute paths, but still replace '\' by '/' */
+	if (is_absolute_path(arg))
+		pfx_len = 0;
+	else if (pfx_len)
+		memcpy(path, pfx, pfx_len);
+	strcpy(path + pfx_len, arg);
+	for (p = path + pfx_len; *p; p++)
+		if (*p == '\\')
+			*p = '/';
+#endif
+	return path;
+}
diff --git a/parse-options-cb.c b/parse-options-cb.c
new file mode 100644
index 0000000..c248f66
--- /dev/null
+++ b/parse-options-cb.c
@@ -0,0 +1,125 @@
+#include "git-compat-util.h"
+#include "parse-options.h"
+#include "cache.h"
+#include "commit.h"
+#include "color.h"
+#include "string-list.h"
+
+/*----- some often used options -----*/
+
+int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
+{
+	int v;
+
+	if (!arg) {
+		v = unset ? 0 : DEFAULT_ABBREV;
+	} else {
+		v = strtol(arg, (char **)&arg, 10);
+		if (*arg)
+			return opterror(opt, "expects a numerical value", 0);
+		if (v && v < MINIMUM_ABBREV)
+			v = MINIMUM_ABBREV;
+		else if (v > 40)
+			v = 40;
+	}
+	*(int *)(opt->value) = v;
+	return 0;
+}
+
+int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
+			     int unset)
+{
+	*(unsigned long *)(opt->value) = approxidate(arg);
+	return 0;
+}
+
+int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
+			    int unset)
+{
+	int value;
+
+	if (!arg)
+		arg = unset ? "never" : (const char *)opt->defval;
+	value = git_config_colorbool(NULL, arg, -1);
+	if (value < 0)
+		return opterror(opt,
+			"expects \"always\", \"auto\", or \"never\"", 0);
+	*(int *)opt->value = value;
+	return 0;
+}
+
+int parse_opt_verbosity_cb(const struct option *opt, const char *arg,
+			   int unset)
+{
+	int *target = opt->value;
+
+	if (unset)
+		/* --no-quiet, --no-verbose */
+		*target = 0;
+	else if (opt->short_name == 'v') {
+		if (*target >= 0)
+			(*target)++;
+		else
+			*target = 1;
+	} else {
+		if (*target <= 0)
+			(*target)--;
+		else
+			*target = -1;
+	}
+	return 0;
+}
+
+int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)
+{
+	unsigned char sha1[20];
+	struct commit *commit;
+
+	if (!arg)
+		return -1;
+	if (get_sha1(arg, sha1))
+		return error("malformed object name %s", arg);
+	commit = lookup_commit_reference(sha1);
+	if (!commit)
+		return error("no such commit %s", arg);
+	commit_list_insert(commit, opt->value);
+	return 0;
+}
+
+int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
+{
+	int *target = opt->value;
+	*target = unset ? 2 : 1;
+	return 0;
+}
+
+int parse_options_concat(struct option *dst, size_t dst_size, struct option *src)
+{
+	int i, j;
+
+	for (i = 0; i < dst_size; i++)
+		if (dst[i].type == OPTION_END)
+			break;
+	for (j = 0; i < dst_size; i++, j++) {
+		dst[i] = src[j];
+		if (src[j].type == OPTION_END)
+			return 0;
+	}
+	return -1;
+}
+
+int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
+{
+	struct string_list *v = opt->value;
+
+	if (unset) {
+		string_list_clear(v, 0);
+		return 0;
+	}
+
+	if (!arg)
+		return -1;
+
+	string_list_append(v, xstrdup(arg));
+	return 0;
+}
diff --git a/parse-options.c b/parse-options.c
index 7b061af..503ab5d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -3,7 +3,6 @@
 #include "cache.h"
 #include "commit.h"
 #include "color.h"
-#include "string-list.h"
 
 static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 			       const char * const *usagestr,
@@ -584,123 +583,3 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 	return usage_with_options_internal(ctx, usagestr, opts, 0, err);
 }
 
-
-/*----- some often used options -----*/
-#include "cache.h"
-
-int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
-{
-	int v;
-
-	if (!arg) {
-		v = unset ? 0 : DEFAULT_ABBREV;
-	} else {
-		v = strtol(arg, (char **)&arg, 10);
-		if (*arg)
-			return opterror(opt, "expects a numerical value", 0);
-		if (v && v < MINIMUM_ABBREV)
-			v = MINIMUM_ABBREV;
-		else if (v > 40)
-			v = 40;
-	}
-	*(int *)(opt->value) = v;
-	return 0;
-}
-
-int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
-			     int unset)
-{
-	*(unsigned long *)(opt->value) = approxidate(arg);
-	return 0;
-}
-
-int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
-			    int unset)
-{
-	int value;
-
-	if (!arg)
-		arg = unset ? "never" : (const char *)opt->defval;
-	value = git_config_colorbool(NULL, arg, -1);
-	if (value < 0)
-		return opterror(opt,
-			"expects \"always\", \"auto\", or \"never\"", 0);
-	*(int *)opt->value = value;
-	return 0;
-}
-
-int parse_opt_verbosity_cb(const struct option *opt, const char *arg,
-			   int unset)
-{
-	int *target = opt->value;
-
-	if (unset)
-		/* --no-quiet, --no-verbose */
-		*target = 0;
-	else if (opt->short_name == 'v') {
-		if (*target >= 0)
-			(*target)++;
-		else
-			*target = 1;
-	} else {
-		if (*target <= 0)
-			(*target)--;
-		else
-			*target = -1;
-	}
-	return 0;
-}
-
-int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)
-{
-	unsigned char sha1[20];
-	struct commit *commit;
-
-	if (!arg)
-		return -1;
-	if (get_sha1(arg, sha1))
-		return error("malformed object name %s", arg);
-	commit = lookup_commit_reference(sha1);
-	if (!commit)
-		return error("no such commit %s", arg);
-	commit_list_insert(commit, opt->value);
-	return 0;
-}
-
-int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
-{
-	int *target = opt->value;
-	*target = unset ? 2 : 1;
-	return 0;
-}
-
-int parse_options_concat(struct option *dst, size_t dst_size, struct option *src)
-{
-	int i, j;
-
-	for (i = 0; i < dst_size; i++)
-		if (dst[i].type == OPTION_END)
-			break;
-	for (j = 0; i < dst_size; i++, j++) {
-		dst[i] = src[j];
-		if (src[j].type == OPTION_END)
-			return 0;
-	}
-	return -1;
-}
-
-int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
-{
-	struct string_list *v = opt->value;
-
-	if (unset) {
-		string_list_clear(v, 0);
-		return 0;
-	}
-
-	if (!arg)
-		return -1;
-
-	string_list_append(v, xstrdup(arg));
-	return 0;
-}
diff --git a/setup.c b/setup.c
index 5ea5502..3463819 100644
--- a/setup.c
+++ b/setup.c
@@ -40,34 +40,6 @@ char *prefix_path(const char *prefix, int len, const char *path)
 	return sanitized;
 }
 
-/*
- * Unlike prefix_path, this should be used if the named file does
- * not have to interact with index entry; i.e. name of a random file
- * on the filesystem.
- */
-const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
-{
-	static char path[PATH_MAX];
-#ifndef WIN32
-	if (!pfx_len || is_absolute_path(arg))
-		return arg;
-	memcpy(path, pfx, pfx_len);
-	strcpy(path + pfx_len, arg);
-#else
-	char *p;
-	/* don't add prefix to absolute paths, but still replace '\' by '/' */
-	if (is_absolute_path(arg))
-		pfx_len = 0;
-	else if (pfx_len)
-		memcpy(path, pfx, pfx_len);
-	strcpy(path + pfx_len, arg);
-	for (p = path + pfx_len; *p; p++)
-		if (*p == '\\')
-			*p = '/';
-#endif
-	return path;
-}
-
 int check_filename(const char *prefix, const char *arg)
 {
 	const char *name;
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] parse-options: export opterr, optbug
  2011-08-11  9:15 ` [PATCH v3 1/2] parse-options: export opterr, optbug Dmitry Ivankov
@ 2011-08-11 10:42   ` Jonathan Nieder
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Nieder @ 2011-08-11 10:42 UTC (permalink / raw)
  To: Dmitry Ivankov; +Cc: git, David Barr, Stephen Boyd

(+cc: Stephen)
Dmitry Ivankov wrote:

> opterror and optbug functions are used by some of parsing routines
> in parse-options.c to report errors and bugs respectively.
>
> Export these functions to allow more custom parsing routines to use
> them in a uniform way.

In other words, exposing opterror() allows custom option types to
behave more like the built-in ones when producing messages like
"option `<opt>' expects a numerical value".  What should they pass
in the "flags" argument?  Does this deserve a mention in the
"Option Callbacks" section of
Documentation/technical/api-parse-options.txt?

Would opterror() be enough?  I don't see any current users of optbug
outside of parse_options_check() (which is part of low-level
machinery).

Aside from that, seems sensible.

[quoting in full for Stephen's convenience.  One quick comment
below.]
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> ---
>  parse-options.c |    4 ++--
>  parse-options.h |    2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 879ea82..7b061af 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -12,14 +12,14 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
>  #define OPT_SHORT 1
>  #define OPT_UNSET 2
>  
> -static int optbug(const struct option *opt, const char *reason)
> +int optbug(const struct option *opt, const char *reason)
>  {
>  	if (opt->long_name)
>  		return error("BUG: option '%s' %s", opt->long_name, reason);
>  	return error("BUG: switch '%c' %s", opt->short_name, reason);
>  }
>  
> -static int opterror(const struct option *opt, const char *reason, int flags)
> +int opterror(const struct option *opt, const char *reason, int flags)
>  {
>  	if (flags & OPT_SHORT)
>  		return error("switch `%c' %s", opt->short_name, reason);
> diff --git a/parse-options.h b/parse-options.h
> index 05eb09b..59e0b52 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -165,6 +165,8 @@ extern NORETURN void usage_msg_opt(const char *msg,
>  				   const char * const *usagestr,
>  				   const struct option *options);
>  
> +extern int optbug(const struct option *opt, const char *reason);
> +extern int opterror(const struct option *opt, const char *reason, int flags);
>  /*----- incremental advanced APIs -----*/

A blank line above the comment would make this more readable.

>  
>  enum {
> -- 

Except as noted above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Some (though not all) of the possible changes noted above implemented
below, plus an example caller (which made reading the patch a little
easier).  What do you think?

 builtin/read-tree.c |    2 +-
 parse-options.c     |    2 +-
 parse-options.h     |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git c/builtin/read-tree.c i/builtin/read-tree.c
index df6c4c88..6013090c 100644
--- c/builtin/read-tree.c
+++ i/builtin/read-tree.c
@@ -53,7 +53,7 @@ static int exclude_per_directory_cb(const struct option *opt, const char *arg,
 	opts = (struct unpack_trees_options *)opt->value;
 
 	if (opts->dir)
-		die("more than one --exclude-per-directory given.");
+		return opterror(opt, "can only be supplied once", 0);
 
 	dir = xcalloc(1, sizeof(*opts->dir));
 	dir->flags |= DIR_SHOW_IGNORED;
diff --git c/parse-options.c i/parse-options.c
index 7b061afc..777611b1 100644
--- c/parse-options.c
+++ i/parse-options.c
@@ -12,7 +12,7 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
-int optbug(const struct option *opt, const char *reason)
+static int optbug(const struct option *opt, const char *reason)
 {
 	if (opt->long_name)
 		return error("BUG: option '%s' %s", opt->long_name, reason);
diff --git c/parse-options.h i/parse-options.h
index 59e0b524..6d31ad3a 100644
--- c/parse-options.h
+++ i/parse-options.h
@@ -165,8 +165,8 @@ extern NORETURN void usage_msg_opt(const char *msg,
 				   const char * const *usagestr,
 				   const struct option *options);
 
-extern int optbug(const struct option *opt, const char *reason);
 extern int opterror(const struct option *opt, const char *reason, int flags);
+
 /*----- incremental advanced APIs -----*/
 
 enum {

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] Reduce parse-options.o dependencies
  2011-08-11  9:15 ` [PATCH v3 2/2] Reduce parse-options.o dependencies Dmitry Ivankov
@ 2011-08-11 11:04   ` Jonathan Nieder
  2011-08-11 23:35     ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Nieder @ 2011-08-11 11:04 UTC (permalink / raw)
  To: Dmitry Ivankov; +Cc: git, David Barr, Stephen Boyd

(+cc: Stephen)
Dmitry Ivankov wrote:

> Currently parse-options.o pulls quite a big bunch of dependencies.
> his complicates it's usage in contrib/ because it pulls external
> dependencies and it also increases executables size.
>
> Split off less generic and more internal to git part of
> parse-options.c to parse-options-cb.c.
>
> Move prefix_filename function from setup.c to abspath.c. abspath.o
> and wrapper.o pull each other, so it's unlikely to increase the
> dependencies. It was a dependency of parse-options.o that pulled
> many others.
>
> Now parse-options.o pulls just abspath.o, ctype.o, strbuf.o, usage.o,
> wrapper.o, libc directly and strlcpy.o indirectly.

So, in other words, currently linking to parse-options involves
linking to git's object access machinery and diff machinery, hence
libz, libssl for SHA-1, libpcre, etc.  That is a waste of space,
startup time, and build complexity for simple programs that do not
need access to git's object db.

This patch does two things to address that:

 - option callbacks which freely use the git object db and other
   facilities move to a separate parse-options-cb.o translation
   unit, so simple programs can avoid them;

 - prefix_filename, which is used to support OPTION_FILENAME,
   moves from setup.c to abspath.c, so it can be used without
   pulling in unrelated git machinery.

The result is, as you say, that use of parse-options.o only pulls
in abspath, ctype, strbuf, usage, wrapper, and -lc --- which is to
say, just utility functions --- and you can build a program using
parse-options with a simple commandline like

	cc -o test-program -Wall -W -O2 test-program.c libgit.a

Okay, on to the patch itself (comments sparsely scattered within).

>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> ---
>  Makefile           |    3 +-
>  abspath.c          |   28 ++++++++++++
>  parse-options-cb.c |  125 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  parse-options.c    |  121 --------------------------------------------------
>  setup.c            |   28 ------------
>  5 files changed, 155 insertions(+), 150 deletions(-)
>  create mode 100644 parse-options-cb.c
> 
> diff --git a/Makefile b/Makefile
> index 62ad0c2..7d47bdb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -642,6 +642,7 @@ LIB_OBJS += pack-revindex.o
>  LIB_OBJS += pack-write.o
>  LIB_OBJS += pager.o
>  LIB_OBJS += parse-options.o
> +LIB_OBJS += parse-options-cb.o
>  LIB_OBJS += patch-delta.o
>  LIB_OBJS += patch-ids.o
>  LIB_OBJS += path.o
> @@ -2204,7 +2205,7 @@ test-delta$X: diff-delta.o patch-delta.o
>  
>  test-line-buffer$X: vcs-svn/lib.a
>  
> -test-parse-options$X: parse-options.o
> +test-parse-options$X: parse-options.o parse-options-cb.o
>  
>  test-string-pool$X: vcs-svn/lib.a
>  
> diff --git a/abspath.c b/abspath.c
> index 37287f8..f04ac18 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -139,3 +139,31 @@ const char *absolute_path(const char *path)
>  	}
>  	return buf;
>  }
> +
> +/*
> + * Unlike prefix_path, this should be used if the named file does
> + * not have to interact with index entry; i.e. name of a random file
> + * on the filesystem.
> + */
> +const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
> +{
> +	static char path[PATH_MAX];
> +#ifndef WIN32
> +	if (!pfx_len || is_absolute_path(arg))
> +		return arg;
> +	memcpy(path, pfx, pfx_len);
> +	strcpy(path + pfx_len, arg);
> +#else
> +	char *p;
> +	/* don't add prefix to absolute paths, but still replace '\' by '/' */
> +	if (is_absolute_path(arg))
> +		pfx_len = 0;
> +	else if (pfx_len)
> +		memcpy(path, pfx, pfx_len);
> +	strcpy(path + pfx_len, arg);
> +	for (p = path + pfx_len; *p; p++)
> +		if (*p == '\\')
> +			*p = '/';
> +#endif
> +	return path;
> +}
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> new file mode 100644
> index 0000000..c248f66
> --- /dev/null
> +++ b/parse-options-cb.c
> @@ -0,0 +1,125 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +#include "cache.h"

Style: Files in git tend to use only one of "git-compat-util.h",
"cache.h", or "builtin.h" and put it at the top.  So in this case, it
should probably use just "cache.h".

> +#include "commit.h"
> +#include "color.h"
> +#include "string-list.h"
> +
> +/*----- some often used options -----*/
> +
> +int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
> +{
> +	int v;
> +
> +	if (!arg) {
> +		v = unset ? 0 : DEFAULT_ABBREV;
> +	} else {
> +		v = strtol(arg, (char **)&arg, 10);
> +		if (*arg)
> +			return opterror(opt, "expects a numerical value", 0);
> +		if (v && v < MINIMUM_ABBREV)
> +			v = MINIMUM_ABBREV;
> +		else if (v > 40)
> +			v = 40;
> +	}
> +	*(int *)(opt->value) = v;
> +	return 0;
> +}
> +
> +int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
> +			     int unset)
> +{
> +	*(unsigned long *)(opt->value) = approxidate(arg);
> +	return 0;
> +}
> +
> +int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
> +			    int unset)
> +{
> +	int value;
> +
> +	if (!arg)
> +		arg = unset ? "never" : (const char *)opt->defval;
> +	value = git_config_colorbool(NULL, arg, -1);
> +	if (value < 0)
> +		return opterror(opt,
> +			"expects \"always\", \"auto\", or \"never\"", 0);
> +	*(int *)opt->value = value;
> +	return 0;
> +}
> +
> +int parse_opt_verbosity_cb(const struct option *opt, const char *arg,
> +			   int unset)
> +{
> +	int *target = opt->value;
> +
> +	if (unset)
> +		/* --no-quiet, --no-verbose */
> +		*target = 0;
> +	else if (opt->short_name == 'v') {
> +		if (*target >= 0)
> +			(*target)++;
> +		else
> +			*target = 1;
> +	} else {
> +		if (*target <= 0)
> +			(*target)--;
> +		else
> +			*target = -1;
> +	}
> +	return 0;
> +}
> +
> +int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)
> +{
> +	unsigned char sha1[20];
> +	struct commit *commit;
> +
> +	if (!arg)
> +		return -1;
> +	if (get_sha1(arg, sha1))
> +		return error("malformed object name %s", arg);
> +	commit = lookup_commit_reference(sha1);
> +	if (!commit)
> +		return error("no such commit %s", arg);
> +	commit_list_insert(commit, opt->value);
> +	return 0;
> +}
> +
> +int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
> +{
> +	int *target = opt->value;
> +	*target = unset ? 2 : 1;
> +	return 0;
> +}
> +
> +int parse_options_concat(struct option *dst, size_t dst_size, struct option *src)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < dst_size; i++)
> +		if (dst[i].type == OPTION_END)
> +			break;
> +	for (j = 0; i < dst_size; i++, j++) {
> +		dst[i] = src[j];
> +		if (src[j].type == OPTION_END)
> +			return 0;
> +	}
> +	return -1;
> +}
> +
> +int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
> +{
> +	struct string_list *v = opt->value;
> +
> +	if (unset) {
> +		string_list_clear(v, 0);
> +		return 0;
> +	}
> +
> +	if (!arg)
> +		return -1;
> +
> +	string_list_append(v, xstrdup(arg));
> +	return 0;
> +}
> diff --git a/parse-options.c b/parse-options.c
> index 7b061af..503ab5d 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -3,7 +3,6 @@
>  #include "cache.h"
>  #include "commit.h"
>  #include "color.h"
> -#include "string-list.h"
>  
>  static int parse_options_usage(struct parse_opt_ctx_t *ctx,
>  			       const char * const *usagestr,
> @@ -584,123 +583,3 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
>  	return usage_with_options_internal(ctx, usagestr, opts, 0, err);
>  }
>  
> -
> -/*----- some often used options -----*/
> -#include "cache.h"
> -
> -int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
[... snip code moved verbatim ...]
> -}
> diff --git a/setup.c b/setup.c
> index 5ea5502..3463819 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -40,34 +40,6 @@ char *prefix_path(const char *prefix, int len, const char *path)
>  	return sanitized;
>  }
>  
> -/*
> - * Unlike prefix_path, this should be used if the named file does
> - * not have to interact with index entry; i.e. name of a random file
> - * on the filesystem.
> - */
> -const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
> -{
> -	static char path[PATH_MAX];
> -#ifndef WIN32
> -	if (!pfx_len || is_absolute_path(arg))
> -		return arg;
> -	memcpy(path, pfx, pfx_len);
> -	strcpy(path + pfx_len, arg);
> -#else
> -	char *p;
> -	/* don't add prefix to absolute paths, but still replace '\' by '/' */
> -	if (is_absolute_path(arg))
> -		pfx_len = 0;
> -	else if (pfx_len)
> -		memcpy(path, pfx, pfx_len);
> -	strcpy(path + pfx_len, arg);
> -	for (p = path + pfx_len; *p; p++)
> -		if (*p == '\\')
> -			*p = '/';
> -#endif
> -	return path;
> -}
> -
>  int check_filename(const char *prefix, const char *arg)
>  {
>  	const char *name;
> -- 
> 1.7.3.4
> 

Except for the commit message, and with or without the #include tweak
mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.  This seems to be in pretty good shape (and as mentioned before,
gets closer to fulfillment of a longstanding wish :)).

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] Reduce parse-options.o dependencies
  2011-08-11 11:04   ` Jonathan Nieder
@ 2011-08-11 23:35     ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2011-08-11 23:35 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Dmitry Ivankov, git, David Barr, Stephen Boyd

Jonathan Nieder <jrnieder@gmail.com> writes:

>> diff --git a/parse-options-cb.c b/parse-options-cb.c
>> new file mode 100644
>> index 0000000..c248f66
>> --- /dev/null
>> +++ b/parse-options-cb.c
>> @@ -0,0 +1,125 @@
>> +#include "git-compat-util.h"
>> +#include "parse-options.h"
>> +#include "cache.h"
>
> Style: Files in git tend to use only one of "git-compat-util.h",
> "cache.h", or "builtin.h" and put it at the top.  So in this case, it
> should probably use just "cache.h".

This needs a bit of clarification.

The compatibility rule actually is to have git-compat-util.h at the very
beginning. As cache.h is a very widely used header almost everybody that
needs to access the internals include, it includes git-compat-util.h as
its first thing to include.

So if this is an old file that existed before git-compat-util.h, then it
is perfectly fine to start it with

	#include "cache.h"
        #include "parse-options.h"

Otherwise the three-line include above is also just fine.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-08-11 23:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-11  9:15 [PATCH v3 0/2] Reduce parse-options.o dependencies Dmitry Ivankov
2011-08-11  9:15 ` [PATCH v3 1/2] parse-options: export opterr, optbug Dmitry Ivankov
2011-08-11 10:42   ` Jonathan Nieder
2011-08-11  9:15 ` [PATCH v3 2/2] Reduce parse-options.o dependencies Dmitry Ivankov
2011-08-11 11:04   ` Jonathan Nieder
2011-08-11 23:35     ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.