{"thread":{"id":"10662","subject":"[PATCH] Use parseopts in builtin-fetch","startedAt":"2007-11-05T03:35:34Z","lastAt":"2007-11-05T19:48:16Z","messageCount":8,"participants":["Daniel Barkalow","Pierre Habouzit","Kristian Høgsberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"58344","messageId":"Pine.LNX.4.64.0711042233590.7357@iabervon.org","threadId":"10662","inReplyTo":null,"subject":"[PATCH] Use parseopts in builtin-fetch","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-11-05T03:35:34Z","receivedAt":"2007-11-05T03:35:34Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"Signed-off-by: Daniel Barkalow \n---\nI mostly did this and the next one for practice with the API. I'm \nimpressed that \"git fetch -vv\" is even handled correctly without anything \nspecial. Now that I've done it, assuming I did it right, it might as well \nget added to the series.\n\n builtin-fetch.c | 95 +++++++++++++++---------------------------------------\n 1 files changed, 27 insertions(+), 68 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 6b1750d..a079cb0 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -8,8 +8,12 @@\n #include \"path-list.h\"\n #include \"remote.h\"\n #include \"transport.h\"\n+#include \"parse-options.h\"\n \n-static const char fetch_usage[] = \"git-fetch [-a | --append] [--upload-pack ] [-f | --force] [--no-tags] [-t | --tags] [-k | --keep] [-u | --update-head-ok] [--depth ] [-v | --verbose] [ ...]\";\n+static const char * const fetch_usage[] = {\n+\t\"git-fetch [option] [ ...]\",\n+\tNULL\n+};\n \n static int append, force, tags, no_tags, update_head_ok, verbose, quiet;\n static char *default_rla = NULL;\n@@ -470,71 +474,21 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tint cmd_len = 0;\n \tconst char *depth = NULL, *upload_pack = NULL;\n \tint keep = 0;\n-\n-\tfor (i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n-\t\tcmd_len += strlen(arg);\n-\n-\t\tif (arg[0] != '-')\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"--append\") || !strcmp(arg, \"-a\")) {\n-\t\t\tappend = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!prefixcmp(arg, \"--upload-pack=\")) {\n-\t\t\tupload_pack = arg + 14;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--upload-pack\")) {\n-\t\t\ti++;\n-\t\t\tif (i == argc)\n-\t\t\t\tusage(fetch_usage);\n-\t\t\tupload_pack = argv[i];\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--force\") || !strcmp(arg, \"-f\")) {\n-\t\t\tforce = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--no-tags\")) {\n-\t\t\tno_tags = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--tags\") || !strcmp(arg, \"-t\")) {\n-\t\t\ttags = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--keep\") || !strcmp(arg, \"-k\")) {\n-\t\t\tkeep = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--update-head-ok\") || !strcmp(arg, \"-u\")) {\n-\t\t\tupdate_head_ok = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!prefixcmp(arg, \"--depth=\")) {\n-\t\t\tdepth = arg + 8;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--depth\")) {\n-\t\t\ti++;\n-\t\t\tif (i == argc)\n-\t\t\t\tusage(fetch_usage);\n-\t\t\tdepth = argv[i];\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--quiet\") || !strcmp(arg, \"-q\")) {\n-\t\t\tquiet = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!strcmp(arg, \"--verbose\") || !strcmp(arg, \"-v\")) {\n-\t\t\tverbose++;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tusage(fetch_usage);\n-\t}\n-\n-\tfor (j = i; j < argc; j++)\n+\tstruct option options[] = {\n+\t\tOPT__VERBOSE(&verbose),\n+\t\tOPT_BOOLEAN('a', \"append\", &append, \"append fetched data to exisitng FETCH_HEAD\"),\n+\t\tOPT_BOOLEAN('f', \"force\", &force, \"force\"),\n+\t\tOPT_BOOLEAN( 0 , \"no-tags\", &no_tags, \"don't fetch tags\"),\n+\t\tOPT_BOOLEAN('t', \"tags\", &tags, \"fetch all tags\"),\n+\t\tOPT_BOOLEAN('k', \"keep\", &keep, \"keep pack file\"),\n+\t\tOPT_STRING( 0, \"depth\", &depth, \"depth\", \"deepen the repository by commits\"),\n+\t\tOPT_BOOLEAN('u', \"update-head-ok\", &update_head_ok, \"allow updating head\"),\n+\t\tOPT_BOOLEAN('q', \"quiet\", &quiet, \"fetch silently\"),\n+\t\tOPT_STRING( 0 , \"upload-pack\", &upload_pack, \"upload-pack\", \"remote executable for git-upload-pack\"),\n+\t\tOPT_END(),\n+\t};\n+\n+\tfor (j = 1; j < argc; j++)\n \t\tcmd_len += strlen(argv[j]);\n \n \tdefault_rla = xmalloc(cmd_len + 5 + argc + 1);\n@@ -545,12 +499,16 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\trla_offset += strlen(argv[j]) + 1;\n \t}\n \n-\tif (i == argc)\n+\targc = parse_options(argc, argv, options, fetch_usage, 0);\n+\n+\tif (argc == 0)\n \t\tremote = remote_get(NULL);\n \telse\n-\t\tremote = remote_get(argv[i++]);\n+\t\tremote = remote_get(argv[0]);\n \n \ttransport = transport_get(remote, remote->url[0]);\n+\tif (!transport)\n+\t\tdie(\"couldn't get transport\");\n \tif (verbose >= 2)\n \t\ttransport->verbose = 1;\n \tif (quiet)\n@@ -565,6 +523,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (!transport->url)\n \t\tdie(\"Where do you want to fetch from today?\");\n \n+\ti = 1;\n \tif (i < argc) {\n \t\tint j = 0;\n \t\trefs = xcalloc(argc - i + 1, sizeof(const char *));\n-- \n1.5.3.5.1528.gb6568-dirty\n"},{"id":"58353","messageId":"20071105084333.GA25574@artemis.corp","threadId":"10662","inReplyTo":"Pine.LNX.4.64.0711042233590.7357@iabervon.org","subject":"Re: [PATCH] Use parseopts in builtin-fetch","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T08:43:33Z","receivedAt":"2007-11-05T08:43:33Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:\n> Signed-off-by: Daniel Barkalow \n> ---\n> I mostly did this and the next one for practice with the API. I'm \n> impressed that \"git fetch -vv\" is even handled correctly without anything \n> special. Now that I've done it, assuming I did it right, it might as well \n> get added to the series.\n\n I believe the same patches (or very similar ones) are in pu but are\nnot in next yet because they conflict with the builtin-fetch recent\nseries.\n\n see http://git.madism.org/?p=git.git;a=blobdiff;f=builtin-fetch.c;h=12b1c4;hp=6b1750d;hb=7407915;hpb=61610e6\n\n> +\t\tOPT_BOOLEAN('q', \"quiet\", &quiet, \"fetch silently\"),\n\n there is an OPT__QUIET(&quiet) for this one.\n\n> +\ti = 1;\n> \tif (i < argc) {\n> \t\tint j = 0;\n> \t\trefs = xcalloc(argc - i + 1, sizeof(const char *));\n\n this is wrong, you meant i = 0, and frankly, it's better to just strip\ni altogether.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"58354","messageId":"20071105085513.GB25574@artemis.corp","threadId":"10662","inReplyTo":"Pine.LNX.4.64.0711042233590.7357@iabervon.org","subject":"Re: [PATCH] Use parseopts in builtin-fetch","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T08:55:13Z","receivedAt":"2007-11-05T08:55:13Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:\n> I mostly did this and the next one for practice with the API. I'm \n> impressed that \"git fetch -vv\" is even handled correctly without anything \n> special.\n\n About that: OPTION_BOOLEAN increments the associated variable, to\nsupport this case specifically.\n\n The last thing that really miss in parse-options is a way to recurse\ninto a sub-array of struct option, to be able to port the generic diff\nand revision arguments.\n\n Though, there is a difficulty here that I've not yet found how to\ncircumvent tastefully: right now options take an absolute pointer to\n_the_ variable that will be filled with values. I need to be able to\nrelocate such a structure for sub-arrays for quite obvious reasons, and\nthat is quite hard to achieve without hazardous APIs. I currently lean\nin the direction of simply memdup-ing the array and do fix-ups on\n*values pointers. Though how to do that in a graceful way is not obvious\nto me yet :)\n\n This is the issue that currently prevents me from migrating any log\nand diff based, though git diff -wbBCM that doesn't work is irritating me\nmore and more every day, so I'm confident I'll find a solution soon :)\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"58391","messageId":"1194282174.13968.8.camel@hinata.boston.redhat.com","threadId":"10662","inReplyTo":"20071105085513.GB25574@artemis.corp","subject":"Re: [PATCH] Use parseopts in builtin-fetch","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-11-05T17:02:54Z","receivedAt":"2007-11-05T17:02:54Z","isPatch":true,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"On Mon, 2007-11-05 at 09:55 +0100, Pierre Habouzit wrote:\n> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:\n> > I mostly did this and the next one for practice with the API. I'm \n> > impressed that \"git fetch -vv\" is even handled correctly without anything \n> > special.\n> \n> About that: OPTION_BOOLEAN increments the associated variable, to\n> support this case specifically.\n> \n> The last thing that really miss in parse-options is a way to recurse\n> into a sub-array of struct option, to be able to port the generic diff\n> and revision arguments.\n> \n> Though, there is a difficulty here that I've not yet found how to\n> circumvent tastefully: right now options take an absolute pointer to\n> _the_ variable that will be filled with values. I need to be able to\n> relocate such a structure for sub-arrays for quite obvious reasons, and\n> that is quite hard to achieve without hazardous APIs. I currently lean\n> in the direction of simply memdup-ing the array and do fix-ups on\n> *values pointers. Though how to do that in a graceful way is not obvious\n> to me yet :)\n\nWhat about requiring sub-arrays entries to take a pointer to a struct as\ntheir 'value' and requiring that all value pointers in the sub-array be\nrelative to this struct, ie\n\n (void *) offsetof(struct diff_options, detect_rename)\n\nThen you can have something like\n\n OPT__SUBARRAY(diff_options, &rev.diff_opt);\n\nin your options array and it will fill out the specified my_diff_opts.\n\ncheers,\nKristian\n"},{"id":"58392","messageId":"Pine.LNX.4.64.0711051155330.7357@iabervon.org","threadId":"10662","inReplyTo":"20071105084333.GA25574@artemis.corp","subject":"Re: [PATCH] Use parseopts in builtin-fetch","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-11-05T17:14:47Z","receivedAt":"2007-11-05T17:14:47Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 5 Nov 2007, Pierre Habouzit wrote:\n\n> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:\n> > Signed-off-by: Daniel Barkalow \n> > ---\n> > I mostly did this and the next one for practice with the API. I'm \n> > impressed that \"git fetch -vv\" is even handled correctly without anything \n> > special. Now that I've done it, assuming I did it right, it might as well \n> > get added to the series.\n> \n> I believe the same patches (or very similar ones) are in pu but are\n> not in next yet because they conflict with the builtin-fetch recent\n> series.\n>\n> see http://git.madism.org/?p=git.git;a=blobdiff;f=builtin-fetch.c;h=12b1c4;hp=6b1750d;hb=7407915;hpb=61610e6\n\nAh, okay, forgot to look there. In any case, I was mostly looking for what \nmistakes I shouldn't make in future conversions.\n\n> > +\t\tOPT_BOOLEAN('q', \"quiet\", &quiet, \"fetch silently\"),\n> \n> there is an OPT__QUIET(&quiet) for this one.\n> \n> > +\ti = 1;\n> > \tif (i < argc) {\n> > \t\tint j = 0;\n> > \t\trefs = xcalloc(argc - i + 1, sizeof(const char *));\n> \n> this is wrong, you meant i = 0, and frankly, it's better to just strip\n> i altogether.\n\nI didn't consume the remote name's slot, and started at the next one. But \nyou're right that it's probably nicest to do to *argv++, argc-- thing and \nbe zero-based for the list afterwards. I think I need the 'i' in this \ncase, because the names get somewhat converted from the exact list given.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"58394","messageId":"Pine.LNX.4.64.0711051215260.7357@iabervon.org","threadId":"10662","inReplyTo":"20071105085513.GB25574@artemis.corp","subject":"Re: [PATCH] Use parseopts in builtin-fetch","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-11-05T17:41:04Z","receivedAt":"2007-11-05T17:41:04Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 5 Nov 2007, Pierre Habouzit wrote:\n\n> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:\n> > I mostly did this and the next one for practice with the API. I'm \n> > impressed that \"git fetch -vv\" is even handled correctly without anything \n> > special.\n> \n> About that: OPTION_BOOLEAN increments the associated variable, to\n> support this case specifically.\n> \n> The last thing that really miss in parse-options is a way to recurse\n> into a sub-array of struct option, to be able to port the generic diff\n> and revision arguments.\n> \n> Though, there is a difficulty here that I've not yet found how to\n> circumvent tastefully: right now options take an absolute pointer to\n> _the_ variable that will be filled with values. I need to be able to\n> relocate such a structure for sub-arrays for quite obvious reasons, and\n> that is quite hard to achieve without hazardous APIs. I currently lean\n> in the direction of simply memdup-ing the array and do fix-ups on\n> *values pointers. Though how to do that in a graceful way is not obvious\n> to me yet :)\n\nI'm not entirely clear as to why the \"diff options\" to an arbitrary \ncommand can't configure variables in the diff engine. The tricky thing \nwould be supporting a single command that has two sets of the same \noptions (if git log could show the same stuff in two different ways at the \nsame time, for example).\n\nBut, in any case, it should be possible for an OPT_* macro to take a \nstruct type and a member name, do the offsetof and store that, and the \ninclusion macro would take the absolute pointer to the struct.\n\nNext time I get a chance, I'll see if I can come up with something \nsuitable.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"58402","messageId":"7vir4gqzdu.fsf@gitster.siamese.dyndns.org","threadId":"10662","inReplyTo":"20071105085513.GB25574@artemis.corp","subject":"Re: [PATCH] Use parseopts in builtin-fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-05T19:13:33Z","receivedAt":"2007-11-05T19:13:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:\n>> I mostly did this and the next one for practice with the API. I'm \n>> impressed that \"git fetch -vv\" is even handled correctly without anything \n>> special.\n>\n> About that: OPTION_BOOLEAN increments the associated variable, to\n> support this case specifically.\n>\n> The last thing that really miss in parse-options is a way to recurse\n> into a sub-array of struct option, to be able to port the generic diff\n> and revision arguments.\n\nAnother micronit is I found lacking is that it is a bit too\ncumbersome to accept only a subset of integer as a value\n(e.g. \"this option takes a positive integer, not zero nor\nnegative\"). The caller can set up a callback to handle that,\nthough.\n"},{"id":"58406","messageId":"20071105194816.GA8939@artemis.corp","threadId":"10662","inReplyTo":"7vir4gqzdu.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Use parseopts in builtin-fetch","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-11-05T19:48:16Z","receivedAt":"2007-11-05T19:48:16Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Mon, Nov 05, 2007 at 07:13:33PM +0000, Junio C Hamano wrote:\n> Pierre Habouzit writes:\n> \n> > On Mon, Nov 05, 2007 at 03:35:34AM +0000, Daniel Barkalow wrote:\n> >> I mostly did this and the next one for practice with the API. I'm \n> >> impressed that \"git fetch -vv\" is even handled correctly without anything \n> >> special.\n> >\n> > About that: OPTION_BOOLEAN increments the associated variable, to\n> > support this case specifically.\n> >\n> > The last thing that really miss in parse-options is a way to recurse\n> > into a sub-array of struct option, to be able to port the generic diff\n> > and revision arguments.\n> \n> Another micronit is I found lacking is that it is a bit too\n> cumbersome to accept only a subset of integer as a value\n> (e.g. \"this option takes a positive integer, not zero nor\n> negative\"). The caller can set up a callback to handle that,\n> though.\n\nAs a general rule we may want to have some kind of \"ranged\" integers.\nbut like you said the callback is always here for that, and if we begin\nto have dozens of those we may consider creating a new type for those if\nneeded.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"}]}