{"thread":{"id":"15970","subject":"Re: [PATCH] parse-opt: migrate builtin-checkout-index.","startedAt":"2008-10-19T18:30:40Z","lastAt":"2008-10-19T20:59:05Z","messageCount":5,"participants":["Junio C Hamano","Pierre Habouzit","Miklos Vajna"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"93469","messageId":"1224441040-5071-1-git-send-email-vmiklos@frugalware.org","threadId":"15970","inReplyTo":"1224292643-28704-1-git-send-email-vmiklos@frugalware.org","subject":"[PATCH] parse-opt: migrate builtin-checkout-index.","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-19T18:30:40Z","receivedAt":"2008-10-19T18:30:40Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Signed-off-by: Miklos Vajna \n---\n\nOn Sat, Oct 18, 2008 at 03:17:23AM +0200, Miklos Vajna wrote:\n> > This adds a new feature to say --no-z from the command line, doesn't\n> > it?\n> > And I suspect the feature is broken ;-).\n>\n> Right, I fixed this in option_parse_z(). --no-z should set\n> line_termination to \\n instead of 1.\n\nOriginally in option_parse_z() I had\n\n line_termination = unset;\n\nwhich is in fact right, because (as Pierre pointed out) unset for short\noptions are always false, but I changed it to\n\n line_termination = 0;\n\nto make it more readable.\n\n> > Is this comment still correct? Do the original and your version act\n> > the\n> > same way when the user says \"checkout --stdin -f\", for example? I\n> > suspect\n> > the original refused it and yours take it (and do much more sensible\n> > thing), which would be an improvement, but then the error message\n> > should\n> > be reworded perhaps?\n>\n> Unless I missed something, that was a limitation of the option parser.\n> checkout-index --stdin -f works fine for me after removing those two\n> lines, so I left them out from the updated patch.\n\nI still think that check is necessary, but the reasoning was false: what\nwe need to check is the usage of --stdin and filenames togother. But a\nbit later there is a\n\n die(\"git checkout-index: don't mix '--stdin' and explicit filenames\");\n\nso the check I removed was unnecessary.\n\nHere is a patch with the cleaned up option_parse_z().\n\n builtin-checkout-index.c | 149 +++++++++++++++++++++++++---------------------\n 1 files changed, 80 insertions(+), 69 deletions(-)\n\ndiff --git a/builtin-checkout-index.c b/builtin-checkout-index.c\nindex 4ba2702..2c558dd 100644\n--- a/builtin-checkout-index.c\n+++ b/builtin-checkout-index.c\n@@ -40,6 +40,7 @@\n #include \"cache.h\"\n #include \"quote.h\"\n #include \"cache-tree.h\"\n+#include \"parse-options.h\"\n \n #define CHECKOUT_ALL 4\n static int line_termination = '\\n';\n@@ -153,11 +154,55 @@ static void checkout_all(const char *prefix, int prefix_length)\n \t\texit(128);\n }\n \n-static const char checkout_cache_usage[] =\n-\"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=] [--temp] [--] ...\";\n+static const char * const builtin_checkout_index_usage[] = {\n+\t\"git checkout-index [options] [--] ...\",\n+\tNULL\n+};\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+\tif (*newfd < 0)\n+\t\t*newfd = hold_locked_index(&lock_file, 1);\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+\tline_termination = 0;\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+\tif (!strcmp(arg, \"all\")) {\n+\t\tto_tempfile = 1;\n+\t\tcheckout_stage = CHECKOUT_ALL;\n+\t} else {\n+\t\tint ch = arg[0];\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}\n+\treturn 0;\n+}\n+\n int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -165,6 +210,33 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \tint all = 0;\n \tint read_from_stdin = 0;\n \tint prefix_length;\n+\tint force = 0, quiet = 0, not_new = 0;\n+\tstruct option builtin_checkout_index_options[] = {\n+\t\tOPT_BOOLEAN('a', \"all\", &all,\n+\t\t\t\"checks out all files in the index\"),\n+\t\tOPT_BOOLEAN('f', \"force\", &force,\n+\t\t\t\"forces overwrite of existing files\"),\n+\t\tOPT__QUIET(&quiet),\n+\t\tOPT_BOOLEAN('n', \"no-create\", ¬_new,\n+\t\t\t\"don't checkout new files\"),\n+\t\t{ OPTION_CALLBACK, 'u', \"index\", &newfd, NULL,\n+\t\t\t\"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\t\"paths are separated with NUL character\",\n+\t\t\tPARSE_OPT_NOARG, option_parse_z },\n+\t\tOPT_BOOLEAN(0, \"stdin\", &read_from_stdin,\n+\t\t\t\"read list of paths from the standard input\"),\n+\t\tOPT_BOOLEAN(0, \"temp\", &to_tempfile,\n+\t\t\t\"write the content to temporary files\"),\n+\t\tOPT_CALLBACK(0, \"prefix\", NULL, \"string\",\n+\t\t\t\"when creating files, prepend \",\n+\t\t\toption_parse_prefix),\n+\t\tOPT_CALLBACK(0, \"stage\", NULL, NULL,\n+\t\t\t\"copy out the files from named stage\",\n+\t\t\toption_parse_stage),\n+\t\tOPT_END()\n+\t};\n \n \tgit_config(git_default_config, NULL);\n \tstate.base_dir = \"\";\n@@ -174,72 +246,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tdie(\"invalid cache\");\n \t}\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n-\n-\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-a\") || !strcmp(arg, \"--all\")) {\n-\t\t\tall = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-f\") || !strcmp(arg, \"--force\")) {\n-\t\t\tstate.force = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-q\") || !strcmp(arg, \"--quiet\")) {\n-\t\t\tstate.quiet = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-n\") || !strcmp(arg, \"--no-create\")) {\n-\t\t\tstate.not_new = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-u\") || !strcmp(arg, \"--index\")) {\n-\t\t\tstate.refresh_cache = 1;\n-\t\t\tif (newfd < 0)\n-\t\t\t\tnewfd = hold_locked_index(&lock_file, 1);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-z\")) {\n-\t\t\tline_termination = 0;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--stdin\")) {\n-\t\t\tif (i != argc - 1)\n-\t\t\t\tdie(\"--stdin must be at the end\");\n-\t\t\tread_from_stdin = 1;\n-\t\t\ti++; /* do not consider arg as a file name */\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--temp\")) {\n-\t\t\tto_tempfile = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!prefixcmp(arg, \"--prefix=\")) {\n-\t\t\tstate.base_dir = arg+9;\n-\t\t\tstate.base_dir_len = strlen(state.base_dir);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!prefixcmp(arg, \"--stage=\")) {\n-\t\t\tif (!strcmp(arg + 8, \"all\")) {\n-\t\t\t\tto_tempfile = 1;\n-\t\t\t\tcheckout_stage = CHECKOUT_ALL;\n-\t\t\t} else {\n-\t\t\t\tint ch = arg[8];\n-\t\t\t\tif ('1' <= ch && ch <= '3')\n-\t\t\t\t\tcheckout_stage = arg[8] - '0';\n-\t\t\t\telse\n-\t\t\t\t\tdie(\"stage should be between 1 and 3 or all\");\n-\t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (arg[0] == '-')\n-\t\t\tusage(checkout_cache_usage);\n-\t\tbreak;\n-\t}\n+\targc = parse_options(argc, argv, builtin_checkout_index_options,\n+\t\t\tbuiltin_checkout_index_usage, 0);\n+\tstate.force = force;\n+\tstate.quiet = quiet;\n+\tstate.not_new = not_new;\n \n \tif (state.base_dir_len || to_tempfile) {\n \t\t/* when --prefix is specified we do not\n@@ -253,7 +264,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t}\n \n \t/* Check out named files first */\n-\tfor ( ; i < argc; i++) {\n+\tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tconst char *p;\n \n-- \n1.6.0.2\n"},{"id":"93468","messageId":"20081019183157.GF536@genesis.frugalware.org","threadId":"15970","inReplyTo":"1224441040-5071-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] parse-opt: migrate builtin-checkout-index.","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-10-19T18:31:57Z","receivedAt":"2008-10-19T18:31:57Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Sun, Oct 19, 2008 at 08:30:40PM +0200, Miklos Vajna wrote:\n> I still think that check is necessary, but the reasoning was false: what\n\n ^ unnecessary\n\nSorry for not reading over my mail before sending it.\n"},{"id":"93458","messageId":"7vabd073bg.fsf@gitster.siamese.dyndns.org","threadId":"15970","inReplyTo":"1224441040-5071-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] parse-opt: migrate builtin-checkout-index.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-19T20:11:31Z","receivedAt":"2008-10-19T20:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> Signed-off-by: Miklos Vajna \n> ---\n>\n> On Sat, Oct 18, 2008 at 03:17:23AM +0200, Miklos Vajna wrote:\n>> > This adds a new feature to say --no-z from the command line, doesn't\n>> > it?\n>> > And I suspect the feature is broken ;-).\n>>\n>> Right, I fixed this in option_parse_z(). --no-z should set\n>> line_termination to \\n instead of 1.\n>\n> Originally in option_parse_z() I had\n>\n> line_termination = unset;\n>\n> which is in fact right, because (as Pierre pointed out) unset for short\n> options are always false, but I changed it to\n>\n> line_termination = 0;\n>\n> to make it more readable.\n\nI think Pierre's comment is short-sighted. Think of what would happen\nwhen somebody adds \"--nul\" as a longer equivalent to \"-z\", since it is\nextremely easy to do things like that with the use of parse-opt API?\n"},{"id":"93456","messageId":"20081019201934.GO16610@artemis.corp","threadId":"15970","inReplyTo":"7vabd073bg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] parse-opt: migrate builtin-checkout-index.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-10-19T20:19:34Z","receivedAt":"2008-10-19T20:19:34Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Oct 19, 2008 at 08:11:31PM +0000, Junio C Hamano wrote:\n> Miklos Vajna writes:\n> \n> > Signed-off-by: Miklos Vajna \n> > ---\n> >\n> > On Sat, Oct 18, 2008 at 03:17:23AM +0200, Miklos Vajna wrote:\n> >> > This adds a new feature to say --no-z from the command line, doesn't\n> >> > it?\n> >> > And I suspect the feature is broken ;-).\n> >>\n> >> Right, I fixed this in option_parse_z(). --no-z should set\n> >> line_termination to \\n instead of 1.\n> >\n> > Originally in option_parse_z() I had\n> >\n> > line_termination = unset;\n> >\n> > which is in fact right, because (as Pierre pointed out) unset for short\n> > options are always false, but I changed it to\n> >\n> > line_termination = 0;\n> >\n> > to make it more readable.\n> \n> I think Pierre's comment is short-sighted. Think of what would happen\n> when somebody adds \"--nul\" as a longer equivalent to \"-z\", since it is\n> extremely easy to do things like that with the use of parse-opt API?\n\nErr I was only pointing out that --no-z would no nothing, I actually\ndidn't really read the argument :) I didn't say having --null was a bad\nidea, and I think we have -z/--null at a couple of places already, and\nit's probably a good thing to actually _add_ --null.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"93455","messageId":"7v4p387146.fsf@gitster.siamese.dyndns.org","threadId":"15970","inReplyTo":"20081019201934.GO16610@artemis.corp","subject":"Re: [PATCH] parse-opt: migrate builtin-checkout-index.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-19T20:59:05Z","receivedAt":"2008-10-19T20:59:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> On Sun, Oct 19, 2008 at 08:11:31PM +0000, Junio C Hamano wrote:\n>> Miklos Vajna writes:\n>\n>> >> Right, I fixed this in option_parse_z(). --no-z should set\n>> >> line_termination to \\n instead of 1.\n>> >\n>> > Originally in option_parse_z() I had\n>> >\n>> > line_termination = unset;\n>> >\n>> > which is in fact right, because (as Pierre pointed out) unset for short\n>> > options are always false, but I changed it to\n>> >\n>> > line_termination = 0;\n>> >\n>> > to make it more readable.\n>> \n>> I think Pierre's comment is short-sighted. Think of what would happen\n>> when somebody adds \"--nul\" as a longer equivalent to \"-z\", since it is\n>> extremely easy to do things like that with the use of parse-opt API?\n>\n> Err I was only pointing out that --no-z would no nothing, I actually\n> didn't really read the argument :) I didn't say having --null was a bad\n> idea,...\n\nIt is a good practice to anticipate potential future breakages, assess the\ncost to avoid them, avoid the ones that can be avoided with minimum cost\n(and document the others you know will be broken). That's the key to\nproduce maintainable piece of code. The change necessary to Miklos's\noriginal code to do this is quite simple (i.e. flip between '\\0' and\n'\\n'), and I didn't see any room for argument like \"short option won't let\nyou say --no-z\". That's where my comment about \"short-sighted\"-ness comes\nfrom.\n"}]}