{"thread":{"id":"38832","subject":"[PATCH 02/14] cacheinfo_callback(): use convert_ui() when handling \"--cacheinfo\"","startedAt":"2015-03-17T16:00:02Z","lastAt":"2015-03-25T21:59:11Z","messageCount":43,"participants":["Michael Haggerty","Junio C Hamano","Duy Nguyen","Jeff King","Eric Sunshine","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":14},"messages":[{"id":"257859","messageId":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":null,"subject":"[PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:02Z","receivedAt":"2015-03-17T16:00:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Sorry for the long email. Feel free to skip over the background info\nand continue reading at \"My solution\" below.\n\nThis odyssey started with a typo:\n\n $ git shortlog -snl v2.2.0..v2.3.0\n 14795 Junio C Hamano\n 1658 Jeff King\n 1400 Shawn O. Pearce\n 1109 Linus Torvalds\n 760 Jonathan Nieder\n [...]\n\nIt took me a minute to realize why so many commits are listed. It\nturns out that \"-l\", which I added by accident, requires an integer\nargument, but it happily swallows \"v2.2.0..v2.3.0\" without error. This\nleaves no range argument, so the command traverses the entire history\nof HEAD.\n\nThe relevant code is\n\n else if ((argcount = short_opt('l', av, &optarg))) {\n options->rename_limit = strtoul(optarg, NULL, 10);\n return argcount;\n }\n\n, which is broken in many ways. First of all, strtoul() is way too\npermissive for simple tasks like this:\n\n* It allows leading whitespace.\n\n* It allows arbitrary trailing characters.\n\n* It allows a leading sign character ('+' or '-'), even though the\n result is unsigned.\n\n* If the string doesn't contain a number at all, it sets its \"endptr\"\n argument to point at the start of the string but doesn't set errno.\n\n* If the value is larger than fits in an unsigned long, it returns the\n value clamped to the range 0..ULONG_MAX (setting errno to ERANGE).\n\n* If the value is between -ULONG_MAX and 0, it returns the positive\n integer with the same bit pattern, without setting errno(!) (I can\n hardly think of a scenario where this would be useful.)\n\n* For base=0 (autodetect base), it allows a base specifier prefix \"0x\"\n or \"0\" and parses the number accordingly. For base=16 it also allows\n a \"0x\" prefix.\n\nstrtol() has similar foibles.\n\nThe caller compounds the permissivity of strtoul() with further sins:\n\n* It does absolutely no error detection.\n\n* It assigns the return value, which is an unsigned long, to an int\n value. This could truncate the result, perhaps even resulting in\n rename_limit being set to a negative value.\n\nWhen I looked around, I found scores of sites that call atoi(),\nstrtoul(), and strtol() carelessly. And it's understandable. Calling\nany of these functions safely is so much work as to be completely\nimpractical in day-to-day code.\n\ngit-compat-util.h has two functions, strtoul_ui() and strtol_i(), that\ntry to make parsing integers a little bit easier. But they are only\nused in a few places, they hardly restrict the pathological\nflexibility of strtoul()/strtol(), strtoul_ui() doesn't implement\nclamping consistently when converting from unsigned long to unsigned\nint, and neither function can be used when the integer value *is*\nfollowed by other characters.\n\n\nMy solution: the numparse module\n\nSo I hereby propose a new module, numparse, to make it easier to parse\nintegral values from strings in a convenient, safe, and flexible way.\nThe first commit introduces the new module, and subsequent commits\nchange a sample (a small fraction!) of call sites to use the new\nfunctions. Consider it a proof-of-concept. If people are OK with this\napproach, I will continue sending patches to fix other call sites. (I\nalready have another two dozen such patches in my repo).\n\nPlease see the docstrings in numparse.h in the first commit for\ndetailed API docs. Briefly, there are eight new functions:\n\n parse_{l,ul,i,ui}(const char *s, unsigned int flags,\n T *result, char **endptr);\n convert_{l,ul,i,ui}(const char *s, unsigned int flags, T *result);\n\nThe parse_*() functions are for parsing a number from the start of a\nstring; the convert_*() functions are for parsing a string that\nconsists of a single number. The flags argument selects not only the\nbase of the number, but also which of strtol()/strtoul()'s many\nfeatures should be allowed. The *_i() and *.ui() functions are for\nparsing int and unsigned int values; they are careful about how they\ntruncate the corresponding long values. The functions all return 0 on\nsuccess and a negative number on error.\n\nHere are a few examples of how these functions can be used (taken from\nthe header of numparse.h):\n\n* Convert hexadecimal string s into an unsigned int. Die if there are\n any characters in s besides hexadecimal digits, or if the result\n exceeds the range of an unsigned int:\n\n if (convert_ui(s, 16, &result))\n die(\"...\");\n\n* Read a base-ten long number from the front of a string, allowing\n sign characters and setting endptr to point at any trailing\n characters:\n\n if (parse_l(s, 10 | NUM_SIGN | NUM_TRAILING, &result, &endptr))\n die(\"...\");\n\n* Convert decimal string s into a signed int, but not allowing the\n string to contain a '+' or '-' prefix (and thereby indirectly\n ensuring that the result will be non-negative):\n\n if (convert_i(s, 10, &result))\n die(\"...\");\n\n* Convert s into a signed int, interpreting prefix \"0x\" to mean\n hexadecimal and \"0\" to mean octal. If the value doesn't fit in an\n unsigned int, set result to INT_MIN or INT_MAX.\n\n if (convert_i(s, NUM_SLOPPY, &result))\n die(\"...\");\n\nMy main questions:\n\n* Do people like the API? My main goal was to make these functions as\n painless as possible to use correctly, because there are so many\n call sites.\n\n* Is it too gimmicky to encode the base together with other options in\n `flags`? (I think it is worth it to avoid the need for another\n parameter, which callers could easily put in the wrong order.)\n\n* Am I making callers too strict? In many cases where a positive\n integer is expected (e.g., \"--abbrev=\"), I have been replacing\n code like\n\n result = strtoul(s, NULL, 10);\n\n with\n\n if (convert_i(s, 10, &result))\n die(\"...\");\n\n Strictly speaking, this is backwards incompatible, because the\n former allows things like\n\n --abbrev=\"+10\"\n --abbrev=\" 10\"\n --abbrev=\"10 bananas\"\n --abbrev=-18446744073709551606\n\n (all equivalent to \"--abbrev=10\"), whereas the new code rejects all\n of them. It would be easy to change the new code to allow the first\n three, by adding flags NUM_PLUS, NUM_LEADING_WHITESPACE, or\n NUM_TRAILING respectively. But my inclination is to be strict\n (though I could easily be persuaded to permit the first one).\n\n* Should I submit tiny patches, each rewriting a single call site, or\n make changes in larger chunks (say, file by file)? For example, in\n revision.c there is a function handle_revision_opt() that contains\n about 10 call sites that should be rewritten, for 10 different\n options that take integers. My gut feeling is that the rewrite of\n \"--min-age=\" should be done in a different patch than that of\n \"--min-parents=\", especially since the invocations might use\n different levels of parsing strictness that might be topics of\n discussion on the ML. On the other hand, I feel silly bombarding the\n list with tons of tiny patches.\n\n* I couldn't think of any places where today's sloppy parsing could\n result in security vulnerabilities, but people should think about\n this, too. I would be especially wary of sites that call strtoul()\n and assign the result to an int, seemingly not considering that the\n result could end up negative.\n\nThese patches apply to \"master\". They are also available on my GitHub\nrepo [1].\n\nMichael\n\n[1] https://github.com/mhagger/git.git, branch \"numparse1\"\n\nMichael Haggerty (14):\n numparse: new module for parsing integral numbers\n cacheinfo_callback(): use convert_ui() when handling \"--cacheinfo\"\n write_subdirectory(): use convert_ui() for parsing mode\n handle_revision_opt(): use skip_prefix() in many places\n handle_revision_opt(): use convert_i() when handling \"-\"\n strtoul_ui(), strtol_i(): remove functions\n handle_revision_opt(): use convert_ui() when handling \"--abbrev=\"\n builtin_diff(): detect errors when parsing --unified argument\n opt_arg(): val is always non-NULL\n opt_arg(): use convert_i() in implementation\n opt_arg(): report errors parsing option values\n opt_arg(): simplify pointer handling\n diff_opt_parse(): use convert_i() when handling \"-l\"\n diff_opt_parse(): use convert_i() when handling --abbrev=\n\n Makefile | 1 +\n builtin/update-index.c | 3 +-\n contrib/convert-objects/convert-objects.c | 3 +-\n diff.c | 55 ++++----\n git-compat-util.h | 26 ----\n numparse.c | 180 ++++++++++++++++++++++++++\n numparse.h | 207 ++++++++++++++++++++++++++++++\n revision.c | 64 ++++-----\n 8 files changed, 447 insertions(+), 92 deletions(-)\n create mode 100644 numparse.c\n create mode 100644 numparse.h\n\n-- \n2.1.4\n"},{"id":"257858","messageId":"1426608016-2978-2-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 01/14] numparse: new module for parsing integral numbers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:03Z","receivedAt":"2015-03-17T16:00:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Implement wrappers for strtol() and strtoul() that are safer and more\nconvenient to use.\n\nSigned-off-by: Michael Haggerty \n---\n Makefile | 1 +\n numparse.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n numparse.h | 207 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 388 insertions(+)\n create mode 100644 numparse.c\n create mode 100644 numparse.h\n\ndiff --git a/Makefile b/Makefile\nindex 44f1dd1..6c0cfcc 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -732,6 +732,7 @@ LIB_OBJS += notes.o\n LIB_OBJS += notes-cache.o\n LIB_OBJS += notes-merge.o\n LIB_OBJS += notes-utils.o\n+LIB_OBJS += numparse.o\n LIB_OBJS += object.o\n LIB_OBJS += pack-bitmap.o\n LIB_OBJS += pack-bitmap-write.o\ndiff --git a/numparse.c b/numparse.c\nnew file mode 100644\nindex 0000000..90b44ce\n--- /dev/null\n+++ b/numparse.c\n@@ -0,0 +1,180 @@\n+#include \"git-compat-util.h\"\n+#include \"numparse.h\"\n+\n+#define NUM_NEGATIVE (1 << 16)\n+\n+\n+static int parse_precheck(const char *s, unsigned int *flags)\n+{\n+\tconst char *number;\n+\n+\tif (isspace(*s)) {\n+\t\tif (!(*flags & NUM_LEADING_WHITESPACE))\n+\t\t\treturn -NUM_LEADING_WHITESPACE;\n+\t\tdo {\n+\t\t\ts++;\n+\t\t} while (isspace(*s));\n+\t}\n+\n+\tif (*s == '+') {\n+\t\tif (!(*flags & NUM_PLUS))\n+\t\t\treturn -NUM_PLUS;\n+\t\tnumber = s + 1;\n+\t\t*flags &= ~NUM_NEGATIVE;\n+\t} else if (*s == '-') {\n+\t\tif (!(*flags & NUM_MINUS))\n+\t\t\treturn -NUM_MINUS;\n+\t\tnumber = s + 1;\n+\t\t*flags |= NUM_NEGATIVE;\n+\t} else {\n+\t\tnumber = s;\n+\t\t*flags &= ~NUM_NEGATIVE;\n+\t}\n+\n+\tif (!(*flags & NUM_BASE_SPECIFIER)) {\n+\t\tint base = *flags & NUM_BASE_MASK;\n+\t\tif (base == 0) {\n+\t\t\t/* This is a pointless combination of options. */\n+\t\t\tdie(\"BUG: base=0 specified without NUM_BASE_SPECIFIER\");\n+\t\t} else if (base == 16 && starts_with(number, \"0x\")) {\n+\t\t\t/*\n+\t\t\t * We want to treat this as zero terminated by\n+\t\t\t * an 'x', whereas strtol()/strtoul() would\n+\t\t\t * silently eat the \"0x\". We accomplish this\n+\t\t\t * by treating it as a base 10 number:\n+\t\t\t */\n+\t\t\t*flags = (*flags & ~NUM_BASE_MASK) | 10;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+int parse_l(const char *s, unsigned int flags, long *result, char **endptr)\n+{\n+\tlong l;\n+\tconst char *end;\n+\tint err = 0;\n+\n+\terr = parse_precheck(s, &flags);\n+\tif (err)\n+\t\treturn err;\n+\n+\t/*\n+\t * Now let strtol() do the heavy lifting:\n+\t */\n+\terrno = 0;\n+\tl = strtol(s, (char **)&end, flags & NUM_BASE_MASK);\n+\tif (errno) {\n+\t\tif (errno == ERANGE) {\n+\t\t\tif (!(flags & NUM_SATURATE))\n+\t\t\t\treturn -NUM_SATURATE;\n+\t\t} else {\n+\t\t\treturn -NUM_OTHER_ERROR;\n+\t\t}\n+\t}\n+\tif (end == s)\n+\t\treturn -NUM_NO_DIGITS;\n+\n+\tif (*end && !(flags & NUM_TRAILING))\n+\t\treturn -NUM_TRAILING;\n+\n+\t/* Everything was OK */\n+\t*result = l;\n+\tif (endptr)\n+\t\t*endptr = (char *)end;\n+\treturn 0;\n+}\n+\n+int parse_ul(const char *s, unsigned int flags,\n+\t unsigned long *result, char **endptr)\n+{\n+\tunsigned long ul;\n+\tconst char *end;\n+\tint err = 0;\n+\n+\terr = parse_precheck(s, &flags);\n+\tif (err)\n+\t\treturn err;\n+\n+\t/*\n+\t * Now let strtoul() do the heavy lifting:\n+\t */\n+\terrno = 0;\n+\tul = strtoul(s, (char **)&end, flags & NUM_BASE_MASK);\n+\tif (errno) {\n+\t\tif (errno == ERANGE) {\n+\t\t\tif (!(flags & NUM_SATURATE))\n+\t\t\t\treturn -NUM_SATURATE;\n+\t\t} else {\n+\t\t\treturn -NUM_OTHER_ERROR;\n+\t\t}\n+\t}\n+\tif (end == s)\n+\t\treturn -NUM_NO_DIGITS;\n+\n+\t/*\n+\t * strtoul(), perversely, accepts negative numbers, converting\n+\t * them to the positive number with the same bit pattern. We\n+\t * don't ever want that.\n+\t */\n+\tif ((flags & NUM_NEGATIVE) && ul) {\n+\t\tif (!(flags & NUM_SATURATE))\n+\t\t\treturn -NUM_SATURATE;\n+\t\tul = 0;\n+\t}\n+\n+\tif (*end && !(flags & NUM_TRAILING))\n+\t\treturn -NUM_TRAILING;\n+\n+\t/* Everything was OK */\n+\t*result = ul;\n+\tif (endptr)\n+\t\t*endptr = (char *)end;\n+\treturn 0;\n+}\n+\n+int parse_i(const char *s, unsigned int flags, int *result, char **endptr)\n+{\n+\tlong l;\n+\tint err;\n+\tchar *end;\n+\n+\terr = parse_l(s, flags, &l, &end);\n+\tif (err)\n+\t\treturn err;\n+\n+\tif ((int)l == l)\n+\t\t*result = l;\n+\telse if (!(flags & NUM_SATURATE))\n+\t\treturn -NUM_SATURATE;\n+\telse\n+\t\t*result = (l <= 0) ? INT_MIN : INT_MAX;\n+\n+\tif (endptr)\n+\t\t*endptr = end;\n+\n+\treturn 0;\n+}\n+\n+int parse_ui(const char *s, unsigned int flags, unsigned int *result, char **endptr)\n+{\n+\tunsigned long ul;\n+\tint err;\n+\tchar *end;\n+\n+\terr = parse_ul(s, flags, &ul, &end);\n+\tif (err)\n+\t\treturn err;\n+\n+\tif ((unsigned int)ul == ul)\n+\t\t*result = ul;\n+\telse if (!(flags & NUM_SATURATE))\n+\t\treturn -NUM_SATURATE;\n+\telse\n+\t\t*result = UINT_MAX;\n+\n+\tif (endptr)\n+\t\t*endptr = end;\n+\n+\treturn 0;\n+}\ndiff --git a/numparse.h b/numparse.h\nnew file mode 100644\nindex 0000000..4de5e10\n--- /dev/null\n+++ b/numparse.h\n@@ -0,0 +1,207 @@\n+#ifndef NUMPARSE_H\n+#define NUMPARSE_H\n+\n+/*\n+ * Functions for parsing integral numbers.\n+ *\n+ * strtol() and strtoul() are very flexible, in fact too flexible for\n+ * many purposes. These functions wrap them to make them easier to use\n+ * in a stricter way.\n+ *\n+ * There are two classes of function, parse_*() and convert_*(). The\n+ * former try to read a number from the front of a string and report a\n+ * pointer to the character following the number. The latter don't\n+ * report the end of the number, and are meant to be used when the\n+ * input string should contain only a single number, with no trailing\n+ * characters.\n+ *\n+ * Each class of functions has four variants:\n+ *\n+ * - parse_l(), convert_l() -- parse long ints\n+ * - parse_ul(), convert_ul() -- parse unsigned long ints\n+ * - parse_i(), convert_i() -- parse ints\n+ * - parse_ui(), convert_ui() -- parse unsigned ints\n+ *\n+ * The style of parsing is controlled by a flags argument which\n+ * encodes both the base of the number and many other options. The\n+ * base is encoded by its numerical value (2 <= base <= 36), or zero\n+ * if it should be determined automatically based on whether the\n+ * number has a \"0x\" or \"0\" prefix.\n+ *\n+ * The functions all return zero on success. On error, they return a\n+ * negative integer indicating the first error that was detected. For\n+ * example, if no sign characters were allowed but the string\n+ * contained a '-', the function will return -NUM_MINUS. If there is\n+ * any kind of error, *result and *endptr are unchanged.\n+ *\n+ * Examples:\n+ *\n+ * - Convert hexadecimal string s into an unsigned int. Die if there\n+ * are any characters in s besides hexadecimal digits, or if the\n+ * result exceeds the range of an unsigned int:\n+ *\n+ * if (convert_ui(s, 16, &result))\n+ * die(\"...\");\n+ *\n+ * - Read a base-ten long number from the front of a string, allowing\n+ * sign characters and setting endptr to point at any trailing\n+ * characters:\n+ *\n+ * if (parse_l(s, 10 | NUM_SIGN | NUM_TRAILING, &result, &endptr))\n+ * die(\"...\");\n+ *\n+ * - Convert decimal string s into a signed int, but not allowing the\n+ * string to contain a '+' or '-' prefix (and thereby indirectly\n+ * ensuring that the result will be non-negative):\n+ *\n+ * if (convert_i(s, 10, &result))\n+ * die(\"...\");\n+ *\n+ * - Convert s into a signed int, interpreting prefix \"0x\" to mean\n+ * hexadecimal and \"0\" to mean octal. If the value doesn't fit in an\n+ * unsigned int, set result to INT_MIN or INT_MAX.\n+ *\n+ * if (convert_i(s, NUM_SLOPPY, &result))\n+ * die(\"...\");\n+ */\n+\n+\n+/*\n+ * Constants for parsing numbers.\n+ *\n+ * These can be passed in flags to allow the specified features. Also,\n+ * if there is an error parsing a number, the parsing functions return\n+ * the negated value of one of these constants (or NUM_NO_DIGITS or\n+ * NUM_OTHER_ERROR) to indicate the first error detected.\n+ */\n+\n+/*\n+ * The lowest 6 bits of flags hold the numerical base that should be\n+ * used to parse the number, 2 <= base <= 36. If base is set to 0,\n+ * then NUM_BASE_SPECIFIER must be set too; in this case, the base is\n+ * detected automatically from the string's prefix.\n+ */\n+#define NUM_BASE_MASK 0x3f\n+\n+/* Skip any whitespace before the number. */\n+#define NUM_LEADING_WHITESPACE (1 << 8)\n+\n+/* Allow a leading '+'. */\n+#define NUM_PLUS (1 << 9)\n+\n+/* Allow a leading '-'. */\n+#define NUM_MINUS (1 << 10)\n+\n+/*\n+ * Allow a leading base specifier:\n+ * - If base is 0: a leading \"0x\" indicates base 16; a leading \"0\"\n+ * indicates base 8; otherwise, assume base 10.\n+ * - If base is 16: a leading \"0x\" is allowed and skipped over.\n+ */\n+#define NUM_BASE_SPECIFIER (1 << 11)\n+\n+/*\n+ * If the number is not in the allowed range, return the smallest or\n+ * largest representable value instead.\n+ */\n+#define NUM_SATURATE (1 << 12)\n+\n+/*\n+ * Just parse until the end of the number, ignoring any subsequent\n+ * characters. If this option is not specified, then it is an error if\n+ * the whole string cannot be parsed.\n+ */\n+#define NUM_TRAILING (1 << 13)\n+\n+\n+/* Additional errors that can come from parsing numbers: */\n+\n+/* There were no valid digits */\n+#define NUM_NO_DIGITS (1 << 14)\n+/* There was some other error reported by strtol()/strtoul(): */\n+#define NUM_OTHER_ERROR (1 << 15)\n+\n+/*\n+ * Please note that there is also a NUM_NEGATIVE, which is used\n+ * internally.\n+ */\n+\n+/*\n+ * Now define some useful combinations of parsing options:\n+ */\n+\n+/* A bunch of digits with an optional sign. */\n+#define NUM_SIGN (NUM_PLUS | NUM_MINUS)\n+\n+/*\n+ * Be as liberal as possible with the form of the number itself\n+ * (though if you also want to allow leading whitespace and/or\n+ * trailing characters, you should combine this with\n+ * NUM_LEADING_WHITESPACE and/or NUM_TRAILING).\n+ */\n+#define NUM_SLOPPY (NUM_SIGN | NUM_SATURATE | NUM_BASE_SPECIFIER)\n+\n+\n+/*\n+ * Number parsing functions:\n+ *\n+ * The following functions parse a number (long, unsigned long, int,\n+ * or unsigned int respectively) from the front of s, storing the\n+ * value to *result and storing a pointer to the first character after\n+ * the number to *endptr. flags specifies how the number should be\n+ * parsed, including which base should be used. flags is a combination\n+ * of the numerical base (2-36) and the NUM_* constants above (see).\n+ * Return 0 on success or a negative value if there was an error. On\n+ * failure, *result and *entptr are left unchanged.\n+ *\n+ * Please note that if NUM_TRAILING is not set, then it is\n+ * nevertheless an error if there are any characters between the end\n+ * of the number and the end of the string.\n+ */\n+\n+int parse_l(const char *s, unsigned int flags,\n+\t long *result, char **endptr);\n+\n+int parse_ul(const char *s, unsigned int flags,\n+\t unsigned long *result, char **endptr);\n+\n+int parse_i(const char *s, unsigned int flags,\n+\t int *result, char **endptr);\n+\n+int parse_ui(const char *s, unsigned int flags,\n+\t unsigned int *result, char **endptr);\n+\n+\n+/*\n+ * Number conversion functions:\n+ *\n+ * The following functions parse a string into a number. They are\n+ * identical to the parse_*() functions above, except that the endptr\n+ * is not returned. These are most useful when parsing a whole string\n+ * into a number; i.e., when (flags & NUM_TRAILING) is unset.\n+ */\n+static inline int convert_l(const char *s, unsigned int flags,\n+\t\t\t long *result)\n+{\n+\treturn parse_l(s, flags, result, NULL);\n+}\n+\n+static inline int convert_ul(const char *s, unsigned int flags,\n+\t\t\t unsigned long *result)\n+{\n+\treturn parse_ul(s, flags, result, NULL);\n+}\n+\n+static inline int convert_i(const char *s, unsigned int flags,\n+\t\t\t int *result)\n+{\n+\treturn parse_i(s, flags, result, NULL);\n+}\n+\n+static inline int convert_ui(const char *s, unsigned int flags,\n+\t\t\t unsigned int *result)\n+{\n+\treturn parse_ui(s, flags, result, NULL);\n+}\n+\n+#endif /* NUMPARSE_H */\n-- \n2.1.4\n"},{"id":"257855","messageId":"1426608016-2978-3-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 02/14] cacheinfo_callback(): use convert_ui() when handling \"--cacheinfo\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:04Z","receivedAt":"2015-03-17T16:00:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use convert_ui() instead of strtoul_ui() to parse the argument.\nThis tightens up the parsing a bit:\n\n* Leading whitespace is no longer allowed\n* '+' and '-' are no longer allowed\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-index.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 5878986..845e944 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -4,6 +4,7 @@\n * Copyright (C) Linus Torvalds, 2005\n */\n #include \"cache.h\"\n+#include \"numparse.h\"\n #include \"lockfile.h\"\n #include \"quote.h\"\n #include \"cache-tree.h\"\n@@ -672,7 +673,7 @@ static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,\n \t}\n \tif (ctx->argc <= 3)\n \t\treturn error(\"option 'cacheinfo' expects ,,\");\n-\tif (strtoul_ui(*++ctx->argv, 8, &mode) ||\n+\tif (convert_ui(*++ctx->argv, 8, &mode) ||\n \t get_sha1_hex(*++ctx->argv, sha1) ||\n \t add_cacheinfo(mode, sha1, *++ctx->argv, 0))\n \t\tdie(\"git update-index: --cacheinfo cannot add %s\", *ctx->argv);\n-- \n2.1.4\n"},{"id":"257861","messageId":"1426608016-2978-4-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 03/14] write_subdirectory(): use convert_ui() for parsing mode","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:05Z","receivedAt":"2015-03-17T16:00:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use convert_ui() instead of strtoul_ui() when parsing tree entries'\nmodes. This tightens up the parsing a bit:\n\n* Leading whitespace is no longer allowed\n* '+' and '-' are no longer allowed\n\nSigned-off-by: Michael Haggerty \n---\n contrib/convert-objects/convert-objects.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/convert-objects/convert-objects.c b/contrib/convert-objects/convert-objects.c\nindex f3b57bf..4f484a4 100644\n--- a/contrib/convert-objects/convert-objects.c\n+++ b/contrib/convert-objects/convert-objects.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"numparse.h\"\n #include \"blob.h\"\n #include \"commit.h\"\n #include \"tree.h\"\n@@ -88,7 +89,7 @@ static int write_subdirectory(void *buffer, unsigned long size, const char *base\n \t\tunsigned int mode;\n \t\tchar *slash, *origpath;\n \n-\t\tif (!path || strtoul_ui(buffer, 8, &mode))\n+\t\tif (!path || convert_ui(buffer, 8, &mode))\n \t\t\tdie(\"bad tree conversion\");\n \t\tmode = convert_mode(mode);\n \t\tpath++;\n-- \n2.1.4\n"},{"id":"257856","messageId":"1426608016-2978-5-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 04/14] handle_revision_opt(): use skip_prefix() in many places","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:06Z","receivedAt":"2015-03-17T16:00:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This reduces the need for \"magic numbers\".\n\nSigned-off-by: Michael Haggerty \n---\n revision.c | 59 ++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 30 insertions(+), 29 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 66520c6..25838fc 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1719,8 +1719,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->max_count = atoi(argv[1]);\n \t\trevs->no_walk = 0;\n \t\treturn 2;\n-\t} else if (starts_with(arg, \"-n\")) {\n-\t\trevs->max_count = atoi(arg + 2);\n+\t} else if (skip_prefix(arg, \"-n\", &arg)) {\n+\t\trevs->max_count = atoi(arg);\n \t\trevs->no_walk = 0;\n \t} else if ((argcount = parse_long_opt(\"max-age\", argv, &optarg))) {\n \t\trevs->max_age = atoi(optarg);\n@@ -1779,15 +1779,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--author-date-order\")) {\n \t\trevs->sort_order = REV_SORT_BY_AUTHOR_DATE;\n \t\trevs->topo_order = 1;\n-\t} else if (starts_with(arg, \"--early-output\")) {\n+\t} else if (skip_prefix(arg, \"--early-output\", &arg)) {\n \t\tint count = 100;\n-\t\tswitch (arg[14]) {\n+\t\tswitch (*arg++) {\n \t\tcase '=':\n-\t\t\tcount = atoi(arg+15);\n+\t\t\tcount = atoi(arg);\n \t\t\t/* Fallthrough */\n \t\tcase 0:\n \t\t\trevs->topo_order = 1;\n-\t\t revs->early_output = count;\n+\t\t\trevs->early_output = count;\n \t\t}\n \t} else if (!strcmp(arg, \"--parents\")) {\n \t\trevs->rewrite_parents = 1;\n@@ -1804,12 +1804,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->min_parents = 2;\n \t} else if (!strcmp(arg, \"--no-merges\")) {\n \t\trevs->max_parents = 1;\n-\t} else if (starts_with(arg, \"--min-parents=\")) {\n-\t\trevs->min_parents = atoi(arg+14);\n+\t} else if (skip_prefix(arg, \"--min-parents=\", &arg)) {\n+\t\trevs->min_parents = atoi(arg);\n \t} else if (starts_with(arg, \"--no-min-parents\")) {\n \t\trevs->min_parents = 0;\n-\t} else if (starts_with(arg, \"--max-parents=\")) {\n-\t\trevs->max_parents = atoi(arg+14);\n+\t} else if (skip_prefix(arg, \"--max-parents=\", &arg)) {\n+\t\trevs->max_parents = atoi(arg);\n \t} else if (starts_with(arg, \"--no-max-parents\")) {\n \t\trevs->max_parents = -1;\n \t} else if (!strcmp(arg, \"--boundary\")) {\n@@ -1891,26 +1891,27 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->verbose_header = 1;\n \t\trevs->pretty_given = 1;\n \t\tget_commit_format(NULL, revs);\n-\t} else if (starts_with(arg, \"--pretty=\") || starts_with(arg, \"--format=\")) {\n+\t} else if (skip_prefix(arg, \"--pretty=\", &arg) ||\n+\t\t skip_prefix(arg, \"--format=\", &arg)) {\n \t\t/*\n \t\t * Detached form (\"--pretty X\" as opposed to \"--pretty=X\")\n \t\t * not allowed, since the argument is optional.\n \t\t */\n \t\trevs->verbose_header = 1;\n \t\trevs->pretty_given = 1;\n-\t\tget_commit_format(arg+9, revs);\n+\t\tget_commit_format(arg, revs);\n \t} else if (!strcmp(arg, \"--show-notes\") || !strcmp(arg, \"--notes\")) {\n \t\trevs->show_notes = 1;\n \t\trevs->show_notes_given = 1;\n \t\trevs->notes_opt.use_default_notes = 1;\n \t} else if (!strcmp(arg, \"--show-signature\")) {\n \t\trevs->show_signature = 1;\n-\t} else if (!strcmp(arg, \"--show-linear-break\") ||\n-\t\t starts_with(arg, \"--show-linear-break=\")) {\n-\t\tif (starts_with(arg, \"--show-linear-break=\"))\n-\t\t\trevs->break_bar = xstrdup(arg + 20);\n-\t\telse\n-\t\t\trevs->break_bar = \" ..........\";\n+\t} else if (!strcmp(arg, \"--show-linear-break\")) {\n+\t\trevs->break_bar = \" ..........\";\n+\t\trevs->track_linear = 1;\n+\t\trevs->track_first_time = 1;\n+\t} else if (skip_prefix(arg, \"--show-linear-break=\", &arg)) {\n+\t\trevs->break_bar = xstrdup(arg);\n \t\trevs->track_linear = 1;\n \t\trevs->track_first_time = 1;\n \t} else if (starts_with(arg, \"--show-notes=\") ||\n@@ -1961,8 +1962,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->abbrev = 0;\n \t} else if (!strcmp(arg, \"--abbrev\")) {\n \t\trevs->abbrev = DEFAULT_ABBREV;\n-\t} else if (starts_with(arg, \"--abbrev=\")) {\n-\t\trevs->abbrev = strtoul(arg + 9, NULL, 10);\n+\t} else if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n+\t\trevs->abbrev = strtoul(arg, NULL, 10);\n \t\tif (revs->abbrev < MINIMUM_ABBREV)\n \t\t\trevs->abbrev = MINIMUM_ABBREV;\n \t\telse if (revs->abbrev > 40)\n@@ -2112,20 +2113,20 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t} else if ((argcount = parse_long_opt(\"exclude\", argv, &optarg))) {\n \t\tadd_ref_exclusion(&revs->ref_excludes, optarg);\n \t\treturn argcount;\n-\t} else if (starts_with(arg, \"--branches=\")) {\n+\t} else if (skip_prefix(arg, \"--branches=\", &arg)) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 11, \"refs/heads/\", &cb);\n+\t\tfor_each_glob_ref_in(handle_one_ref, arg, \"refs/heads/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (starts_with(arg, \"--tags=\")) {\n+\t} else if (skip_prefix(arg, \"--tags=\", &arg)) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 7, \"refs/tags/\", &cb);\n+\t\tfor_each_glob_ref_in(handle_one_ref, arg, \"refs/tags/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n-\t} else if (starts_with(arg, \"--remotes=\")) {\n+\t} else if (skip_prefix(arg, \"--remotes=\", &arg)) {\n \t\tstruct all_refs_cb cb;\n \t\tinit_all_refs_cb(&cb, revs, *flags);\n-\t\tfor_each_glob_ref_in(handle_one_ref, arg + 10, \"refs/remotes/\", &cb);\n+\t\tfor_each_glob_ref_in(handle_one_ref, arg, \"refs/remotes/\", &cb);\n \t\tclear_ref_exclusion(&revs->ref_excludes);\n \t} else if (!strcmp(arg, \"--reflog\")) {\n \t\tadd_reflogs_to_pending(revs, *flags);\n@@ -2135,14 +2136,14 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\t*flags ^= UNINTERESTING | BOTTOM;\n \t} else if (!strcmp(arg, \"--no-walk\")) {\n \t\trevs->no_walk = REVISION_WALK_NO_WALK_SORTED;\n-\t} else if (starts_with(arg, \"--no-walk=\")) {\n+\t} else if (skip_prefix(arg, \"--no-walk=\", &arg)) {\n \t\t/*\n \t\t * Detached form (\"--no-walk X\" as opposed to \"--no-walk=X\")\n \t\t * not allowed, since the argument is optional.\n \t\t */\n-\t\tif (!strcmp(arg + 10, \"sorted\"))\n+\t\tif (!strcmp(arg, \"sorted\"))\n \t\t\trevs->no_walk = REVISION_WALK_NO_WALK_SORTED;\n-\t\telse if (!strcmp(arg + 10, \"unsorted\"))\n+\t\telse if (!strcmp(arg, \"unsorted\"))\n \t\t\trevs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;\n \t\telse\n \t\t\treturn error(\"invalid argument to --no-walk\");\n-- \n2.1.4\n"},{"id":"257857","messageId":"1426608016-2978-6-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 05/14] handle_revision_opt(): use convert_i() when handling \"-\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:07Z","receivedAt":"2015-03-17T16:00:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This tightens up the parsing a bit:\n\n* Leading whitespace is no longer allowed\n* '+' and '-' are no longer allowed\n\nIt also removes the need to check separately that max_count is\nnon-negative.\n\nSigned-off-by: Michael Haggerty \n---\n revision.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 25838fc..4908e66 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"numparse.h\"\n #include \"tag.h\"\n #include \"blob.h\"\n #include \"tree.h\"\n@@ -1709,8 +1710,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\treturn argcount;\n \t} else if ((*arg == '-') && isdigit(arg[1])) {\n \t\t/* accept -, like traditional \"head\" */\n-\t\tif (strtol_i(arg + 1, 10, &revs->max_count) < 0 ||\n-\t\t revs->max_count < 0)\n+\t\tif (convert_i(arg + 1, 10, &revs->max_count))\n \t\t\tdie(\"'%s': not a non-negative integer\", arg + 1);\n \t\trevs->no_walk = 0;\n \t} else if (!strcmp(arg, \"-n\")) {\n-- \n2.1.4\n"},{"id":"257860","messageId":"1426608016-2978-7-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 06/14] strtoul_ui(), strtol_i(): remove functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:08Z","receivedAt":"2015-03-17T16:00:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Their callers have been changed to use the numparse module.\n\nSigned-off-by: Michael Haggerty \n---\n git-compat-util.h | 26 --------------------------\n 1 file changed, 26 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a3095be..cbe7f16 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -751,32 +751,6 @@ static inline int sane_iscase(int x, int is_lower)\n \t\treturn (x & 0x20) == 0;\n }\n \n-static inline int strtoul_ui(char const *s, int base, unsigned int *result)\n-{\n-\tunsigned long ul;\n-\tchar *p;\n-\n-\terrno = 0;\n-\tul = strtoul(s, &p, base);\n-\tif (errno || *p || p == s || (unsigned int) ul != ul)\n-\t\treturn -1;\n-\t*result = ul;\n-\treturn 0;\n-}\n-\n-static inline int strtol_i(char const *s, int base, int *result)\n-{\n-\tlong ul;\n-\tchar *p;\n-\n-\terrno = 0;\n-\tul = strtol(s, &p, base);\n-\tif (errno || *p || p == s || (int) ul != ul)\n-\t\treturn -1;\n-\t*result = ul;\n-\treturn 0;\n-}\n-\n #ifdef INTERNAL_QSORT\n void git_qsort(void *base, size_t nmemb, size_t size,\n \t int(*compar)(const void *, const void *));\n-- \n2.1.4\n"},{"id":"257862","messageId":"1426608016-2978-8-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 07/14] handle_revision_opt(): use convert_ui() when handling \"--abbrev=\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:09Z","receivedAt":"2015-03-17T16:00:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This adds error checking, where previously there was none. It also\ndisallows '+' and '-', leading whitespace, and trailing junk.\n\nSigned-off-by: Michael Haggerty \n---\n revision.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 4908e66..a6f7c2e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1963,7 +1963,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--abbrev\")) {\n \t\trevs->abbrev = DEFAULT_ABBREV;\n \t} else if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n-\t\trevs->abbrev = strtoul(arg, NULL, 10);\n+\t\tif (convert_ui(arg, 10, &revs->abbrev))\n+\t\t\tdie(\"--abbrev requires a non-negative integer argument\");\n \t\tif (revs->abbrev < MINIMUM_ABBREV)\n \t\t\trevs->abbrev = MINIMUM_ABBREV;\n \t\telse if (revs->abbrev > 40)\n-- \n2.1.4\n"},{"id":"257865","messageId":"1426608016-2978-9-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 08/14] builtin_diff(): detect errors when parsing --unified argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:10Z","receivedAt":"2015-03-17T16:00:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The previous code used strtoul() without any checks that it succeeded.\nInstead use convert_l(), in strict mode, and die() if there is an\nerror. This tightens up the parsing:\n\n* Leading whitespace is no longer allowed\n* '+' and '-' are no longer allowed\n* Trailing junk is not allowed\n\nSigned-off-by: Michael Haggerty \n---\n diff.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex abc32c8..a350677 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2,6 +2,7 @@\n * Copyright (C) 2005 Junio C Hamano\n */\n #include \"cache.h\"\n+#include \"numparse.h\"\n #include \"quote.h\"\n #include \"diff.h\"\n #include \"diffcore.h\"\n@@ -2393,12 +2394,12 @@ static void builtin_diff(const char *name_a,\n \t\t\txecfg.flags |= XDL_EMIT_FUNCCONTEXT;\n \t\tif (pe)\n \t\t\txdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);\n-\t\tif (!diffopts)\n-\t\t\t;\n-\t\telse if (skip_prefix(diffopts, \"--unified=\", &v))\n-\t\t\txecfg.ctxlen = strtoul(v, NULL, 10);\n-\t\telse if (skip_prefix(diffopts, \"-u\", &v))\n-\t\t\txecfg.ctxlen = strtoul(v, NULL, 10);\n+\t\tif (diffopts\n+\t\t && (skip_prefix(diffopts, \"--unified=\", &v) ||\n+\t\t\tskip_prefix(diffopts, \"-u\", &v))) {\n+\t\t\tif (convert_l(v, 10, &xecfg.ctxlen))\n+\t\t\t\tdie(\"--unified argument must be a non-negative integer\");\n+\t\t}\n \t\tif (o->word_diff)\n \t\t\tinit_diff_words_data(&ecbdata, o, one, two);\n \t\txdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,\n-- \n2.1.4\n"},{"id":"257869","messageId":"1426608016-2978-10-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 09/14] opt_arg(): val is always non-NULL","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:11Z","receivedAt":"2015-03-17T16:00:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"opt_arg() is never called with val set to NULL, so remove the code for\nhandling that eventuality (which anyway wasn't handled consistently in\nthe function).\n\nSigned-off-by: Michael Haggerty \n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex a350677..6e3f498 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3367,7 +3367,7 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va\n \t\tc = *++arg;\n \t\tif (!c)\n \t\t\treturn 1;\n-\t\tif (val && isdigit(c)) {\n+\t\tif (isdigit(c)) {\n \t\t\tchar *end;\n \t\t\tint n = strtoul(arg, &end, 10);\n \t\t\tif (*end)\n-- \n2.1.4\n"},{"id":"257867","messageId":"1426608016-2978-11-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 10/14] opt_arg(): use convert_i() in implementation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:12Z","receivedAt":"2015-03-17T16:00:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This shortens the code and avoids the old code's careless truncation\nfrom unsigned long to int.\n\nSigned-off-by: Michael Haggerty \n---\n diff.c | 28 ++++++++--------------------\n 1 file changed, 8 insertions(+), 20 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6e3f498..77c7acb 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3366,16 +3366,10 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va\n \tif (c == arg_short) {\n \t\tc = *++arg;\n \t\tif (!c)\n-\t\t\treturn 1;\n-\t\tif (isdigit(c)) {\n-\t\t\tchar *end;\n-\t\t\tint n = strtoul(arg, &end, 10);\n-\t\t\tif (*end)\n-\t\t\t\treturn 0;\n-\t\t\t*val = n;\n-\t\t\treturn 1;\n-\t\t}\n-\t\treturn 0;\n+\t\t\treturn 1; /* optional argument was missing */\n+\t\tif (convert_i(arg, 10, val))\n+\t\t\treturn 0;\n+\t\treturn 1;\n \t}\n \tif (c != '-')\n \t\treturn 0;\n@@ -3384,16 +3378,10 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va\n \tlen = eq - arg;\n \tif (!len || strncmp(arg, arg_long, len))\n \t\treturn 0;\n-\tif (*eq) {\n-\t\tint n;\n-\t\tchar *end;\n-\t\tif (!isdigit(*++eq))\n-\t\t\treturn 0;\n-\t\tn = strtoul(eq, &end, 10);\n-\t\tif (*end)\n-\t\t\treturn 0;\n-\t\t*val = n;\n-\t}\n+\tif (!*eq)\n+\t\treturn 1; /* '=' and optional argument were missing */\n+\tif (convert_i(eq + 1, 10, val))\n+\t\treturn 0;\n \treturn 1;\n }\n \n-- \n2.1.4\n"},{"id":"257868","messageId":"1426608016-2978-12-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 11/14] opt_arg(): report errors parsing option values","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:13Z","receivedAt":"2015-03-17T16:00:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If an argument is there, but it can't be parsed as a non-positive\nnumber, then die() rather than returning 0.\n\nSigned-off-by: Michael Haggerty \n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 77c7acb..03cdabf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3368,7 +3368,7 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va\n \t\tif (!c)\n \t\t\treturn 1; /* optional argument was missing */\n \t\tif (convert_i(arg, 10, val))\n-\t\t\treturn 0;\n+\t\t\tdie(\"The value for -%c must be a non-negative integer\", arg_short);\n \t\treturn 1;\n \t}\n \tif (c != '-')\n@@ -3381,7 +3381,7 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va\n \tif (!*eq)\n \t\treturn 1; /* '=' and optional argument were missing */\n \tif (convert_i(eq + 1, 10, val))\n-\t\treturn 0;\n+\t\tdie(\"The value for --%s must be a non-negative integer\", arg_long);\n \treturn 1;\n }\n \n-- \n2.1.4\n"},{"id":"257863","messageId":"1426608016-2978-13-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 12/14] opt_arg(): simplify pointer handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:14Z","receivedAt":"2015-03-17T16:00:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Increment \"arg\" when a character is consumed, not just before consuming\nthe next character.\n\nSigned-off-by: Michael Haggerty \n---\n diff.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 03cdabf..bc1e3c3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3358,14 +3358,13 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va\n \tchar c, *eq;\n \tint len;\n \n-\tif (*arg != '-')\n+\tif (*arg++ != '-')\n \t\treturn 0;\n-\tc = *++arg;\n+\tc = *arg++;\n \tif (!c)\n \t\treturn 0;\n \tif (c == arg_short) {\n-\t\tc = *++arg;\n-\t\tif (!c)\n+\t\tif (!*arg)\n \t\t\treturn 1; /* optional argument was missing */\n \t\tif (convert_i(arg, 10, val))\n \t\t\tdie(\"The value for -%c must be a non-negative integer\", arg_short);\n@@ -3373,7 +3372,6 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va\n \t}\n \tif (c != '-')\n \t\treturn 0;\n-\targ++;\n \teq = strchrnul(arg, '=');\n \tlen = eq - arg;\n \tif (!len || strncmp(arg, arg_long, len))\n-- \n2.1.4\n"},{"id":"257864","messageId":"1426608016-2978-14-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 13/14] diff_opt_parse(): use convert_i() when handling \"-l\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:15Z","receivedAt":"2015-03-17T16:00:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"die() with an error message if the argument is not a non-negative\ninteger. This change tightens up parsing: '+' and '-', leading\nwhitespace, and trailing junk are all disallowed now.\n\nSigned-off-by: Michael Haggerty \n---\n diff.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex bc1e3c3..be389ae 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3799,7 +3799,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \telse if (!strcmp(arg, \"-z\"))\n \t\toptions->line_termination = 0;\n \telse if ((argcount = short_opt('l', av, &optarg))) {\n-\t\toptions->rename_limit = strtoul(optarg, NULL, 10);\n+\t\tif (convert_i(optarg, 10, &options->rename_limit))\n+\t\t\tdie(\"-l requires a non-negative integer argument\");\n \t\treturn argcount;\n \t}\n \telse if ((argcount = short_opt('S', av, &optarg))) {\n-- \n2.1.4\n"},{"id":"257866","messageId":"1426608016-2978-15-git-send-email-mhagger@alum.mit.edu","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 14/14] diff_opt_parse(): use convert_i() when handling --abbrev=","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T16:00:16Z","receivedAt":"2015-03-17T16:00:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"die() with an error message if the argument is not a non-negative\ninteger. This change tightens up parsing: '+' and '-', leading\nwhitespace, and trailing junk are all disallowed now.\n\nSigned-off-by: Michael Haggerty \n---\n diff.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex be389ae..1cc5428 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3830,7 +3830,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \telse if (!strcmp(arg, \"--abbrev\"))\n \t\toptions->abbrev = DEFAULT_ABBREV;\n \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n-\t\toptions->abbrev = strtoul(arg, NULL, 10);\n+\t\tif (convert_i(arg, 10, &options->abbrev))\n+\t\t\tdie(\"--abbrev requires an integer argument\");\n \t\tif (options->abbrev < MINIMUM_ABBREV)\n \t\t\toptions->abbrev = MINIMUM_ABBREV;\n \t\telse if (40 < options->abbrev)\n-- \n2.1.4\n"},{"id":"257877","messageId":"xmqqk2yfo3y0.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T18:48:39Z","receivedAt":"2015-03-17T18:48:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> When I looked around, I found scores of sites that call atoi(),\n> strtoul(), and strtol() carelessly. And it's understandable. Calling\n> any of these functions safely is so much work as to be completely\n> impractical in day-to-day code.\n>\n> git-compat-util.h has two functions, strtoul_ui() and strtol_i(), that\n> try to make parsing integers a little bit easier.\n\nExactly. They were introduced to prevent sloppy callers from\npassing NULL to the &end parameter to underlying strtoul(3). The\nfirst thing that came to my mind while reading your message up to\nthis point was \"why not use them more, tighten them, adding\ndifferent variants if necessary, instead of introducing an entirely\nnew set of functions in a new namespace?\"\n\nFor example:\n\n> * Am I making callers too strict? In many cases where a positive\n> integer is expected (e.g., \"--abbrev=\"), I have been replacing\n> code like\n>\n> result = strtoul(s, NULL, 10);\n>\n> with\n>\n> if (convert_i(s, 10, &result))\n> die(\"...\");\n\nI think strictness would be good and those who did \"--abbrev=' 20'\"\ndeserve what they get from the additional strictness, but \n\n\tif (strtol_i(s, 10, &result))\n\nwould have been more familiar.\n"},{"id":"257880","messageId":"550884B3.5010102@alum.mit.edu","threadId":"38832","inReplyTo":"xmqqk2yfo3y0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-17T19:46:59Z","receivedAt":"2015-03-17T19:46:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/17/2015 07:48 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> When I looked around, I found scores of sites that call atoi(),\n>> strtoul(), and strtol() carelessly. And it's understandable. Calling\n>> any of these functions safely is so much work as to be completely\n>> impractical in day-to-day code.\n>>\n>> git-compat-util.h has two functions, strtoul_ui() and strtol_i(), that\n>> try to make parsing integers a little bit easier.\n> \n> Exactly. They were introduced to prevent sloppy callers from\n> passing NULL to the &end parameter to underlying strtoul(3). The\n> first thing that came to my mind while reading your message up to\n> this point was \"why not use them more, tighten them, adding\n> different variants if necessary, instead of introducing an entirely\n> new set of functions in a new namespace?\"\n\nHere were my thoughts:\n\n* I wanted to change the interface to look less like\n strtol()/strtoul(), so it seemed appropriate to make the names\n more dissimilar.\n\n* The functions seemed long enough that they shouldn't be inline,\n and I wanted to put them in their own module rather than put\n them in git-compat-util.h.\n\n* It wasn't obvious to me how to generalize the names, strtoul_ui()\n and strtol_i(), to the eight functions that I wanted.\n\nThat being said, I'm not married to the names. Suggestions are\nwelcome--but we would need names for 8 functions, not 4 [1].\n\nMichael\n\n> For example:\n> \n>> * Am I making callers too strict? In many cases where a positive\n>> integer is expected (e.g., \"--abbrev=\"), I have been replacing\n>> code like\n>>\n>> result = strtoul(s, NULL, 10);\n>>\n>> with\n>>\n>> if (convert_i(s, 10, &result))\n>> die(\"...\");\n> \n> I think strictness would be good and those who did \"--abbrev=' 20'\"\n> deserve what they get from the additional strictness, but \n> \n> \tif (strtol_i(s, 10, &result))\n> \n> would have been more familiar.\n\n[1] It could be that when we're done, it will turn out that some of the\neight variants are not needed. If so, we can delete them then.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257899","messageId":"CACsJy8AiHHKBD4vV5aJec9CGzBqfSCNU6-j=LjDhsp2VrC55+A@mail.gmail.com","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-17T23:05:15Z","receivedAt":"2015-03-17T23:05:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 17, 2015 at 11:00 PM, Michael Haggerty wrote:\n> Michael Haggerty (14):\n> numparse: new module for parsing integral numbers\n> cacheinfo_callback(): use convert_ui() when handling \"--cacheinfo\"\n> write_subdirectory(): use convert_ui() for parsing mode\n> handle_revision_opt(): use skip_prefix() in many places\n> handle_revision_opt(): use convert_i() when handling \"-\"\n> strtoul_ui(), strtol_i(): remove functions\n> handle_revision_opt(): use convert_ui() when handling \"--abbrev=\"\n> builtin_diff(): detect errors when parsing --unified argument\n> opt_arg(): val is always non-NULL\n> opt_arg(): use convert_i() in implementation\n> opt_arg(): report errors parsing option values\n> opt_arg(): simplify pointer handling\n> diff_opt_parse(): use convert_i() when handling \"-l\"\n> diff_opt_parse(): use convert_i() when handling --abbrev=\n\nThank you for doing it. I was about to write another number parser and\nyou did it :D Maybe you can add another patch to convert the only\nstrtol in upload-pack.c to parse_ui. This place should accept positive\nnumber in base 10, plus sign is not accepted.\n-- \nDuy\n"},{"id":"257924","messageId":"550949BC.1020500@alum.mit.edu","threadId":"38832","inReplyTo":"CACsJy8AiHHKBD4vV5aJec9CGzBqfSCNU6-j=LjDhsp2VrC55+A@mail.gmail.com","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-18T09:47:40Z","receivedAt":"2015-03-18T09:47:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/18/2015 12:05 AM, Duy Nguyen wrote:\n> On Tue, Mar 17, 2015 at 11:00 PM, Michael Haggerty wrote:\n>> Michael Haggerty (14):\n>> numparse: new module for parsing integral numbers\n>> cacheinfo_callback(): use convert_ui() when handling \"--cacheinfo\"\n>> write_subdirectory(): use convert_ui() for parsing mode\n>> handle_revision_opt(): use skip_prefix() in many places\n>> handle_revision_opt(): use convert_i() when handling \"-\"\n>> strtoul_ui(), strtol_i(): remove functions\n>> handle_revision_opt(): use convert_ui() when handling \"--abbrev=\"\n>> builtin_diff(): detect errors when parsing --unified argument\n>> opt_arg(): val is always non-NULL\n>> opt_arg(): use convert_i() in implementation\n>> opt_arg(): report errors parsing option values\n>> opt_arg(): simplify pointer handling\n>> diff_opt_parse(): use convert_i() when handling \"-l\"\n>> diff_opt_parse(): use convert_i() when handling --abbrev=\n> \n> Thank you for doing it. I was about to write another number parser and\n> you did it :D Maybe you can add another patch to convert the only\n> strtol in upload-pack.c to parse_ui. This place should accept positive\n> number in base 10, plus sign is not accepted.\n\nIf the general direction of this patch series is accepted, I'll\ngradually try to go through the codebase, replacing *all* integer\nparsing with these functions. So there's no need to request particular\ncallers of strtol()/strtoul() to be converted; I'll get to them all\nsooner or later (I hope).\n\nBut in case you have some reason that you want upload-pack.c to be\nconverted right away, I just pushed that change (plus some related\ncleanups) to my GitHub repo [1]. The branch depends only on the first\npatch of the \"numparse\" patch series.\n\nBy the way, some other packet line parsing code in that file doesn't\nverify that there are no trailing characters on the lines that they\nprocess. That might be another thing that should be tightened up.\n\nMichael\n\n[1] https://github.com/mhagger/git.git, branch \"upload-pack-numparse\"\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257934","messageId":"CACsJy8CS1h+7JNLpvzpZd1AmOGHQA-sfHSiMJmJ0KF8Bj9+PiA@mail.gmail.com","threadId":"38832","inReplyTo":"550949BC.1020500@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-18T09:58:00Z","receivedAt":"2015-03-18T09:58:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 18, 2015 at 4:47 PM, Michael Haggerty wrote:\n>> Thank you for doing it. I was about to write another number parser and\n>> you did it :D Maybe you can add another patch to convert the only\n>> strtol in upload-pack.c to parse_ui. This place should accept positive\n>> number in base 10, plus sign is not accepted.\n>\n> If the general direction of this patch series is accepted, I'll\n> gradually try to go through the codebase, replacing *all* integer\n> parsing with these functions. So there's no need to request particular\n> callers of strtol()/strtoul() to be converted; I'll get to them all\n> sooner or later (I hope).\n\nGood to know. No there's no hurry in converting this particular place.\nIt's just tightening things up, not bug fixing or anything.\n-- \nDuy\n"},{"id":"257935","messageId":"20150318100338.GA26998@peff.net","threadId":"38832","inReplyTo":"550949BC.1020500@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-18T10:03:38Z","receivedAt":"2015-03-18T10:03:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 18, 2015 at 10:47:40AM +0100, Michael Haggerty wrote:\n\n> But in case you have some reason that you want upload-pack.c to be\n> converted right away, I just pushed that change (plus some related\n> cleanups) to my GitHub repo [1]. The branch depends only on the first\n> patch of the \"numparse\" patch series.\n> \n> By the way, some other packet line parsing code in that file doesn't\n> verify that there are no trailing characters on the lines that they\n> process. That might be another thing that should be tightened up.\n\nDo you mean that upload-pack gets a pkt-line of length N that contains a\nline of length M, and then doesn't check that M==N? We use the space\nbetween M and N for passing capabilities and other metadata around.\n\nOr do you mean that we see lines like:\n\n want [0-9a-f]{40} ...\\n\n\nand do not bother looking at the \"...\" that comes after the data we\nexpect? That I can believe, and I don't think it would hurt to tighten\nup (we shouldn't need it for extensibility, as anybody trying to stick\nextra data there should do so only after using a capability flag earlier\nin the protocol).\n\n-Peff\n"},{"id":"257936","messageId":"55095159.40106@alum.mit.edu","threadId":"38832","inReplyTo":"20150318100338.GA26998@peff.net","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-18T10:20:09Z","receivedAt":"2015-03-18T10:20:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/18/2015 11:03 AM, Jeff King wrote:\n> On Wed, Mar 18, 2015 at 10:47:40AM +0100, Michael Haggerty wrote:\n> \n>> But in case you have some reason that you want upload-pack.c to be\n>> converted right away, I just pushed that change (plus some related\n>> cleanups) to my GitHub repo [1]. The branch depends only on the first\n>> patch of the \"numparse\" patch series.\n>>\n>> By the way, some other packet line parsing code in that file doesn't\n>> verify that there are no trailing characters on the lines that they\n>> process. That might be another thing that should be tightened up.\n> \n> Do you mean that upload-pack gets a pkt-line of length N that contains a\n> line of length M, and then doesn't check that M==N? We use the space\n> between M and N for passing capabilities and other metadata around.\n> \n> Or do you mean that we see lines like:\n> \n> want [0-9a-f]{40} ...\\n\n> \n> and do not bother looking at the \"...\" that comes after the data we\n> expect? That I can believe, and I don't think it would hurt to tighten\n> up (we shouldn't need it for extensibility, as anybody trying to stick\n> extra data there should do so only after using a capability flag earlier\n> in the protocol).\n\nThe latter. For example here [1], the \"have\" command and its SHA-1 are\nread from the line, but I don't see a check that there are no characters\nafter the SHA-1. The same here [2].\n\nMichael\n\n[1]\nhttps://github.com/gitster/git/blob/9ab698f4000a736864c41f57fbae1e021ac27799/upload-pack.c#L404-L429\n[2]\nhttps://github.com/gitster/git/blob/9ab698f4000a736864c41f57fbae1e021ac27799/upload-pack.c#L550-L565\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"257969","messageId":"CAPig+cTyCS2-gys0MQSsA4K_k2gnGRvitXzDybyhO5q41OMD_w@mail.gmail.com","threadId":"38832","inReplyTo":"1426608016-2978-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 01/14] numparse: new module for parsing integral numbers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-18T18:27:56Z","receivedAt":"2015-03-18T18:27:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tuesday, March 17, 2015, Michael Haggerty wrote:\n> Implement wrappers for strtol() and strtoul() that are safer and more\n> convenient to use.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> diff --git a/numparse.c b/numparse.c\n> new file mode 100644\n> index 0000000..90b44ce\n> --- /dev/null\n> +++ b/numparse.c\n> @@ -0,0 +1,180 @@\n> +int parse_l(const char *s, unsigned int flags, long *result, char **endptr)\n> +{\n> + long l;\n> + const char *end;\n> + int err = 0;\n> +\n> + err = parse_precheck(s, &flags);\n> + if (err)\n> + return err;\n> + /*\n> + * Now let strtol() do the heavy lifting:\n> + */\n\nPerhaps use a /* one-line style comment */ to reduce vertical space\nconsumption a bit, thus make it (very slightly) easier to run the eye\nover the code.\n\n> + errno = 0;\n> + l = strtol(s, (char **)&end, flags & NUM_BASE_MASK);\n> + if (errno) {\n> + if (errno == ERANGE) {\n> + if (!(flags & NUM_SATURATE))\n> + return -NUM_SATURATE;\n> + } else {\n> + return -NUM_OTHER_ERROR;\n> + }\n> + }\n\nWould it reduce cognitive load slightly (and reduce vertical space\nconsumption) to rephrase the conditionals as:\n\n if (errno == ERANGE && !(flags & NUM_SATURATE))\n return -NUM_SATURATE;\n\n if (errno && errno != ERANGE)\n return -NUM_OTHER_ERROR;\n\nor something similar?\n\nMore below.\n\n> + if (end == s)\n> + return -NUM_NO_DIGITS;\n> +\n> + if (*end && !(flags & NUM_TRAILING))\n> + return -NUM_TRAILING;\n> +\n> + /* Everything was OK */\n> + *result = l;\n> + if (endptr)\n> + *endptr = (char *)end;\n> + return 0;\n> +}\n> diff --git a/numparse.h b/numparse.h\n> new file mode 100644\n> index 0000000..4de5e10\n> --- /dev/null\n> +++ b/numparse.h\n> @@ -0,0 +1,207 @@\n> +#ifndef NUMPARSE_H\n> +#define NUMPARSE_H\n> +\n> +/*\n> + * Functions for parsing integral numbers.\n> + *\n> + * Examples:\n> + *\n> + * - Convert s into a signed int, interpreting prefix \"0x\" to mean\n> + * hexadecimal and \"0\" to mean octal. If the value doesn't fit in an\n> + * unsigned int, set result to INT_MIN or INT_MAX.\n\nDid you mean s/unsigned int/signed int/ ?\n\n> + *\n> + * if (convert_i(s, NUM_SLOPPY, &result))\n> + * die(\"...\");\n> + */\n> +\n> +/*\n> + * The lowest 6 bits of flags hold the numerical base that should be\n> + * used to parse the number, 2 <= base <= 36. If base is set to 0,\n> + * then NUM_BASE_SPECIFIER must be set too; in this case, the base is\n> + * detected automatically from the string's prefix.\n\nDoes this restriction go against the goal of making these functions\nconvenient, even while remaining strict? Is there a strong reason for\nnot merely inferring NUM_BASE_SPECIFIER when base is 0? Doing so would\nmake it consistent with strto*l() without (I think) introducing any\nambiguity.\n\n> + */\n> +/*\n> + * Number parsing functions:\n> + *\n> + * The following functions parse a number (long, unsigned long, int,\n> + * or unsigned int respectively) from the front of s, storing the\n> + * value to *result and storing a pointer to the first character after\n> + * the number to *endptr. flags specifies how the number should be\n> + * parsed, including which base should be used. flags is a combination\n> + * of the numerical base (2-36) and the NUM_* constants above (see).\n\n\"(see)\" what?\n\n> + * Return 0 on success or a negative value if there was an error. On\n> + * failure, *result and *entptr are left unchanged.\n> + *\n> + * Please note that if NUM_TRAILING is not set, then it is\n> + * nevertheless an error if there are any characters between the end\n> + * of the number and the end of the string.\n\nAgain, on the subject of convenience, why this restriction? The stated\npurpose of the parse_*() functions is to parse the number from the\nfront of the string and return a pointer to the first non-numeric\ncharacter following. As a reader of this API, I interpret that as\nmeaning that NUM_TRAILING is implied. Is there a strong reason for not\ninferring NUM_TRAILING for the parse_*() functions at the API level?\n(I realize that the convert_*() functions are built atop parse_*(),\nbut that's an implementation detail.)\n\n> + */\n> +\n> +int parse_l(const char *s, unsigned int flags,\n> + long *result, char **endptr);\n\nDo we want to perpetuate the ugly (char **) convention for 'endptr'\nfrom strto*l()? Considering that the incoming string is const, it\nseems undesirable to return a non-const pointer to some place inside\nthat string.\n\n> +/*\n> + * Number conversion functions:\n> + *\n> + * The following functions parse a string into a number. They are\n> + * identical to the parse_*() functions above, except that the endptr\n> + * is not returned. These are most useful when parsing a whole string\n> + * into a number; i.e., when (flags & NUM_TRAILING) is unset.\n\nI can formulate arguments for allowing or disallowing NUM_TRAILING\nwith convert_*(), however, given their purpose of parsing the entire\nstring into a number, as a reader of the API, I would kind of expect\nthe convert_*() functions to ensure that NUM_TRAILING is not set\n(either by forcibly clearing it or erroring out as an inconsistent\nrequest if it is set).\n\n> + */\n> +static inline int convert_l(const char *s, unsigned int flags,\n> + long *result)\n> +{\n> + return parse_l(s, flags, result, NULL);\n> +}\n"},{"id":"257993","messageId":"550A008B.3050900@alum.mit.edu","threadId":"38832","inReplyTo":"CAPig+cTyCS2-gys0MQSsA4K_k2gnGRvitXzDybyhO5q41OMD_w@mail.gmail.com","subject":"Re: [PATCH 01/14] numparse: new module for parsing integral numbers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-18T22:47:39Z","receivedAt":"2015-03-18T22:47:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/18/2015 07:27 PM, Eric Sunshine wrote:\n> On Tuesday, March 17, 2015, Michael Haggerty wrote:\n>> Implement wrappers for strtol() and strtoul() that are safer and more\n>> convenient to use.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> diff --git a/numparse.c b/numparse.c\n>> new file mode 100644\n>> index 0000000..90b44ce\n>> --- /dev/null\n>> +++ b/numparse.c\n>> @@ -0,0 +1,180 @@\n>> +int parse_l(const char *s, unsigned int flags, long *result, char **endptr)\n>> +{\n>> + long l;\n>> + const char *end;\n>> + int err = 0;\n>> +\n>> + err = parse_precheck(s, &flags);\n>> + if (err)\n>> + return err;\n>> + /*\n>> + * Now let strtol() do the heavy lifting:\n>> + */\n> \n> Perhaps use a /* one-line style comment */ to reduce vertical space\n> consumption a bit, thus make it (very slightly) easier to run the eye\n> over the code.\n\nYes, will change.\n\n>> + errno = 0;\n>> + l = strtol(s, (char **)&end, flags & NUM_BASE_MASK);\n>> + if (errno) {\n>> + if (errno == ERANGE) {\n>> + if (!(flags & NUM_SATURATE))\n>> + return -NUM_SATURATE;\n>> + } else {\n>> + return -NUM_OTHER_ERROR;\n>> + }\n>> + }\n> \n> Would it reduce cognitive load slightly (and reduce vertical space\n> consumption) to rephrase the conditionals as:\n> \n> if (errno == ERANGE && !(flags & NUM_SATURATE))\n> return -NUM_SATURATE;\n> \n> if (errno && errno != ERANGE)\n> return -NUM_OTHER_ERROR;\n> \n> or something similar?\n\nThe most common case by far should be that errno is zero. The code as\nwritten only needs one test for that case, whereas your code needs two\ntests. I think it is worth compromising on code clarity a tiny bit to\navoid the extra test.\n\n> More below.\n> \n>> + if (end == s)\n>> + return -NUM_NO_DIGITS;\n>> +\n>> + if (*end && !(flags & NUM_TRAILING))\n>> + return -NUM_TRAILING;\n>> +\n>> + /* Everything was OK */\n>> + *result = l;\n>> + if (endptr)\n>> + *endptr = (char *)end;\n>> + return 0;\n>> +}\n>> diff --git a/numparse.h b/numparse.h\n>> new file mode 100644\n>> index 0000000..4de5e10\n>> --- /dev/null\n>> +++ b/numparse.h\n>> @@ -0,0 +1,207 @@\n>> +#ifndef NUMPARSE_H\n>> +#define NUMPARSE_H\n>> +\n>> +/*\n>> + * Functions for parsing integral numbers.\n>> + *\n>> + * Examples:\n>> + *\n>> + * - Convert s into a signed int, interpreting prefix \"0x\" to mean\n>> + * hexadecimal and \"0\" to mean octal. If the value doesn't fit in an\n>> + * unsigned int, set result to INT_MIN or INT_MAX.\n> \n> Did you mean s/unsigned int/signed int/ ?\n\nThanks for catching this. I will fix it.\n\n>> + *\n>> + * if (convert_i(s, NUM_SLOPPY, &result))\n>> + * die(\"...\");\n>> + */\n>> +\n>> +/*\n>> + * The lowest 6 bits of flags hold the numerical base that should be\n>> + * used to parse the number, 2 <= base <= 36. If base is set to 0,\n>> + * then NUM_BASE_SPECIFIER must be set too; in this case, the base is\n>> + * detected automatically from the string's prefix.\n> \n> Does this restriction go against the goal of making these functions\n> convenient, even while remaining strict? Is there a strong reason for\n> not merely inferring NUM_BASE_SPECIFIER when base is 0? Doing so would\n> make it consistent with strto*l() without (I think) introducing any\n> ambiguity.\n\nI thought about doing this. If it were possible to eliminate\nNUM_BASE_SPECIFIER altogether, then there is no doubt that it would be a\ngood change. But NUM_BASE_SPECIFIER also has an effect when base==16;\nnamely, that an \"0x\" prefix, if present, is consumed. So\n\n parse_i(\"0xff\", 16 | NUM_BASE_SPECIFIER, &result, &endptr)\n\ngives result==255 and endptr==s+4, whereas\n\n parse_i(\"0xff\", 16, &result, &endptr)\n\ngives result==0 and entptr==s+1 (it treats the \"x\" as the end of the\nstring).\n\nWe could forgo that feature, effectively allowing a base specifier if\nand only if base==0. But I didn't want to rule out allowing an optional\nbase specifier for base==16, in which case NUM_BASE_SPECIFIER can't be\ndispensed with entirely.\n\nIf you agree with that, then the remaining question is: which policy is\nless error-prone? My thinking was that forcing the caller to specify\nNUM_BASE_SPECIFIER explicitly when they select base==0 will serve as a\nreminder that the two features are intertwined. Because another\nimaginable policy (arguably more consistent with the policy for base!=0)\nwould be that\n\n convert_i(s, 0, &result)\n\n, because it *doesn't* specify NUM_BASE_SPECIFIER, doesn't allow a base\nprefix, and therefore indirectly only allows base-10 numbers.\n\nBut I don't feel strongly about this.\n\n>> + */\n>> +/*\n>> + * Number parsing functions:\n>> + *\n>> + * The following functions parse a number (long, unsigned long, int,\n>> + * or unsigned int respectively) from the front of s, storing the\n>> + * value to *result and storing a pointer to the first character after\n>> + * the number to *endptr. flags specifies how the number should be\n>> + * parsed, including which base should be used. flags is a combination\n>> + * of the numerical base (2-36) and the NUM_* constants above (see).\n> \n> \"(see)\" what?\n\nThat was meant to be a pointer to the documentation for the NUM_*\nconstants. But obviously it was too oblique, so I will make it more\nexplicit, maybe\n\n ... and the NUM_* constants documented above.\n\n>> + * Return 0 on success or a negative value if there was an error. On\n>> + * failure, *result and *entptr are left unchanged.\n>> + *\n>> + * Please note that if NUM_TRAILING is not set, then it is\n>> + * nevertheless an error if there are any characters between the end\n>> + * of the number and the end of the string.\n> \n> Again, on the subject of convenience, why this restriction? The stated\n> purpose of the parse_*() functions is to parse the number from the\n> front of the string and return a pointer to the first non-numeric\n> character following. As a reader of this API, I interpret that as\n> meaning that NUM_TRAILING is implied. Is there a strong reason for not\n> inferring NUM_TRAILING for the parse_*() functions at the API level?\n> (I realize that the convert_*() functions are built atop parse_*(),\n> but that's an implementation detail.)\n\nYes, I'd also thought about that change:\n\n* Make NUM_TRAILING private.\n* Make the current parse_*() functions private.\n* Add new parse_*(s, flags, result, endptr) functions that imply\nNUM_TRAILING (and maybe they should *require* a non-null endptr argument).\n* Change the convert_*() to not allow the NUM_TRAILING flag.\n\nThis would add a little bit of code, so I didn't do it originally. But\nsince you seem to like the idea too, I guess I will make the change.\n\n>> + */\n>> +\n>> +int parse_l(const char *s, unsigned int flags,\n>> + long *result, char **endptr);\n> \n> Do we want to perpetuate the ugly (char **) convention for 'endptr'\n> from strto*l()? Considering that the incoming string is const, it\n> seems undesirable to return a non-const pointer to some place inside\n> that string.\n\nYes, I guess we could do some internal casting and expose endptr as\n(const char **). It would make it a bit harder if the user actually\nwants to pass a non-const string to the function and then modify the\nstring via the returned endptr, but I haven't run into that pattern yet\nso let's make the change you suggested.\n\n>> +/*\n>> + * Number conversion functions:\n>> + *\n>> + * The following functions parse a string into a number. They are\n>> + * identical to the parse_*() functions above, except that the endptr\n>> + * is not returned. These are most useful when parsing a whole string\n>> + * into a number; i.e., when (flags & NUM_TRAILING) is unset.\n> \n> I can formulate arguments for allowing or disallowing NUM_TRAILING\n> with convert_*(), however, given their purpose of parsing the entire\n> string into a number, as a reader of the API, I would kind of expect\n> the convert_*() functions to ensure that NUM_TRAILING is not set\n> (either by forcibly clearing it or erroring out as an inconsistent\n> request if it is set).\n\nYes, I think I will change this, as discussed above.\n\n>> + */\n>> +static inline int convert_l(const char *s, unsigned int flags,\n>> + long *result)\n>> +{\n>> + return parse_l(s, flags, result, NULL);\n>> +}\n\nThanks for your careful and thoughtful comments!\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"258008","messageId":"20150319052620.GA30645@peff.net","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T05:26:20Z","receivedAt":"2015-03-19T05:26:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 17, 2015 at 05:00:02PM +0100, Michael Haggerty wrote:\n\n> My main questions:\n> \n> * Do people like the API? My main goal was to make these functions as\n> painless as possible to use correctly, because there are so many\n> call sites.\n> \n> * Is it too gimmicky to encode the base together with other options in\n> `flags`? (I think it is worth it to avoid the need for another\n> parameter, which callers could easily put in the wrong order.)\n\nI definitely like the overall direction of this. My first thought was\nthat it does seem like there are a lot of possible options to the\nfunctions (and OR-ing the flags with the base does seem weird, though I\ncan't think of a plausible case where it would actually cause errors).\nMany of those options don't seem used in the example conversions (I'm\nnot clear who would want NUM_SATURATE, for example).\n\nI wondered if we could do away with the radix entirely. Wouldn't we be\nasking for base 10 most of the time? Of course, your first few patches\nshow octal parsing, but I wonder if we should actually have a separate\nparse_mode() for that, since that seems to be the general reason for\ndoing octal parsing. 100000644 does not overflow an int, but it is\nhardly a reasonable mode.\n\nI also wondered if we could get rid of NUM_SIGN in favor of just having\nthe type imply it (e.g., convert_l would always allow negative numbers,\nwhereas convert_ul would not). But I suppose there are times when we end\nup using an \"int\" to store an unsigned value for a good reason (e.g.,\n\"-1\" is a sentinel value, but we expect only positive values from the\nuser). So that might be a bad idea.\n\nI notice that you go up to \"unsigned long\" here for sizes. If we want to\nuse this everywhere, we'll need something larger for parsing off_t\nvalues on 32-bit machines (though I would not at all be surprised with\nthe current code if 32-bit machines have a hard time configuring a\npack.packSizeLimit above 4G).\n\nI wonder how much of the boilerplate in the parse_* functions could be\nfactored out to use a uintmax_t, with the caller just providing the\nrange. That would make it easier to add new types like off_t, and\npossibly even constrained types (e.g., an integer from 0 to 100). On the\nother hand, you mentioned to me elsewhere that there may be some bugs in\nthe range-checks of config.c's integer parsing. I suspect they are\nrelated to exactly this kind of refactoring, so perhaps writing things\nout is best.\n\n> * Am I making callers too strict? In many cases where a positive\n> integer is expected (e.g., \"--abbrev=\"), I have been replacing\n> code like\n> [...]\n\nIMHO most of the tightening happening here is a good thing, and means we\nare more likely to notice mistakes rather than silently doing something\nstupid.\n\nFor sites that currently allow it, I could imagine people using hex\nnotation for some values, though, depending on the context. It looks\nthere aren't many of them ((it is just when the radix is \"0\", right?).\nSome of them look to be accidental (does anybody really ask for\n--threads=0x10?), but others might not be (the pack index-version\ncontains an offset field that might be quite big).\n\n\nFeel free to ignore any or all of that. It is not so much criticism as\na dump of thoughts I had while reading the patches. Perhaps you can pick\nsomething useful out of that, and perhaps not. :)\n\n-Peff\n"},{"id":"258010","messageId":"xmqq7fudld61.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"1426608016-2978-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T06:22:14Z","receivedAt":"2015-03-19T06:22:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> * It allows leading whitespace.\n\nThis might be blessing in disguise. Our friends on MacOS may be\nrelying on that\n\n git cmd --abbrev=\"$(wc -c * It allows arbitrary trailing characters.\n\nWhich is why we have introduced strtoul_ui() and such.\n\n> * It allows a leading sign character ('+' or '-'), even though the\n> result is unsigned.\n\nI do not particularly see it a bad thing to accept \"--abbrev=+7\".\nUsing unsigned type to accept a width and parsing \"--abbrev=-7\" into\na large positive integer _is_ a problem, and we may want to fix it,\nbut I think that is still within the scope of the original \"better\nstrtol/strtoul\", and I do not see it as a justification to introduce\na set of functions with completely unfamiliar name.\n\n> * If the string doesn't contain a number at all, it sets its \"endptr\"\n> argument to point at the start of the string but doesn't set errno.\n\nWhy is that a problem? A caller that cares is supposed to check\nendptr and the string it gave, no? Now, if strtoul_ui() and friends\ndo not do so, I again think that is still within the scope of the\noriginal \"better strtol/strtoul\".\n\n> * If the value is larger than fits in an unsigned long, it returns the\n> value clamped to the range 0..ULONG_MAX (setting errno to ERANGE).\n\nDitto.\n\n> * If the value is between -ULONG_MAX and 0, it returns the positive\n> integer with the same bit pattern, without setting errno(!) (I can\n> hardly think of a scenario where this would be useful.)\n\nDitto.\n\n> * For base=0 (autodetect base), it allows a base specifier prefix \"0x\"\n> or \"0\" and parses the number accordingly. For base=16 it also allows\n> a \"0x\" prefix.\n\nWhy is it a problem to allow \"git cmd --hexval=0x1234\", even if \"git\ncmd --hexval=1234\" would suffice?\n\n> When I looked around, I found scores of sites that call atoi(),\n> strtoul(), and strtol() carelessly. And it's understandable. Calling\n> any of these functions safely is so much work as to be completely\n> impractical in day-to-day code.\n\nYes, these burdens on the caller were exactly why strtoul_ui()\netc. wanted to reduce---and it will be a welcome change to do\nnecessary checks that are currently not done.\n\n> Please see the docstrings in numparse.h in the first commit for\n> detailed API docs. Briefly, there are eight new functions:\n>\n> parse_{l,ul,i,ui}(const char *s, unsigned int flags,\n> T *result, char **endptr);\n> convert_{l,ul,i,ui}(const char *s, unsigned int flags, T *result);\n>\n> The parse_*() functions are for parsing a number from the start of a\n> string; the convert_*() functions are for parsing a string that\n> consists of a single number.\n\nI am not sure if I follow. Why not unify them into one 4-function\nset, with optional endptr that could be NULL?\n\nWhile we are on the topic of improving number parsing, one thing\nthat I found somewhat frustrating is that config.c layer knows the\nscaling suffix but option parsing layer does not. These functions\nshould offer an option (via their \"flags\") to say \"I allow scaled\nnumbers like 2k, 4M, etc.\".\n"},{"id":"258011","messageId":"xmqq1tkllcr9.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"550884B3.5010102@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T06:31:06Z","receivedAt":"2015-03-19T06:31:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Here were my thoughts:\n>\n> * I wanted to change the interface to look less like\n> strtol()/strtoul(), so it seemed appropriate to make the names\n> more dissimilar.\n\nOne reason I prefer the names in the compat-util is that it makes it\nclear that what is converted into what (e.g. \"str\" to \"ul\") and what\ntype the result is stored (\"i\" or \"ui\"), and as an added bonus, with\na short-and-sweet \"to\" it is already so clear we are \"converting\"\nthat we do not have to use verbs like \"parse_\" or \"convert_\". From\nparse_i() and convert_i(), I cannot tell which one lacks the endptr\nand parses to the end and which one takes the endptr and tells the\ncaller where the conversion ended (and it also does not tell us that\nthey are converting from a string).\n\n> * The functions seemed long enough that they shouldn't be inline,\n> and I wanted to put them in their own module rather than put\n> them in git-compat-util.h.\n\nI actually agree on this, but that is not a justification why they\nhave to be named very differently.\n\nHaving said that, I would say that it will be futile to discuss this\nfurther, as we seem to agree to disagree.\n"},{"id":"258012","messageId":"xmqqwq2djy0l.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"1426608016-2978-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 05/14] handle_revision_opt(): use convert_i() when handling \"-\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T06:34:50Z","receivedAt":"2015-03-19T06:34:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This tightens up the parsing a bit:\n>\n> * Leading whitespace is no longer allowed\n> * '+' and '-' are no longer allowed\n>\n> It also removes the need to check separately that max_count is\n> non-negative.\n\nHmmm, on the surface this sound nice, but I am not sure if it is a\ngood idea. \"git cmd-in-log-family -3 --max-count=-1\" would have\nbeen a nice way to flip a max-count given earlier on the command\nline back to the default (unlimited) and we are removing it without\ngiving any escape hatch?\n\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> revision.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 25838fc..4908e66 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1,4 +1,5 @@\n> #include \"cache.h\"\n> +#include \"numparse.h\"\n> #include \"tag.h\"\n> #include \"blob.h\"\n> #include \"tree.h\"\n> @@ -1709,8 +1710,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> \t\treturn argcount;\n> \t} else if ((*arg == '-') && isdigit(arg[1])) {\n> \t\t/* accept -, like traditional \"head\" */\n> -\t\tif (strtol_i(arg + 1, 10, &revs->max_count) < 0 ||\n> -\t\t revs->max_count < 0)\n> +\t\tif (convert_i(arg + 1, 10, &revs->max_count))\n> \t\t\tdie(\"'%s': not a non-negative integer\", arg + 1);\n"},{"id":"258013","messageId":"xmqqsid1jxwt.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"1426608016-2978-15-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 14/14] diff_opt_parse(): use convert_i() when handling --abbrev=","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T06:37:06Z","receivedAt":"2015-03-19T06:37:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> die() with an error message if the argument is not a non-negative\n> integer. This change tightens up parsing: '+' and '-', leading\n> whitespace, and trailing junk are all disallowed now.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> diff.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index be389ae..1cc5428 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3830,7 +3830,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n> \telse if (!strcmp(arg, \"--abbrev\"))\n> \t\toptions->abbrev = DEFAULT_ABBREV;\n> \telse if (skip_prefix(arg, \"--abbrev=\", &arg)) {\n> -\t\toptions->abbrev = strtoul(arg, NULL, 10);\n> +\t\tif (convert_i(arg, 10, &options->abbrev))\n> +\t\t\tdie(\"--abbrev requires an integer argument\");\n\nEverybody leading up to this step said \"a non-negative integer\", but\nthis one is different?\n\n> \t\tif (options->abbrev < MINIMUM_ABBREV)\n> \t\t\toptions->abbrev = MINIMUM_ABBREV;\n> \t\telse if (40 < options->abbrev)\n"},{"id":"258014","messageId":"xmqqoanpjxpx.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"20150319052620.GA30645@peff.net","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T06:41:14Z","receivedAt":"2015-03-19T06:41:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wondered if we could do away with the radix entirely. Wouldn't we be\n> asking for base 10 most of the time? Of course, your first few patches\n> show octal parsing, but I wonder if we should actually have a separate\n> parse_mode() for that, since that seems to be the general reason for\n> doing octal parsing. 100000644 does not overflow an int, but it is\n> hardly a reasonable mode.\n\nTrue.\n\n> I also wondered if we could get rid of NUM_SIGN in favor of just having\n> the type imply it (e.g., convert_l would always allow negative numbers,\n> whereas convert_ul would not). But I suppose there are times when we end\n> up using an \"int\" to store an unsigned value for a good reason (e.g.,\n> \"-1\" is a sentinel value, but we expect only positive values from the\n> user). So that might be a bad idea.\n\nYeah, there is a reason why ssize_t exists.\n\n> IMHO most of the tightening happening here is a good thing, and means we\n> are more likely to notice mistakes rather than silently doing something\n> stupid.\n\nI do like the general idea of making sure we avoid use of bare\natoi() and unchecked use of strtol() and such, and have a set of\nwell defined helper functions to perform the common checks, exactly\nfor the reason you said.\n"},{"id":"258017","messageId":"xmqqk2ydjvcd.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"20150319052620.GA30645@peff.net","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T07:32:34Z","receivedAt":"2015-03-19T07:32:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wonder how much of the boilerplate in the parse_* functions could be\n> factored out to use a uintmax_t, with the caller just providing the\n> range. That would make it easier to add new types like off_t, and\n> possibly even constrained types (e.g., an integer from 0 to 100). On the\n> other hand, you mentioned to me elsewhere that there may be some bugs in\n> the range-checks of config.c's integer parsing. I suspect they are\n> related to exactly this kind of refactoring, so perhaps writing things\n> out is best.\n\nI like this idea very well. I wonder if we can implement the family\nof\n\n parse_{type}(const char *, unsigned int flags,\n \t\t const char **endptr, {type} *result)\n\nfunctions by calls a helper that internally deals with the numbers\nin uintmax_t, and then checks if the value fits within the possible\nrange of the *result before returning.\n\n int parse_i(const char *str, unsigned flags,\n\t\tconst char **endptr, int *result) {\n\tuintmax_t val;\n int sign = 1, status;\n if (*str == '-') {\n\t\tsign = -1; \n str++;\n\t}\n status = parse_num(str, flags, endptr, &val, INT_MAX);\n if (status < 0)\n \treturn status;\n\t*result = sign < 0 ? -val : val;\n return 0;\n }\n\n(assuming the last parameter to parse_num is used to check if the\nresult fits within that range). Or that may be easier and cleaner\nto be done in the caller with or something like that:\n\n\tstatus = parse_num(str, flags, endptr, &val);\n if (status < 0)\n \treturn status;\n\tif (INT_MAX <= val * sign || val * sign <= INT_MIN) {\n \terrno = ERANGE;\n return -1;\n\t}\n\nIf we wanted to, we may even be able to avoid duplication of\nboilerplate by wrapping the above pattern within a macro,\nparameterizing the TYPE_{MIN,MAX} and using token pasting, to\nexpand to four necessary result types.\n\nThere is no reason for the implementation of the parse_num() helper\nto be using strtoul(3) or strtoull(3); its behaviour will be under\nour total control. It can become fancier by enriching the flags\nbits (e.g. allow scaling factor, etc.) only once and all variants\nfor various result types will get the same feature.\n"},{"id":"258093","messageId":"CAPig+cTht9bCoJx3dCj3m_z59zp0C-PdzUYJVbZ7kO5ryNaOQQ@mail.gmail.com","threadId":"38832","inReplyTo":"550A008B.3050900@alum.mit.edu","subject":"Re: [PATCH 01/14] numparse: new module for parsing integral numbers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-20T08:54:42Z","receivedAt":"2015-03-20T08:54:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 18, 2015 at 6:47 PM, Michael Haggerty wrote:\n> On 03/18/2015 07:27 PM, Eric Sunshine wrote:\n>> On Tuesday, March 17, 2015, Michael Haggerty wrote:\n>>> Implement wrappers for strtol() and strtoul() that are safer and more\n>>> convenient to use.\n>>> + * The lowest 6 bits of flags hold the numerical base that should be\n>>> + * used to parse the number, 2 <= base <= 36. If base is set to 0,\n>>> + * then NUM_BASE_SPECIFIER must be set too; in this case, the base is\n>>> + * detected automatically from the string's prefix.\n>>\n>> Does this restriction go against the goal of making these functions\n>> convenient, even while remaining strict? Is there a strong reason for\n>> not merely inferring NUM_BASE_SPECIFIER when base is 0? Doing so would\n>> make it consistent with strto*l() without (I think) introducing any\n>> ambiguity.\n>\n> I thought about doing this. If it were possible to eliminate\n> NUM_BASE_SPECIFIER altogether, then there is no doubt that it would be a\n> good change. But NUM_BASE_SPECIFIER also has an effect when base==16;\n> namely, that an \"0x\" prefix, if present, is consumed. So\n>\n> parse_i(\"0xff\", 16 | NUM_BASE_SPECIFIER, &result, &endptr)\n>\n> gives result==255 and endptr==s+4, whereas\n>\n> parse_i(\"0xff\", 16, &result, &endptr)\n>\n> gives result==0 and entptr==s+1 (it treats the \"x\" as the end of the\n> string).\n>\n> We could forgo that feature, effectively allowing a base specifier if\n> and only if base==0. But I didn't want to rule out allowing an optional\n> base specifier for base==16, in which case NUM_BASE_SPECIFIER can't be\n> dispensed with entirely.\n\nMaking base==0 a special case doesn't have to mean ruling out or\neliminating NUM_BASE_SPECIFIER for the base==16 case. However, a\nspecial case base==0 would make the API a bit non-orthogonal and\nrequire extra documentation. But for those familiar with how strto*l()\ntreats base==0 specially, the rule of least surprise may apply.\n\n> If you agree with that, then the remaining question is: which policy is\n> less error-prone? My thinking was that forcing the caller to specify\n> NUM_BASE_SPECIFIER explicitly when they select base==0 will serve as a\n> reminder that the two features are intertwined.\n\nSince base==0 would unambiguously imply NUM_BASE_SPECIFIER, being able\nto tersely say convert_i(s, 0, &result) would be a win from a\nconvenience perspective. However...\n\n> Because another\n> imaginable policy (arguably more consistent with the policy for base!=0)\n> would be that\n>\n> convert_i(s, 0, &result)\n>\n> , because it *doesn't* specify NUM_BASE_SPECIFIER, doesn't allow a base\n> prefix, and therefore indirectly only allows base-10 numbers.\n>\n> But I don't feel strongly about this.\n\nI don't feel strongly about it either, and can formulate arguments\neither way. Assuming you stick with your current design, if the\nstrictness of having to specify NUM_BASE_SPECIFIER for base==0 proves\ntoo burdensome, it can be loosened later by making base==0 a special\ncase.\n\nOn the other hand, if you go with Junio's suggestion of choosing names\nfor these functions which more closely mirror strto*l() names, then\nthe rule of least surprise might suggest that a special case for\nbase==0 has merit.\n\n>>> + * Return 0 on success or a negative value if there was an error. On\n>>> + * failure, *result and *entptr are left unchanged.\n>>> + *\n>>> + * Please note that if NUM_TRAILING is not set, then it is\n>>> + * nevertheless an error if there are any characters between the end\n>>> + * of the number and the end of the string.\n>>\n>> Again, on the subject of convenience, why this restriction? The stated\n>> purpose of the parse_*() functions is to parse the number from the\n>> front of the string and return a pointer to the first non-numeric\n>> character following. As a reader of this API, I interpret that as\n>> meaning that NUM_TRAILING is implied. Is there a strong reason for not\n>> inferring NUM_TRAILING for the parse_*() functions at the API level?\n>> (I realize that the convert_*() functions are built atop parse_*(),\n>> but that's an implementation detail.)\n>\n> Yes, I'd also thought about that change:\n>\n> * Make NUM_TRAILING private.\n> * Make the current parse_*() functions private.\n> * Add new parse_*(s, flags, result, endptr) functions that imply\n> NUM_TRAILING (and maybe they should *require* a non-null endptr argument).\n> * Change the convert_*() to not allow the NUM_TRAILING flag.\n>\n> This would add a little bit of code, so I didn't do it originally. But\n> since you seem to like the idea too, I guess I will make the change.\n\nThe other option (as Junio also suggested) is to collapse this API\ninto just the four parse_*() functions. All of the call-sites you\nconverted in this series invoked convert_*(), but it's not clear that\nhaving two sets of functions which do almost the same thing is much of\na win. If the default is for NUM_TRAILING to be off, and if NULL\n'endptr' is allowed, then:\n\n parse_ul(s, 10, &result, NULL);\n\ndoesn't seem particularly burdensome compared with:\n\n convert_ul(s, 10, &result);\n\nA more compact API, with only the parse_*() functions, means less to\nremember and less to document, and often is more orthogonal.\n\nThe argument for dropping the convert_*() functions is strengthened if\nyou follow Junio's suggestion to name these functions after the\nstrto*l() variations.\n\n>>> +int parse_l(const char *s, unsigned int flags,\n>>> + long *result, char **endptr);\n>>\n>> Do we want to perpetuate the ugly (char **) convention for 'endptr'\n>> from strto*l()? Considering that the incoming string is const, it\n>> seems undesirable to return a non-const pointer to some place inside\n>> that string.\n>\n> Yes, I guess we could do some internal casting and expose endptr as\n> (const char **). It would make it a bit harder if the user actually\n> wants to pass a non-const string to the function and then modify the\n> string via the returned endptr, but I haven't run into that pattern yet\n> so let's make the change you suggested.\n\nI don't feel strongly about this either. It always struck me as a bit\nof an API wart, but I can see the convenience value of non-const\n'endptr' if the caller intends to modify the string. If you rename\nthese functions to mirror strto*l(), then the rule of least surprise\nwould suggest that 'endptr' should remain non-const.\n"},{"id":"258143","messageId":"xmqq7fubftfk.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"1426608016-2978-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 01/14] numparse: new module for parsing integral numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-20T17:51:59Z","receivedAt":"2015-03-20T17:51:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +static int parse_precheck(const char *s, unsigned int *flags)\n> +{\n> +\tconst char *number;\n> +\n> +\tif (isspace(*s)) {\n> +\t\tif (!(*flags & NUM_LEADING_WHITESPACE))\n> +\t\t\treturn -NUM_LEADING_WHITESPACE;\n> +\t\tdo {\n> +\t\t\ts++;\n> +\t\t} while (isspace(*s));\n> +\t}\n> +\n> +\tif (*s == '+') {\n> +\t\tif (!(*flags & NUM_PLUS))\n> +\t\t\treturn -NUM_PLUS;\n> +\t\tnumber = s + 1;\n> +\t\t*flags &= ~NUM_NEGATIVE;\n> +\t} else if (*s == '-') {\n> +\t\tif (!(*flags & NUM_MINUS))\n> +\t\t\treturn -NUM_MINUS;\n> +\t\tnumber = s + 1;\n> +\t\t*flags |= NUM_NEGATIVE;\n> +\t} else {\n> +\t\tnumber = s;\n> +\t\t*flags &= ~NUM_NEGATIVE;\n> +\t}\n> +\n> +\tif (!(*flags & NUM_BASE_SPECIFIER)) {\n> +\t\tint base = *flags & NUM_BASE_MASK;\n> +\t\tif (base == 0) {\n> +\t\t\t/* This is a pointless combination of options. */\n> +\t\t\tdie(\"BUG: base=0 specified without NUM_BASE_SPECIFIER\");\n> +\t\t} else if (base == 16 && starts_with(number, \"0x\")) {\n> +\t\t\t/*\n> +\t\t\t * We want to treat this as zero terminated by\n> +\t\t\t * an 'x', whereas strtol()/strtoul() would\n> +\t\t\t * silently eat the \"0x\". We accomplish this\n> +\t\t\t * by treating it as a base 10 number:\n> +\t\t\t */\n> +\t\t\t*flags = (*flags & ~NUM_BASE_MASK) | 10;\n> +\t\t}\n> +\t}\n> +\treturn 0;\n> +}\n\nI somehow feel that a pre-processing that only _inspects_ part of\nthe string, without munging that string (e.g. notice '-' but feed\nthat to underlying strtol(3)) somewhat a brittle approach. When I\nread the above for the first time, I somehow expected that the code\nwould notice leading '-', strip that leading '-' and remember the\nfact that it did so in the *flags, let the strtol(3) to parse the\nremainder _and_ always make sure the returned result is not negative\n(because that would imply that the original input had two leading\nminuses and digits), and give the sign based on what this preprocess\nfound out in *flags, and then seeing that there is no sign of such\nprocessing in the caller I scratched my head.\n\nI still have not convinced myself that what I am seeing in the\nbase==16 part in the above is correct.\n"},{"id":"258411","messageId":"55117D54.103@alum.mit.edu","threadId":"38832","inReplyTo":"20150319052620.GA30645@peff.net","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-24T15:05:56Z","receivedAt":"2015-03-24T15:05:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/19/2015 06:26 AM, Jeff King wrote:\n> On Tue, Mar 17, 2015 at 05:00:02PM +0100, Michael Haggerty wrote:\n> \n>> My main questions:\n>>\n>> * Do people like the API? My main goal was to make these functions as\n>> painless as possible to use correctly, because there are so many\n>> call sites.\n>>\n>> * Is it too gimmicky to encode the base together with other options in\n>> `flags`? (I think it is worth it to avoid the need for another\n>> parameter, which callers could easily put in the wrong order.)\n> \n> I definitely like the overall direction of this. My first thought was\n> that it does seem like there are a lot of possible options to the\n> functions (and OR-ing the flags with the base does seem weird, though I\n> can't think of a plausible case where it would actually cause errors).\n> Many of those options don't seem used in the example conversions (I'm\n> not clear who would want NUM_SATURATE, for example).\n\nThere are a lot of options, but so far only few of them have been used.\nAs the call sites are rewritten we will see which of these features are\nuseful, and which can be dispensed with. If groups of options tend to be\nused together, we can define constants for them like I've done with\nNUM_SLOPPY and NUM_SIGN.\n\nRegarding NUM_SATURATE: I'm not sure who would want it either, but I\nthought there might be places where the user wants to specify\n(effectively) \"infinity\", and it might be convenient to let him specify\nsomething easy to type like \"--max=999999999999\" rather than\n\"--max=2147483647\".\n\n> I wondered if we could do away with the radix entirely. Wouldn't we be\n> asking for base 10 most of the time? Of course, your first few patches\n> show octal parsing, but I wonder if we should actually have a separate\n> parse_mode() for that, since that seems to be the general reason for\n> doing octal parsing. 100000644 does not overflow an int, but it is\n> hardly a reasonable mode.\n\nAgain, as a first pass I wanted to just have a really flexible API so\nthat call sites can be rewritten without a lot of extra thought. If\nsomebody wants to add a parse_mode() function, it will be easy to build\non top of convert_ui(). But that change can be done after this one.\n\n> I also wondered if we could get rid of NUM_SIGN in favor of just having\n> the type imply it (e.g., convert_l would always allow negative numbers,\n> whereas convert_ul would not). But I suppose there are times when we end\n> up using an \"int\" to store an unsigned value for a good reason (e.g.,\n> \"-1\" is a sentinel value, but we expect only positive values from the\n> user). So that might be a bad idea.\n\nYes, as I was rewriting call sites, I found many that used the unsigned\nvariants of the parsing functions but stored the result in an int.\nProbably some of these use -1 to denote \"unset\"; it might be that there\nare other cases where the variable could actually be declared to be\n\"unsigned int\".\n\nProhibiting signs when parsing signed quantities isn't really elegant\nfrom an API purity point of view, but it sure is handy!\n\n> I notice that you go up to \"unsigned long\" here for sizes. If we want to\n> use this everywhere, we'll need something larger for parsing off_t\n> values on 32-bit machines (though I would not at all be surprised with\n> the current code if 32-bit machines have a hard time configuring a\n> pack.packSizeLimit above 4G).\n\nYes, probably. I haven't run into such call sites yet.\n\n> I wonder how much of the boilerplate in the parse_* functions could be\n> factored out to use a uintmax_t, with the caller just providing the\n> range. That would make it easier to add new types like off_t, and\n> possibly even constrained types (e.g., an integer from 0 to 100). On the\n> other hand, you mentioned to me elsewhere that there may be some bugs in\n> the range-checks of config.c's integer parsing. I suspect they are\n> related to exactly this kind of refactoring, so perhaps writing things\n> out is best.\n\nIt's not a lot of code yet. If we find out we need variants for size_t\nand off_t and uintmax_t and intmax_t then such a refactoring would\ndefinitely be worth considering.\n\n>> * Am I making callers too strict? In many cases where a positive\n>> integer is expected (e.g., \"--abbrev=\"), I have been replacing\n>> code like\n>> [...]\n> \n> IMHO most of the tightening happening here is a good thing, and means we\n> are more likely to notice mistakes rather than silently doing something\n> stupid.\n> \n> For sites that currently allow it, I could imagine people using hex\n> notation for some values, though, depending on the context. It looks\n> there aren't many of them ((it is just when the radix is \"0\", right?).\n> Some of them look to be accidental (does anybody really ask for\n> --threads=0x10?), but others might not be (the pack index-version\n> contains an offset field that might be quite big).\n\nYes, we can even debate whether we want to implement a general policy\nthat user-entered integers can be specified in any radix. Probably\nnobody will ever specify \"--threads=0x10\", but is there harm in allowing it?\n\nAgainst the gain in flexibility, I see the following potential\ndisadvantages:\n\n* Added cognitive burden for rarely-used flexibility.\n* Other Git clients might feel obliged to be just as flexible, causing\ntheir implementers extra work.\n* Users might see commands written in unfamiliar ways (e.g. in scripts\nor stackoverflow) and get confused.\n* Octal is ambiguous with decimal. What to make of \"--count=010\"? People\nwho expect octal numbers to be allowed will think it means 8, whereas\npeople who don't expect octal numbers to be allowed (or don't even know\nwhat an octal number is!) will think that it means 10. Such uses might\neven already exist and have their meaning changed if we start allowing\noctal.\n\nAll in all, I thought it is less error-prone to default to allowing only\ndecimal, except in selected situations where hex and octal are\ntraditionally used (e.g., file modes, memory limits).\n\nMost of the call sites so far have explicitly specified decimal parsing,\nand I have left them unchanged.\n\n> Feel free to ignore any or all of that. It is not so much criticism as\n> a dump of thoughts I had while reading the patches. Perhaps you can pick\n> something useful out of that, and perhaps not. :)\n\nThanks very much for the feedback!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"258418","messageId":"551185D9.6050200@alum.mit.edu","threadId":"38832","inReplyTo":"xmqq7fudld61.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-24T15:42:17Z","receivedAt":"2015-03-24T15:42:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/19/2015 07:22 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> * It allows leading whitespace.\n> \n> This might be blessing in disguise. Our friends on MacOS may be\n> relying on that\n> \n> git cmd --abbrev=\"$(wc -c \n> to work \"as expected\", even though their \"wc\" gives leading spaces,\n> for example.\n\nYuck. If you'd like, I can make sure to continue allowing leading\nwhitespace everywhere that it is allowed now. Alternatively, we could\nmake the parsing tighter and see if anybody squeals. What do you think?\n\n>> * It allows arbitrary trailing characters.\n> \n> Which is why we have introduced strtoul_ui() and such.\n> \n>> * It allows a leading sign character ('+' or '-'), even though the\n>> result is unsigned.\n> \n> I do not particularly see it a bad thing to accept \"--abbrev=+7\".\n> Using unsigned type to accept a width and parsing \"--abbrev=-7\" into\n> a large positive integer _is_ a problem, and we may want to fix it,\n> but I think that is still within the scope of the original \"better\n> strtol/strtoul\", and I do not see it as a justification to introduce\n> a set of functions with completely unfamiliar name.\n\nIt is easy to allow \"--abbrev=+7\"; I would just need to add NUM_PLUS to\nthose call sites. Should I do so?\n\nIt would even be possible to allow \"--abbrev=-0\", which is also a\nnon-negative integer. But it's more work and it seems pretty silly to me.\n\n>> * If the string doesn't contain a number at all, it sets its \"endptr\"\n>> argument to point at the start of the string but doesn't set errno.\n> \n> Why is that a problem? A caller that cares is supposed to check\n> endptr and the string it gave, no? Now, if strtoul_ui() and friends\n> do not do so, I again think that is still within the scope of the\n> original \"better strtol/strtoul\".\n\nThe text you are quoting was my argument for why we need wrappers around\nstrtol() and strtoul(), not for how the wrappers should be named.\n\nThe \"endptr\" convention for detecting errors is fine in theory, but in\npractice a large majority of callers didn't check for errors correctly.\nThis is partly because the \"endptr\" convention is so awkward.\n\nThe existing strtoul_ui() and strtol_i() did check endptr correctly, but\nthere were only int-sized variants of the functions, and they didn't\ngive the caller the chance to capture endptr if the caller wanted to\nallow trailing characters.\n\nWhy did I rename the wrapper functions?\n\n* The old names, I think, emphasize that they take the long-sized\nresults of strtou?l() and convert them to integer size, which I think is\nan implementation detail.\n* The new functions will also have long versions. The obvious way to\ngeneralize the existing function names for long variants (srtoul_ul()\nand strtol_l()) seem kindof redundant.\n* I wanted to change the signature and behavior of the functions, so\nknowledge of the existing functions wouldn't be super helpful in\nunderstanding the new ones.\n\n>> * If the value is larger than fits in an unsigned long, it returns the\n>> value clamped to the range 0..ULONG_MAX (setting errno to ERANGE).\n> \n> Ditto.\n> \n>> * If the value is between -ULONG_MAX and 0, it returns the positive\n>> integer with the same bit pattern, without setting errno(!) (I can\n>> hardly think of a scenario where this would be useful.)\n> \n> Ditto.\n> \n>> * For base=0 (autodetect base), it allows a base specifier prefix \"0x\"\n>> or \"0\" and parses the number accordingly. For base=16 it also allows\n>> a \"0x\" prefix.\n> \n> Why is it a problem to allow \"git cmd --hexval=0x1234\", even if \"git\n> cmd --hexval=1234\" would suffice?\n\nIn some cases we would like to allow that flexibility; in some cases\nnot. But the strtol()/strtoul() functions *always* allow it.\n\n>> When I looked around, I found scores of sites that call atoi(),\n>> strtoul(), and strtol() carelessly. And it's understandable. Calling\n>> any of these functions safely is so much work as to be completely\n>> impractical in day-to-day code.\n> \n> Yes, these burdens on the caller were exactly why strtoul_ui()\n> etc. wanted to reduce---and it will be a welcome change to do\n> necessary checks that are currently not done.\n> \n>> Please see the docstrings in numparse.h in the first commit for\n>> detailed API docs. Briefly, there are eight new functions:\n>>\n>> parse_{l,ul,i,ui}(const char *s, unsigned int flags,\n>> T *result, char **endptr);\n>> convert_{l,ul,i,ui}(const char *s, unsigned int flags, T *result);\n>>\n>> The parse_*() functions are for parsing a number from the start of a\n>> string; the convert_*() functions are for parsing a string that\n>> consists of a single number.\n> \n> I am not sure if I follow. Why not unify them into one 4-function\n> set, with optional endptr that could be NULL?\n\nIn the next version of this patch series I plan to include only four\nfunctions, str_to_{i,ui,l,ul}(), named that way to make them more\nreminiscent of the functions that they replace. They will take an entptr\nargument, but if that argument is set to NULL, then the function will\nerror out if there are any trailing characters.\n\n> While we are on the topic of improving number parsing, one thing\n> that I found somewhat frustrating is that config.c layer knows the\n> scaling suffix but option parsing layer does not. These functions\n> should offer an option (via their \"flags\") to say \"I allow scaled\n> numbers like 2k, 4M, etc.\".\n\nThat's a good idea. But I think I will leave that for a later patch\nseries, if that is OK with you.\n\nThanks for the great feedback!\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"258415","messageId":"xmqq619q8k0h.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"551185D9.6050200@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T15:58:38Z","receivedAt":"2015-03-24T15:58:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It is easy to allow \"--abbrev=+7\"; I would just need to add NUM_PLUS to\n> those call sites. Should I do so?\n\nThe more relevant question to ask from my point of view is why you\nneed to \"add\" NUM_PLUS to \"enable\" it. What valid reason do you\nhave to forbid it anywhere? Only because you do not accept it by\ndefault, you need to \"add\" to \"enable\".\n\n>> Why is it a problem to allow \"git cmd --hexval=0x1234\", even if \"git\n>> cmd --hexval=1234\" would suffice?\n>\n> In some cases we would like to allow that flexibility; in some cases\n> not. But the strtol()/strtoul() functions *always* allow it.\n\nThe same issue. Whare are these \"some cases\"?\n"},{"id":"258416","messageId":"55118B74.1030201@alum.mit.edu","threadId":"38832","inReplyTo":"xmqqk2ydjvcd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-24T16:06:12Z","receivedAt":"2015-03-24T16:06:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/19/2015 08:32 AM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> I wonder how much of the boilerplate in the parse_* functions could be\n>> factored out to use a uintmax_t, with the caller just providing the\n>> range. That would make it easier to add new types like off_t, and\n>> possibly even constrained types (e.g., an integer from 0 to 100). On the\n>> other hand, you mentioned to me elsewhere that there may be some bugs in\n>> the range-checks of config.c's integer parsing. I suspect they are\n>> related to exactly this kind of refactoring, so perhaps writing things\n>> out is best.\n> \n> I like this idea very well. I wonder if we can implement the family\n> of\n> \n> parse_{type}(const char *, unsigned int flags,\n> \t\t const char **endptr, {type} *result)\n> \n> functions by calls a helper that internally deals with the numbers\n> in uintmax_t, and then checks if the value fits within the possible\n> range of the *result before returning.\n> \n> int parse_i(const char *str, unsigned flags,\n> \t\tconst char **endptr, int *result) {\n> \tuintmax_t val;\n> int sign = 1, status;\n> if (*str == '-') {\n> \t\tsign = -1; \n> str++;\n> \t}\n> status = parse_num(str, flags, endptr, &val, INT_MAX);\n> if (status < 0)\n> \treturn status;\n> \t*result = sign < 0 ? -val : val;\n> return 0;\n> }\n> \n> (assuming the last parameter to parse_num is used to check if the\n> result fits within that range). Or that may be easier and cleaner\n> to be done in the caller with or something like that:\n> \n> \tstatus = parse_num(str, flags, endptr, &val);\n> if (status < 0)\n> \treturn status;\n> \tif (INT_MAX <= val * sign || val * sign <= INT_MIN) {\n> \terrno = ERANGE;\n> return -1;\n> \t}\n> \n> If we wanted to, we may even be able to avoid duplication of\n> boilerplate by wrapping the above pattern within a macro,\n> parameterizing the TYPE_{MIN,MAX} and using token pasting, to\n> expand to four necessary result types.\n> \n> There is no reason for the implementation of the parse_num() helper\n> to be using strtoul(3) or strtoull(3); its behaviour will be under\n> our total control. It can become fancier by enriching the flags\n> bits (e.g. allow scaling factor, etc.) only once and all variants\n> for various result types will get the same feature.\n\nParsing numbers is not rocket science, but there are a lot of pitfalls,\nespecially around overflow. It's even harder to write such code via\nmacros and the result is less readable.\n\nThis patch series is mostly about finding a reasonable API and whipping\nthe callers into shape. That seems ambitious enough for me. I'd rather\nstick with boring wrappers for now and lean on strtol()/strtoul() to do\nthe dirty work. It will be easy for a motivated person to change the\nimplementation later.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"258417","messageId":"xmqq1tke8jiy.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"xmqq619q8k0h.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T16:09:09Z","receivedAt":"2015-03-24T16:09:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Michael Haggerty writes:\n>\n>> It is easy to allow \"--abbrev=+7\"; I would just need to add NUM_PLUS to\n>> those call sites. Should I do so?\n>\n> The more relevant question to ask from my point of view is why you\n> need to \"add\" NUM_PLUS to \"enable\" it. What valid reason do you\n> have to forbid it anywhere? Only because you do not accept it by\n> default, you need to \"add\" to \"enable\".\n>\n>>> Why is it a problem to allow \"git cmd --hexval=0x1234\", even if \"git\n>>> cmd --hexval=1234\" would suffice?\n>>\n>> In some cases we would like to allow that flexibility; in some cases\n>> not. But the strtol()/strtoul() functions *always* allow it.\n>\n> The same issue. Whare are these \"some cases\"?\n\nAnd the same issue appears in the \"leading whitespace\" thing I did\nnot mention in the earlier part of your message I responded to. I\nalso notice you answered yourself that there may not be a valid\nreason to forbid end-user supplied \"0x\" prefix to arguments we\nexpect an integer for in your other message.\n\nIn short, if it is not a clearly bogus input that indicates a typo\nor something (e.g. \"--size=48l? did the user meant 48, 48k, or\n48m?\"), and if it is clear we can tell the user meant what the code\nwould naturally interpret as (e.g. \"--hexval=0x1234\"), why forbid\nit?\n"},{"id":"258421","messageId":"551195B6.9040402@web.de","threadId":"38832","inReplyTo":"55118B74.1030201@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-03-24T16:49:58Z","receivedAt":"2015-03-24T16:49:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.03.2015 um 17:06 schrieb Michael Haggerty:\n> Parsing numbers is not rocket science, but there are a lot of pitfalls,\n> especially around overflow. It's even harder to write such code via\n> macros and the result is less readable.\n>\n> This patch series is mostly about finding a reasonable API and whipping\n> the callers into shape. That seems ambitious enough for me. I'd rather\n> stick with boring wrappers for now and lean on strtol()/strtoul() to do\n> the dirty work. It will be easy for a motivated person to change the\n> implementation later.\n\nThe OpenBSD developers invented strtonum for that. Are you aware of it? \n Would it fit? This discussion may be useful:\n\n\thttp://www.tedunangst.com/flak/post/the-design-of-strtonum\n\nRené\n"},{"id":"258431","messageId":"5511A166.3090009@alum.mit.edu","threadId":"38832","inReplyTo":"xmqq619q8k0h.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-24T17:39:50Z","receivedAt":"2015-03-24T17:39:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/24/2015 04:58 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> It is easy to allow \"--abbrev=+7\"; I would just need to add NUM_PLUS to\n>> those call sites. Should I do so?\n> \n> The more relevant question to ask from my point of view is why you\n> need to \"add\" NUM_PLUS to \"enable\" it. What valid reason do you\n> have to forbid it anywhere? Only because you do not accept it by\n> default, you need to \"add\" to \"enable\".\n\nI want to be able to plunge into this project without first auditing all\ncall sites to see which features will turn out to be needed. So I'm\nerring on the side of flexibility. For now, I want to be able to\nprohibit '+' signs.\n\nCurrently all of the flags cause additional features to be enabled. My\nguess was that most callers *won't* need most features, so it seemed\neasiest and most consistent to have all features be turned off by\ndefault and let the caller add the features that it wants to allow.\n\nRegarding specifically allowing/disallowing a leading '+': I saw a\ncouple of callsites that explicitly check that the first character is a\ndigit before calling strtol(). I assumed that is to disallow sign\ncharacters [1]. For example,\n\n diff.c: optarg()\n builtin/apply.c: parse_num()\n maybe date.c: match_multi_number()\n\nThere are other callsites that call strtoul(), but store the result in a\nsigned variable. Those would presumably not want to allow leading '-',\nbut I'm not sure.\n\nI also imagine that there are places in protocols or file formats where\nsigns should not be allowed (e.g., timestamps in commits?).\n\n>>> Why is it a problem to allow \"git cmd --hexval=0x1234\", even if \"git\n>>> cmd --hexval=1234\" would suffice?\n>>\n>> In some cases we would like to allow that flexibility; in some cases\n>> not. But the strtol()/strtoul() functions *always* allow it.\n> \n> The same issue. Whare are these \"some cases\"?\n\nI admit I'm not sure there are such places for hexadecimal numbers.\n\nI'm coming around to an alternate plan:\n\nStep 1: write a NUM_DEFAULT combination-of-options that gives the new\nfunctions behavior very like strtol()/strtoul() but without their insane\nfeatures.\n\nStep 2: rewrite all callers to use that option (and usually endptr=NULL,\nmeaning no trailing characters) unless it is manifestly clear that they\nare already trying to forbid some other features. This will already\nproduce the largest benefit: avoiding overflows, missing error checking,\netc.\n\nSteps 3 through ∞: as time allows, rewrite individual callsites to be\nstricter where appropriate.\n\nHopefully steps 1 and 2 will not be too controversial.\n\nMichael\n\n[1] That assumption is based on a rather quick look over the code,\nbecause with well over 100 callsites, it is not practical to study each\ncallsite carefully.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"258439","messageId":"xmqq1tke6zfv.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"5511A166.3090009@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T18:08:20Z","receivedAt":"2015-03-24T18:08:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Regarding specifically allowing/disallowing a leading '+': I saw a\n> couple of callsites that explicitly check that the first character is a\n> digit before calling strtol(). I assumed that is to disallow sign\n> characters [1]. For example,\n>\n> diff.c: optarg()\n\nThis one I know is from \"if not a digit we know it is not a number\";\nit is not an attempt to say \"we must forbid numbers to be spelled\nwith '+'\", but more about \"we do not need it and this is easier to\ncode without a full fledged str-to-num helper API\" sloppiness.\n\n> builtin/apply.c: parse_num()\n\nThis parses \"@@ -l,k +m,n @@@\" after stripping the punctuation\naround the numbers, so this is a valid reason why you would want an\noptional feature \"CANNOT_HAVE_SIGN\" in the str-to-num parser and use\nit.\n\n> maybe date.c: match_multi_number()\n\nThe approxidate callers parse random garbage input and attempt to\nmake best sense out of it, so they tend to try limitting the damage\ncaused by incorrect guesses by insisting \"we do not consider +0 to\nbe zero\" and such. So this is a valid reason why you would want an\noptional feature \"CANNOT_HAVE_SIGN\" in the str-to-num parser and use\nit.\n\n> I'm coming around to an alternate plan:\n>\n> Step 1: write a NUM_DEFAULT combination-of-options that gives the new\n> functions behavior very like strtol()/strtoul() but without their insane\n> features.\n>\n> Step 2: rewrite all callers to use that option (and usually endptr=NULL,\n> meaning no trailing characters) unless it is manifestly clear that they\n> are already trying to forbid some other features. This will already\n> produce the largest benefit: avoiding overflows, missing error checking,\n> etc.\n>\n> Steps 3 through ∞: as time allows, rewrite individual callsites to be\n> stricter where appropriate.\n>\n> Hopefully steps 1 and 2 will not be too controversial.\n\nAll sounds sensible to me.\n"},{"id":"258524","messageId":"5513252A.7050601@alum.mit.edu","threadId":"38832","inReplyTo":"551195B6.9040402@web.de","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-03-25T21:14:18Z","receivedAt":"2015-03-25T21:14:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/24/2015 05:49 PM, René Scharfe wrote:\n> Am 24.03.2015 um 17:06 schrieb Michael Haggerty:\n>> Parsing numbers is not rocket science, but there are a lot of pitfalls,\n>> especially around overflow. It's even harder to write such code via\n>> macros and the result is less readable.\n>>\n>> This patch series is mostly about finding a reasonable API and whipping\n>> the callers into shape. That seems ambitious enough for me. I'd rather\n>> stick with boring wrappers for now and lean on strtol()/strtoul() to do\n>> the dirty work. It will be easy for a motivated person to change the\n>> implementation later.\n> \n> The OpenBSD developers invented strtonum for that. Are you aware of it?\n> Would it fit? This discussion may be useful:\n> \n> http://www.tedunangst.com/flak/post/the-design-of-strtonum\n\nI wasn't aware of strtonum; thanks for the reference. It has an\nuntraditional interface. Their willingness to sacrifice half of the\nunsigned range and requirement that the user specify minval and maxval\nhave the nice effect of permitting one function to be used for all\ninteger types.\n\nI think git will need more flexibility, for example to support other\nradixes and to allow trailing characters. So personally I don't think we\nshould use (or imitate) strtonum().\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"258527","messageId":"xmqqegoc20y8.fsf@gitster.dls.corp.google.com","threadId":"38832","inReplyTo":"5513252A.7050601@alum.mit.edu","subject":"Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-25T21:59:11Z","receivedAt":"2015-03-25T21:59:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n[jc: dropped a non-working address from Cc list]\n\n> I wasn't aware of strtonum; thanks for the reference. It has an\n> untraditional interface. Their willingness to sacrifice half of the\n> unsigned range and requirement that the user specify minval and maxval\n> have the nice effect of permitting one function to be used for all\n> integer types.\n> ...\n> I think git will need more flexibility, for example to support other\n> radixes and to allow trailing characters. So personally I don't think we\n> should use (or imitate) strtonum().\n\nI had the same impression. An earlier suggestion by Peff about\nuintmax_t may be something we might want to keep in mind; we might\nwant to use strtoll/strtoull as the underlying implementation if we\nwanted to avoid rolling our own.\n\nThanks.\n"}]}