{"thread":{"id":"42371","subject":"[PATCH] upload-pack.c: use of parse-options API","startedAt":"2016-05-18T16:40:19Z","lastAt":"2016-06-01T15:49:43Z","messageCount":26,"participants":["Antoine Queru","Jeff King","Matthieu Moy","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"286895","messageId":"20160518164019.26443-1-Antoine.Queru@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":null,"subject":"[PATCH] upload-pack.c: use of parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-05-18T16:40:19Z","receivedAt":"2016-05-18T16:40:19Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"Option parsing now uses the parser API instead of a local parser.\nCode is now more compact.\n\nSigned-off-by: Antoine Queru \nSigned-off-by: Matthieu Moy \n---\nThis is our first project as a warm up. It was taken from the GSoC microproject list. \n upload-pack.c | 51 ++++++++++++++++-----------------------------------\n 1 file changed, 16 insertions(+), 35 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex dc802a0..80f65eb 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -14,8 +14,12 @@\n #include \"sigchain.h\"\n #include \"version.h\"\n #include \"string-list.h\"\n+#include \"parse-options.h\"\n \n-static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=] \";\n+static const char * const upload_pack_usage[] = {\n+\tN_(\"git upload-pack [--strict] [--timeout=] \"),\n+\tNULL\n+};\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -820,49 +824,26 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n int main(int argc, char **argv)\n {\n \tchar *dir;\n-\tint i;\n \tint strict = 0;\n+\tstruct option options[] = {\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_BOOL(0, \"strict\", &strict, NULL),\n+\t\tOPT_INTEGER(0, \"timeout\", &timeout, NULL),\n+\t\tOPT_END()\n+\t};\n \n \tgit_setup_gettext();\n \n \tpacket_trace_identity(\"upload-pack\");\n \tgit_extract_argv0_path(argv[0]);\n \tcheck_replace_refs = 0;\n-\n-\tfor (i = 1; i < argc; i++) {\n-\t\tchar *arg = argv[i];\n-\n-\t\tif (arg[0] != '-')\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"--advertise-refs\")) {\n-\t\t\tadvertise_refs = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n-\t\t\tstateless_rpc = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--strict\")) {\n-\t\t\tstrict = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with(arg, \"--timeout=\")) {\n-\t\t\ttimeout = atoi(arg+10);\n-\t\t\tdaemon_mode = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\tif (i != argc-1)\n-\t\tusage(upload_pack_usage);\n-\n+\targc = parse_options(argc, (const char **)argv, NULL, options, upload_pack_usage, 0);\n+\tif (timeout != 0)\n+\t\tdaemon_mode = 1;\n \tsetup_path();\n \n-\tdir = argv[i];\n+\tdir = argv[0];\n \n \tif (!enter_repo(dir, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n-- \n2.8.2.403.gaa9c3f6\n"},{"id":"286906","messageId":"20160518180800.GC5796@sigill.intra.peff.net","threadId":"42371","inReplyTo":"20160518164019.26443-1-Antoine.Queru@ensimag.grenoble-inp.fr","subject":"Re: [PATCH] upload-pack.c: use of parse-options API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T18:08:00Z","receivedAt":"2016-05-18T18:08:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 18, 2016 at 06:40:19PM +0200, Antoine Queru wrote:\n\n> Option parsing now uses the parser API instead of a local parser.\n> Code is now more compact.\n\nSounds like a good goal.\n\n> -static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=] \";\n> +static const char * const upload_pack_usage[] = {\n> +\tN_(\"git upload-pack [--strict] [--timeout=] \"),\n> +\tNULL\n> +};\n\nDo we need to enumerate the options here now? The usage message should\nlist the options from \"struct options\", which make these redundant.\n\nSomething like:\n\n git -upload-pack [options] \n\nprobably makes more sense.\n\nOf course, it's hard to read the usage message because...\n\n> +\tstruct option options[] = {\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_BOOL(0, \"strict\", &strict, NULL),\n> +\t\tOPT_INTEGER(0, \"timeout\", &timeout, NULL),\n> +\t\tOPT_END()\n> +\t};\n\nYou've left the description text for each of these options as NULL, so\nrunning \"git-upload-pack -h\" segfaults.\n\nI'm not sure whether it is worth hiding the first two options. We\ntypically hide \"internal\" options like this for user-facing programs, so\nas not to clutter the \"-h\" output. But upload-pack isn't a user-facing\nprogram. Anybody who is calling it directly with \"-h\" may be interested\nin even its more esoteric options.\n\n> -\tfor (i = 1; i < argc; i++) {\n> -\t\tchar *arg = argv[i];\n> -\n> -\t\tif (arg[0] != '-')\n> -\t\t\tbreak;\n> -\t\tif (!strcmp(arg, \"--advertise-refs\")) {\n> -\t\t\tadvertise_refs = 1;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n> -\t\t\tstateless_rpc = 1;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tif (!strcmp(arg, \"--strict\")) {\n> -\t\t\tstrict = 1;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tif (starts_with(arg, \"--timeout=\")) {\n> -\t\t\ttimeout = atoi(arg+10);\n> -\t\t\tdaemon_mode = 1;\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tif (!strcmp(arg, \"--\")) {\n> -\t\t\ti++;\n> -\t\t\tbreak;\n> -\t\t}\n> -\t}\n\nA common pitfall in converting a for-loop to parse-options is when there\nis anything stateful in the parsing loop. But I don't see anything here.\nThe trickiest thing is that --timeout implies daemon_mode. You've\nhandled that below as:\n\n> +\tif (timeout != 0)\n> +\t\tdaemon_mode = 1;\n\nI think that is OK. It would not be correct if, for example, some other\ncode set \"timeout\" to a non-zero value (e.g., a config option), but I\ndon't see that here.\n\nAs a style nit, we usually spell comparison-with-zero as just:\n\n if (timeout)\n\tdaemon_mode = 1;\n\nLooking at at daemon_mode, though, it appears that it cannot be set in\nany other way except here. And it does nothing except to disable\nprogress in some cases. Weird. There may be some opportunities for\nrefactoring or simplification there, but I that is outside the scope of\nyour patch.\n\n> +\targc = parse_options(argc, (const char **)argv, NULL, options, upload_pack_usage, 0);\n\nPerhaps this is a good opportunity to use \"const\" in the declaration of\nmain(), as most other git programs do. Then you can drop this cast.\n\n> \tsetup_path();\n> \n> -\tdir = argv[i];\n> +\tdir = argv[0];\n> \n> \tif (!enter_repo(dir, strict))\n> \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n\nPrior to your patch, we used to check:\n\n - if (i != argc-1)\n - usage(upload_pack_usage);\n\nwhich ensured that \"dir\" was non-NULL. But with your patch, we may pass\nNULL to enter_repo. It fortunately catches this, but then we pass the\nNULL to die, which can segfault (though on glibc systems, stdio is kind\nenough to replace it with the \"(null)\").\n\nRelated, we silently accept extra arguments after your patch (whereas\nbefore we showed the usage message). You probably want to check \"argc ==\n1\", and otherwise show the usage message.\n\n-Peff\n"},{"id":"286973","messageId":"721240639.4127450.1463652631815.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":"20160518180800.GC5796@sigill.intra.peff.net","subject":"Re: [PATCH] upload-pack.c: use of parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-05-19T10:10:31Z","receivedAt":"2016-05-19T10:10:31Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"Thanks for your input.\n\n> > -static const char upload_pack_usage[] = \"git upload-pack [--strict]\n> > [--timeout=] \";\n> > +static const char * const upload_pack_usage[] = {\n> > +\tN_(\"git upload-pack [--strict] [--timeout=] \"),\n> > +\tNULL\n> > +};\n> \n> Do we need to enumerate the options here now? The usage message should\n> list the options from \"struct options\", which make these redundant.\n> \n> Something like:\n> \n> git -upload-pack [options] \n> \n> probably makes more sense.\n> \n\nYes, you are right.\n\n> Of course, it's hard to read the usage message because...\n> \n> > +\tstruct option options[] = {\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_BOOL(0, \"strict\", &strict, NULL),\n> > +\t\tOPT_INTEGER(0, \"timeout\", &timeout, NULL),\n> > +\t\tOPT_END()\n> > +\t};\n> \n> You've left the description text for each of these options as NULL, so\n> running \"git-upload-pack -h\" segfaults.\n> \n> I'm not sure whether it is worth hiding the first two options. We\n> typically hide \"internal\" options like this for user-facing programs, so\n> as not to clutter the \"-h\" output. But upload-pack isn't a user-facing\n> program. Anybody who is calling it directly with \"-h\" may be interested\n> in even its more esoteric options.\n> \n\nIn fact, to do this, I looked at builtin/receive-pack.c, where the parser API\nwas already implemented, and these first two options were hidden. There were \nalso no description for any options, so I thought it was not needed. Maybe we \ncould update this file too ?\n\n> As a style nit, we usually spell comparison-with-zero as just:\n> \n> if (timeout)\n> \tdaemon_mode = 1;\n\nBecause timeout is an int, I personnally think it is more understable to \ntreat it as it. But I'll update itfor consistency. \n\n> > +\targc = parse_options(argc, (const char **)argv, NULL, options,\n> > upload_pack_usage, 0);\n> \n> Perhaps this is a good opportunity to use \"const\" in the declaration of\n> main(), as most other git programs do. Then you can drop this cast.\n> \n\nOk.\n\n> > \tsetup_path();\n> > \n> > -\tdir = argv[i];\n> > +\tdir = argv[0];\n> > \n> > \tif (!enter_repo(dir, strict))\n> > \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n> \n> Prior to your patch, we used to check:\n> \n> - if (i != argc-1)\n> - usage(upload_pack_usage);\n> \n> which ensured that \"dir\" was non-NULL. But with your patch, we may pass\n> NULL to enter_repo. It fortunately catches this, but then we pass the\n> NULL to die, which can segfault (though on glibc systems, stdio is kind\n> enough to replace it with the \"(null)\").\n> \n> Related, we silently accept extra arguments after your patch (whereas\n> before we showed the usage message). You probably want to check \"argc ==\n> 1\", and otherwise show the usage message.\n\nOk.\n\n-Antoine\n"},{"id":"286977","messageId":"20160519115725.GB3050@sigill.intra.peff.net","threadId":"42371","inReplyTo":"721240639.4127450.1463652631815.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH] upload-pack.c: use of parse-options API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-19T11:57:26Z","receivedAt":"2016-05-19T11:57:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 19, 2016 at 12:10:31PM +0200, Antoine Queru wrote:\n\n> > I'm not sure whether it is worth hiding the first two options. We\n> > typically hide \"internal\" options like this for user-facing programs, so\n> > as not to clutter the \"-h\" output. But upload-pack isn't a user-facing\n> > program. Anybody who is calling it directly with \"-h\" may be interested\n> > in even its more esoteric options.\n> \n> In fact, to do this, I looked at builtin/receive-pack.c, where the parser API\n> was already implemented, and these first two options were hidden. There were \n> also no description for any options, so I thought it was not needed. Maybe we \n> could update this file too ?\n\nYeah, I don't think it's that bad to hide them, and perhaps consistency\nwith receive-pack is better. AFAICT, descriptions for hidden options are\npointless; they are never shown (in fact, it seems like OPT_HIDDEN_BOOL\nprobably shouldn't even take such an option?).\n\nBut the non-hidden options _do_ need non-NULL descriptions.\n\n-Peff\n"},{"id":"286985","messageId":"800962099.10901198.1463668599601.JavaMail.zimbra@imag.fr","threadId":"42371","inReplyTo":"20160519115725.GB3050@sigill.intra.peff.net","subject":"Re: [PATCH] upload-pack.c: use of parse-options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-19T14:36:39Z","receivedAt":"2016-05-19T14:36:39Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King wrote:\n> On Thu, May 19, 2016 at 12:10:31PM +0200, Antoine Queru wrote:\n> \n> > > I'm not sure whether it is worth hiding the first two options. We\n> > > typically hide \"internal\" options like this for user-facing programs, so\n> > > as not to clutter the \"-h\" output. But upload-pack isn't a user-facing\n> > > program. Anybody who is calling it directly with \"-h\" may be interested\n> > > in even its more esoteric options.\n> > \n> > In fact, to do this, I looked at builtin/receive-pack.c, where the parser\n> > API\n> > was already implemented, and these first two options were hidden. There\n> > were\n> > also no description for any options, so I thought it was not needed. Maybe\n> > we\n> > could update this file too ?\n> \n> Yeah, I don't think it's that bad to hide them, and perhaps consistency\n> with receive-pack is better.\n\nIIRC, part of the reason receive-pack has hidden options is that it was a\nGSoC microproject, and writing an accurate description is much harder than\nwhat we expect from a microproject.\n\nIOW, I'm all for un-hiding these options, but that shouldn't be a requirement\nfor a beginner's project.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"286990","messageId":"20160519153903.22104-1-Antoine.Queru@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":"20160518164019.26443-1-Antoine.Queru@ensimag.grenoble-inp.fr","subject":"[PATCH v2] upload-pack.c: use of parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-05-19T15:39:03Z","receivedAt":"2016-05-19T15:39:03Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"Option parsing now uses the parser API instead of a local parser.\nCode is now more compact.\nDescription for -stateless-rpc and --advertise-refs\ncome from the commit (gmane/131517) where there were implemented.\n\nSigned-off-by: Antoine Queru \nSigned-off-by: Matthieu Moy \n---\n\ndiff v1 v2:\nUsage display \"[options]\" instead of \"[--strict] [--timeout=]\".\n\"argv\" is now \"const char **\".\n\"-stateless-rpc and --advertise-refs\" are no more hidden.\nDescription has been added for every option.\nUsage is displayed if there is not exactly one non option argument.\n\nIf we agree on not having hidden option anymore, I will update the doc. \n upload-pack.c | 62 +++++++++++++++++++++++++----------------------------------\n 1 file changed, 26 insertions(+), 36 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex dc802a0..a8617ac 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -14,8 +14,12 @@\n #include \"sigchain.h\"\n #include \"version.h\"\n #include \"string-list.h\"\n+#include \"parse-options.h\"\n \n-static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=] \";\n+static const char * const upload_pack_usage[] = {\n+\tN_(\"git upload-pack [options] \"),\n+\tNULL\n+};\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n-int main(int argc, char **argv)\n+int main(int argc, const char **argv)\n {\n-\tchar *dir;\n-\tint i;\n+\tconst char *dir;\n \tint strict = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc,\n+\t\t\t N_(\"may perform only a single read-write cycle with stdin and stdout\")),\n+\t\tOPT_BOOL(0, \"advertise-refs\", &advertise_refs,\n+\t\t\t N_(\"only the initial ref advertisement is output, program exits immediately\")),\n+\t\tOPT_BOOL(0, \"strict\", &strict,\n+\t\t\t N_(\"do not try /.git/ if is no Git directory\")),\n+\t\tOPT_INTEGER(0, \"timeout\", &timeout,\n+\t\t\t N_(\"interrupt transfer after seconds of inactivity\")),\n+\t\tOPT_END()\n+\t};\n \n \tgit_setup_gettext();\n \n@@ -829,41 +843,17 @@ int main(int argc, char **argv)\n \tgit_extract_argv0_path(argv[0]);\n \tcheck_replace_refs = 0;\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tchar *arg = argv[i];\n-\n-\t\tif (arg[0] != '-')\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"--advertise-refs\")) {\n-\t\t\tadvertise_refs = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n-\t\t\tstateless_rpc = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--strict\")) {\n-\t\t\tstrict = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with(arg, \"--timeout=\")) {\n-\t\t\ttimeout = atoi(arg+10);\n-\t\t\tdaemon_mode = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\tif (i != argc-1)\n-\t\tusage(upload_pack_usage);\n+\targc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);\n+\t\n+\tif (argc != 1)\n+\t\tusage_with_options(upload_pack_usage, options);\n \n-\tsetup_path();\n+\tif (timeout)\n+\t\tdaemon_mode = 1;\n \n-\tdir = argv[i];\n+\tsetup_path();\t\n \n+\tdir = argv[0];\n \tif (!enter_repo(dir, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \n-- \n2.8.2.403.gf2352ca\n"},{"id":"286993","messageId":"431335092.10912437.1463674220365.JavaMail.zimbra@imag.fr","threadId":"42371","inReplyTo":"20160519153903.22104-1-Antoine.Queru@ensimag.grenoble-inp.fr","subject":"Re: [PATCH v2] upload-pack.c: use of parse-options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-19T16:10:20Z","receivedAt":"2016-05-19T16:10:20Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Antoine.Queru@ensimag.grenoble-inp.fr wrote:\n> Option parsing now uses the parser API instead of a local parser.\n> Code is now more compact.\n> Description for -stateless-rpc and --advertise-refs\n> come from the commit (gmane/131517)\n\nPlease, use a real commit id instead of a Gmane link.\n\nWe don't know how long Gmane will remain up, but a self\nreference from Git's history to itself will always remain valid.\n\nThe following alias is handy for this:\n\n[alias]\n whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short\n\nIn your case it would allow writing: \n\nDescription for --stateless-rpc and --advertise-refs is taken\nfrom commit 42526b4 (Add stateless RPC options to upload-pack,\nreceive-pack, 2009-10-30).\n\n> diff v1 v2:\n\nWe usually say \"diff\" to refer to an actual diff. I'd write \"changes since v1\" here.\n\n> +\t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc,\n> +\t\t\t N_(\"may perform only a single read-write cycle with stdin and stdout\")),\n\nIt's weird to define what an option does with \"may\". It's a\nproperty of --stateless-rpc, but does not really define it.\n\n> +\tif (argc != 1)\n> +\t\tusage_with_options(upload_pack_usage, options);\n> \n> -\tsetup_path();\n> +\tif (timeout)\n> +\t\tdaemon_mode = 1;\n> \n> -\tdir = argv[i];\n> +\tsetup_path();\n> \n> +\tdir = argv[0];\n\nNot a problem with your code, but the patch shows \"setup_path()\"\nas moved while it is not really. Maybe using \"send-email\n--patience\" or some other diff option could make the patch nicer.\nNot really important as it does not change the final state.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"286994","messageId":"xmqqa8jmknc7.fsf@gitster.mtv.corp.google.com","threadId":"42371","inReplyTo":"431335092.10912437.1463674220365.JavaMail.zimbra@imag.fr","subject":"Re: [PATCH v2] upload-pack.c: use of parse-options API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T16:46:16Z","receivedAt":"2016-05-19T16:46:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Antoine.Queru@ensimag.grenoble-inp.fr wrote:\n>> Option parsing now uses the parser API instead of a local parser.\n>> Code is now more compact.\n>> Description for -stateless-rpc and --advertise-refs\n>> come from the commit (gmane/131517)\n>\n> Please, use a real commit id instead of a Gmane link.\n>\n> We don't know how long Gmane will remain up, but a self\n> reference from Git's history to itself will always remain valid.\n>\n> The following alias is handy for this:\n>\n> [alias]\n> whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short\n>\n> In your case it would allow writing: \n>\n> Description for --stateless-rpc and --advertise-refs is taken\n> from commit 42526b4 (Add stateless RPC options to upload-pack,\n> receive-pack, 2009-10-30).\n\nGood suggestion; a real question may be how you went from\n$gmane/131517 to 42526b4 (I vaguely recall that you have and publish\na database of sort; this would be a good place to advertise it ;-).\n\n>\n>> diff v1 v2:\n>\n> We usually say \"diff\" to refer to an actual diff. I'd write \"changes since v1\" here.\n>\n>> +\t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc,\n>> +\t\t\t N_(\"may perform only a single read-write cycle with stdin and stdout\")),\n>\n> It's weird to define what an option does with \"may\". It's a\n> property of --stateless-rpc, but does not really define it.\n\nIf this \"may\" were to express \"The program might or might not do\nthis, and we do not know what it would do until we try\", then it\nindeed would be weird. But the way the word \"may\" was used in\n42526b4 is \"the program is ALLOWED to do only a single exchange\".\n\nI agree that the phrasing is bad, in the sense that it can be\nmisread as a non-definition; perhaps\n\n quit after a single request/response exchange\n\nor something?\n"},{"id":"286995","messageId":"xmqq60uakmr4.fsf@gitster.mtv.corp.google.com","threadId":"42371","inReplyTo":"431335092.10912437.1463674220365.JavaMail.zimbra@imag.fr","subject":"Re: [PATCH v2] upload-pack.c: use of parse-options API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-19T16:58:55Z","receivedAt":"2016-05-19T16:58:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>> +\tif (argc != 1)\n>> +\t\tusage_with_options(upload_pack_usage, options);\n>> \n>> -\tsetup_path();\n>> +\tif (timeout)\n>> +\t\tdaemon_mode = 1;\n>> \n>> -\tdir = argv[i];\n>> +\tsetup_path();\n>> \n>> +\tdir = argv[0];\n>\n> Not a problem with your code, but the patch shows \"setup_path()\"\n> as moved while it is not really. Maybe using \"send-email\n> --patience\" or some other diff option could make the patch nicer.\n\nEncouraging use of \"send-email\" with \"--patience\" is a losing\napproach if your goal is to present more reviewable diff, isn't it?\nUsing \"send-email\" as if it is a front-end of \"format-patch\" means\nyou lose the opportunity for the final proof-reading (not just\nfinding typoes in the message, but the shape of diff, like you are\npointing out).\n\nUsing \"format-patch --patience\" or some other diff option, and pick\nthe best one to give to \"send-email\" would indeed be a way to do so.\n\n> Not really important as it does not change the final state.\n\nI wondered if this is an example of real-world fallout from\n0018da1^2~1 (xdiff: implement empty line chunk heuristic,\n2016-04-19), but it does not seem to be so.\n\nWhat is happening is that Antoine's patch (which is slightly\ndifferent from what you quoted above) has trailing whitespace after\n\"setup_path();\", so it indeed is the original setup_path(); is\nremoved, a few lines were inserted, argv[i] reference is removed\nand then a totally different \"setup_path();\t\" was added there.\n\nWith that whitespace-breakage corrected, the resulting patch ends\nmore like this:\n\n+\tif (argc != 1)\n+\t\tusage_with_options(upload_pack_usage, options);\n \n-\tif (i != argc-1)\n-\t\tusage(upload_pack_usage);\n+\tif (timeout)\n+\t\tdaemon_mode = 1;\n \n \tsetup_path();\n \n-\tdir = argv[i];\n-\n+\tdir = argv[0];\n \tif (!enter_repo(dir, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \nwhich is more reasonable.\n\nSo in the end, this was not \"Not a problem with your code\" ;-) It\nwas.\n"},{"id":"287070","messageId":"1446970271.10941827.1463727190993.JavaMail.zimbra@imag.fr","threadId":"42371","inReplyTo":"xmqq60uakmr4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] upload-pack.c: use of parse-options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-20T06:53:10Z","receivedAt":"2016-05-20T06:53:10Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> Encouraging use of \"send-email\" with \"--patience\" is a losing\n> approach if your goal is to present more reviewable diff, isn't it?\n> Using \"send-email\" as if it is a front-end of \"format-patch\" means\n> you lose the opportunity for the final proof-reading (not just\n> finding typoes in the message, but the shape of diff, like you are\n> pointing out).\n> \n> Using \"format-patch --patience\" or some other diff option, and pick\n> the best one to give to \"send-email\" would indeed be a way to do so.\n\nIt's a matter of taste. My flow is \"send-email-only\", I do as much as\npossible in-tree, and when I notice something to do during \"git send-email\",\nI just abort and retry. So \"Oops, looks ugly, I'll try with another\noption\" is OK in this flow.\n\n> What is happening is that Antoine's patch (which is slightly\n> different from what you quoted above) has trailing whitespace after\n> \"setup_path();\"\n\nNice catch!\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287071","messageId":"779725885.10941844.1463727315713.JavaMail.zimbra@imag.fr","threadId":"42371","inReplyTo":"xmqqa8jmknc7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] upload-pack.c: use of parse-options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-20T06:55:15Z","receivedAt":"2016-05-20T06:55:15Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio Hamano wrote:\n> Matthieu Moy writes:\n> \n> > Antoine.Queru@ensimag.grenoble-inp.fr wrote:\n> >> come from the commit (gmane/131517)\n> >\n> > Please, use a real commit id instead of a Gmane link.\n> >\n> > We don't know how long Gmane will remain up, but a self\n> > reference from Git's history to itself will always remain valid.\n> >\n> > The following alias is handy for this:\n> >\n> > [alias]\n> > whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short\n> >\n> > In your case it would allow writing:\n> >\n> > Description for --stateless-rpc and --advertise-refs is taken\n> > from commit 42526b4 (Add stateless RPC options to upload-pack,\n> > receive-pack, 2009-10-30).\n> \n> Good suggestion; a real question may be how you went from\n> $gmane/131517 to 42526b4 (I vaguely recall that you have and publish\n> a database of sort; this would be a good place to advertise it ;-).\n\nI don't (I think someone has, but I'm not this someone).\n\nI just \"git log --grep\"-ed the subject line in Git's history.\n\n> quit after a single request/response exchange\n\nSeems much clearer to me.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287106","messageId":"xmqq60u8fz96.fsf@gitster.mtv.corp.google.com","threadId":"42371","inReplyTo":"1446970271.10941827.1463727190993.JavaMail.zimbra@imag.fr","subject":"Re: [PATCH v2] upload-pack.c: use of parse-options API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T16:52:21Z","receivedAt":"2016-05-20T16:52:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>> Using \"format-patch --patience\" or some other diff option, and pick\n>> the best one to give to \"send-email\" would indeed be a way to do so.\n>\n> It's a matter of taste. My flow is \"send-email-only\", I do as much as\n> possible in-tree, and when I notice something to do during \"git send-email\",\n> I just abort and retry. So \"Oops, looks ugly, I'll try with another\n> option\" is OK in this flow.\n\nAh, I didn't consider \"final-review in send-email and aborting\".\n\nI agree that it is just the matter of preference, if it is easy to\nreview everything that you would be sending out, and decide to\nabort. I just thought that final review would be cumbersome in that\nflow.\n"},{"id":"287241","messageId":"20160523130231.8725-1-Antoine.Queru@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":"20160519153903.22104-1-Antoine.Queru@ensimag.grenoble-inp.fr","subject":"[PATCH v3] upload-pack.c: use of parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-05-23T13:02:31Z","receivedAt":"2016-05-23T13:02:31Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"Option parsing now uses the parser API instead of a local parser.\nCode is now more compact.\nDescription for -stateless-rpc and --advertise-refs\ncome from the commit 42526b4 (Add stateless RPC options to upload-pack, receive-pack, 2009-10-30).\n\nSigned-off-by: Antoine Queru \nSigned-off-by: Matthieu Moy \n---\nChange since v2:\n\nCommit message updated. \nWhiteSpace removed after the \"setup_path();\".\nDescription for --stateless-rpc changed to : \"quit after a single request/response exchange\".\n\nFor the description, i don't really understand what this option is doing, so i just copy pasted\nJunio's sentence. \n upload-pack.c | 59 +++++++++++++++++++++++++----------------------------------\n 1 file changed, 25 insertions(+), 34 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex dc802a0..2d53c7b 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -14,8 +14,12 @@\n #include \"sigchain.h\"\n #include \"version.h\"\n #include \"string-list.h\"\n+#include \"parse-options.h\"\n \n-static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=] \";\n+static const char * const upload_pack_usage[] = {\n+\tN_(\"git upload-pack [options] \"),\n+\tNULL\n+};\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n-int main(int argc, char **argv)\n+int main(int argc, const char **argv)\n {\n-\tchar *dir;\n-\tint i;\n+\tconst char *dir;\n \tint strict = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc,\n+\t\t\t N_(\"quit after a single request/response exchange\")),\n+\t\tOPT_BOOL(0, \"advertise-refs\", &advertise_refs,\n+\t\t\t N_(\"only the initial ref advertisement is output, program exits immediately\")),\n+\t\tOPT_BOOL(0, \"strict\", &strict,\n+\t\t\t N_(\"do not try /.git/ if is no Git directory\")),\n+\t\tOPT_INTEGER(0, \"timeout\", &timeout,\n+\t\t\t N_(\"interrupt transfer after seconds of inactivity\")),\n+\t\tOPT_END()\n+\t};\n \n \tgit_setup_gettext();\n \n@@ -829,40 +843,17 @@ int main(int argc, char **argv)\n \tgit_extract_argv0_path(argv[0]);\n \tcheck_replace_refs = 0;\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tchar *arg = argv[i];\n-\n-\t\tif (arg[0] != '-')\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"--advertise-refs\")) {\n-\t\t\tadvertise_refs = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n-\t\t\tstateless_rpc = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--strict\")) {\n-\t\t\tstrict = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with(arg, \"--timeout=\")) {\n-\t\t\ttimeout = atoi(arg+10);\n-\t\t\tdaemon_mode = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\targc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);\n+\t\n+\tif (argc != 1)\n+\t\tusage_with_options(upload_pack_usage, options);\n \n-\tif (i != argc-1)\n-\t\tusage(upload_pack_usage);\n+\tif (timeout)\n+\t\tdaemon_mode = 1;\n \n \tsetup_path();\n \n-\tdir = argv[i];\n+\tdir = argv[0];\n \n \tif (!enter_repo(dir, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n-- \n2.8.2.403.gf2352ca\n"},{"id":"287265","messageId":"vpqy470fy8h.fsf@anie.imag.fr","threadId":"42371","inReplyTo":"20160523130231.8725-1-Antoine.Queru@ensimag.grenoble-inp.fr","subject":"Re: [PATCH v3] upload-pack.c: use of parse-options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-23T18:03:26Z","receivedAt":"2016-05-23T18:03:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Antoine Queru writes:\n\n> Option parsing now uses the parser API instead of a local parser.\n> Code is now more compact.\n> Description for -stateless-rpc and --advertise-refs\n> come from the commit 42526b4 (Add stateless RPC options to upload-pack, receive-pack, 2009-10-30).\n\nWe usually wrap commit messages before 80 columns, and separate\nparagraphs with blank lines. Have a look at \"git log --no-merges\" and\ncompare with your message.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287677","messageId":"20160527141628.1677-1-Antoine.Queru@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":"20160523130231.8725-1-Antoine.Queru@ensimag.grenoble-inp.fr","subject":"[PATCH v4] upload-pack.c: use of parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-05-27T14:16:28Z","receivedAt":"2016-05-27T14:16:28Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"Option parsing now uses the parser API instead of a local parser.\nCode is now more compact.\n\nDescription for --stateless-rpc and --advertise-refs come from\nthe commit 42526b4 (Add stateless RPC options to upload-pack,\nreceive-pack, 2009-10-30).\n\nSigned-off-by: Antoine Queru \nSigned-off-by: Matthieu Moy \n---\n\nChange since v3 :\n\nCommit message is now well formated.\nUpdated documentation.\n\n Documentation/git-upload-pack.txt | 17 +++++++++--\n upload-pack.c | 59 +++++++++++++++++----------------------\n 2 files changed, 40 insertions(+), 36 deletions(-)\n\ndiff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\nindex 0abc806..22ddf8a 100644\n--- a/Documentation/git-upload-pack.txt\n+++ b/Documentation/git-upload-pack.txt\n@@ -9,8 +9,8 @@ git-upload-pack - Send objects packed back to git-fetch-pack\n SYNOPSIS\n --------\n [verse]\n-'git-upload-pack' [--strict] [--timeout=] \n-\n+'git-upload-pack' [--strict] [--timeout=] [--stateless-rpc]\n+\t\t [--advertise-refs] \n DESCRIPTION\n -----------\n Invoked by 'git fetch-pack', learns what\n@@ -31,6 +31,19 @@ OPTIONS\n --timeout=::\n \tInterrupt transfer after seconds of inactivity.\n \n+--stateless-rpc::\n+\tthe programs now assume they may perform only a single read-write\n+\tcycle with stdin and stdout. This fits with the HTTP POST request\n+\tprocessing model where a program may read the request, write a\n+\tresponse, and must exit.\n+\n+--advertise-refs::\n+\tWhen --advertise-refs is passed as a command line parameter only\n+ \tthe initial ref advertisement is output, and the program exits\n+ \timmediately. This fits with the HTTP GET request model, where\n+ \tno request content is received but a response must be produced.\n+\n+\n ::\n \tThe repository to sync from.\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex dc802a0..2d53c7b 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -14,8 +14,12 @@\n #include \"sigchain.h\"\n #include \"version.h\"\n #include \"string-list.h\"\n+#include \"parse-options.h\"\n \n-static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=] \";\n+static const char * const upload_pack_usage[] = {\n+\tN_(\"git upload-pack [options] \"),\n+\tNULL\n+};\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n-int main(int argc, char **argv)\n+int main(int argc, const char **argv)\n {\n-\tchar *dir;\n-\tint i;\n+\tconst char *dir;\n \tint strict = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc,\n+\t\t\t N_(\"quit after a single request/response exchange\")),\n+\t\tOPT_BOOL(0, \"advertise-refs\", &advertise_refs,\n+\t\t\t N_(\"only the initial ref advertisement is output, program exits immediately\")),\n+\t\tOPT_BOOL(0, \"strict\", &strict,\n+\t\t\t N_(\"do not try /.git/ if is no Git directory\")),\n+\t\tOPT_INTEGER(0, \"timeout\", &timeout,\n+\t\t\t N_(\"interrupt transfer after seconds of inactivity\")),\n+\t\tOPT_END()\n+\t};\n \n \tgit_setup_gettext();\n \n@@ -829,40 +843,17 @@ int main(int argc, char **argv)\n \tgit_extract_argv0_path(argv[0]);\n \tcheck_replace_refs = 0;\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tchar *arg = argv[i];\n-\n-\t\tif (arg[0] != '-')\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"--advertise-refs\")) {\n-\t\t\tadvertise_refs = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n-\t\t\tstateless_rpc = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--strict\")) {\n-\t\t\tstrict = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with(arg, \"--timeout=\")) {\n-\t\t\ttimeout = atoi(arg+10);\n-\t\t\tdaemon_mode = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\targc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);\n+\t\n+\tif (argc != 1)\n+\t\tusage_with_options(upload_pack_usage, options);\n \n-\tif (i != argc-1)\n-\t\tusage(upload_pack_usage);\n+\tif (timeout)\n+\t\tdaemon_mode = 1;\n \n \tsetup_path();\n \n-\tdir = argv[i];\n+\tdir = argv[0];\n \n \tif (!enter_repo(dir, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n-- \n2.8.2.403.g31e708f\n"},{"id":"287680","messageId":"vpqpos7o8n2.fsf@anie.imag.fr","threadId":"42371","inReplyTo":"20160527141628.1677-1-Antoine.Queru@ensimag.grenoble-inp.fr","subject":"Re: [PATCH v4] upload-pack.c: use of parse-options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-27T14:52:49Z","receivedAt":"2016-05-27T14:52:49Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"> Subject: [PATCH v4] upload-pack.c: use of parse-options API\n\nI'd drop the \"of\" and say just \"use parse-options API\"\n\nAntoine Queru writes:\n\n> Description for --stateless-rpc and --advertise-refs come from\n> the commit 42526b4 (Add stateless RPC options to upload-pack,\n\nNit: s/the//\n\n> +--advertise-refs::\n> +\tWhen --advertise-refs is passed as a command line parameter only\n> + \tthe initial ref advertisement is output, and the program exits\n> + \timmediately. This fits with the HTTP GET request model, where\n> + \tno request content is received but a response must be produced.\n\nAnother nit: mix space/tab in indentation (renders badly on some\nconfigurations).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287690","messageId":"CAPig+cRipGvwqXQhY1koOLR2h=VcHqrTMhp_zbphYtaabQ0qAw@mail.gmail.com","threadId":"42371","inReplyTo":"20160527141628.1677-1-Antoine.Queru@ensimag.grenoble-inp.fr","subject":"Re: [PATCH v4] upload-pack.c: use of parse-options API","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-27T16:59:14Z","receivedAt":"2016-05-27T16:59:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 27, 2016 at 10:16 AM, Antoine Queru\n wrote:\n> upload-pack.c: use of parse-options API\n\nMatthieu already mentioned that this should use imperative mood:\n\n upload-pack: use parse-options API\n\n> Option parsing now uses the parser API instead of a local parser.\n> Code is now more compact.\n\nImperative:\n\n Use the parse-options API rather than a hand-rolled\n option parser.\n\nThat the code becomes more compact is a nice result of this change,\nbut not particularly important, thus not really worth a mention in the\ncommit message.\n\n> Description for --stateless-rpc and --advertise-refs come from\n> the commit 42526b4 (Add stateless RPC options to upload-pack,\n> receive-pack, 2009-10-30).\n\ns/from the commit/from/\n\n> Signed-off-by: Antoine Queru \n> Signed-off-by: Matthieu Moy \n> ---\n> diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\n> @@ -31,6 +31,19 @@ OPTIONS\n> +--stateless-rpc::\n> + the programs now assume they may perform only a single read-write\n\ns/the/The/\n\nAlso, to what \"programs\" does this refer? And what does \"now\" mean here?\n\n> + cycle with stdin and stdout. This fits with the HTTP POST request\n> + processing model where a program may read the request, write a\n> + response, and must exit.\n> +\n> +--advertise-refs::\n> + When --advertise-refs is passed as a command line parameter only\n\nThe entire \"When ... parameter\" bit is redundant, isn't it? Why not\njust say \"Perform only...\"?\n\n> + the initial ref advertisement is output, and the program exits\n> + immediately. This fits with the HTTP GET request model, where\n> + no request content is received but a response must be produced.\n> +\n> +\n\nStyle: Drop the extra blank line.\n\n> ::\n> The repository to sync from.\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> @@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n> -int main(int argc, char **argv)\n> +int main(int argc, const char **argv)\n> {\n> - char *dir;\n> - int i;\n> + const char *dir;\n> int strict = 0;\n> + struct option options[] = {\n> + OPT_BOOL(0, \"stateless-rpc\", &stateless_rpc,\n> + N_(\"quit after a single request/response exchange\")),\n> + OPT_BOOL(0, \"advertise-refs\", &advertise_refs,\n> + N_(\"only the initial ref advertisement is output, program exits immediately\")),\n\nPossible rewrite: \"exit immediately after initial ref advertisement\"\n\n> + OPT_BOOL(0, \"strict\", &strict,\n> + N_(\"do not try /.git/ if is no Git directory\")),\n\nUse of OPT_BOOL introduces a --no-strict option which didn't exist\nbefore. Does this need to be documented? (Genuine question.)\n\n> + OPT_INTEGER(0, \"timeout\", &timeout,\n> + N_(\"interrupt transfer after seconds of inactivity\")),\n> + OPT_END()\n> + };\n"},{"id":"287820","messageId":"607762163.175275.1464618463522.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":"CAPig+cRipGvwqXQhY1koOLR2h=VcHqrTMhp_zbphYtaabQ0qAw@mail.gmail.com","subject":"Re: [PATCH v4] upload-pack.c: use of parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-05-30T14:27:43Z","receivedAt":"2016-05-30T14:27:43Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"Hello Eric,\nThank you for answer. Your remarks have been added in the next version.\n> > + OPT_BOOL(0, \"strict\", &strict,\n> > + N_(\"do not try /.git/ if is\n> > no Git directory\")),\n> \n> Use of OPT_BOOL introduces a --no-strict option which didn't exist\n> before. Does this need to be documented? (Genuine question.)\n> \nYou're right. I've added it in the documentation.\n\nAntoine\n \n"},{"id":"287824","messageId":"20160530145347.15643-1-antoine.queru@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":"20160527141628.1677-1-Antoine.Queru@ensimag.grenoble-inp.fr","subject":"[PATCH v5] upload-pack.c: use parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-05-30T14:53:47Z","receivedAt":"2016-05-30T14:53:47Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"From: Antoine Queru \n\nUse the parse-options API rather than a hand-rolled option parser.\n\nDescription for --stateless-rpc and --advertise-refs come from\n42526b4 (Add stateless RPC options to upload-pack,\nreceive-pack, 2009-10-30).\n\nSigned-off-by: Antoine Queru \nSigned-off-by: Matthieu Moy \n---\nChange since v4:\nImperative mood for the commit message.\n\"Code is now more compact.\" removed.\nDocumentation for option is now clearer.\nHelp message for \"--advertise-refs\" rewritten.\n\"no\" for \"strict\" option added to the doc.\n\nDocumentation/git-upload-pack.txt | 16 +++++++++--\n upload-pack.c | 59 +++++++++++++++++----------------------\n 2 files changed, 38 insertions(+), 37 deletions(-)\n\ndiff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\nindex 0abc806..8a03bed 100644\n--- a/Documentation/git-upload-pack.txt\n+++ b/Documentation/git-upload-pack.txt\n@@ -9,8 +9,8 @@ git-upload-pack - Send objects packed back to git-fetch-pack\n SYNOPSIS\n --------\n [verse]\n-'git-upload-pack' [--strict] [--timeout=] \n-\n+'git-upload-pack' [--[no-]strict] [--timeout=] [--stateless-rpc]\n+\t\t [--advertise-refs] \n DESCRIPTION\n -----------\n Invoked by 'git fetch-pack', learns what\n@@ -25,12 +25,22 @@ repository. For push operations, see 'git send-pack'.\n OPTIONS\n -------\n \n---strict::\n+--[no-]strict::\n \tDo not try /.git/ if is no Git directory.\n \n --timeout=::\n \tInterrupt transfer after seconds of inactivity.\n \n+--stateless-rpc::\n+\tPerform only a single read-write cycle with stdin and stdout.\n+\tThis fits with the HTTP POST request processing model where\n+\ta program may read the request, write a response, and must exit.\n+\n+--advertise-refs::\n+\tOnly the initial ref advertisement is output, and the program exits\n+ \timmediately. This fits with the HTTP GET request model, where\n+ \tno request content is received but a response must be produced.\n+\n ::\n \tThe repository to sync from.\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex dc802a0..083d068 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -14,8 +14,12 @@\n #include \"sigchain.h\"\n #include \"version.h\"\n #include \"string-list.h\"\n+#include \"parse-options.h\"\n \n-static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=] \";\n+static const char * const upload_pack_usage[] = {\n+\tN_(\"git upload-pack [options] \"),\n+\tNULL\n+};\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n-int main(int argc, char **argv)\n+int main(int argc, const char **argv)\n {\n-\tchar *dir;\n-\tint i;\n+\tconst char *dir;\n \tint strict = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc,\n+\t\t\t N_(\"quit after a single request/response exchange\")),\n+\t\tOPT_BOOL(0, \"advertise-refs\", &advertise_refs,\n+\t\t\t N_(\"exit immediately after intial ref advertisement\")),\n+\t\tOPT_BOOL(0, \"strict\", &strict,\n+\t\t\t N_(\"do not try /.git/ if is no Git directory\")),\n+\t\tOPT_INTEGER(0, \"timeout\", &timeout,\n+\t\t\t N_(\"interrupt transfer after seconds of inactivity\")),\n+\t\tOPT_END()\n+\t};\n \n \tgit_setup_gettext();\n \n@@ -829,40 +843,17 @@ int main(int argc, char **argv)\n \tgit_extract_argv0_path(argv[0]);\n \tcheck_replace_refs = 0;\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tchar *arg = argv[i];\n-\n-\t\tif (arg[0] != '-')\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"--advertise-refs\")) {\n-\t\t\tadvertise_refs = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n-\t\t\tstateless_rpc = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--strict\")) {\n-\t\t\tstrict = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with(arg, \"--timeout=\")) {\n-\t\t\ttimeout = atoi(arg+10);\n-\t\t\tdaemon_mode = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\targc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);\n+\t\n+\tif (argc != 1)\n+\t\tusage_with_options(upload_pack_usage, options);\n \n-\tif (i != argc-1)\n-\t\tusage(upload_pack_usage);\n+\tif (timeout)\n+\t\tdaemon_mode = 1;\n \n \tsetup_path();\n \n-\tdir = argv[i];\n+\tdir = argv[0];\n \n \tif (!enter_repo(dir, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n-- \n2.8.2.403.g897e5a5\n"},{"id":"287825","messageId":"vpqa8j77fgo.fsf@anie.imag.fr","threadId":"42371","inReplyTo":"20160530145347.15643-1-antoine.queru@ensimag.grenoble-inp.fr","subject":"Re: [PATCH v5] upload-pack.c: use parse-options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-30T15:06:31Z","receivedAt":"2016-05-30T15:06:31Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Antoine Queru writes:\n\n> From: Antoine Queru \n\n[ Insert here the sentence I've been repeating a lot lately about this\n useless From ;-) ]\n\n> Documentation/git-upload-pack.txt | 16 +++++++++--\n> upload-pack.c | 59 +++++++++++++++++----------------------\n> 2 files changed, 38 insertions(+), 37 deletions(-)\n\nThe patch contains a few whitespace errors:\n\nDocumentation/git-upload-pack.txt:41: space before tab in indent.\n+ immediately. This fits with the HTTP GET request model, where\nDocumentation/git-upload-pack.txt:42: space before tab in indent.\n+ no request content is received but a response must be produced.\nupload-pack.c:846: trailing whitespace.\n+ \n\nYou should notice them immediately if you use \"git add -p\" (big red\nwarning in the patch hunk), and you can see all of them with \"git diff\n--check\" or \"git show --check\".\n\nNot sure if it deserves a reroll. Junio?\n\nOther than that, the patch is now\n\nReviewed-by: Matthieu Moy \n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287849","messageId":"xmqqd1o38i0m.fsf@gitster.mtv.corp.google.com","threadId":"42371","inReplyTo":"20160530145347.15643-1-antoine.queru@ensimag.grenoble-inp.fr","subject":"Re: [PATCH v5] upload-pack.c: use parse-options API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-30T19:26:01Z","receivedAt":"2016-05-30T19:26:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Queru writes:\n\n> From: Antoine Queru \n\nDon't you want to be known to the project as the email that matches\nyour Signed-off-by: line?\n\n> Use the parse-options API rather than a hand-rolled option parser.\n>\n> Description for --stateless-rpc and --advertise-refs come from\n> 42526b4 (Add stateless RPC options to upload-pack,\n> receive-pack, 2009-10-30).\n>\n> Signed-off-by: Antoine Queru \n> Signed-off-by: Matthieu Moy \n> ---\n> ---strict::\n> +--[no-]strict::\n> \tDo not try /.git/ if is no Git directory.\n\nNot a new problem, but \"is no Git ...\" may technically be correct,\nbut it would be easier to read if phrased \"is not a Git ...\" or\nsomething like that.\n\n\tI am NOT asking _you_ to change/fix that in this patch. It\n\tis just a note for \"a low hanging fruit\" for people to pick\n\tup with a separate patch.\n\n> +--stateless-rpc::\n> +\tPerform only a single read-write cycle with stdin and stdout.\n> +\tThis fits with the HTTP POST request processing model where\n> +\ta program may read the request, write a response, and must exit.\n> +\n> +--advertise-refs::\n> +\tOnly the initial ref advertisement is output, and the program exits\n> + \timmediately. This fits with the HTTP GET request model, where\n> + \tno request content is received but a response must be produced.\n> +\n\nGood.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index dc802a0..083d068 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -14,8 +14,12 @@\n> #include \"sigchain.h\"\n> #include \"version.h\"\n> #include \"string-list.h\"\n> +#include \"parse-options.h\"\n> \n> -static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=] \";\n> +static const char * const upload_pack_usage[] = {\n> +\tN_(\"git upload-pack [options] \"),\n> +\tNULL\n> +};\n\nOutput from \"git grep -e '\\[option' -e '\\[]\" would be more in line with the established\nconvention than \"[options]\".\n\n\tI personally wish if all these hits from the above grep\n\tconsistently spelled it as \"[options]\", because there is not\n\tmuch gained by enclosing the word in <> to highlight it as a\n\tplaceholder. An argument that \"it is done for uniformity\n\twith descriptions for other non option arguments, i.e. not\n\tto special case 'options'\" would not hold water, e.g. in\n\n builtin/merge.c: N_(\"git merge [] [...]\"),\n\n\t is still special-cased in that it implies multiple\n\tthings, unlike \"...\" notation that has to explicitly\n\tsay that can have multiple. [...] would have been\n\tjustifiable with the \"make it uniform with non-option args\",\n\tthough.\n\n\tBUT this patch is not about \"make usage string better\"\n\tpatch, so I do NOT want you to switch upload-pack's usage\n\tstring to use the \"options is special anyway, so let's not\n\twaste two display columns with enclosing <>\" convention.\n\nSo, in conclusion, \"git upload-pack [] \".\n\nThe remainder of the patch looked OK to me.\n\nThanks.\n"},{"id":"287887","messageId":"766819198.192241.1464688434974.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":"xmqqd1o38i0m.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5] upload-pack.c: use parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-05-31T09:53:54Z","receivedAt":"2016-05-31T09:53:54Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"Hello Junio, thanks for your answer. The next version (and hopefully the last)\nwill come fast.\n\n> > From: Antoine Queru \n>\n> Don't you want to be known to the project as the email that matches\n> your Signed-off-by: line?\n\nThanks for noticing it, it will be updated.\n \n> > +--[no-]strict::\n> > \tDo not try /.git/ if is no Git directory.\n> \n> Not a new problem, but \"is no Git ...\" may technically be correct,\n> but it would be easier to read if phrased \"is not a Git ...\" or\n> something like that.\n> \n> \tI am NOT asking _you_ to change/fix that in this patch. It\n> \tis just a note for \"a low hanging fruit\" for people to pick\n> \tup with a separate patch.\n> \n\nOk, so this will not be changed.\n \n\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index dc802a0..083d068 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -14,8 +14,12 @@\n> > #include \"sigchain.h\"\n> > #include \"version.h\"\n> > #include \"string-list.h\"\n> > +#include \"parse-options.h\"\n> > \n> > -static const char upload_pack_usage[] = \"git upload-pack [--strict]\n> > [--timeout=] \";\n> > +static const char * const upload_pack_usage[] = {\n> > +\tN_(\"git upload-pack [options] \"),\n> > +\tNULL\n> > +};\n> \n> Output from \"git grep -e '\\[option' -e '\\[ that \"[]\" would be more in line with the established\n> convention than \"[options]\".\n> \n> \tI personally wish if all these hits from the above grep\n> \tconsistently spelled it as \"[options]\", because there is not\n> \tmuch gained by enclosing the word in <> to highlight it as a\n> \tplaceholder. An argument that \"it is done for uniformity\n> \twith descriptions for other non option arguments, i.e. not\n> \tto special case 'options'\" would not hold water, e.g. in\n> \n> builtin/merge.c: N_(\"git merge [] [...]\"),\n> \n> \t is still special-cased in that it implies multiple\n> \tthings, unlike \"...\" notation that has to explicitly\n> \tsay that can have multiple. [...] would have been\n> \tjustifiable with the \"make it uniform with non-option args\",\n> \tthough.\n> \n> \tBUT this patch is not about \"make usage string better\"\n> \tpatch, so I do NOT want you to switch upload-pack's usage\n> \tstring to use the \"options is special anyway, so let's not\n> \twaste two display columns with enclosing <>\" convention.\n> \n> So, in conclusion, \"git upload-pack [] \".\n>\n\nOk, so it will be fixed. I didn't notice this convention, my goal \nwasn't to change it, my bad. \n"},{"id":"287889","messageId":"20160531095708.14941-1-Antoine.Queru@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":"20160530145347.15643-1-antoine.queru@ensimag.grenoble-inp.fr","subject":"[PATCH v6] upload-pack.c: use parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-05-31T09:57:08Z","receivedAt":"2016-05-31T09:57:08Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"Use the parse-options API rather than a hand-rolled option parser.\n\nDescription for --stateless-rpc and --advertise-refs come from\n42526b4 (Add stateless RPC options to upload-pack,\nreceive-pack, 2009-10-30).\n\nSigned-off-by: Antoine Queru \nSigned-off-by: Matthieu Moy \n---\nChange since v5 :\n\nSigned-off fixed.\nWhitespace in doc removed.\n\"[options]\" replaced by \"[]\" in usage.\n\n Documentation/git-upload-pack.txt | 16 ++++++++---\n upload-pack.c | 57 +++++++++++++++++----------------------\n 2 files changed, 37 insertions(+), 36 deletions(-)\n\ndiff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\nindex 0abc806..822ad59 100644\n--- a/Documentation/git-upload-pack.txt\n+++ b/Documentation/git-upload-pack.txt\n@@ -9,8 +9,8 @@ git-upload-pack - Send objects packed back to git-fetch-pack\n SYNOPSIS\n --------\n [verse]\n-'git-upload-pack' [--strict] [--timeout=] \n-\n+'git-upload-pack' [--[no-]strict] [--timeout=] [--stateless-rpc]\n+\t\t [--advertise-refs] \n DESCRIPTION\n -----------\n Invoked by 'git fetch-pack', learns what\n@@ -25,12 +25,22 @@ repository. For push operations, see 'git send-pack'.\n OPTIONS\n -------\n \n---strict::\n+--[no-]strict::\n \tDo not try /.git/ if is no Git directory.\n \n --timeout=::\n \tInterrupt transfer after seconds of inactivity.\n \n+--stateless-rpc::\n+\tPerform only a single read-write cycle with stdin and stdout.\n+\tThis fits with the HTTP POST request processing model where\n+\ta program may read the request, write a response, and must exit.\n+\n+--advertise-refs::\n+\tOnly the initial ref advertisement is output, and the program exits\n+\timmediately. This fits with the HTTP GET request model, where\n+\tno request content is received but a response must be produced.\n+\n ::\n \tThe repository to sync from.\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex dc802a0..d653404 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -14,8 +14,12 @@\n #include \"sigchain.h\"\n #include \"version.h\"\n #include \"string-list.h\"\n+#include \"parse-options.h\"\n \n-static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=] \";\n+static const char * const upload_pack_usage[] = {\n+\tN_(\"git upload-pack [] \"),\n+\tNULL\n+};\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n-int main(int argc, char **argv)\n+int main(int argc, const char **argv)\n {\n-\tchar *dir;\n-\tint i;\n+\tconst char *dir;\n \tint strict = 0;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"stateless-rpc\", &stateless_rpc,\n+\t\t\t N_(\"quit after a single request/response exchange\")),\n+\t\tOPT_BOOL(0, \"advertise-refs\", &advertise_refs,\n+\t\t\t N_(\"exit immediately after intial ref advertisement\")),\n+\t\tOPT_BOOL(0, \"strict\", &strict,\n+\t\t\t N_(\"do not try /.git/ if is no Git directory\")),\n+\t\tOPT_INTEGER(0, \"timeout\", &timeout,\n+\t\t\t N_(\"interrupt transfer after seconds of inactivity\")),\n+\t\tOPT_END()\n+\t};\n \n \tgit_setup_gettext();\n \n@@ -829,40 +843,17 @@ int main(int argc, char **argv)\n \tgit_extract_argv0_path(argv[0]);\n \tcheck_replace_refs = 0;\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tchar *arg = argv[i];\n+\targc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);\n \n-\t\tif (arg[0] != '-')\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"--advertise-refs\")) {\n-\t\t\tadvertise_refs = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--stateless-rpc\")) {\n-\t\t\tstateless_rpc = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--strict\")) {\n-\t\t\tstrict = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (starts_with(arg, \"--timeout=\")) {\n-\t\t\ttimeout = atoi(arg+10);\n-\t\t\tdaemon_mode = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\tif (argc != 1)\n+\t\tusage_with_options(upload_pack_usage, options);\n \n-\tif (i != argc-1)\n-\t\tusage(upload_pack_usage);\n+\tif (timeout)\n+\t\tdaemon_mode = 1;\n \n \tsetup_path();\n \n-\tdir = argv[i];\n+\tdir = argv[0];\n \n \tif (!enter_repo(dir, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n-- \n2.8.2.403.gef8af9c.dirty\n"},{"id":"287890","messageId":"vpqbn3mxy9v.fsf@anie.imag.fr","threadId":"42371","inReplyTo":"xmqqd1o38i0m.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5] upload-pack.c: use parse-options API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-05-31T11:27:56Z","receivedAt":"2016-05-31T11:27:56Z","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> Antoine Queru writes:\n>\n>> From: Antoine Queru \n>> Signed-off-by: Antoine Queru \n>\n> Don't you want to be known to the project as the email that matches\n> your Signed-off-by: line?\n\nActually, that's even: don't you want to use a valid email address in\nyour Signed-off-by: ? ;-)\n\n@ensimag.grenoble-inp.fr => student's current adress\n@grenoble-inp.org => students lifelong address. If not already done,\n students can already set it up to redirect to their\n current address.\n@grenoble-inp.fr => only for staff (i.e. me but not students).\n\nI have a preference for the @grenoble-inp.org in the From and\nSigned-off-by as the Git history will remain after the current adress\nbecome invalid.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"287918","messageId":"xmqqlh2q5erd.fsf@gitster.mtv.corp.google.com","threadId":"42371","inReplyTo":"vpqbn3mxy9v.fsf@anie.imag.fr","subject":"Re: [PATCH v5] upload-pack.c: use parse-options API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-31T17:16:54Z","receivedAt":"2016-05-31T17:16:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Actually, that's even: don't you want to use a valid email address in\n> your Signed-off-by: ? ;-)\n>\n> @ensimag.grenoble-inp.fr => student's current adress\n> @grenoble-inp.org => students lifelong address. If not already done,\n> students can already set it up to redirect to their\n> current address.\n> @grenoble-inp.fr => only for staff (i.e. me but not students).\n>\n> I have a preference for the @grenoble-inp.org in the From and\n> Signed-off-by as the Git history will remain after the current adress\n> become invalid.\n\nI was wondering about that \"ensimag.\" part myself as I had a vague\nrecollection of your mentioning it in your class projects in\nprevious years.\n\nAntoine? I can munge the author and sign-off in your v6 to name you\nas\n\n Antoine Queru \n\nif you want locally, or I can queue it as-is. What's your\npreference?\n"},{"id":"288011","messageId":"1221452211.282731.1464796183821.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"42371","inReplyTo":"xmqqlh2q5erd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5] upload-pack.c: use parse-options API","fromName":"Antoine Queru","fromEmail":"antoine.queru@ensimag.grenoble-inp.fr","sentAt":"2016-06-01T15:49:43Z","receivedAt":"2016-06-01T15:49:43Z","isPatch":true,"sender":{"key":"antoine.queru@ensimag.grenoble-inp.fr","avatar":null},"body":"> Matthieu Moy writes:\n> \n> > Actually, that's even: don't you want to use a valid email address in\n> > your Signed-off-by: ? ;-)\n> >\n> > @ensimag.grenoble-inp.fr => student's current adress\n> > @grenoble-inp.org => students lifelong address. If not already done,\n> > students can already set it up to redirect to their\n> > current address.\n> > @grenoble-inp.fr => only for staff (i.e. me but not students).\n> >\n> > I have a preference for the @grenoble-inp.org in the From and\n> > Signed-off-by as the Git history will remain after the current adress\n> > become invalid.\n> \n> I was wondering about that \"ensimag.\" part myself as I had a vague\n> recollection of your mentioning it in your class projects in\n> previous years.\n> \n> Antoine? I can munge the author and sign-off in your v6 to name you\n> as\n> \n> Antoine Queru \n> \n> if you want locally, or I can queue it as-is. What's your\n> preference?\n> \n\nOk thanks for telling me, we weren't sure what to do about this.\nSo yeah, it will be better with Antoine.Queru@grenoble-inp.org in the v6, thanks.\n"}]}