{"thread":{"id":"28066","subject":"[PATCH v3 0/2] Reduce parse-options.o dependencies","startedAt":"2011-08-11T09:15:36Z","lastAt":"2011-08-11T23:35:02Z","messageCount":6,"participants":["Dmitry Ivankov","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"173267","messageId":"1313054138-30885-1-git-send-email-divanorama@gmail.com","threadId":"28066","inReplyTo":null,"subject":"[PATCH v3 0/2] Reduce parse-options.o dependencies","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-11T09:15:36Z","receivedAt":"2011-08-11T09:15:36Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"This is a reroll of [1]. The main purpose is to make parse-options.o more\nself-contained so that it at least doesn't pull any extlibs. Immediate usage\nis for stuff in contrib (svn-fe). Distant application is that some day we could\npublish this small library separately.\n\nMostly no changes since [1], just commit message line wrapping, fixup for a\nMakefile typo and rebase on top of master (new parse_opt_string_list moved to\nparse-options-cb.c too).\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/176318/focus=176574\n\nDmitry Ivankov (2):\n  parse-options: export opterr, optbug\n  Reduce parse-options.o dependencies\n\n Makefile           |    3 +-\n abspath.c          |   28 ++++++++++++\n parse-options-cb.c |  125 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n parse-options.c    |  125 +---------------------------------------------------\n parse-options.h    |    2 +\n setup.c            |   28 ------------\n 6 files changed, 159 insertions(+), 152 deletions(-)\n create mode 100644 parse-options-cb.c\n\n-- \n1.7.3.4\n"},{"id":"173268","messageId":"1313054138-30885-2-git-send-email-divanorama@gmail.com","threadId":"28066","inReplyTo":"1313054138-30885-1-git-send-email-divanorama@gmail.com","subject":"[PATCH v3 1/2] parse-options: export opterr, optbug","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-11T09:15:37Z","receivedAt":"2011-08-11T09:15:37Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"opterror and optbug functions are used by some of parsing routines\nin parse-options.c to report errors and bugs respectively.\n\nExport these functions to allow more custom parsing routines to use\nthem in a uniform way.\n\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\n---\n parse-options.c |    4 ++--\n parse-options.h |    2 ++\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 879ea82..7b061af 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -12,14 +12,14 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n-static int optbug(const struct option *opt, const char *reason)\n+int optbug(const struct option *opt, const char *reason)\n {\n \tif (opt->long_name)\n \t\treturn error(\"BUG: option '%s' %s\", opt->long_name, reason);\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n-static int opterror(const struct option *opt, const char *reason, int flags)\n+int opterror(const struct option *opt, const char *reason, int flags)\n {\n \tif (flags & OPT_SHORT)\n \t\treturn error(\"switch `%c' %s\", opt->short_name, reason);\ndiff --git a/parse-options.h b/parse-options.h\nindex 05eb09b..59e0b52 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -165,6 +165,8 @@ extern NORETURN void usage_msg_opt(const char *msg,\n \t\t\t\t   const char * const *usagestr,\n \t\t\t\t   const struct option *options);\n \n+extern int optbug(const struct option *opt, const char *reason);\n+extern int opterror(const struct option *opt, const char *reason, int flags);\n /*----- incremental advanced APIs -----*/\n \n enum {\n-- \n1.7.3.4\n"},{"id":"173269","messageId":"1313054138-30885-3-git-send-email-divanorama@gmail.com","threadId":"28066","inReplyTo":"1313054138-30885-1-git-send-email-divanorama@gmail.com","subject":"[PATCH v3 2/2] Reduce parse-options.o dependencies","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-08-11T09:15:38Z","receivedAt":"2011-08-11T09:15:38Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"Currently parse-options.o pulls quite a big bunch of dependencies.\nhis complicates it's usage in contrib/ because it pulls external\ndependencies and it also increases executables size.\n\nSplit off less generic and more internal to git part of\nparse-options.c to parse-options-cb.c.\n\nMove prefix_filename function from setup.c to abspath.c. abspath.o\nand wrapper.o pull each other, so it's unlikely to increase the\ndependencies. It was a dependency of parse-options.o that pulled\nmany others.\n\nNow parse-options.o pulls just abspath.o, ctype.o, strbuf.o, usage.o,\nwrapper.o, libc directly and strlcpy.o indirectly.\n\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\n---\n Makefile           |    3 +-\n abspath.c          |   28 ++++++++++++\n parse-options-cb.c |  125 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n parse-options.c    |  121 --------------------------------------------------\n setup.c            |   28 ------------\n 5 files changed, 155 insertions(+), 150 deletions(-)\n create mode 100644 parse-options-cb.c\n\ndiff --git a/Makefile b/Makefile\nindex 62ad0c2..7d47bdb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -642,6 +642,7 @@ LIB_OBJS += pack-revindex.o\n LIB_OBJS += pack-write.o\n LIB_OBJS += pager.o\n LIB_OBJS += parse-options.o\n+LIB_OBJS += parse-options-cb.o\n LIB_OBJS += patch-delta.o\n LIB_OBJS += patch-ids.o\n LIB_OBJS += path.o\n@@ -2204,7 +2205,7 @@ test-delta$X: diff-delta.o patch-delta.o\n \n test-line-buffer$X: vcs-svn/lib.a\n \n-test-parse-options$X: parse-options.o\n+test-parse-options$X: parse-options.o parse-options-cb.o\n \n test-string-pool$X: vcs-svn/lib.a\n \ndiff --git a/abspath.c b/abspath.c\nindex 37287f8..f04ac18 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -139,3 +139,31 @@ const char *absolute_path(const char *path)\n \t}\n \treturn buf;\n }\n+\n+/*\n+ * Unlike prefix_path, this should be used if the named file does\n+ * not have to interact with index entry; i.e. name of a random file\n+ * on the filesystem.\n+ */\n+const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)\n+{\n+\tstatic char path[PATH_MAX];\n+#ifndef WIN32\n+\tif (!pfx_len || is_absolute_path(arg))\n+\t\treturn arg;\n+\tmemcpy(path, pfx, pfx_len);\n+\tstrcpy(path + pfx_len, arg);\n+#else\n+\tchar *p;\n+\t/* don't add prefix to absolute paths, but still replace '\\' by '/' */\n+\tif (is_absolute_path(arg))\n+\t\tpfx_len = 0;\n+\telse if (pfx_len)\n+\t\tmemcpy(path, pfx, pfx_len);\n+\tstrcpy(path + pfx_len, arg);\n+\tfor (p = path + pfx_len; *p; p++)\n+\t\tif (*p == '\\\\')\n+\t\t\t*p = '/';\n+#endif\n+\treturn path;\n+}\ndiff --git a/parse-options-cb.c b/parse-options-cb.c\nnew file mode 100644\nindex 0000000..c248f66\n--- /dev/null\n+++ b/parse-options-cb.c\n@@ -0,0 +1,125 @@\n+#include \"git-compat-util.h\"\n+#include \"parse-options.h\"\n+#include \"cache.h\"\n+#include \"commit.h\"\n+#include \"color.h\"\n+#include \"string-list.h\"\n+\n+/*----- some often used options -----*/\n+\n+int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\tint v;\n+\n+\tif (!arg) {\n+\t\tv = unset ? 0 : DEFAULT_ABBREV;\n+\t} else {\n+\t\tv = strtol(arg, (char **)&arg, 10);\n+\t\tif (*arg)\n+\t\t\treturn opterror(opt, \"expects a numerical value\", 0);\n+\t\tif (v && v < MINIMUM_ABBREV)\n+\t\t\tv = MINIMUM_ABBREV;\n+\t\telse if (v > 40)\n+\t\t\tv = 40;\n+\t}\n+\t*(int *)(opt->value) = v;\n+\treturn 0;\n+}\n+\n+int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n+\t\t\t     int unset)\n+{\n+\t*(unsigned long *)(opt->value) = approxidate(arg);\n+\treturn 0;\n+}\n+\n+int parse_opt_color_flag_cb(const struct option *opt, const char *arg,\n+\t\t\t    int unset)\n+{\n+\tint value;\n+\n+\tif (!arg)\n+\t\targ = unset ? \"never\" : (const char *)opt->defval;\n+\tvalue = git_config_colorbool(NULL, arg, -1);\n+\tif (value < 0)\n+\t\treturn opterror(opt,\n+\t\t\t\"expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\", 0);\n+\t*(int *)opt->value = value;\n+\treturn 0;\n+}\n+\n+int parse_opt_verbosity_cb(const struct option *opt, const char *arg,\n+\t\t\t   int unset)\n+{\n+\tint *target = opt->value;\n+\n+\tif (unset)\n+\t\t/* --no-quiet, --no-verbose */\n+\t\t*target = 0;\n+\telse if (opt->short_name == 'v') {\n+\t\tif (*target >= 0)\n+\t\t\t(*target)++;\n+\t\telse\n+\t\t\t*target = 1;\n+\t} else {\n+\t\tif (*target <= 0)\n+\t\t\t(*target)--;\n+\t\telse\n+\t\t\t*target = -1;\n+\t}\n+\treturn 0;\n+}\n+\n+int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)\n+{\n+\tunsigned char sha1[20];\n+\tstruct commit *commit;\n+\n+\tif (!arg)\n+\t\treturn -1;\n+\tif (get_sha1(arg, sha1))\n+\t\treturn error(\"malformed object name %s\", arg);\n+\tcommit = lookup_commit_reference(sha1);\n+\tif (!commit)\n+\t\treturn error(\"no such commit %s\", arg);\n+\tcommit_list_insert(commit, opt->value);\n+\treturn 0;\n+}\n+\n+int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)\n+{\n+\tint *target = opt->value;\n+\t*target = unset ? 2 : 1;\n+\treturn 0;\n+}\n+\n+int parse_options_concat(struct option *dst, size_t dst_size, struct option *src)\n+{\n+\tint i, j;\n+\n+\tfor (i = 0; i < dst_size; i++)\n+\t\tif (dst[i].type == OPTION_END)\n+\t\t\tbreak;\n+\tfor (j = 0; i < dst_size; i++, j++) {\n+\t\tdst[i] = src[j];\n+\t\tif (src[j].type == OPTION_END)\n+\t\t\treturn 0;\n+\t}\n+\treturn -1;\n+}\n+\n+int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct string_list *v = opt->value;\n+\n+\tif (unset) {\n+\t\tstring_list_clear(v, 0);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!arg)\n+\t\treturn -1;\n+\n+\tstring_list_append(v, xstrdup(arg));\n+\treturn 0;\n+}\ndiff --git a/parse-options.c b/parse-options.c\nindex 7b061af..503ab5d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -3,7 +3,6 @@\n #include \"cache.h\"\n #include \"commit.h\"\n #include \"color.h\"\n-#include \"string-list.h\"\n \n static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n \t\t\t       const char * const *usagestr,\n@@ -584,123 +583,3 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n \treturn usage_with_options_internal(ctx, usagestr, opts, 0, err);\n }\n \n-\n-/*----- some often used options -----*/\n-#include \"cache.h\"\n-\n-int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n-{\n-\tint v;\n-\n-\tif (!arg) {\n-\t\tv = unset ? 0 : DEFAULT_ABBREV;\n-\t} else {\n-\t\tv = strtol(arg, (char **)&arg, 10);\n-\t\tif (*arg)\n-\t\t\treturn opterror(opt, \"expects a numerical value\", 0);\n-\t\tif (v && v < MINIMUM_ABBREV)\n-\t\t\tv = MINIMUM_ABBREV;\n-\t\telse if (v > 40)\n-\t\t\tv = 40;\n-\t}\n-\t*(int *)(opt->value) = v;\n-\treturn 0;\n-}\n-\n-int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n-\t\t\t     int unset)\n-{\n-\t*(unsigned long *)(opt->value) = approxidate(arg);\n-\treturn 0;\n-}\n-\n-int parse_opt_color_flag_cb(const struct option *opt, const char *arg,\n-\t\t\t    int unset)\n-{\n-\tint value;\n-\n-\tif (!arg)\n-\t\targ = unset ? \"never\" : (const char *)opt->defval;\n-\tvalue = git_config_colorbool(NULL, arg, -1);\n-\tif (value < 0)\n-\t\treturn opterror(opt,\n-\t\t\t\"expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\", 0);\n-\t*(int *)opt->value = value;\n-\treturn 0;\n-}\n-\n-int parse_opt_verbosity_cb(const struct option *opt, const char *arg,\n-\t\t\t   int unset)\n-{\n-\tint *target = opt->value;\n-\n-\tif (unset)\n-\t\t/* --no-quiet, --no-verbose */\n-\t\t*target = 0;\n-\telse if (opt->short_name == 'v') {\n-\t\tif (*target >= 0)\n-\t\t\t(*target)++;\n-\t\telse\n-\t\t\t*target = 1;\n-\t} else {\n-\t\tif (*target <= 0)\n-\t\t\t(*target)--;\n-\t\telse\n-\t\t\t*target = -1;\n-\t}\n-\treturn 0;\n-}\n-\n-int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)\n-{\n-\tunsigned char sha1[20];\n-\tstruct commit *commit;\n-\n-\tif (!arg)\n-\t\treturn -1;\n-\tif (get_sha1(arg, sha1))\n-\t\treturn error(\"malformed object name %s\", arg);\n-\tcommit = lookup_commit_reference(sha1);\n-\tif (!commit)\n-\t\treturn error(\"no such commit %s\", arg);\n-\tcommit_list_insert(commit, opt->value);\n-\treturn 0;\n-}\n-\n-int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)\n-{\n-\tint *target = opt->value;\n-\t*target = unset ? 2 : 1;\n-\treturn 0;\n-}\n-\n-int parse_options_concat(struct option *dst, size_t dst_size, struct option *src)\n-{\n-\tint i, j;\n-\n-\tfor (i = 0; i < dst_size; i++)\n-\t\tif (dst[i].type == OPTION_END)\n-\t\t\tbreak;\n-\tfor (j = 0; i < dst_size; i++, j++) {\n-\t\tdst[i] = src[j];\n-\t\tif (src[j].type == OPTION_END)\n-\t\t\treturn 0;\n-\t}\n-\treturn -1;\n-}\n-\n-int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n-{\n-\tstruct string_list *v = opt->value;\n-\n-\tif (unset) {\n-\t\tstring_list_clear(v, 0);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!arg)\n-\t\treturn -1;\n-\n-\tstring_list_append(v, xstrdup(arg));\n-\treturn 0;\n-}\ndiff --git a/setup.c b/setup.c\nindex 5ea5502..3463819 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -40,34 +40,6 @@ char *prefix_path(const char *prefix, int len, const char *path)\n \treturn sanitized;\n }\n \n-/*\n- * Unlike prefix_path, this should be used if the named file does\n- * not have to interact with index entry; i.e. name of a random file\n- * on the filesystem.\n- */\n-const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)\n-{\n-\tstatic char path[PATH_MAX];\n-#ifndef WIN32\n-\tif (!pfx_len || is_absolute_path(arg))\n-\t\treturn arg;\n-\tmemcpy(path, pfx, pfx_len);\n-\tstrcpy(path + pfx_len, arg);\n-#else\n-\tchar *p;\n-\t/* don't add prefix to absolute paths, but still replace '\\' by '/' */\n-\tif (is_absolute_path(arg))\n-\t\tpfx_len = 0;\n-\telse if (pfx_len)\n-\t\tmemcpy(path, pfx, pfx_len);\n-\tstrcpy(path + pfx_len, arg);\n-\tfor (p = path + pfx_len; *p; p++)\n-\t\tif (*p == '\\\\')\n-\t\t\t*p = '/';\n-#endif\n-\treturn path;\n-}\n-\n int check_filename(const char *prefix, const char *arg)\n {\n \tconst char *name;\n-- \n1.7.3.4\n"},{"id":"173280","messageId":"20110811104236.GB5012@elie.gateway.2wire.net","threadId":"28066","inReplyTo":"1313054138-30885-2-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH v3 1/2] parse-options: export opterr, optbug","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-11T10:42:36Z","receivedAt":"2011-08-11T10:42:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Stephen)\nDmitry Ivankov wrote:\n\n> opterror and optbug functions are used by some of parsing routines\n> in parse-options.c to report errors and bugs respectively.\n>\n> Export these functions to allow more custom parsing routines to use\n> them in a uniform way.\n\nIn other words, exposing opterror() allows custom option types to\nbehave more like the built-in ones when producing messages like\n\"option `<opt>' expects a numerical value\".  What should they pass\nin the \"flags\" argument?  Does this deserve a mention in the\n\"Option Callbacks\" section of\nDocumentation/technical/api-parse-options.txt?\n\nWould opterror() be enough?  I don't see any current users of optbug\noutside of parse_options_check() (which is part of low-level\nmachinery).\n\nAside from that, seems sensible.\n\n[quoting in full for Stephen's convenience.  One quick comment\nbelow.]\n>\n> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>\n> ---\n>  parse-options.c |    4 ++--\n>  parse-options.h |    2 ++\n>  2 files changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 879ea82..7b061af 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -12,14 +12,14 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n>  #define OPT_SHORT 1\n>  #define OPT_UNSET 2\n>  \n> -static int optbug(const struct option *opt, const char *reason)\n> +int optbug(const struct option *opt, const char *reason)\n>  {\n>  \tif (opt->long_name)\n>  \t\treturn error(\"BUG: option '%s' %s\", opt->long_name, reason);\n>  \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n>  }\n>  \n> -static int opterror(const struct option *opt, const char *reason, int flags)\n> +int opterror(const struct option *opt, const char *reason, int flags)\n>  {\n>  \tif (flags & OPT_SHORT)\n>  \t\treturn error(\"switch `%c' %s\", opt->short_name, reason);\n> diff --git a/parse-options.h b/parse-options.h\n> index 05eb09b..59e0b52 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -165,6 +165,8 @@ extern NORETURN void usage_msg_opt(const char *msg,\n>  \t\t\t\t   const char * const *usagestr,\n>  \t\t\t\t   const struct option *options);\n>  \n> +extern int optbug(const struct option *opt, const char *reason);\n> +extern int opterror(const struct option *opt, const char *reason, int flags);\n>  /*----- incremental advanced APIs -----*/\n\nA blank line above the comment would make this more readable.\n\n>  \n>  enum {\n> -- \n\nExcept as noted above,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nSome (though not all) of the possible changes noted above implemented\nbelow, plus an example caller (which made reading the patch a little\neasier).  What do you think?\n\n builtin/read-tree.c |    2 +-\n parse-options.c     |    2 +-\n parse-options.h     |    2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git c/builtin/read-tree.c i/builtin/read-tree.c\nindex df6c4c88..6013090c 100644\n--- c/builtin/read-tree.c\n+++ i/builtin/read-tree.c\n@@ -53,7 +53,7 @@ static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n \topts = (struct unpack_trees_options *)opt->value;\n \n \tif (opts->dir)\n-\t\tdie(\"more than one --exclude-per-directory given.\");\n+\t\treturn opterror(opt, \"can only be supplied once\", 0);\n \n \tdir = xcalloc(1, sizeof(*opts->dir));\n \tdir->flags |= DIR_SHOW_IGNORED;\ndiff --git c/parse-options.c i/parse-options.c\nindex 7b061afc..777611b1 100644\n--- c/parse-options.c\n+++ i/parse-options.c\n@@ -12,7 +12,7 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n-int optbug(const struct option *opt, const char *reason)\n+static int optbug(const struct option *opt, const char *reason)\n {\n \tif (opt->long_name)\n \t\treturn error(\"BUG: option '%s' %s\", opt->long_name, reason);\ndiff --git c/parse-options.h i/parse-options.h\nindex 59e0b524..6d31ad3a 100644\n--- c/parse-options.h\n+++ i/parse-options.h\n@@ -165,8 +165,8 @@ extern NORETURN void usage_msg_opt(const char *msg,\n \t\t\t\t   const char * const *usagestr,\n \t\t\t\t   const struct option *options);\n \n-extern int optbug(const struct option *opt, const char *reason);\n extern int opterror(const struct option *opt, const char *reason, int flags);\n+\n /*----- incremental advanced APIs -----*/\n \n enum {\n"},{"id":"173281","messageId":"20110811110441.GD5012@elie.gateway.2wire.net","threadId":"28066","inReplyTo":"1313054138-30885-3-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH v3 2/2] Reduce parse-options.o dependencies","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-08-11T11:04:41Z","receivedAt":"2011-08-11T11:04:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Stephen)\nDmitry Ivankov wrote:\n\n> Currently parse-options.o pulls quite a big bunch of dependencies.\n> his complicates it's usage in contrib/ because it pulls external\n> dependencies and it also increases executables size.\n>\n> Split off less generic and more internal to git part of\n> parse-options.c to parse-options-cb.c.\n>\n> Move prefix_filename function from setup.c to abspath.c. abspath.o\n> and wrapper.o pull each other, so it's unlikely to increase the\n> dependencies. It was a dependency of parse-options.o that pulled\n> many others.\n>\n> Now parse-options.o pulls just abspath.o, ctype.o, strbuf.o, usage.o,\n> wrapper.o, libc directly and strlcpy.o indirectly.\n\nSo, in other words, currently linking to parse-options involves\nlinking to git's object access machinery and diff machinery, hence\nlibz, libssl for SHA-1, libpcre, etc.  That is a waste of space,\nstartup time, and build complexity for simple programs that do not\nneed access to git's object db.\n\nThis patch does two things to address that:\n\n - option callbacks which freely use the git object db and other\n   facilities move to a separate parse-options-cb.o translation\n   unit, so simple programs can avoid them;\n\n - prefix_filename, which is used to support OPTION_FILENAME,\n   moves from setup.c to abspath.c, so it can be used without\n   pulling in unrelated git machinery.\n\nThe result is, as you say, that use of parse-options.o only pulls\nin abspath, ctype, strbuf, usage, wrapper, and -lc --- which is to\nsay, just utility functions --- and you can build a program using\nparse-options with a simple commandline like\n\n\tcc -o test-program -Wall -W -O2 test-program.c libgit.a\n\nOkay, on to the patch itself (comments sparsely scattered within).\n\n>\n> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>\n> ---\n>  Makefile           |    3 +-\n>  abspath.c          |   28 ++++++++++++\n>  parse-options-cb.c |  125 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  parse-options.c    |  121 --------------------------------------------------\n>  setup.c            |   28 ------------\n>  5 files changed, 155 insertions(+), 150 deletions(-)\n>  create mode 100644 parse-options-cb.c\n> \n> diff --git a/Makefile b/Makefile\n> index 62ad0c2..7d47bdb 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -642,6 +642,7 @@ LIB_OBJS += pack-revindex.o\n>  LIB_OBJS += pack-write.o\n>  LIB_OBJS += pager.o\n>  LIB_OBJS += parse-options.o\n> +LIB_OBJS += parse-options-cb.o\n>  LIB_OBJS += patch-delta.o\n>  LIB_OBJS += patch-ids.o\n>  LIB_OBJS += path.o\n> @@ -2204,7 +2205,7 @@ test-delta$X: diff-delta.o patch-delta.o\n>  \n>  test-line-buffer$X: vcs-svn/lib.a\n>  \n> -test-parse-options$X: parse-options.o\n> +test-parse-options$X: parse-options.o parse-options-cb.o\n>  \n>  test-string-pool$X: vcs-svn/lib.a\n>  \n> diff --git a/abspath.c b/abspath.c\n> index 37287f8..f04ac18 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -139,3 +139,31 @@ const char *absolute_path(const char *path)\n>  \t}\n>  \treturn buf;\n>  }\n> +\n> +/*\n> + * Unlike prefix_path, this should be used if the named file does\n> + * not have to interact with index entry; i.e. name of a random file\n> + * on the filesystem.\n> + */\n> +const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)\n> +{\n> +\tstatic char path[PATH_MAX];\n> +#ifndef WIN32\n> +\tif (!pfx_len || is_absolute_path(arg))\n> +\t\treturn arg;\n> +\tmemcpy(path, pfx, pfx_len);\n> +\tstrcpy(path + pfx_len, arg);\n> +#else\n> +\tchar *p;\n> +\t/* don't add prefix to absolute paths, but still replace '\\' by '/' */\n> +\tif (is_absolute_path(arg))\n> +\t\tpfx_len = 0;\n> +\telse if (pfx_len)\n> +\t\tmemcpy(path, pfx, pfx_len);\n> +\tstrcpy(path + pfx_len, arg);\n> +\tfor (p = path + pfx_len; *p; p++)\n> +\t\tif (*p == '\\\\')\n> +\t\t\t*p = '/';\n> +#endif\n> +\treturn path;\n> +}\n> diff --git a/parse-options-cb.c b/parse-options-cb.c\n> new file mode 100644\n> index 0000000..c248f66\n> --- /dev/null\n> +++ b/parse-options-cb.c\n> @@ -0,0 +1,125 @@\n> +#include \"git-compat-util.h\"\n> +#include \"parse-options.h\"\n> +#include \"cache.h\"\n\nStyle: Files in git tend to use only one of \"git-compat-util.h\",\n\"cache.h\", or \"builtin.h\" and put it at the top.  So in this case, it\nshould probably use just \"cache.h\".\n\n> +#include \"commit.h\"\n> +#include \"color.h\"\n> +#include \"string-list.h\"\n> +\n> +/*----- some often used options -----*/\n> +\n> +int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tint v;\n> +\n> +\tif (!arg) {\n> +\t\tv = unset ? 0 : DEFAULT_ABBREV;\n> +\t} else {\n> +\t\tv = strtol(arg, (char **)&arg, 10);\n> +\t\tif (*arg)\n> +\t\t\treturn opterror(opt, \"expects a numerical value\", 0);\n> +\t\tif (v && v < MINIMUM_ABBREV)\n> +\t\t\tv = MINIMUM_ABBREV;\n> +\t\telse if (v > 40)\n> +\t\t\tv = 40;\n> +\t}\n> +\t*(int *)(opt->value) = v;\n> +\treturn 0;\n> +}\n> +\n> +int parse_opt_approxidate_cb(const struct option *opt, const char *arg,\n> +\t\t\t     int unset)\n> +{\n> +\t*(unsigned long *)(opt->value) = approxidate(arg);\n> +\treturn 0;\n> +}\n> +\n> +int parse_opt_color_flag_cb(const struct option *opt, const char *arg,\n> +\t\t\t    int unset)\n> +{\n> +\tint value;\n> +\n> +\tif (!arg)\n> +\t\targ = unset ? \"never\" : (const char *)opt->defval;\n> +\tvalue = git_config_colorbool(NULL, arg, -1);\n> +\tif (value < 0)\n> +\t\treturn opterror(opt,\n> +\t\t\t\"expects \\\"always\\\", \\\"auto\\\", or \\\"never\\\"\", 0);\n> +\t*(int *)opt->value = value;\n> +\treturn 0;\n> +}\n> +\n> +int parse_opt_verbosity_cb(const struct option *opt, const char *arg,\n> +\t\t\t   int unset)\n> +{\n> +\tint *target = opt->value;\n> +\n> +\tif (unset)\n> +\t\t/* --no-quiet, --no-verbose */\n> +\t\t*target = 0;\n> +\telse if (opt->short_name == 'v') {\n> +\t\tif (*target >= 0)\n> +\t\t\t(*target)++;\n> +\t\telse\n> +\t\t\t*target = 1;\n> +\t} else {\n> +\t\tif (*target <= 0)\n> +\t\t\t(*target)--;\n> +\t\telse\n> +\t\t\t*target = -1;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tunsigned char sha1[20];\n> +\tstruct commit *commit;\n> +\n> +\tif (!arg)\n> +\t\treturn -1;\n> +\tif (get_sha1(arg, sha1))\n> +\t\treturn error(\"malformed object name %s\", arg);\n> +\tcommit = lookup_commit_reference(sha1);\n> +\tif (!commit)\n> +\t\treturn error(\"no such commit %s\", arg);\n> +\tcommit_list_insert(commit, opt->value);\n> +\treturn 0;\n> +}\n> +\n> +int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tint *target = opt->value;\n> +\t*target = unset ? 2 : 1;\n> +\treturn 0;\n> +}\n> +\n> +int parse_options_concat(struct option *dst, size_t dst_size, struct option *src)\n> +{\n> +\tint i, j;\n> +\n> +\tfor (i = 0; i < dst_size; i++)\n> +\t\tif (dst[i].type == OPTION_END)\n> +\t\t\tbreak;\n> +\tfor (j = 0; i < dst_size; i++, j++) {\n> +\t\tdst[i] = src[j];\n> +\t\tif (src[j].type == OPTION_END)\n> +\t\t\treturn 0;\n> +\t}\n> +\treturn -1;\n> +}\n> +\n> +int parse_opt_string_list(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tstruct string_list *v = opt->value;\n> +\n> +\tif (unset) {\n> +\t\tstring_list_clear(v, 0);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (!arg)\n> +\t\treturn -1;\n> +\n> +\tstring_list_append(v, xstrdup(arg));\n> +\treturn 0;\n> +}\n> diff --git a/parse-options.c b/parse-options.c\n> index 7b061af..503ab5d 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -3,7 +3,6 @@\n>  #include \"cache.h\"\n>  #include \"commit.h\"\n>  #include \"color.h\"\n> -#include \"string-list.h\"\n>  \n>  static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n>  \t\t\t       const char * const *usagestr,\n> @@ -584,123 +583,3 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n>  \treturn usage_with_options_internal(ctx, usagestr, opts, 0, err);\n>  }\n>  \n> -\n> -/*----- some often used options -----*/\n> -#include \"cache.h\"\n> -\n> -int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)\n[... snip code moved verbatim ...]\n> -}\n> diff --git a/setup.c b/setup.c\n> index 5ea5502..3463819 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -40,34 +40,6 @@ char *prefix_path(const char *prefix, int len, const char *path)\n>  \treturn sanitized;\n>  }\n>  \n> -/*\n> - * Unlike prefix_path, this should be used if the named file does\n> - * not have to interact with index entry; i.e. name of a random file\n> - * on the filesystem.\n> - */\n> -const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)\n> -{\n> -\tstatic char path[PATH_MAX];\n> -#ifndef WIN32\n> -\tif (!pfx_len || is_absolute_path(arg))\n> -\t\treturn arg;\n> -\tmemcpy(path, pfx, pfx_len);\n> -\tstrcpy(path + pfx_len, arg);\n> -#else\n> -\tchar *p;\n> -\t/* don't add prefix to absolute paths, but still replace '\\' by '/' */\n> -\tif (is_absolute_path(arg))\n> -\t\tpfx_len = 0;\n> -\telse if (pfx_len)\n> -\t\tmemcpy(path, pfx, pfx_len);\n> -\tstrcpy(path + pfx_len, arg);\n> -\tfor (p = path + pfx_len; *p; p++)\n> -\t\tif (*p == '\\\\')\n> -\t\t\t*p = '/';\n> -#endif\n> -\treturn path;\n> -}\n> -\n>  int check_filename(const char *prefix, const char *arg)\n>  {\n>  \tconst char *name;\n> -- \n> 1.7.3.4\n> \n\nExcept for the commit message, and with or without the #include tweak\nmentioned above,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.  This seems to be in pretty good shape (and as mentioned before,\ngets closer to fulfillment of a longstanding wish :)).\n"},{"id":"173327","messageId":"7vr54ro72x.fsf@alter.siamese.dyndns.org","threadId":"28066","inReplyTo":"20110811110441.GD5012@elie.gateway.2wire.net","subject":"Re: [PATCH v3 2/2] Reduce parse-options.o dependencies","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-11T23:35:02Z","receivedAt":"2011-08-11T23:35:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> diff --git a/parse-options-cb.c b/parse-options-cb.c\n>> new file mode 100644\n>> index 0000000..c248f66\n>> --- /dev/null\n>> +++ b/parse-options-cb.c\n>> @@ -0,0 +1,125 @@\n>> +#include \"git-compat-util.h\"\n>> +#include \"parse-options.h\"\n>> +#include \"cache.h\"\n>\n> Style: Files in git tend to use only one of \"git-compat-util.h\",\n> \"cache.h\", or \"builtin.h\" and put it at the top.  So in this case, it\n> should probably use just \"cache.h\".\n\nThis needs a bit of clarification.\n\nThe compatibility rule actually is to have git-compat-util.h at the very\nbeginning. As cache.h is a very widely used header almost everybody that\nneeds to access the internals include, it includes git-compat-util.h as\nits first thing to include.\n\nSo if this is an old file that existed before git-compat-util.h, then it\nis perfectly fine to start it with\n\n\t#include \"cache.h\"\n        #include \"parse-options.h\"\n\nOtherwise the three-line include above is also just fine.\n"}]}