{"thread":{"id":"41293","subject":"[PATCH 0/6] post-strbuf_getline cleanups","startedAt":"2016-01-31T11:22:16Z","lastAt":"2016-02-02T05:29:41Z","messageCount":19,"participants":["Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"277105","messageId":"20160131112215.GA4589@sigill.intra.peff.net","threadId":"41293","inReplyTo":null,"subject":"[PATCH 0/6] post-strbuf_getline cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-31T11:22:16Z","receivedAt":"2016-01-31T11:22:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As promised, here are the \"how about this on top\" patches that came out\nof the discussion for the \"strbuf_getline\" series in:\n\n  http://thread.gmane.org/gmane.comp.version-control.msysgit/21773/focus=284001\n\nAs I looked further at some of the option-parsing cleanups, I realized\nthat some of them are more than cleanups; we're actually fixing bizarre\nbehavior and segfaults.\n\n  [1/6]: give \"nbuf\" strbuf a more meaningful name\n  [2/6]: checkout-index: simplify \"-z\" option parsing\n  [3/6]: checkout-index: handle \"--no-prefix\" option\n  [4/6]: checkout-index: handle \"--no-index\" option\n  [5/6]: checkout-index: disallow \"--no-stage\" option\n  [6/6]: apply, ls-files: simplify \"-z\" parsing\n\n-Peff\n"},{"id":"277106","messageId":"20160131112526.GA5116@sigill.intra.peff.net","threadId":"41293","inReplyTo":"20160131112215.GA4589@sigill.intra.peff.net","subject":"[PATCH 1/6] give \"nbuf\" strbuf a more meaningful name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-31T11:25:26Z","receivedAt":"2016-01-31T11:25:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's a common pattern in our code to read paths from stdin,\nseparated either by newlines or NULs, and unquote as\nnecessary. In each of these five cases we use \"nbuf\" to\ntemporarily store the unquoted value. Let's give it the more\nmeaningful name \"unquoted\", which makes it easier to\nunderstand the purpose of the variable.\n\nWhile we're at it, let's also static-initialize all of our\nstrbufs. It's not wrong to call strbuf_init, but it\nincreases the cognitive load on the reader, who might wonder\n\"do we sometimes avoid initializing them?  why?\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nFor those seeing this patch for the first time, I'll copy my\nregrets from the earlier posting:\n\n> It's a shame that we can't just factor out this common\n> code, but I don't think it's quite long enough to merit\n> the boilerplate. The interesting part of each function\n> happens inside the loop. If C had lambdas, we could do\n> something like:\n> \n>   foreach_path_from(stdin, nul_term_line) {\n>         /* now do something interesting with \"buf\"\n>            and some other local variables */\n>   }\n>\n> but using function pointers for this kind of iteration is\n> pretty awful (define the two-line loop body as a separate\n> function, stuff any local variables into a struct and pass\n> it as \"void *\", etc). You can overcome that with macros\n> and make the above code work if you don't mind creating an\n> undebuggable mess. :)\n\n builtin/check-attr.c     | 13 ++++++-------\n builtin/check-ignore.c   | 13 ++++++-------\n builtin/checkout-index.c | 11 ++++++-----\n builtin/hash-object.c    | 11 ++++++-----\n builtin/update-index.c   | 11 ++++++-----\n 5 files changed, 30 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 087325e..53a5a18 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -72,24 +72,23 @@ static void check_attr(const char *prefix, int cnt,\n static void check_attr_stdin_paths(const char *prefix, int cnt,\n \tstruct git_attr_check *check)\n {\n-\tstruct strbuf buf, nbuf;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf unquoted = STRBUF_INIT;\n \tstrbuf_getline_fn getline_fn;\n \n \tgetline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;\n-\tstrbuf_init(&buf, 0);\n-\tstrbuf_init(&nbuf, 0);\n \twhile (getline_fn(&buf, stdin) != EOF) {\n \t\tif (!nul_term_line && buf.buf[0] == '\"') {\n-\t\t\tstrbuf_reset(&nbuf);\n-\t\t\tif (unquote_c_style(&nbuf, buf.buf, NULL))\n+\t\t\tstrbuf_reset(&unquoted);\n+\t\t\tif (unquote_c_style(&unquoted, buf.buf, NULL))\n \t\t\t\tdie(\"line is badly quoted\");\n-\t\t\tstrbuf_swap(&buf, &nbuf);\n+\t\t\tstrbuf_swap(&buf, &unquoted);\n \t\t}\n \t\tcheck_attr(prefix, cnt, check, buf.buf);\n \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n \t}\n \tstrbuf_release(&buf);\n-\tstrbuf_release(&nbuf);\n+\tstrbuf_release(&unquoted);\n }\n \n static NORETURN void error_with_usage(const char *msg)\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 4f0b09e..1d73d3c 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -115,20 +115,19 @@ static int check_ignore(struct dir_struct *dir,\n \n static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)\n {\n-\tstruct strbuf buf, nbuf;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf unquoted = STRBUF_INIT;\n \tchar *pathspec[2] = { NULL, NULL };\n \tstrbuf_getline_fn getline_fn;\n \tint num_ignored = 0;\n \n \tgetline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;\n-\tstrbuf_init(&buf, 0);\n-\tstrbuf_init(&nbuf, 0);\n \twhile (getline_fn(&buf, stdin) != EOF) {\n \t\tif (!nul_term_line && buf.buf[0] == '\"') {\n-\t\t\tstrbuf_reset(&nbuf);\n-\t\t\tif (unquote_c_style(&nbuf, buf.buf, NULL))\n+\t\t\tstrbuf_reset(&unquoted);\n+\t\t\tif (unquote_c_style(&unquoted, buf.buf, NULL))\n \t\t\t\tdie(\"line is badly quoted\");\n-\t\t\tstrbuf_swap(&buf, &nbuf);\n+\t\t\tstrbuf_swap(&buf, &unquoted);\n \t\t}\n \t\tpathspec[0] = buf.buf;\n \t\tnum_ignored += check_ignore(dir, prefix,\n@@ -136,7 +135,7 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)\n \t\tmaybe_flush_or_die(stdout, \"check-ignore to stdout\");\n \t}\n \tstrbuf_release(&buf);\n-\tstrbuf_release(&nbuf);\n+\tstrbuf_release(&unquoted);\n \treturn num_ignored;\n }\n \ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex ed888a5..d8d7bd3 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -251,7 +251,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (read_from_stdin) {\n-\t\tstruct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstruct strbuf unquoted = STRBUF_INIT;\n \t\tstrbuf_getline_fn getline_fn;\n \n \t\tif (all)\n@@ -261,16 +262,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\twhile (getline_fn(&buf, stdin) != EOF) {\n \t\t\tchar *p;\n \t\t\tif (!nul_term_line && buf.buf[0] == '\"') {\n-\t\t\t\tstrbuf_reset(&nbuf);\n-\t\t\t\tif (unquote_c_style(&nbuf, buf.buf, NULL))\n+\t\t\t\tstrbuf_reset(&unquoted);\n+\t\t\t\tif (unquote_c_style(&unquoted, buf.buf, NULL))\n \t\t\t\t\tdie(\"line is badly quoted\");\n-\t\t\t\tstrbuf_swap(&buf, &nbuf);\n+\t\t\t\tstrbuf_swap(&buf, &unquoted);\n \t\t\t}\n \t\t\tp = prefix_path(prefix, prefix_length, buf.buf);\n \t\t\tcheckout_file(p, prefix);\n \t\t\tfree(p);\n \t\t}\n-\t\tstrbuf_release(&nbuf);\n+\t\tstrbuf_release(&unquoted);\n \t\tstrbuf_release(&buf);\n \t}\n \ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 3bc5ec1..d3cb4e5 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -58,20 +58,21 @@ static void hash_object(const char *path, const char *type, const char *vpath,\n static void hash_stdin_paths(const char *type, int no_filters, unsigned flags,\n \t\t\t     int literally)\n {\n-\tstruct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf unquoted = STRBUF_INIT;\n \n \twhile (strbuf_getline_lf(&buf, stdin) != EOF) {\n \t\tif (buf.buf[0] == '\"') {\n-\t\t\tstrbuf_reset(&nbuf);\n-\t\t\tif (unquote_c_style(&nbuf, buf.buf, NULL))\n+\t\t\tstrbuf_reset(&unquoted);\n+\t\t\tif (unquote_c_style(&unquoted, buf.buf, NULL))\n \t\t\t\tdie(\"line is badly quoted\");\n-\t\t\tstrbuf_swap(&buf, &nbuf);\n+\t\t\tstrbuf_swap(&buf, &unquoted);\n \t\t}\n \t\thash_object(buf.buf, type, no_filters ? NULL : buf.buf, flags,\n \t\t\t    literally);\n \t}\n \tstrbuf_release(&buf);\n-\tstrbuf_release(&nbuf);\n+\tstrbuf_release(&unquoted);\n }\n \n int cmd_hash_object(int argc, const char **argv, const char *prefix)\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 7c5c143..f052faf 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1075,16 +1075,17 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (read_from_stdin) {\n-\t\tstruct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstruct strbuf unquoted = STRBUF_INIT;\n \n \t\tsetup_work_tree();\n \t\twhile (getline_fn(&buf, stdin) != EOF) {\n \t\t\tchar *p;\n \t\t\tif (!nul_term_line && buf.buf[0] == '\"') {\n-\t\t\t\tstrbuf_reset(&nbuf);\n-\t\t\t\tif (unquote_c_style(&nbuf, buf.buf, NULL))\n+\t\t\t\tstrbuf_reset(&unquoted);\n+\t\t\t\tif (unquote_c_style(&unquoted, buf.buf, NULL))\n \t\t\t\t\tdie(\"line is badly quoted\");\n-\t\t\t\tstrbuf_swap(&buf, &nbuf);\n+\t\t\t\tstrbuf_swap(&buf, &unquoted);\n \t\t\t}\n \t\t\tp = prefix_path(prefix, prefix_length, buf.buf);\n \t\t\tupdate_one(p);\n@@ -1092,7 +1093,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\t\tchmod_path(set_executable_bit, p);\n \t\t\tfree(p);\n \t\t}\n-\t\tstrbuf_release(&nbuf);\n+\t\tstrbuf_release(&unquoted);\n \t\tstrbuf_release(&buf);\n \t}\n \n-- \n2.7.0.489.g6faad84\n"},{"id":"277107","messageId":"20160131112543.GB5116@sigill.intra.peff.net","threadId":"41293","inReplyTo":"20160131112215.GA4589@sigill.intra.peff.net","subject":"[PATCH 2/6] checkout-index: simplify \"-z\" option parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-31T11:25:43Z","receivedAt":"2016-01-31T11:25:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that we act as a simple bool, there's no need to use a\ncustom callback.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout-index.c | 12 ++----------\n 1 file changed, 2 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex d8d7bd3..3b913d1 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -142,13 +142,6 @@ static int option_parse_u(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_z(const struct option *opt,\n-\t\t\t  const char *arg, int unset)\n-{\n-\tnul_term_line = !unset;\n-\treturn 0;\n-}\n-\n static int option_parse_prefix(const struct option *opt,\n \t\t\t       const char *arg, int unset)\n {\n@@ -192,9 +185,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK, 'u', \"index\", &newfd, NULL,\n \t\t\tN_(\"update stat information in the index file\"),\n \t\t\tPARSE_OPT_NOARG, option_parse_u },\n-\t\t{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,\n-\t\t\tN_(\"paths are separated with NUL character\"),\n-\t\t\tPARSE_OPT_NOARG, option_parse_z },\n+\t\tOPT_BOOL('z', NULL, &nul_term_line,\n+\t\t\tN_(\"paths are separated with NUL character\")),\n \t\tOPT_BOOL(0, \"stdin\", &read_from_stdin,\n \t\t\tN_(\"read list of paths from the standard input\")),\n \t\tOPT_BOOL(0, \"temp\", &to_tempfile,\n-- \n2.7.0.489.g6faad84\n"},{"id":"277108","messageId":"20160131112615.GC5116@sigill.intra.peff.net","threadId":"41293","inReplyTo":"20160131112215.GA4589@sigill.intra.peff.net","subject":"[PATCH 3/6] checkout-index: handle \"--no-prefix\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-31T11:26:16Z","receivedAt":"2016-01-31T11:26:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use a custom callback to parse \"--prefix\", but it does\nnot handle the \"unset\" case. As a result, passing\n\"--no-prefix\" will cause a segfault.\n\nWe can fix this by switching it to an OPT_STRING, which\nmakes \"--no-prefix\" counteract a previous \"--prefix\". Note\nthat this assigns NULL, so we bump our default-case\ninitialization to lower in the main function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout-index.c | 18 ++++++------------\n 1 file changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 3b913d1..43bedde 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -142,14 +142,6 @@ static int option_parse_u(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_prefix(const struct option *opt,\n-\t\t\t       const char *arg, int unset)\n-{\n-\tstate.base_dir = arg;\n-\tstate.base_dir_len = strlen(arg);\n-\treturn 0;\n-}\n-\n static int option_parse_stage(const struct option *opt,\n \t\t\t      const char *arg, int unset)\n {\n@@ -191,9 +183,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"read list of paths from the standard input\")),\n \t\tOPT_BOOL(0, \"temp\", &to_tempfile,\n \t\t\tN_(\"write the content to temporary files\")),\n-\t\tOPT_CALLBACK(0, \"prefix\", NULL, N_(\"string\"),\n-\t\t\tN_(\"when creating files, prepend <string>\"),\n-\t\t\toption_parse_prefix),\n+\t\tOPT_STRING(0, \"prefix\", &state.base_dir, N_(\"string\"),\n+\t\t\tN_(\"when creating files, prepend <string>\")),\n \t\tOPT_CALLBACK(0, \"stage\", NULL, NULL,\n \t\t\tN_(\"copy out the files from named stage\"),\n \t\t\toption_parse_stage),\n@@ -204,7 +195,6 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_checkout_index_usage,\n \t\t\t\t   builtin_checkout_index_options);\n \tgit_config(git_default_config, NULL);\n-\tstate.base_dir = \"\";\n \tprefix_length = prefix ? strlen(prefix) : 0;\n \n \tif (read_cache() < 0) {\n@@ -217,6 +207,10 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tstate.quiet = quiet;\n \tstate.not_new = not_new;\n \n+\tif (!state.base_dir)\n+\t\tstate.base_dir = \"\";\n+\tstate.base_dir_len = strlen(state.base_dir);\n+\n \tif (state.base_dir_len || to_tempfile) {\n \t\t/* when --prefix is specified we do not\n \t\t * want to update cache.\n-- \n2.7.0.489.g6faad84\n"},{"id":"277109","messageId":"20160131112936.GD5116@sigill.intra.peff.net","threadId":"41293","inReplyTo":"20160131112215.GA4589@sigill.intra.peff.net","subject":"[PATCH 4/6] checkout-index: handle \"--no-index\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-31T11:29:36Z","receivedAt":"2016-01-31T11:29:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parsing of \"--index\" is done in a callback, but it does\nnot handle an \"unset\" option. We don't necessarily expect\nanyone to use this, but the current behavior is to treat it\nexactly like \"--index\", which would probably be surprising.\n\nInstead, let's just turn it into an OPT_BOOL, and handle it\nafter we're done parsing. This makes \"--no-index\" just work\n(it cancels a previous \"--index\").\n\nAs a bonus, this makes the logic easier to follow. The old\ncode opened the index during the option parsing, leaving the\nreader to wonder if there was some timing issue (there\nisn't; none of the other options care that we've opened it).\nAnd then if we found that \"--prefix\" had been given, we had\nto rollback the index. Now we can simply avoid opening it in\nthe first place.\n\nNote that it might make more sense for checkout-index to\ncomplain when \"--index --prefix=foo\" is given (rather than\nsilently ignoring \"--index\"), but since it has been that way\nsince 415e96c ([PATCH] Implement git-checkout-cache -u to\nupdate stat information in the cache., 2005-05-15), it's\nsafer to leave it as-is.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI also reformatted the comment that violated our style\nguidelines, but I am not sure if it is all that helpful. It\nseems we also cancel \"--index\" with \"--temp\" or\n\"--stage=all\", but I do not know why. I left the content\nas-is, but if somebody knows enough to elaborate, it might\nbe worth doing.\n\n builtin/checkout-index.c | 34 ++++++++++------------------------\n 1 file changed, 10 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 43bedde..f8179a7 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -130,18 +130,6 @@ static const char * const builtin_checkout_index_usage[] = {\n \n static struct lock_file lock_file;\n \n-static int option_parse_u(const struct option *opt,\n-\t\t\t      const char *arg, int unset)\n-{\n-\tint *newfd = opt->value;\n-\n-\tstate.refresh_cache = 1;\n-\tstate.istate = &the_index;\n-\tif (*newfd < 0)\n-\t\t*newfd = hold_locked_index(&lock_file, 1);\n-\treturn 0;\n-}\n-\n static int option_parse_stage(const struct option *opt,\n \t\t\t      const char *arg, int unset)\n {\n@@ -166,6 +154,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tint read_from_stdin = 0;\n \tint prefix_length;\n \tint force = 0, quiet = 0, not_new = 0;\n+\tint index_opt = 0;\n \tstruct option builtin_checkout_index_options[] = {\n \t\tOPT_BOOL('a', \"all\", &all,\n \t\t\tN_(\"check out all files in the index\")),\n@@ -174,9 +163,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"no warning for existing files and files not in index\")),\n \t\tOPT_BOOL('n', \"no-create\", &not_new,\n \t\t\tN_(\"don't checkout new files\")),\n-\t\t{ OPTION_CALLBACK, 'u', \"index\", &newfd, NULL,\n-\t\t\tN_(\"update stat information in the index file\"),\n-\t\t\tPARSE_OPT_NOARG, option_parse_u },\n+\t\tOPT_BOOL('u', \"index\", &index_opt,\n+\t\t\t N_(\"update stat information in the index file\")),\n \t\tOPT_BOOL('z', NULL, &nul_term_line,\n \t\t\tN_(\"paths are separated with NUL character\")),\n \t\tOPT_BOOL(0, \"stdin\", &read_from_stdin,\n@@ -211,15 +199,13 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tstate.base_dir = \"\";\n \tstate.base_dir_len = strlen(state.base_dir);\n \n-\tif (state.base_dir_len || to_tempfile) {\n-\t\t/* when --prefix is specified we do not\n-\t\t * want to update cache.\n-\t\t */\n-\t\tif (state.refresh_cache) {\n-\t\t\trollback_lock_file(&lock_file);\n-\t\t\tnewfd = -1;\n-\t\t}\n-\t\tstate.refresh_cache = 0;\n+\t/*\n+\t * when --prefix is specified we do not want to update cache.\n+\t */\n+\tif (index_opt && !state.base_dir_len && !to_tempfile) {\n+\t\tstate.refresh_cache = 1;\n+\t\tstate.istate = &the_index;\n+\t\tnewfd = hold_locked_index(&lock_file, 1);\n \t}\n \n \t/* Check out named files first */\n-- \n2.7.0.489.g6faad84\n"},{"id":"277110","messageId":"20160131113029.GE5116@sigill.intra.peff.net","threadId":"41293","inReplyTo":"20160131112215.GA4589@sigill.intra.peff.net","subject":"[PATCH 5/6] checkout-index: disallow \"--no-stage\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-31T11:30:30Z","receivedAt":"2016-01-31T11:30:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not really expect people to use \"--no-stage\", but if\nthey do, git currently segfaults. We could instead have it\nundo the effects of a previous \"--stage\", but this gets\ntricky around the \"to_tempfile\" flag. We cannot simply reset\nit to 0, because we don't know if it was set by a previous\n\"--stage=all\" or an explicit \"--temp\" option.\n\nWe could solve this by setting a flag and resolving\nto_tempfile later, but it's not worth the effort. Nobody\nactually wants to use \"--no-stage\"; we are just trying to\nfix a potential segfault here.\n\nWhile we're in the area adding a translated string, let's\nmark the other possible error message in this function as\ntranslatable.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout-index.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex f8179a7..7a9b561 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -133,6 +133,8 @@ static struct lock_file lock_file;\n static int option_parse_stage(const struct option *opt,\n \t\t\t      const char *arg, int unset)\n {\n+\tif (unset)\n+\t\treturn error(_(\"--stage cannot be negated\"));\n \tif (!strcmp(arg, \"all\")) {\n \t\tto_tempfile = 1;\n \t\tcheckout_stage = CHECKOUT_ALL;\n@@ -141,7 +143,7 @@ static int option_parse_stage(const struct option *opt,\n \t\tif ('1' <= ch && ch <= '3')\n \t\t\tcheckout_stage = arg[0] - '0';\n \t\telse\n-\t\t\tdie(\"stage should be between 1 and 3 or all\");\n+\t\t\tdie(_(\"stage should be between 1 and 3 or all\"));\n \t}\n \treturn 0;\n }\n-- \n2.7.0.489.g6faad84\n"},{"id":"277111","messageId":"20160131113546.GF5116@sigill.intra.peff.net","threadId":"41293","inReplyTo":"20160131112215.GA4589@sigill.intra.peff.net","subject":"[PATCH 6/6] apply, ls-files: simplify \"-z\" parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-31T11:35:46Z","receivedAt":"2016-01-31T11:35:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As a short option, we cannot handle negation. Thus a\ncallback handling \"unset\" is overkill, and we can just use\nOPT_SET_INT instead to handle setting the option.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI left this one for last, because it's the most questionable. Unlike the\nprevious \"-z\" case, we're setting the actual character, so the logic is\ninverted: turning on the option sets it to 0, and turning it off restore\n'\\n'.\n\nThis means OPT_SET_INT would do the wrong thing for the \"unset\" case, as\nit would put a \"0\" into the option. You can't trigger that now, but if\nsomebody were to add a long option (e.g., \"--nul\"), then \"--no-nul\"\nwould do the wrong thing.\n\nI'm on the fence on whether the simplification is worth it, or if we\nshould leave the callbacks as future-proofing.\n\n builtin/apply.c    | 15 ++-------------\n builtin/ls-files.c | 13 ++-----------\n 2 files changed, 4 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex deb1364..565f3fd 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -4464,16 +4464,6 @@ static int option_parse_p(const struct option *opt,\n \treturn 0;\n }\n \n-static int option_parse_z(const struct option *opt,\n-\t\t\t  const char *arg, int unset)\n-{\n-\tif (unset)\n-\t\tline_termination = '\\n';\n-\telse\n-\t\tline_termination = 0;\n-\treturn 0;\n-}\n-\n static int option_parse_space_change(const struct option *opt,\n \t\t\t  const char *arg, int unset)\n {\n@@ -4546,9 +4536,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\t N_( \"attempt three-way merge if a patch does not apply\")),\n \t\tOPT_FILENAME(0, \"build-fake-ancestor\", &fake_ancestor,\n \t\t\tN_(\"build a temporary index based on embedded index information\")),\n-\t\t{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,\n-\t\t\tN_(\"paths are separated with NUL character\"),\n-\t\t\tPARSE_OPT_NOARG, option_parse_z },\n+\t\tOPT_SET_INT('z', NULL, &line_termination,\n+\t\t\tN_(\"paths are separated with NUL character\"), '\\0'),\n \t\tOPT_INTEGER('C', NULL, &p_context,\n \t\t\t\tN_(\"ensure at least <n> lines of context match\")),\n \t\t{ OPTION_CALLBACK, 0, \"whitespace\", &whitespace_option, N_(\"action\"),\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex b6a7cb0..59bad9b 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -359,14 +359,6 @@ static const char * const ls_files_usage[] = {\n \tNULL\n };\n \n-static int option_parse_z(const struct option *opt,\n-\t\t\t  const char *arg, int unset)\n-{\n-\tline_terminator = unset ? '\\n' : '\\0';\n-\n-\treturn 0;\n-}\n-\n static int option_parse_exclude(const struct option *opt,\n \t\t\t\tconst char *arg, int unset)\n {\n@@ -408,9 +400,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tstruct exclude_list *el;\n \tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n \tstruct option builtin_ls_files_options[] = {\n-\t\t{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,\n-\t\t\tN_(\"paths are separated with NUL character\"),\n-\t\t\tPARSE_OPT_NOARG, option_parse_z },\n+\t\tOPT_SET_INT('z', NULL, &line_terminator,\n+\t\t\tN_(\"paths are separated with NUL character\"), '\\0'),\n \t\tOPT_BOOL('t', NULL, &show_tag,\n \t\t\tN_(\"identify the file status with tags\")),\n \t\tOPT_BOOL('v', NULL, &show_valid_bit,\n-- \n2.7.0.489.g6faad84\n"},{"id":"277113","messageId":"alpine.DEB.2.20.1601311245170.2964@virtualbox","threadId":"41293","inReplyTo":"20160131112526.GA5116@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] give \"nbuf\" strbuf a more meaningful name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-31T11:54:29Z","receivedAt":"2016-01-31T11:54:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sun, 31 Jan 2016, Jeff King wrote:\n\n> > It's a shame that we can't just factor out this common\n> > code, but I don't think it's quite long enough to merit\n> > the boilerplate. The interesting part of each function\n> > happens inside the loop. If C had lambdas, we could do\n> > something like:\n> > \n> >   foreach_path_from(stdin, nul_term_line) {\n> >         /* now do something interesting with \"buf\"\n> >            and some other local variables */\n> >   }\n\nTechnically, we do not have to do lambdas for that paradigm, we could\nintroduce a new data type and a reader, i.e. something like this:\n\nstruct path_reader {\n\tFILE *in;\n\tint nul_term_line;\n\tstruct strbuf path;\n};\n#define PATH_READER_INIT { NULL, STRBUF_INIT };\n\nint read_next_path(struct path_reader *reader, FILE *in, int nul_term_line)\n{\n\tif (!reader->in) {\n\t\t... [initialize] ...\n\t}\n\n\t... [read and possibly unquote path] ...\n}\n\nvoid cleanup_path_reader(struct path_reader *reader)\n{\n\tif (reader->in) {\n\t\tfclose(reader->in);\n\t\treader->in = NULL;\n\t}\n\n\tstrbuf_release(&reader->buf);\n}\n\nAnd then the repeated code could be replaced by something like this:\n\n\tstruct path_reader path_reader = PATH_READER_INIT;\n\n\twhile (read_next_path(&reader, stdin, 1)) {\n\t\t... [work with reader->path.buf] ...\n\t}\n\n\tcleanup_path_reader();\n\nProbably this is actually not limited to path names, so the functions\nshould be renamed...\n\n(totally untested, of course...)\n\nCiao,\nDscho\n"},{"id":"277115","messageId":"alpine.DEB.2.20.1601311259090.2964@virtualbox","threadId":"41293","inReplyTo":"20160131113546.GF5116@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] apply, ls-files: simplify \"-z\" parsing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-31T11:59:46Z","receivedAt":"2016-01-31T11:59:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sun, 31 Jan 2016, Jeff King wrote:\n\n> I left this one for last, because it's the most questionable.\n\nI like the simplification... ;-)\n\nThe other patches in this series look fine to me.\n\nCiao,\nDscho\n"},{"id":"277114","messageId":"20160131115947.GA5438@sigill.intra.peff.net","threadId":"41293","inReplyTo":"alpine.DEB.2.20.1601311245170.2964@virtualbox","subject":"Re: [PATCH 1/6] give \"nbuf\" strbuf a more meaningful name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-31T11:59:47Z","receivedAt":"2016-01-31T11:59:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 31, 2016 at 12:54:29PM +0100, Johannes Schindelin wrote:\n\n> Hi Peff,\n> \n> On Sun, 31 Jan 2016, Jeff King wrote:\n> \n> > > It's a shame that we can't just factor out this common\n> > > code, but I don't think it's quite long enough to merit\n> > > the boilerplate. The interesting part of each function\n> > > happens inside the loop. If C had lambdas, we could do\n> > > something like:\n> > > \n> > >   foreach_path_from(stdin, nul_term_line) {\n> > >         /* now do something interesting with \"buf\"\n> > >            and some other local variables */\n> > >   }\n> \n> Technically, we do not have to do lambdas for that paradigm, we could\n> introduce a new data type and a reader, i.e. something like this:\n> [...]\n> And then the repeated code could be replaced by something like this:\n> \n> \tstruct path_reader path_reader = PATH_READER_INIT;\n> \n> \twhile (read_next_path(&reader, stdin, 1)) {\n> \t\t... [work with reader->path.buf] ...\n> \t}\n> \n> \tcleanup_path_reader();\n\nYeah, you're right. I was thinking of lifting the loop completely out of\nthe call-sites, but simplifying it to a single line loop condition is\njust as good.\n\nI still think this crosses my line of \"too much boilerplate to be worth\nit\", though.\n\n-Peff\n"},{"id":"277116","messageId":"alpine.DEB.2.20.1601311300400.2964@virtualbox","threadId":"41293","inReplyTo":"20160131115947.GA5438@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] give \"nbuf\" strbuf a more meaningful name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-31T12:01:02Z","receivedAt":"2016-01-31T12:01:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sun, 31 Jan 2016, Jeff King wrote:\n\n> On Sun, Jan 31, 2016 at 12:54:29PM +0100, Johannes Schindelin wrote:\n> \n> > On Sun, 31 Jan 2016, Jeff King wrote:\n> > \n> > > > It's a shame that we can't just factor out this common\n> > > > code, but I don't think it's quite long enough to merit\n> > > > the boilerplate. The interesting part of each function\n> > > > happens inside the loop. If C had lambdas, we could do\n> > > > something like:\n> > > > \n> > > >   foreach_path_from(stdin, nul_term_line) {\n> > > >         /* now do something interesting with \"buf\"\n> > > >            and some other local variables */\n> > > >   }\n> > \n> > Technically, we do not have to do lambdas for that paradigm, we could\n> > introduce a new data type and a reader, i.e. something like this:\n> > [...]\n> > And then the repeated code could be replaced by something like this:\n> > \n> > \tstruct path_reader path_reader = PATH_READER_INIT;\n> > \n> > \twhile (read_next_path(&reader, stdin, 1)) {\n> > \t\t... [work with reader->path.buf] ...\n> > \t}\n> > \n> > \tcleanup_path_reader();\n> \n> Yeah, you're right. I was thinking of lifting the loop completely out of\n> the call-sites, but simplifying it to a single line loop condition is\n> just as good.\n> \n> I still think this crosses my line of \"too much boilerplate to be worth\n> it\", though.\n\nOh, I totally agree. Just wanted to point out that there are other\noptions.\n\nCiao,\nDscho\n"},{"id":"277147","messageId":"xmqqzivlw51y.fsf@gitster.mtv.corp.google.com","threadId":"41293","inReplyTo":"20160131112215.GA4589@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] post-strbuf_getline cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T02:14:01Z","receivedAt":"2016-02-01T02:14:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> As promised, here are the \"how about this on top\" patches that came out\n> of the discussion for the \"strbuf_getline\" series in:\n>\n>   http://thread.gmane.org/gmane.comp.version-control.msysgit/21773/focus=284001\n\nThanks for following up.\n\n> As I looked further at some of the option-parsing cleanups, I realized\n> that some of them are more than cleanups; we're actually fixing bizarre\n> behavior and segfaults.\n>\n>   [1/6]: give \"nbuf\" strbuf a more meaningful name\n>   [2/6]: checkout-index: simplify \"-z\" option parsing\n>   [3/6]: checkout-index: handle \"--no-prefix\" option\n>   [4/6]: checkout-index: handle \"--no-index\" option\n>   [5/6]: checkout-index: disallow \"--no-stage\" option\n>   [6/6]: apply, ls-files: simplify \"-z\" parsing\n>\n> -Peff\n"},{"id":"277148","messageId":"xmqqvb69w4u8.fsf@gitster.mtv.corp.google.com","threadId":"41293","inReplyTo":"20160131113029.GE5116@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] checkout-index: disallow \"--no-stage\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T02:18:39Z","receivedAt":"2016-02-01T02:18:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We do not really expect people to use \"--no-stage\", but if\n> they do, git currently segfaults. We could instead have it\n> undo the effects of a previous \"--stage\", but this gets\n> tricky around the \"to_tempfile\" flag. We cannot simply reset\n> it to 0, because we don't know if it was set by a previous\n> \"--stage=all\" or an explicit \"--temp\" option.\n>\n> We could solve this by setting a flag and resolving\n> to_tempfile later, but it's not worth the effort. Nobody\n> actually wants to use \"--no-stage\"; we are just trying to\n> fix a potential segfault here.\n>\n> While we're in the area adding a translated string, let's\n> mark the other possible error message in this function as\n> translatable.\n\nThanks.  That's worth fixing and I agree with the decision that it\nis the best way to go to not support '--no-stage'.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/checkout-index.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\n> index f8179a7..7a9b561 100644\n> --- a/builtin/checkout-index.c\n> +++ b/builtin/checkout-index.c\n> @@ -133,6 +133,8 @@ static struct lock_file lock_file;\n>  static int option_parse_stage(const struct option *opt,\n>  \t\t\t      const char *arg, int unset)\n>  {\n> +\tif (unset)\n> +\t\treturn error(_(\"--stage cannot be negated\"));\n\nHmm, it is surprising that there is no parse-options flag that says\n\"this cannot be negated\".\n\n>  \tif (!strcmp(arg, \"all\")) {\n>  \t\tto_tempfile = 1;\n>  \t\tcheckout_stage = CHECKOUT_ALL;\n> @@ -141,7 +143,7 @@ static int option_parse_stage(const struct option *opt,\n>  \t\tif ('1' <= ch && ch <= '3')\n>  \t\t\tcheckout_stage = arg[0] - '0';\n>  \t\telse\n> -\t\t\tdie(\"stage should be between 1 and 3 or all\");\n> +\t\t\tdie(_(\"stage should be between 1 and 3 or all\"));\n>  \t}\n>  \treturn 0;\n>  }\n"},{"id":"277149","messageId":"xmqqr3gxw4j1.fsf@gitster.mtv.corp.google.com","threadId":"41293","inReplyTo":"20160131112936.GD5116@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] checkout-index: handle \"--no-index\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T02:25:22Z","receivedAt":"2016-02-01T02:25:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I also reformatted the comment that violated our style\n> guidelines, but I am not sure if it is all that helpful. It\n> seems we also cancel \"--index\" with \"--temp\" or\n> \"--stage=all\", but I do not know why. I left the content\n> as-is, but if somebody knows enough to elaborate, it might\n> be worth doing.\n\nIsn't the --index about updating the cached stat information, to\nallow us to then say \"the working tree file hasn't been modified\nsince we wrote it out\"?  After writing a file out to somewhere that\nis not its natural location (i.e. using prefix, stage or temp would\nall write the contents of F to somewhere that is not F), the next\n\"diff-files\" would not compare the index entry with the contents\nheld in that relocated location, but with its natural location.\n\nAdmittedly, even if we update the cached stat info from that\nrelocated place, the next \"diff-files\" would certainly not match\n(not just mtime but i-num would be different), but if the real\nworking tree file were clean when --temp checkout happened, that\nwould mean we would force another round of update-index --refresh,\nso...\n\n>  builtin/checkout-index.c | 34 ++++++++++------------------------\n>  1 file changed, 10 insertions(+), 24 deletions(-)\n>\n> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\n> index 43bedde..f8179a7 100644\n> --- a/builtin/checkout-index.c\n> +++ b/builtin/checkout-index.c\n> @@ -130,18 +130,6 @@ static const char * const builtin_checkout_index_usage[] = {\n>  \n>  static struct lock_file lock_file;\n>  \n> -static int option_parse_u(const struct option *opt,\n> -\t\t\t      const char *arg, int unset)\n> -{\n> -\tint *newfd = opt->value;\n> -\n> -\tstate.refresh_cache = 1;\n> -\tstate.istate = &the_index;\n> -\tif (*newfd < 0)\n> -\t\t*newfd = hold_locked_index(&lock_file, 1);\n> -\treturn 0;\n> -}\n> -\n>  static int option_parse_stage(const struct option *opt,\n>  \t\t\t      const char *arg, int unset)\n>  {\n> @@ -166,6 +154,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n>  \tint read_from_stdin = 0;\n>  \tint prefix_length;\n>  \tint force = 0, quiet = 0, not_new = 0;\n> +\tint index_opt = 0;\n>  \tstruct option builtin_checkout_index_options[] = {\n>  \t\tOPT_BOOL('a', \"all\", &all,\n>  \t\t\tN_(\"check out all files in the index\")),\n> @@ -174,9 +163,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n>  \t\t\tN_(\"no warning for existing files and files not in index\")),\n>  \t\tOPT_BOOL('n', \"no-create\", &not_new,\n>  \t\t\tN_(\"don't checkout new files\")),\n> -\t\t{ OPTION_CALLBACK, 'u', \"index\", &newfd, NULL,\n> -\t\t\tN_(\"update stat information in the index file\"),\n> -\t\t\tPARSE_OPT_NOARG, option_parse_u },\n> +\t\tOPT_BOOL('u', \"index\", &index_opt,\n> +\t\t\t N_(\"update stat information in the index file\")),\n>  \t\tOPT_BOOL('z', NULL, &nul_term_line,\n>  \t\t\tN_(\"paths are separated with NUL character\")),\n>  \t\tOPT_BOOL(0, \"stdin\", &read_from_stdin,\n> @@ -211,15 +199,13 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n>  \t\tstate.base_dir = \"\";\n>  \tstate.base_dir_len = strlen(state.base_dir);\n>  \n> -\tif (state.base_dir_len || to_tempfile) {\n> -\t\t/* when --prefix is specified we do not\n> -\t\t * want to update cache.\n> -\t\t */\n> -\t\tif (state.refresh_cache) {\n> -\t\t\trollback_lock_file(&lock_file);\n> -\t\t\tnewfd = -1;\n> -\t\t}\n> -\t\tstate.refresh_cache = 0;\n> +\t/*\n> +\t * when --prefix is specified we do not want to update cache.\n> +\t */\n> +\tif (index_opt && !state.base_dir_len && !to_tempfile) {\n> +\t\tstate.refresh_cache = 1;\n> +\t\tstate.istate = &the_index;\n> +\t\tnewfd = hold_locked_index(&lock_file, 1);\n>  \t}\n>  \n>  \t/* Check out named files first */\n"},{"id":"277152","messageId":"20160201031824.GA12217@sigill.intra.peff.net","threadId":"41293","inReplyTo":"xmqqvb69w4u8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/6] checkout-index: disallow \"--no-stage\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-01T03:18:24Z","receivedAt":"2016-02-01T03:18:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 31, 2016 at 06:18:39PM -0800, Junio C Hamano wrote:\n\n> >  builtin/checkout-index.c | 4 +++-\n> >  1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\n> > index f8179a7..7a9b561 100644\n> > --- a/builtin/checkout-index.c\n> > +++ b/builtin/checkout-index.c\n> > @@ -133,6 +133,8 @@ static struct lock_file lock_file;\n> >  static int option_parse_stage(const struct option *opt,\n> >  \t\t\t      const char *arg, int unset)\n> >  {\n> > +\tif (unset)\n> > +\t\treturn error(_(\"--stage cannot be negated\"));\n> \n> Hmm, it is surprising that there is no parse-options flag that says\n> \"this cannot be negated\".\n\nThere is, but you have to stop using the nice OPT_CALLBACK macro and do\na full-on \"{}\" struct literal in the options. Since we have a callback,\nI figured doing this is less ugly to look at. But it does mean making up\nour own message.\n\nI didn't actually try it yesterday; having just done so, it's a lot less\nbad than I expected. And I also noticed yet another problem with it. :-/\n\nHow about this as a replacement patch?\n\n-- >8 --\nSubject: [PATCH] checkout-index: disallow \"--no-stage\" option\n\nWe do not really expect people to use \"--no-stage\", but if\nthey do, git currently segfaults. We could instead have it\nundo the effects of a previous \"--stage\", but this gets\ntricky around the \"to_tempfile\" flag. We cannot simply reset\nit to 0, because we don't know if it was set by a previous\n\"--stage=all\" or an explicit \"--temp\" option.\n\nWe could solve this by setting a flag and resolving\nto_tempfile later, but it's not worth the effort. Nobody\nactually wants to use \"--no-stage\"; we are just trying to\nfix a potential segfault here.\n\nWhile we're in the area, let's improve the user-facing\nmessages for this option. The error string should be\ntranslatable, and we should give some hint in the \"-h\"\noutput about what can go in the argument field.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI also notice that you cannot use \"--stage=0\" to reset a previous\n\"--stage=1\". It probably would not hurt to allow that, but I left it out\nof this patch.\n\n builtin/checkout-index.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex f8179a7..92c6967 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -141,7 +141,7 @@ static int option_parse_stage(const struct option *opt,\n \t\tif ('1' <= ch && ch <= '3')\n \t\t\tcheckout_stage = arg[0] - '0';\n \t\telse\n-\t\t\tdie(\"stage should be between 1 and 3 or all\");\n+\t\t\tdie(_(\"stage should be between 1 and 3 or all\"));\n \t}\n \treturn 0;\n }\n@@ -173,9 +173,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"write the content to temporary files\")),\n \t\tOPT_STRING(0, \"prefix\", &state.base_dir, N_(\"string\"),\n \t\t\tN_(\"when creating files, prepend <string>\")),\n-\t\tOPT_CALLBACK(0, \"stage\", NULL, NULL,\n+\t\t{ OPTION_CALLBACK, 0, \"stage\", NULL, \"1-3|all\",\n \t\t\tN_(\"copy out the files from named stage\"),\n-\t\t\toption_parse_stage),\n+\t\t\tPARSE_OPT_NONEG, option_parse_stage },\n \t\tOPT_END()\n \t};\n \n-- \n2.7.0.489.g6faad84\n"},{"id":"277153","messageId":"20160201032207.GB12217@sigill.intra.peff.net","threadId":"41293","inReplyTo":"xmqqr3gxw4j1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] checkout-index: handle \"--no-index\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-01T03:22:07Z","receivedAt":"2016-02-01T03:22:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 31, 2016 at 06:25:22PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I also reformatted the comment that violated our style\n> > guidelines, but I am not sure if it is all that helpful. It\n> > seems we also cancel \"--index\" with \"--temp\" or\n> > \"--stage=all\", but I do not know why. I left the content\n> > as-is, but if somebody knows enough to elaborate, it might\n> > be worth doing.\n> \n> Isn't the --index about updating the cached stat information, to\n> allow us to then say \"the working tree file hasn't been modified\n> since we wrote it out\"?  After writing a file out to somewhere that\n> is not its natural location (i.e. using prefix, stage or temp would\n> all write the contents of F to somewhere that is not F), the next\n> \"diff-files\" would not compare the index entry with the contents\n> held in that relocated location, but with its natural location.\n\nYeah, that makes sense to me. I should have said \"...but I do not know\nwhy, and I did not really look into it\" in my original.\n\nThat probably makes it OK to silently ignore (as opposed to complaining\nthat \"--prefix\" is used with \"--index\"). It is not so much \"these\noptions are incompatible\" as the fact that there is no entry to update\nin the case of a prefix or tempfile. So \"--index\" is still in effect, it\njust has no work to do. :)\n\n-Peff\n"},{"id":"277189","messageId":"xmqqzivkt86j.fsf@gitster.mtv.corp.google.com","threadId":"41293","inReplyTo":"20160131113546.GF5116@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] apply, ls-files: simplify \"-z\" parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T21:47:00Z","receivedAt":"2016-02-01T21:47:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> As a short option, we cannot handle negation. Thus a\n> callback handling \"unset\" is overkill, and we can just use\n> OPT_SET_INT instead to handle setting the option.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I left this one for last, because it's the most questionable. Unlike the\n> previous \"-z\" case, we're setting the actual character, so the logic is\n> inverted: turning on the option sets it to 0, and turning it off restore\n> '\\n'.\n>\n> This means OPT_SET_INT would do the wrong thing for the \"unset\" case, as\n> it would put a \"0\" into the option. You can't trigger that now, but if\n> somebody were to add a long option (e.g., \"--nul\"), then \"--no-nul\"\n> would do the wrong thing.\n>\n> I'm on the fence on whether the simplification is worth it, or if we\n> should leave the callbacks as future-proofing.\n\nIf done as two patch series where one does this and the other flips\npolarity of the variable (!!line_termination === !nul_term_line),\nwould that make the result both simpler to read and future-proof?\n\nOf course, a patch adding a \"--nul\" can be the one that does the\npolarity flipping, so in that sense, this simplification is probably\nOK, as long as there is some comment that warns a time-bomb you just\nplanted here ;-)\n\n>  builtin/apply.c    | 15 ++-------------\n>  builtin/ls-files.c | 13 ++-----------\n>  2 files changed, 4 insertions(+), 24 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index deb1364..565f3fd 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -4464,16 +4464,6 @@ static int option_parse_p(const struct option *opt,\n>  \treturn 0;\n>  }\n>  \n> -static int option_parse_z(const struct option *opt,\n> -\t\t\t  const char *arg, int unset)\n> -{\n> -\tif (unset)\n> -\t\tline_termination = '\\n';\n> -\telse\n> -\t\tline_termination = 0;\n> -\treturn 0;\n> -}\n> -\n>  static int option_parse_space_change(const struct option *opt,\n>  \t\t\t  const char *arg, int unset)\n>  {\n> @@ -4546,9 +4536,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n>  \t\t\t N_( \"attempt three-way merge if a patch does not apply\")),\n>  \t\tOPT_FILENAME(0, \"build-fake-ancestor\", &fake_ancestor,\n>  \t\t\tN_(\"build a temporary index based on embedded index information\")),\n> -\t\t{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,\n> -\t\t\tN_(\"paths are separated with NUL character\"),\n> -\t\t\tPARSE_OPT_NOARG, option_parse_z },\n> +\t\tOPT_SET_INT('z', NULL, &line_termination,\n> +\t\t\tN_(\"paths are separated with NUL character\"), '\\0'),\n>  \t\tOPT_INTEGER('C', NULL, &p_context,\n>  \t\t\t\tN_(\"ensure at least <n> lines of context match\")),\n>  \t\t{ OPTION_CALLBACK, 0, \"whitespace\", &whitespace_option, N_(\"action\"),\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index b6a7cb0..59bad9b 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -359,14 +359,6 @@ static const char * const ls_files_usage[] = {\n>  \tNULL\n>  };\n>  \n> -static int option_parse_z(const struct option *opt,\n> -\t\t\t  const char *arg, int unset)\n> -{\n> -\tline_terminator = unset ? '\\n' : '\\0';\n> -\n> -\treturn 0;\n> -}\n> -\n>  static int option_parse_exclude(const struct option *opt,\n>  \t\t\t\tconst char *arg, int unset)\n>  {\n> @@ -408,9 +400,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n>  \tstruct exclude_list *el;\n>  \tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n>  \tstruct option builtin_ls_files_options[] = {\n> -\t\t{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,\n> -\t\t\tN_(\"paths are separated with NUL character\"),\n> -\t\t\tPARSE_OPT_NOARG, option_parse_z },\n> +\t\tOPT_SET_INT('z', NULL, &line_terminator,\n> +\t\t\tN_(\"paths are separated with NUL character\"), '\\0'),\n>  \t\tOPT_BOOL('t', NULL, &show_tag,\n>  \t\t\tN_(\"identify the file status with tags\")),\n>  \t\tOPT_BOOL('v', NULL, &show_valid_bit,\n"},{"id":"277194","messageId":"xmqqio28t55d.fsf@gitster.mtv.corp.google.com","threadId":"41293","inReplyTo":"xmqqzivkt86j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/6] apply, ls-files: simplify \"-z\" parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T22:52:30Z","receivedAt":"2016-02-01T22:52:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Of course, a patch adding a \"--nul\" can be the one that does the\n> polarity flipping, so in that sense, this simplification is probably\n> OK, as long as there is some comment that warns a time-bomb you just\n> planted here ;-)\n\nI'll queue it with this tweak for now.\n\nThe idea is to have them run \"blame\" to find the last paragraph of\nthe commit log message.\n\nThanks.\n\n    From: Jeff King <peff@peff.net>\n    Date: Sun, 31 Jan 2016 06:35:46 -0500\n    Subject: [PATCH] apply, ls-files: simplify \"-z\" parsing\n\n    As a short option, we cannot handle negation. Thus a callback\n    handling \"unset\" is overkill, and we can just use OPT_SET_INT\n    instead to handle setting the option.\n\n +  Anybody who adds \"--nul\" synonym to this later would need to be\n +  careful not to break \"--no-nul\", which should mean that lines are\n +  terminated with LF at the end.\n\n    Signed-off-by: Jeff King <peff@peff.net>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 565f3fd..d61ac65 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -4536,6 +4536,7 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)\n \t\t\t N_( \"attempt three-way merge if a patch does not apply\")),\n \t\tOPT_FILENAME(0, \"build-fake-ancestor\", &fake_ancestor,\n \t\t\tN_(\"build a temporary index based on embedded index information\")),\n+\t\t/* Think twice before adding \"--nul\" synonym to this */\n \t\tOPT_SET_INT('z', NULL, &line_termination,\n \t\t\tN_(\"paths are separated with NUL character\"), '\\0'),\n \t\tOPT_INTEGER('C', NULL, &p_context,\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 59bad9b..467699b 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -400,6 +400,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tstruct exclude_list *el;\n \tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n \tstruct option builtin_ls_files_options[] = {\n+\t\t/* Think twice before adding \"--nul\" synonym to this */\n \t\tOPT_SET_INT('z', NULL, &line_terminator,\n \t\t\tN_(\"paths are separated with NUL character\"), '\\0'),\n \t\tOPT_BOOL('t', NULL, &show_tag,\n"},{"id":"277208","messageId":"20160202052941.GB16983@sigill.intra.peff.net","threadId":"41293","inReplyTo":"xmqqio28t55d.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/6] apply, ls-files: simplify \"-z\" parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-02T05:29:41Z","receivedAt":"2016-02-02T05:29:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 01, 2016 at 02:52:30PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Of course, a patch adding a \"--nul\" can be the one that does the\n> > polarity flipping, so in that sense, this simplification is probably\n> > OK, as long as there is some comment that warns a time-bomb you just\n> > planted here ;-)\n> \n> I'll queue it with this tweak for now.\n> \n> The idea is to have them run \"blame\" to find the last paragraph of\n> the commit log message.\n\nThat looks like a good compromise. Thanks.\n\n-Peff\n"}]}