{"thread":{"id":"10766","subject":"[PATCH] Make builtin-tag.c use parse_options.","startedAt":"2007-11-09T13:42:56Z","lastAt":"2007-11-12T19:48:46Z","messageCount":11,"participants":["Carlos Rica","Jakub Narebski","Johannes Schindelin","Junio C Hamano","Pierre Habouzit","Kristian Høgsberg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"59044","messageId":"473463E0.7000406@gmail.com","threadId":"10766","inReplyTo":null,"subject":"[PATCH] Make builtin-tag.c use parse_options.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-11-09T13:42:56Z","receivedAt":"2007-11-09T13:42:56Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"Also, this removes those tests ensuring that repeated\n-m options don't allocate memory more than once, because now\nthis is done after parsing options, using the last one\nwhen more are given. The same for -F.\n\nSigned-off-by: Carlos Rica <jasampler@gmail.com>\n---\n\n    Applied to \"next\".\n    Comments welcomed.\n\n builtin-tag.c  |  141 ++++++++++++++++++++++++--------------------------------\n t/t7004-tag.sh |    8 +---\n 2 files changed, 61 insertions(+), 88 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 66e5a58..5af1950 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -11,9 +11,15 @@\n #include \"refs.h\"\n #include \"tag.h\"\n #include \"run-command.h\"\n-\n-static const char builtin_tag_usage[] =\n-  \"git-tag [-n [<num>]] -l [<pattern>] | [-a | -s | -u <key-id>] [-f | -d | -v] [-m <msg> | -F <file>] <tagname> [<head>]\";\n+#include \"parse-options.h\"\n+\n+static const char * const git_tag_usage[] = {\n+\t\"git-tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]\",\n+\t\"git-tag -d <tagname>...\",\n+\t\"git-tag [-n [<num>]] -l [<pattern>]\",\n+\t\"git-tag -v <tagname>...\",\n+\tNULL\n+};\n\n static char signingkey[1000];\n\n@@ -308,101 +314,74 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf;\n \tunsigned char object[20], prev[20];\n-\tint annotate = 0, sign = 0, force = 0, lines = 0, message = 0;\n \tchar ref[PATH_MAX];\n \tconst char *object_ref, *tag;\n-\tint i;\n \tstruct ref_lock *lock;\n\n+\tint annotate = 0, sign = 0, force = 0, lines = 0,\n+\t\t\t\t\tdelete = 0, verify = 0;\n+\tchar *list = NULL, *msg = NULL, *msgfile = NULL, *keyid = NULL;\n+\tconst char *no_pattern = \"NO_PATTERN\";\n+\tstruct option options[] = {\n+\t\t{ OPTION_STRING, 'l', NULL, &list, \"pattern\", \"list tag names\",\n+\t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) no_pattern },\n+\t\t{ OPTION_INTEGER, 'n', NULL, &lines, NULL,\n+\t\t\t\t\"print n lines of each tag message\",\n+\t\t\t\tPARSE_OPT_OPTARG, NULL, 1 },\n+\t\tOPT_BOOLEAN('d', NULL, &delete, \"delete tags\"),\n+\t\tOPT_BOOLEAN('v', NULL, &verify, \"verify tags\"),\n+\n+\t\tOPT_GROUP(\"Tag creation options\"),\n+\t\tOPT_BOOLEAN('a', NULL, &annotate,\n+\t\t\t\t\t\"annotated tag, needs a message\"),\n+\t\tOPT_STRING('m', NULL, &msg, \"msg\", \"message for the tag\"),\n+\t\tOPT_STRING('F', NULL, &msgfile, \"file\", \"message in a file\"),\n+\t\tOPT_BOOLEAN('s', NULL, &sign, \"annotated and GPG-signed tag\"),\n+\t\tOPT_STRING('u', NULL, &keyid, \"key-id\",\n+\t\t\t\t\t\"use another key to sign the tag\"),\n+\t\tOPT_BOOLEAN('f', NULL, &force, \"replace the tag if exists\"),\n+\t\tOPT_END()\n+\t};\n+\n \tgit_config(git_tag_config);\n-\tstrbuf_init(&buf, 0);\n\n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n+\targc = parse_options(argc, argv, options, git_tag_usage, 0);\n\n-\t\tif (arg[0] != '-')\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"-a\")) {\n-\t\t\tannotate = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-s\")) {\n-\t\t\tannotate = 1;\n-\t\t\tsign = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-f\")) {\n-\t\t\tforce = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-n\")) {\n-\t\t\tif (i + 1 == argc || *argv[i + 1] == '-')\n-\t\t\t\t/* no argument */\n-\t\t\t\tlines = 1;\n-\t\t\telse\n-\t\t\t\tlines = isdigit(*argv[++i]) ?\n-\t\t\t\t\tatoi(argv[i]) : 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-m\")) {\n-\t\t\tannotate = 1;\n-\t\t\ti++;\n-\t\t\tif (i == argc)\n-\t\t\t\tdie(\"option -m needs an argument.\");\n-\t\t\tif (message)\n-\t\t\t\tdie(\"only one -F or -m option is allowed.\");\n-\t\t\tstrbuf_addstr(&buf, argv[i]);\n-\t\t\tmessage = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-F\")) {\n-\t\t\tannotate = 1;\n-\t\t\ti++;\n-\t\t\tif (i == argc)\n-\t\t\t\tdie(\"option -F needs an argument.\");\n-\t\t\tif (message)\n-\t\t\t\tdie(\"only one -F or -m option is allowed.\");\n-\n-\t\t\tif (!strcmp(argv[i], \"-\")) {\n+\tif (list)\n+\t\treturn list_tags(list == no_pattern ? NULL : list, lines);\n+\tif (delete)\n+\t\treturn for_each_tag_name(argv, delete_tag);\n+\tif (verify)\n+\t\treturn for_each_tag_name(argv, verify_tag);\n+\n+\tstrbuf_init(&buf, 0);\n+\tif (msg || msgfile) {\n+\t\tif (msg && msgfile)\n+\t\t\tdie(\"only one -F or -m option is allowed.\");\n+\t\tannotate = 1;\n+\t\tif (msg)\n+\t\t\tstrbuf_addstr(&buf, msg);\n+\t\telse {\n+\t\t\tif (!strcmp(msgfile, \"-\")) {\n \t\t\t\tif (strbuf_read(&buf, 0, 1024) < 0)\n-\t\t\t\t\tdie(\"cannot read %s\", argv[i]);\n+\t\t\t\t\tdie(\"cannot read %s\", msgfile);\n \t\t\t} else {\n-\t\t\t\tif (strbuf_read_file(&buf, argv[i], 1024) < 0)\n+\t\t\t\tif (strbuf_read_file(&buf, msgfile, 1024) < 0)\n \t\t\t\t\tdie(\"could not open or read '%s': %s\",\n-\t\t\t\t\t\targv[i], strerror(errno));\n+\t\t\t\t\t\tmsgfile, strerror(errno));\n \t\t\t}\n-\t\t\tmessage = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"-u\")) {\n-\t\t\tannotate = 1;\n-\t\t\tsign = 1;\n-\t\t\ti++;\n-\t\t\tif (i == argc)\n-\t\t\t\tdie(\"option -u needs an argument.\");\n-\t\t\tif (strlcpy(signingkey, argv[i], sizeof(signingkey))\n-\t\t\t\t\t\t\t>= sizeof(signingkey))\n-\t\t\t\tdie(\"argument to option -u too long\");\n-\t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(arg, \"-l\"))\n-\t\t\treturn list_tags(argv[i + 1], lines);\n-\t\tif (!strcmp(arg, \"-d\"))\n-\t\t\treturn for_each_tag_name(argv + i + 1, delete_tag);\n-\t\tif (!strcmp(arg, \"-v\"))\n-\t\t\treturn for_each_tag_name(argv + i + 1, verify_tag);\n-\t\tusage(builtin_tag_usage);\n \t}\n\n-\tif (i == argc) {\n+\tif (argc == 0) {\n \t\tif (annotate)\n-\t\t\tusage(builtin_tag_usage);\n+\t\t\tusage_with_options(git_tag_usage, options);\n \t\treturn list_tags(NULL, lines);\n \t}\n-\ttag = argv[i++];\n+\ttag = argv[0];\n\n-\tobject_ref = i < argc ? argv[i] : \"HEAD\";\n-\tif (i + 1 < argc)\n+\tobject_ref = argc == 2 ? argv[1] : \"HEAD\";\n+\tif (argc > 2)\n \t\tdie(\"too many params\");\n\n \tif (get_sha1(object_ref, object))\n@@ -419,7 +398,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(\"tag '%s' already exists\", tag);\n\n \tif (annotate)\n-\t\tcreate_tag(object, tag, &buf, message, sign, object);\n+\t\tcreate_tag(object, tag, &buf, msg || msgfile, sign, object);\n\n \tlock = lock_any_ref_for_update(ref, prev, 0);\n \tif (!lock)\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 0d07bc3..4b09d28 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -339,20 +339,14 @@ test_expect_success \\\n '\n\n test_expect_success \\\n-\t'trying to create tags giving many -m or -F options should fail' '\n+\t'trying to create tags giving both -m or -F options should fail' '\n \techo \"message file 1\" >msgfile1 &&\n \techo \"message file 2\" >msgfile2 &&\n \t! tag_exists msgtag &&\n-\t! git-tag -m \"message 1\" -m \"message 2\" msgtag &&\n-\t! tag_exists msgtag &&\n-\t! git-tag -F msgfile1 -F msgfile2 msgtag &&\n-\t! tag_exists msgtag &&\n \t! git-tag -m \"message 1\" -F msgfile1 msgtag &&\n \t! tag_exists msgtag &&\n \t! git-tag -F msgfile1 -m \"message 1\" msgtag &&\n \t! tag_exists msgtag &&\n-\t! git-tag -F msgfile1 -m \"message 1\" -F msgfile2 msgtag &&\n-\t! tag_exists msgtag &&\n \t! git-tag -m \"message 1\" -F msgfile1 -m \"message 2\" msgtag &&\n \t! tag_exists msgtag\n '\n-- \n1.5.3.4\n"},{"id":"59047","messageId":"fh1p10$nta$1@ger.gmane.org","threadId":"10766","inReplyTo":"473463E0.7000406@gmail.com","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-11-09T13:57:48Z","receivedAt":"2007-11-09T13:57:48Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Carlos Rica wrote:\n\n> +     struct option options[] = {\n> +             { OPTION_STRING, 'l', NULL, &list, \"pattern\", \"list tag names\",\n> +                     PARSE_OPT_OPTARG, NULL, (intptr_t) no_pattern },\n\n> +             OPT_STRING('F', NULL, &msgfile, \"file\", \"message in a file\"),\n\nDoes it matter that you use OPTION_STRING here and OPT_STRING macro there?\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"59055","messageId":"Pine.LNX.4.64.0711091429120.4362@racer.site","threadId":"10766","inReplyTo":"fh1p10$nta$1@ger.gmane.org","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-09T14:31:13Z","receivedAt":"2007-11-09T14:31:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n[re Cc:ing jasam]\n\nOn Fri, 9 Nov 2007, Jakub Narebski wrote:\n\n> Carlos Rica wrote:\n> \n> > +     struct option options[] = {\n> > +             { OPTION_STRING, 'l', NULL, &list, \"pattern\", \"list tag names\",\n> > +                     PARSE_OPT_OPTARG, NULL, (intptr_t) no_pattern },\n> \n> > +             OPT_STRING('F', NULL, &msgfile, \"file\", \"message in a file\"),\n> \n> Does it matter that you use OPTION_STRING here and OPT_STRING macro there?\n\nI guess it is because of the PARSE_OPT_OPTARG thing, together with \nno_pattern.  We need to know if -l was specified, even if no argument was \npassed in.\n\nHth,\nDscho\n"},{"id":"59172","messageId":"7vabpmpr9y.fsf@gitster.siamese.dyndns.org","threadId":"10766","inReplyTo":"473463E0.7000406@gmail.com","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-10T06:07:37Z","receivedAt":"2007-11-10T06:07:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Rica <jasampler@gmail.com> writes:\n\n> Also, this removes those tests ensuring that repeated\n> -m options don't allocate memory more than once, because now\n> this is done after parsing options, using the last one\n> when more are given. The same for -F.\n\nThe reason for this change is...?  Is this because it is\ncumbersome to detect and refuse multiple -m options using the\nparseopt API?  If so, the API may be what needs to be fixed.\nTaking the last one and discarding earlier ones feels to me an\narbitrary choice.\n\nWhile I freely admit that I do not particularly find the \"One -m\nintroduces one new line, concatenated to form the final\nparagraph\" handling of multiple -m options done by git-commit\nnice nor useful, I suspect that it would make more sense to make\ngit-tag and git-commit handle multiple -m option consistently,\nif you are going to change the existing semantics.  Since some\npeople really seem to like multiple -m handling of git-commit,\nthe avenue of the least resistance for better consistency would\nbe to accept and concatenate (with LF in between) multiple -m\noptions.\n\nWith multiple -F, I think erroring out would be the sensible\nthing to do, but some people might prefer concatenation.  I do\nnot care either way as long as commit and tag behave\nconsistently.\n"},{"id":"59183","messageId":"7vhcjuo3h9.fsf@gitster.siamese.dyndns.org","threadId":"10766","inReplyTo":"7vabpmpr9y.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-10T09:26:58Z","receivedAt":"2007-11-10T09:26:58Z","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> While I freely admit that I do not particularly find the \"One -m\n> introduces one new line, concatenated to form the final\n> paragraph\" handling of multiple -m options done by git-commit\n> nice nor useful, I suspect that it would make more sense to make\n> git-tag and git-commit handle multiple -m option consistently,\n> if you are going to change the existing semantics.  Since some\n> people really seem to like multiple -m handling of git-commit,\n> the avenue of the least resistance for better consistency would\n> be to accept and concatenate (with LF in between) multiple -m\n> options.\n>\n> With multiple -F, I think erroring out would be the sensible\n> thing to do, but some people might prefer concatenation.  I do\n> not care either way as long as commit and tag behave\n> consistently.\n\nAlas, this exposes a regression in kh/commit series.\n\n t/t7501-commit.sh |   23 +++++++++++++++++++++++\n 1 files changed, 23 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 1b444d4..bf5dd86 100644\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -178,4 +178,27 @@ test_expect_success 'amend commit to fix author' '\n \tdiff expected current\n \n '\n+\n+test_expect_success 'sign off' '\n+\n+\t>positive &&\n+\tgit add positive &&\n+\tgit commit -s -m \"thank you\" &&\n+\tactual=$(git cat-file commit HEAD | sed -ne \"s/Signed-off-by: //p\") &&\n+\texpected=$(git var GIT_COMMITTER_IDENT | sed -e \"s/>.*/>/\") &&\n+\ttest \"z$actual\" = \"z$expected\"\n+\n+'\n+\n+test_expect_success 'multiple -m' '\n+\n+\t>negative &&\n+\tgit add negative &&\n+\tgit commit -m \"one\" -m \"two\" -m \"three\" &&\n+\tactual=$(git cat-file commit HEAD | sed -e \"1,/^\\$/d\") &&\n+\texpected=$(echo one; echo; echo two; echo; echo three) &&\n+\ttest \"z$actual\" = \"z$expected\"\n+\n+'\n+\n test_done\n"},{"id":"59185","messageId":"7vabpmo2tf.fsf@gitster.siamese.dyndns.org","threadId":"10766","inReplyTo":"7vhcjuo3h9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-10T09:41:16Z","receivedAt":"2007-11-10T09:41:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is an updated patch to the test script...\n\n t/t7501-commit.sh |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 69 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex b151b51..4dc35bd 100644\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -163,4 +163,73 @@ test_expect_success 'partial commit that involves removal (3)' '\n \n '\n \n+author=\"The Real Author <someguy@his.email.org>\"\n+test_expect_success 'amend commit to fix author' '\n+\n+\toldtick=$GIT_AUTHOR_DATE &&\n+\ttest_tick &&\n+\tgit reset --hard &&\n+\tgit cat-file -p HEAD |\n+\tsed -e \"s/author.*/author $author $oldtick/\" \\\n+\t\t-e \"s/^\\(committer.*> \\).*$/\\1$GIT_COMMITTER_DATE/\" > \\\n+\t\texpected &&\n+\tgit commit --amend --author=\"$author\" &&\n+\tgit cat-file -p HEAD > current &&\n+\tdiff expected current\n+\n+'\n+\n+test_expect_success 'sign off (1)' '\n+\n+\techo 1 >positive &&\n+\tgit add positive &&\n+\tgit commit -s -m \"thank you\" &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n+\t(\n+\t\techo thank you\n+\t\techo\n+\t\tgit var GIT_COMMITTER_IDENT |\n+\t\tsed -e \"s/>.*/>/\" -e \"s/^/Signed-off-by: /\"\n+\t) >expected &&\n+\tdiff -u expected actual\n+\n+'\n+\n+test_expect_success 'sign off (2)' '\n+\n+\techo 2 >positive &&\n+\tgit add positive &&\n+\texisting=\"Signed-off-by: Watch This <watchthis@example.com>\" &&\n+\tgit commit -s -m \"thank you\n+\n+$existing\" &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n+\t(\n+\t\techo thank you\n+\t\techo\n+\t\techo $existing\n+\t\tgit var GIT_COMMITTER_IDENT |\n+\t\tsed -e \"s/>.*/>/\" -e \"s/^/Signed-off-by: /\"\n+\t) >expected &&\n+\tdiff -u expected actual\n+\n+'\n+\n+test_expect_success 'multiple -m' '\n+\n+\t>negative &&\n+\tgit add negative &&\n+\tgit commit -m \"one\" -m \"two\" -m \"three\" &&\n+\tgit cat-file commit HEAD | sed -e \"1,/^\\$/d\" >actual &&\n+\t(\n+\t\techo one\n+\t\techo\n+\t\techo two\n+\t\techo\n+\t\techo three\n+\t) >expected &&\n+\tdiff -u expected actual\n+\n+'\n+\n test_done\n"},{"id":"59202","messageId":"1b46aba20711100425o2f351ac5o81537adc6f09dc80@mail.gmail.com","threadId":"10766","inReplyTo":"7vabpmpr9y.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-11-10T12:25:44Z","receivedAt":"2007-11-10T12:25:44Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"2007/11/10, Junio C Hamano <gitster@pobox.com>:\n> Carlos Rica <jasampler@gmail.com> writes:\n>\n> > Also, this removes those tests ensuring that repeated\n> > -m options don't allocate memory more than once, because now\n> > this is done after parsing options, using the last one\n> > when more are given. The same for -F.\n>\n> The reason for this change is...?  Is this because it is\n> cumbersome to detect and refuse multiple -m options using the\n> parseopt API?  If so, the API may be what needs to be fixed.\n> Taking the last one and discarding earlier ones feels to me an\n> arbitrary choice.\n>\n> While I freely admit that I do not particularly find the \"One -m\n> introduces one new line, concatenated to form the final\n> paragraph\" handling of multiple -m options done by git-commit\n> nice nor useful, I suspect that it would make more sense to make\n> git-tag and git-commit handle multiple -m option consistently,\n> if you are going to change the existing semantics.  Since some\n> people really seem to like multiple -m handling of git-commit,\n> the avenue of the least resistance for better consistency would\n> be to accept and concatenate (with LF in between) multiple -m\n> options.\n>\n> With multiple -F, I think erroring out would be the sensible\n> thing to do, but some people might prefer concatenation.  I do\n> not care either way as long as commit and tag behave\n> consistently.\n\nA solution not needing memory allocation into the option parser\ncould be setting a callback running over the repeated option\narguments, passing them to the function one per each call.\nThen, the user will be able to decide if he wants the arguments\nconcatenated or only need one of them and prefers erroring out.\n\nIs this already possible with the current parser or the callback\nmode only calls using the last option?\n"},{"id":"59208","messageId":"20071110131327.GC25204@artemis.corp","threadId":"10766","inReplyTo":"1b46aba20711100425o2f351ac5o81537adc6f09dc80@mail.gmail.com","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-10T13:13:27Z","receivedAt":"2007-11-10T13:13:27Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sat, Nov 10, 2007 at 12:25:44PM +0000, Carlos Rica wrote:\n> 2007/11/10, Junio C Hamano <gitster@pobox.com>:\n> > Carlos Rica <jasampler@gmail.com> writes:\n\n> A solution not needing memory allocation into the option parser\n> could be setting a callback running over the repeated option\n> arguments, passing them to the function one per each call.\n> Then, the user will be able to decide if he wants the arguments\n> concatenated or only need one of them and prefers erroring out.\n> \n> Is this already possible with the current parser or the callback\n> mode only calls using the last option?\n\n  Everything is possible, you just have to code it. With a callback\nyou have in the struct option two places to store \"things\". The void*\nvalue pointer and the intptr_t defval. _Usually_ the void* is the\npointer to the data that will be _written_ and the defval the data that\nwill be put into the void* under some circumstances (e.g. when your\noption is negated).\n\n  For Your case I'd go with some kind of string list pointed into the\nvoid * value, defval has no or little use. You don't really care about\nallocating memory in the option parser, I mean, option parsing is done\nonce at the initialization phase. It's not evil. In pseudo-C here is how\nI would write the callback:\n\nint parse_opt_stringlist(const struct option *opt, const char *arg, int unset)\n{\n    string_list **l = opt->value;\n    string_list_elem *e;\n\n    if (unset) { /* negationg option clears the list */\n\twhile (*l) {\n\t    string_list_elem_free(string_list_pop(l));\n\t}\n\treturn 0;\n    }\n\n    e = string_list_elem_new();\n    e->data = arg;\n    string_list_push(l, e);\n    return 0;\n}\n\n  And you're done, you can do what you want with that list from the caller.\nThere probably is such a structure in git, if not, it can probably be hacked\nin a few lines.\n\n  Remember, callbacks give you _full_ control on what you can do in the option\nparser, and if you're not happy with Turing complete expressivity, there isn't\nanything I can do for you :P Note that if you do write such a generic\ncallback, it belongs to parse-options.[hc].\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"59480","messageId":"1b46aba20711120509l104792ebo4ea9a51c710510f3@mail.gmail.com","threadId":"10766","inReplyTo":"7vabpmpr9y.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-11-12T13:09:37Z","receivedAt":"2007-11-12T13:09:37Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"2007/11/10, Junio C Hamano <gitster@pobox.com>:\n> Carlos Rica <jasampler@gmail.com> writes:\n>\n> > Also, this removes those tests ensuring that repeated\n> > -m options don't allocate memory more than once, because now\n> > this is done after parsing options, using the last one\n> > when more are given. The same for -F.\n>\n> The reason for this change is...?  Is this because it is\n> cumbersome to detect and refuse multiple -m options using the\n> parseopt API?  If so, the API may be what needs to be fixed.\n> Taking the last one and discarding earlier ones feels to me an\n> arbitrary choice.\n\nYou can do many things with repeated options.\nHere in git-tag we considered two different ways to manage them:\nConcatenating values for the option and/or refusing more than one.\nI found that current option-parser can do both from the client\nusing callbacks, as Pierre shows me, so I think it is the right way to do it.\n\nPierre, by default, I think that the parser should print an error\nwhen more than one option of the same type is given,\nin order to report it to the command-line user,\nbut make this behaviour optional for the programmer.\nSpecifically, I thought in this last option:\n\nenum parse_opt_option_flags {\n\tPARSE_OPT_OPTARG  = 1,\n\tPARSE_OPT_NOARG   = 2,\n\tPARSE_OPT_ALLOWREP = 4\n};\n\n> While I freely admit that I do not particularly find the \"One -m\n> introduces one new line, concatenated to form the final\n> paragraph\" handling of multiple -m options done by git-commit\n> nice nor useful, I suspect that it would make more sense to make\n> git-tag and git-commit handle multiple -m option consistently,\n> if you are going to change the existing semantics.  Since some\n> people really seem to like multiple -m handling of git-commit,\n> the avenue of the least resistance for better consistency would\n> be to accept and concatenate (with LF in between) multiple -m\n> options.\n>\n> With multiple -F, I think erroring out would be the sensible\n> thing to do, but some people might prefer concatenation.  I do\n> not care either way as long as commit and tag behave\n> consistently.\n\nThen, Kristian, what are you willing to do in such case?\nIt seems easier for me to concatenate of -m and -F options, even when\nboth types are given. I don't know why \"people\" want multiple -m options,\nbut I think that mixing -m and -F options could be interesting for them too.\nIf someone know if this have been discussed and decided already,\nplease give me the link.\n"},{"id":"59494","messageId":"20071112145252.GA343@artemis.corp","threadId":"10766","inReplyTo":"1b46aba20711120509l104792ebo4ea9a51c710510f3@mail.gmail.com","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-12T14:52:52Z","receivedAt":"2007-11-12T14:52:52Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Nov 12, 2007 at 01:09:37PM +0000, Carlos Rica wrote:\n> 2007/11/10, Junio C Hamano <gitster@pobox.com>:\n> > Carlos Rica <jasampler@gmail.com> writes:\n> >\n> > > Also, this removes those tests ensuring that repeated\n> > > -m options don't allocate memory more than once, because now\n> > > this is done after parsing options, using the last one\n> > > when more are given. The same for -F.\n> >\n> > The reason for this change is...?  Is this because it is\n> > cumbersome to detect and refuse multiple -m options using the\n> > parseopt API?  If so, the API may be what needs to be fixed.\n> > Taking the last one and discarding earlier ones feels to me an\n> > arbitrary choice.\n> \n> You can do many things with repeated options.\n> Here in git-tag we considered two different ways to manage them:\n> Concatenating values for the option and/or refusing more than one.\n> I found that current option-parser can do both from the client\n> using callbacks, as Pierre shows me, so I think it is the right way to do it.\n> \n> Pierre, by default, I think that the parser should print an error\n> when more than one option of the same type is given,\n\n  I beg to differ. It makes sense for OPTION_STRING options, but not for\nother. Though you cannot always detect that.\n\nAlso note that:\n(1) repeating options was already silent in many git commands, so it's\n    not really a regression ;\n(2) for many commands it actually make sense to allow repeating (for\n    _BOOLEAN e.g.). And I'd argue that for OPTION_BIT it also makes\n    sense as well.\n\n> in order to report it to the command-line user, but make this\n> behaviour optional for the programmer.  Specifically, I thought in\n> this last option:\n> \n> enum parse_opt_option_flags {\n> \tPARSE_OPT_OPTARG   = 1,\n> \tPARSE_OPT_NOARG    = 2,\n> \tPARSE_OPT_ALLOWREP = 4\n> };\n\n  To do that you need to keep a list of the triggered commands to do\nthat, there is no way to achieve that reliably right now. As taking the\nlast one and discarding the other is the usual way for option parsers I\nnever saw this as a big issue.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"59555","messageId":"1194896926.2869.15.camel@hinata.boston.redhat.com","threadId":"10766","inReplyTo":"1b46aba20711120509l104792ebo4ea9a51c710510f3@mail.gmail.com","subject":"Re: [PATCH] Make builtin-tag.c use parse_options.","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-11-12T19:48:46Z","receivedAt":"2007-11-12T19:48:46Z","isPatch":true,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"On Mon, 2007-11-12 at 14:09 +0100, Carlos Rica wrote:\n> 2007/11/10, Junio C Hamano <gitster@pobox.com>:\n> > Carlos Rica <jasampler@gmail.com> writes:\n...\n> Then, Kristian, what are you willing to do in such case?\n> It seems easier for me to concatenate of -m and -F options, even when\n> both types are given. I don't know why \"people\" want multiple -m options,\n> but I think that mixing -m and -F options could be interesting for them too.\n> If someone know if this have been discussed and decided already,\n> please give me the link.\n\nI should be pretty easy to just append the contents of multiple fies,\neven inter-mingled with -m options.  We just do a callback like Johannes\njust did for -m in builtin-commit.c for -F and append to the same\nstrbuf.  strbuf_read() already appends, so the callback could look\nsomething like:\n\nstatic int opt_parse_F(const struct option *opt, const char *arg, int\nunset)\n{\n        struct strbuf *buf = opt->value;\n\n\tif (!strcmp(arg, \"-\")) {\n                if (isatty(0))\n                        fprintf(stderr, \"(reading log message from\nstandard input)\\n\");\n                if (strbuf_read(&sb, 0, 0) < 0)\n                        die(\"could not read log from standard input\");\n\t} else {\n                if (strbuf_read_file(&sb, logfile, 0) < 0)\n                        die(\"could not read log file '%s': %s\",\n                            logfile, strerror(errno));\n\t}\n}\n\nShouldn't be too hard :)\nKristian\n"}]}