{"thread":{"id":"41582","subject":"[PATCH] builtin/receive-pack.c: use parse_options API","startedAt":"2016-03-01T15:36:00Z","lastAt":"2016-03-02T13:53:40Z","messageCount":14,"participants":["Sidhant Sharma [:tk]","Matthieu Moy","Sidhant Sharma","Eric Sunshine","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279975","messageId":"1456846560-9223-1-git-send-email-tigerkid001@gmail.com","threadId":"41582","inReplyTo":null,"subject":"[PATCH] builtin/receive-pack.c: use parse_options API","fromName":"Sidhant Sharma [:tk]","fromEmail":"tigerkid001@gmail.com","sentAt":"2016-03-01T15:36:00Z","receivedAt":"2016-03-01T15:36:00Z","isPatch":true,"sender":{"key":"tigerkid001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7801881?v=4"},"body":"This patch makes receive-pack use the parse_options API,\nbringing it more in line with send-pack and push.\n\nHelped-by: Matthieu Moy \nSigned-off-by: Sidhant Sharma [:tk] \n---\n builtin/receive-pack.c | 55 ++++++++++++++++++++++----------------------------\n 1 file changed, 24 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c8e32b2..fe9a594 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -21,7 +21,10 @@\n #include \"sigchain.h\"\n #include \"fsck.h\"\n\n-static const char receive_pack_usage[] = \"git receive-pack \";\n+static const char * const receive_pack_usage[] = {\n+\tN_(\"git receive-pack \"),\n+\tNULL\n+};\n\n enum deny_action {\n \tDENY_UNCONFIGURED,\n@@ -45,12 +48,12 @@ static int unpack_limit = 100;\n static int report_status;\n static int use_sideband;\n static int use_atomic;\n-static int quiet;\n+static int quiet = 0;\n static int prefer_ofs_delta = 1;\n static int auto_update_server_info;\n static int auto_gc = 1;\n static int fix_thin = 1;\n-static int stateless_rpc;\n+static int stateless_rpc = 0;\n static const char *service_dir;\n static const char *head_name;\n static void *head_name_to_free;\n@@ -1707,45 +1710,35 @@ static int delete_only(struct command *commands)\n int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n {\n \tint advertise_refs = 0;\n-\tint i;\n \tstruct command *commands;\n \tstruct sha1_array shallow = SHA1_ARRAY_INIT;\n \tstruct sha1_array ref = SHA1_ARRAY_INIT;\n \tstruct shallow_info si;\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"quiet\")),\n+\t\tOPT_HIDDEN_BOOL(0, \"stateless-rpc\", &stateless_rpc, NULL),\n+\t\tOPT_HIDDEN_BOOL(0, \"advertise-refs\", &advertise_refs, NULL),\n+\t\t/* Hidden OPT_BOOL option */\n+\t\t{\n+\t\t\tOPTION_SET_INT, 0, \"reject-thin-pack-for-testing\", &fix_thin, NULL,\n+\t\t\tNULL, PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0,\n+\t\t},\n+\t\tOPT_END()\n+\t};\n\n \tpacket_trace_identity(\"receive-pack\");\n\n-\targv++;\n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = *argv++;\n+\targc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);\n\n-\t\tif (*arg == '-') {\n-\t\t\tif (!strcmp(arg, \"--quiet\")) {\n-\t\t\t\tquiet = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\tif (argc > 1)\n+\t\tusage_msg_opt(_(\"Too many arguments.\"), receive_pack_usage, options);\n+\tif (argc == 0)\n+\t\tusage_msg_opt(_(\"You must specify a directory.\"), receive_pack_usage, options);\n\n-\t\t\tif (!strcmp(arg, \"--advertise-refs\")) {\n-\t\t\t\tadvertise_refs = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n-\t\t\t\tstateless_rpc = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(arg, \"--reject-thin-pack-for-testing\")) {\n-\t\t\t\tfix_thin = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\tservice_dir = argv[0];\n\n-\t\t\tusage(receive_pack_usage);\n-\t\t}\n-\t\tif (service_dir)\n-\t\t\tusage(receive_pack_usage);\n-\t\tservice_dir = arg;\n-\t}\n \tif (!service_dir)\n-\t\tusage(receive_pack_usage);\n+\t\tusage_with_options(receive_pack_usage, options);\n\n \tsetup_path();\n\n--\n2.6.2\n"},{"id":"279982","messageId":"vpq60x62jvt.fsf@anie.imag.fr","threadId":"41582","inReplyTo":"1456846560-9223-1-git-send-email-tigerkid001@gmail.com","subject":"Re: [PATCH] builtin/receive-pack.c: use parse_options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-01T17:22:30Z","receivedAt":"2016-03-01T17:22:30Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Hi,\n\nThanks for your patch.\n\n\"Sidhant Sharma [:tk]\" writes:\n\n> This patch makes receive-pack use the parse_options API,\n\nWe usually avoid saying \"this patch\" and use imperative tone: talk to\nyour patch and give it orders like \"Make receive-pack use the\nparse_options API ...\". Or just skip that part which is already in the\ntitle.\n\n> @@ -45,12 +48,12 @@ static int unpack_limit = 100;\n> static int report_status;\n> static int use_sideband;\n> static int use_atomic;\n> -static int quiet;\n> +static int quiet = 0;\n\nstatic int are already initialized to 0, you don't need this explicit \"=\n0\". In the codebase of Git, we prever omiting the initialization.\n\n> +\tstruct option options[] = {\n> +\t\tOPT__QUIET(&quiet, N_(\"quiet\")),\n> +\t\tOPT_HIDDEN_BOOL(0, \"stateless-rpc\", &stateless_rpc, NULL),\n> +\t\tOPT_HIDDEN_BOOL(0, \"advertise-refs\", &advertise_refs, NULL),\n> +\t\t/* Hidden OPT_BOOL option */\n> +\t\t{\n> +\t\t\tOPTION_SET_INT, 0, \"reject-thin-pack-for-testing\", &fix_thin, NULL,\n> +\t\t\tNULL, PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0,\n> +\t\t},\n\nAfter seeing the patch, I think the code would be clearer by using\nsomething like\n\n\tOPT_HIDDEN_BOOL(0, \"reject-thin-pack-for-testing\", &reject_thin, NULL)\n\nand then use !reject_thin where the patch was using fix_thin. Turns 5\nlines into one here, and you just pay a ! later in terms of readability.\n\nStarting from here, the patch is a bit painful to read because the diff\nheuristics grouped hunks in a strange way. You may try \"git format-patch\n--patience\" or --minimal or --histogram to see if it gives a better\nresult. The final commit would be the same, but it may make review\neasier.\n\n(Not blaming you, just pointing a potentially useful hint, don't worry)\n\n> \tpacket_trace_identity(\"receive-pack\");\n>\n> -\targv++;\n> -\tfor (i = 1; i < argc; i++) {\n> -\t\tconst char *arg = *argv++;\n> +\targc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);\n>\n> -\t\tif (*arg == '-') {\n> -\t\t\tif (!strcmp(arg, \"--quiet\")) {\n> -\t\t\t\tquiet = 1;\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> +\tif (argc > 1)\n> +\t\tusage_msg_opt(_(\"Too many arguments.\"), receive_pack_usage, options);\n> +\tif (argc == 0)\n> +\t\tusage_msg_opt(_(\"You must specify a directory.\"), receive_pack_usage, options);\n\nBefore that, the loop was ensuring that service_dir was assigned once\nand only once, and now you check that you have one non-option arg and\nassign it unconditionally:\n\n> +\tservice_dir = argv[0];\n\n... so isn't this \"if\" dead code:\n\n> \tif (!service_dir)\n> -\t\tusage(receive_pack_usage);\n> +\t\tusage_with_options(receive_pack_usage, options);\n\n?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"279985","messageId":"56D5D601.8030601@gmail.com","threadId":"41582","inReplyTo":"vpq60x62jvt.fsf@anie.imag.fr","subject":"Re: [PATCH] builtin/receive-pack.c: use parse_options API","fromName":"Sidhant Sharma","fromEmail":"tigerkid001@gmail.com","sentAt":"2016-03-01T17:48:49Z","receivedAt":"2016-03-01T17:48:49Z","isPatch":true,"sender":{"key":"tigerkid001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7801881?v=4"},"body":"\n> Hi,\n>\n> Thanks for your patch.\n>\n> \"Sidhant Sharma [:tk]\" writes:\n>\n>> This patch makes receive-pack use the parse_options API,\n> We usually avoid saying \"this patch\" and use imperative tone: talk to\n> your patch and give it orders like \"Make receive-pack use the\n> parse_options API ...\". Or just skip that part which is already in the\n> title.\n>\n>> @@ -45,12 +48,12 @@ static int unpack_limit = 100;\n>> static int report_status;\n>> static int use_sideband;\n>> static int use_atomic;\n>> -static int quiet;\n>> +static int quiet = 0;\n> static int are already initialized to 0, you don't need this explicit \"=\n> 0\". In the codebase of Git, we prever omiting the initialization.\n>\n>> +\tstruct option options[] = {\n>> +\t\tOPT__QUIET(&quiet, N_(\"quiet\")),\n>> +\t\tOPT_HIDDEN_BOOL(0, \"stateless-rpc\", &stateless_rpc, NULL),\n>> +\t\tOPT_HIDDEN_BOOL(0, \"advertise-refs\", &advertise_refs, NULL),\n>> +\t\t/* Hidden OPT_BOOL option */\n>> +\t\t{\n>> +\t\t\tOPTION_SET_INT, 0, \"reject-thin-pack-for-testing\", &fix_thin, NULL,\n>> +\t\t\tNULL, PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0,\n>> +\t\t},\n> After seeing the patch, I think the code would be clearer by using\n> something like\n>\n> \tOPT_HIDDEN_BOOL(0, \"reject-thin-pack-for-testing\", &reject_thin, NULL)\n>\n> and then use !reject_thin where the patch was using fix_thin. Turns 5\n> lines into one here, and you just pay a ! later in terms of readability.\nOK, will correct the above points.\n\n>> \tpacket_trace_identity(\"receive-pack\");\n>>\n>> -\targv++;\n>> -\tfor (i = 1; i < argc; i++) {\n>> -\t\tconst char *arg = *argv++;\n>> +\targc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);\n>>\n>> -\t\tif (*arg == '-') {\n>> -\t\t\tif (!strcmp(arg, \"--quiet\")) {\n>> -\t\t\t\tquiet = 1;\n>> -\t\t\t\tcontinue;\n>> -\t\t\t}\n>> +\tif (argc > 1)\n>> +\t\tusage_msg_opt(_(\"Too many arguments.\"), receive_pack_usage, options);\n>> +\tif (argc == 0)\n>> +\t\tusage_msg_opt(_(\"You must specify a directory.\"), receive_pack_usage, options);\n> Before that, the loop was ensuring that service_dir was assigned once\n> and only once, and now you check that you have one non-option arg and\n> assign it unconditionally:\n>\n>> +\tservice_dir = argv[0];\n> ... so isn't this \"if\" dead code:\n>\n>> \tif (!service_dir)\n>> -\t\tusage(receive_pack_usage);\n>> +\t\tusage_with_options(receive_pack_usage, options);\n> ?\n>\n>\nYes, I just realized that is dead code (sorry). Removing the 'if' statement would correct that? Also, is the unconditional assignment to service_dir correct in this case, or should some other test condition be added?\n\nAnother thing I'd like to ask is when I prepare the next patch, should it be sent as reply in this thread, or as a new thread?\n\n\n\nThanks and regards,\nSidhant Sharma [:tk]\n"},{"id":"279987","messageId":"vpqio1613p1.fsf@anie.imag.fr","threadId":"41582","inReplyTo":"56D5D601.8030601@gmail.com","subject":"Re: [PATCH] builtin/receive-pack.c: use parse_options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-01T17:57:30Z","receivedAt":"2016-03-01T17:57:30Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Sidhant Sharma writes:\n\n>>> +\tif (argc > 1)\n>>> +\t\tusage_msg_opt(_(\"Too many arguments.\"), receive_pack_usage, options);\n>>> +\tif (argc == 0)\n>>> +\t\tusage_msg_opt(_(\"You must specify a directory.\"), receive_pack_usage, options);\n>> Before that, the loop was ensuring that service_dir was assigned once\n>> and only once, and now you check that you have one non-option arg and\n>> assign it unconditionally:\n>>\n>>> +\tservice_dir = argv[0];\n>> ... so isn't this \"if\" dead code:\n>>\n>>> \tif (!service_dir)\n>>> -\t\tusage(receive_pack_usage);\n>>> +\t\tusage_with_options(receive_pack_usage, options);\n>> ?\n>>\n>>\n> Yes, I just realized that is dead code (sorry). Removing the 'if'\n> statement would correct that?\n\nYes.\n\n> Also, is the unconditional assignment to service_dir correct in this\n> case, or should some other test condition be added?\n\nSince usage_msg_opt is NORETURN, it's OK: if you reach this point, you\nknow that argv[0] contains something.\n\n> Another thing I'd like to ask is when I prepare the next patch, should\n> it be sent as reply in this thread, or as a new thread?\n\nNo strict rule on that, but I usually use --in-reply-to on the root of\nthe thread for previous iteration. If you don't, include a link (e.g.\ngmane) to the previous iteration in the cover-letter.\n\nformat-patch has a -v2 option to let you get [PATCH v2 ...]\nautomatically.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"279992","messageId":"CAPig+cSSUJkkGKsfDdX7i7fdTkvGd3ppL1tdsqdB7d0hjwdOuQ@mail.gmail.com","threadId":"41582","inReplyTo":"vpqio1613p1.fsf@anie.imag.fr","subject":"Re: [PATCH] builtin/receive-pack.c: use parse_options API","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-01T18:54:13Z","receivedAt":"2016-03-01T18:54:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 1, 2016 at 12:57 PM, Matthieu Moy\n wrote:\n> Sidhant Sharma writes:\n>> Another thing I'd like to ask is when I prepare the next patch, should\n>> it be sent as reply in this thread, or as a new thread?\n>\n> No strict rule on that, but I usually use --in-reply-to on the root of\n> the thread for previous iteration. If you don't, include a link (e.g.\n> gmane) to the previous iteration in the cover-letter.\n\nIt's good manners to include a link to the previous version even if\nyou do use --in-reply-to since not all reviewers will have the\nprevious thread in their mailbox.\n"},{"id":"280000","messageId":"1456863661-22783-1-git-send-email-tigerkid001@gmail.com","threadId":"41582","inReplyTo":"1456846560-9223-1-git-send-email-tigerkid001@gmail.com","subject":"[PATCH v2] builtin/receive-pack.c: use parse_options API","fromName":"Sidhant Sharma [:tk]","fromEmail":"tigerkid001@gmail.com","sentAt":"2016-03-01T20:21:01Z","receivedAt":"2016-03-01T20:21:01Z","isPatch":true,"sender":{"key":"tigerkid001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7801881?v=4"},"body":"Make receive-pack use the parse_options API,\nbringing it more in line with send-pack and push.\n\nHelped-by: Matthieu Moy \nSigned-off-by: Sidhant Sharma [:tk] \n---\n\n Link to previous version: $gmane/288035\n\n builtin/receive-pack.c | 53 +++++++++++++++++++-------------------------------\n 1 file changed, 20 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c8e32b2..220a899 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -21,7 +21,10 @@\n #include \"sigchain.h\"\n #include \"fsck.h\"\n\n-static const char receive_pack_usage[] = \"git receive-pack \";\n+static const char * const receive_pack_usage[] = {\n+\tN_(\"git receive-pack \"),\n+\tNULL\n+};\n\n enum deny_action {\n \tDENY_UNCONFIGURED,\n@@ -49,7 +52,7 @@ static int quiet;\n static int prefer_ofs_delta = 1;\n static int auto_update_server_info;\n static int auto_gc = 1;\n-static int fix_thin = 1;\n+static int reject_thin;\n static int stateless_rpc;\n static const char *service_dir;\n static const char *head_name;\n@@ -1548,7 +1551,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\tif (fsck_objects)\n \t\t\targv_array_pushf(&child.args, \"--strict%s\",\n \t\t\t\tfsck_msg_types.buf);\n-\t\tif (fix_thin)\n+\t\tif (!reject_thin)\n \t\t\targv_array_push(&child.args, \"--fix-thin\");\n \t\tchild.out = -1;\n \t\tchild.err = err_fd;\n@@ -1707,45 +1710,29 @@ static int delete_only(struct command *commands)\n int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n {\n \tint advertise_refs = 0;\n-\tint i;\n \tstruct command *commands;\n \tstruct sha1_array shallow = SHA1_ARRAY_INIT;\n \tstruct sha1_array ref = SHA1_ARRAY_INIT;\n \tstruct shallow_info si;\n\n+\tstruct option options[] = {\n+\t\tOPT__QUIET(&quiet, N_(\"quiet\")),\n+\t\tOPT_HIDDEN_BOOL(0, \"stateless-rpc\", &stateless_rpc, NULL),\n+\t\tOPT_HIDDEN_BOOL(0, \"advertise-refs\", &advertise_refs, NULL),\n+\t\tOPT_HIDDEN_BOOL(0, \"reject-thin-pack-for-testing\", &reject_thin, NULL),\n+\t\tOPT_END()\n+\t};\n+\n \tpacket_trace_identity(\"receive-pack\");\n\n-\targv++;\n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = *argv++;\n+\targc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);\n\n-\t\tif (*arg == '-') {\n-\t\t\tif (!strcmp(arg, \"--quiet\")) {\n-\t\t\t\tquiet = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\tif (argc > 1)\n+\t\tusage_msg_opt(_(\"Too many arguments.\"), receive_pack_usage, options);\n+\tif (argc == 0)\n+\t\tusage_msg_opt(_(\"You must specify a directory.\"), receive_pack_usage, options);\n\n-\t\t\tif (!strcmp(arg, \"--advertise-refs\")) {\n-\t\t\t\tadvertise_refs = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n-\t\t\t\tstateless_rpc = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(arg, \"--reject-thin-pack-for-testing\")) {\n-\t\t\t\tfix_thin = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\tusage(receive_pack_usage);\n-\t\t}\n-\t\tif (service_dir)\n-\t\t\tusage(receive_pack_usage);\n-\t\tservice_dir = arg;\n-\t}\n-\tif (!service_dir)\n-\t\tusage(receive_pack_usage);\n+\tservice_dir = argv[0];\n\n \tsetup_path();\n\n--\n2.7.2\n"},{"id":"280001","messageId":"56D5FC0D.7080006@gmail.com","threadId":"41582","inReplyTo":"1456863661-22783-1-git-send-email-tigerkid001@gmail.com","subject":"Re: [PATCH v2] builtin/receive-pack.c: use parse_options API","fromName":"Sidhant Sharma","fromEmail":"tigerkid001@gmail.com","sentAt":"2016-03-01T20:31:09Z","receivedAt":"2016-03-01T20:31:09Z","isPatch":true,"sender":{"key":"tigerkid001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7801881?v=4"},"body":"> Starting from here, the patch is a bit painful to read because the diff\n> heuristics grouped hunks in a strange way. You may try \"git format-patch\n> --patience\" or --minimal or --histogram to see if it gives a better\n> result. The final commit would be the same, but it may make review\n> easier.\n\n\nI tried using the other algorithms, but results were same for all.\n\n\nRegards,\nSidhant Sharma [:tk]\n"},{"id":"280002","messageId":"vpqvb56yltc.fsf@anie.imag.fr","threadId":"41582","inReplyTo":"1456863661-22783-1-git-send-email-tigerkid001@gmail.com","subject":"Re: [PATCH v2] builtin/receive-pack.c: use parse_options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-01T20:39:43Z","receivedAt":"2016-03-01T20:39:43Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"Sidhant Sharma [:tk]\" writes:\n\n> Make receive-pack use the parse_options API,\n> bringing it more in line with send-pack and push.\n\nThanks. This version looks good to me.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"280011","messageId":"xmqq4mcp7t28.fsf@gitster.mtv.corp.google.com","threadId":"41582","inReplyTo":"vpqvb56yltc.fsf@anie.imag.fr","subject":"Re: [PATCH v2] builtin/receive-pack.c: use parse_options API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-01T22:05:19Z","receivedAt":"2016-03-01T22:05:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> \"Sidhant Sharma [:tk]\" writes:\n>\n>> Make receive-pack use the parse_options API,\n>> bringing it more in line with send-pack and push.\n>\n> Thanks. This version looks good to me.\n\nI'll queue this with your \"Reviewed-by:\" to 'pu', just as a\nMicroproject reward ;-). Given that the program will never see an\ninteractive use from a command line, however, I am not sure if it is\nworth actually merging it down thru 'next' to 'master'.\n\nThanks.\n"},{"id":"280035","messageId":"56D67793.5080308@gmail.com","threadId":"41582","inReplyTo":"xmqq4mcp7t28.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] builtin/receive-pack.c: use parse_options API","fromName":"Sidhant Sharma","fromEmail":"tigerkid001@gmail.com","sentAt":"2016-03-02T05:18:11Z","receivedAt":"2016-03-02T05:18:11Z","isPatch":true,"sender":{"key":"tigerkid001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7801881?v=4"},"body":"\n> Matthieu Moy writes:\n>\n>> \"Sidhant Sharma [:tk]\" writes:\n>>\n>>> Make receive-pack use the parse_options API,\n>>> bringing it more in line with send-pack and push.\n>> Thanks. This version looks good to me.\n> I'll queue this with your \"Reviewed-by:\" to 'pu', just as a\n> Microproject reward ;-). Given that the program will never see an\n> interactive use from a command line, however, I am not sure if it is\n> worth actually merging it down thru 'next' to 'master'.\n>\n> Thanks.\n\nThanks for accepting my patch :)\nNow that this one is complete, I was wondering what should I do next. Is there a list of more such microproject-like projects? I'd be very excited to contribute more patches.\n\n\nThanks and regards,\nSidhant Sharma [:tk]\n"},{"id":"280043","messageId":"vpqsi09xp7x.fsf@anie.imag.fr","threadId":"41582","inReplyTo":"xmqq4mcp7t28.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] builtin/receive-pack.c: use parse_options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-02T08:23:46Z","receivedAt":"2016-03-02T08:23:46Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> \"Sidhant Sharma [:tk]\" writes:\n>>\n>>> Make receive-pack use the parse_options API,\n>>> bringing it more in line with send-pack and push.\n>>\n>> Thanks. This version looks good to me.\n>\n> I'll queue this with your \"Reviewed-by:\" to 'pu', just as a\n> Microproject reward ;-). Given that the program will never see an\n> interactive use from a command line, however, I am not sure if it is\n> worth actually merging it down thru 'next' to 'master'.\n\nGit can certainly live without this patch and users won't see any\ndifference indeed. But the slight code reduction might be worth it:\n\n builtin/receive-pack.c | 53 +++++++++++++++++++-------------------------------\n 1 file changed, 20 insertions(+), 33 deletions(-)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"280048","messageId":"xmqqsi0945zs.fsf@gitster.mtv.corp.google.com","threadId":"41582","inReplyTo":"56D67793.5080308@gmail.com","subject":"Re: [PATCH v2] builtin/receive-pack.c: use parse_options API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-02T08:51:51Z","receivedAt":"2016-03-02T08:51:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sidhant Sharma writes:\n\n>> Matthieu Moy writes:\n>>\n>>> Thanks. This version looks good to me.\n>> I'll queue this with your \"Reviewed-by:\" to 'pu', just as a\n>> Microproject reward ;-). Given that the program will never see an\n>> interactive use from a command line, however, I am not sure if it is\n>> worth actually merging it down thru 'next' to 'master'.\n\nHaving said that, the resulting code looks more modular in that\nadding a new option or extending the behaviour of an existing option\nmay be easier with the patch going forward, so after all we might\nbe better off taking it to the production. I'll think about it a\nbit more and decide.\n\nThanks.\n"},{"id":"280057","messageId":"CACsJy8Dc38BrAHJ2t3HRdrk=A7VR7SFqc03wyajKrydsiCfoNw@mail.gmail.com","threadId":"41582","inReplyTo":"1456863661-22783-1-git-send-email-tigerkid001@gmail.com","subject":"Re: [PATCH v2] builtin/receive-pack.c: use parse_options API","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-02T09:53:54Z","receivedAt":"2016-03-02T09:53:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 2, 2016 at 3:21 AM, Sidhant Sharma [:tk]\n wrote:\n> + struct option options[] = {\n> + OPT__QUIET(&quiet, N_(\"quiet\")),\n> + OPT_HIDDEN_BOOL(0, \"stateless-rpc\", &stateless_rpc, NULL),\n> + OPT_HIDDEN_BOOL(0, \"advertise-refs\", &advertise_refs, NULL),\n> + OPT_HIDDEN_BOOL(0, \"reject-thin-pack-for-testing\", &reject_thin, NULL),\n> + OPT_END()\n> + };\n\nIf the patch is already final, don't bother. If not, I think I prefer\nto keep all these options visible (except the \"...-for-testing\"). This\ncommand is never executed directly by mere mortals. The ones that run\nit need to know all about these hidden tricks because they're\nimplementing new transports.\n\nAnother side note. I'm not so sure if we should N_() and _() strings\nin this command (same reasoning, the command's user is very likely\ndevelopers, not true users). But it does not harm to i18n-ize the\ncommand either. Slightly more work for translators, of course.\n-- \nDuy\n"},{"id":"280063","messageId":"56D6F064.20307@gmail.com","threadId":"41582","inReplyTo":"CACsJy8Dc38BrAHJ2t3HRdrk=A7VR7SFqc03wyajKrydsiCfoNw@mail.gmail.com","subject":"Re: [PATCH v2] builtin/receive-pack.c: use parse_options API","fromName":"Sidhant Sharma","fromEmail":"tigerkid001@gmail.com","sentAt":"2016-03-02T13:53:40Z","receivedAt":"2016-03-02T13:53:40Z","isPatch":true,"sender":{"key":"tigerkid001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7801881?v=4"},"body":"\n> On Wed, Mar 2, 2016 at 3:21 AM, Sidhant Sharma [:tk]\n> wrote:\n>> + struct option options[] = {\n>> + OPT__QUIET(&quiet, N_(\"quiet\")),\n>> + OPT_HIDDEN_BOOL(0, \"stateless-rpc\", &stateless_rpc, NULL),\n>> + OPT_HIDDEN_BOOL(0, \"advertise-refs\", &advertise_refs, NULL),\n>> + OPT_HIDDEN_BOOL(0, \"reject-thin-pack-for-testing\", &reject_thin, NULL),\n>> + OPT_END()\n>> + };\n> If the patch is already final, don't bother. If not, I think I prefer\n> to keep all these options visible (except the \"...-for-testing\"). This\n> command is never executed directly by mere mortals. The ones that run\n> it need to know all about these hidden tricks because they're\n> implementing new transports.\n>\n> Another side note. I'm not so sure if we should N_() and _() strings\n> in this command (same reasoning, the command's user is very likely\n> developers, not true users). But it does not harm to i18n-ize the\n> command either. Slightly more work for translators, of course.\nI can make a patch with the changes, but I think the patch has been finalized.\nShould I make one?\n\n\nRegards,\nSidhant Sharma [:tk]\n"}]}