{"thread":{"id":"21631","subject":"[PATCH RFC] git remote: Separate usage strings for subcommands","startedAt":"2009-11-15T21:43:40Z","lastAt":"2009-11-17T01:39:09Z","messageCount":3,"participants":["Tim Henigan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"127641","messageId":"32c343770911151343y1ed0aak83b43a4ab57eb6ab@mail.gmail.com","threadId":"21631","inReplyTo":null,"subject":"[PATCH RFC] git remote: Separate usage strings for subcommands","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2009-11-15T21:43:40Z","receivedAt":"2009-11-15T21:43:40Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"When the usage string for a subcommand must be printed,\nonly print the information relevant to that command.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nThis patch is based on:\nhttp://article.gmane.org/gmane.comp.version-control.git/132953\n\nAll usage strings are still only located at the top of file.  However,\nseparate usage\nstring arrays have been created for each subcommand.\n\nDoes this look like a sane way to structure the code?\n\nIf anyone else should be added to the CC list, please let me know.\n\n\n builtin-remote.c |   57 +++++++++++++++++++++++++++++++++--------------------\n 1 files changed, 35 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 0777dd7..ec65a4b 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -7,18 +7,35 @@\n #include \"run-command.h\"\n #include \"refs.h\"\n\n+#define REMOTE_BARE_USAGE\t\"git remote [-v | --verbose]\"\n+#define REMOTE_ADD_USAGE\t\"git remote add [-t <branch>] [-m <master>]\n[-f] [--mirror] <name> <url>\"\n+#define REMOTE_RENAME_USAGE\t\"git remote rename <old> <new>\"\n+#define REMOTE_RM_USAGE\t\t\"git remote rm <name>\"\n+#define REMOTE_SETHEAD_USAGE\t\"git remote set-head <name> [-a | -d | <branch>]\"\n+#define REMOTE_SHOW_USAGE\t\"git remote show [-n] <name>\"\n+#define REMOTE_PRUNE_USAGE\t\"git remote prune [-n | --dry-run] <name>\"\n+#define REMOTE_UPDATE_USAGE\t\"git remote [-v | --verbose] update [-p |\n--prune] [group]\"\n+\n static const char * const builtin_remote_usage[] = {\n-\t\"git remote [-v | --verbose]\",\n-\t\"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>\",\n-\t\"git remote rename <old> <new>\",\n-\t\"git remote rm <name>\",\n-\t\"git remote set-head <name> [-a | -d | <branch>]\",\n-\t\"git remote show [-n] <name>\",\n-\t\"git remote prune [-n | --dry-run] <name>\",\n-\t\"git remote [-v | --verbose] update [-p | --prune] [group]\",\n+\tREMOTE_BARE_USAGE,\n+\tREMOTE_ADD_USAGE,\n+\tREMOTE_RENAME_USAGE,\n+\tREMOTE_RM_USAGE,\n+\tREMOTE_SETHEAD_USAGE,\n+\tREMOTE_SHOW_USAGE,\n+\tREMOTE_PRUNE_USAGE,\n+\tREMOTE_UPDATE_USAGE,\n \tNULL\n };\n\n+static const char * const builtin_remote_add_usage[] = {\nREMOTE_ADD_USAGE, NULL };\n+static const char * const builtin_remote_rename_usage[] = {\nREMOTE_RENAME_USAGE, NULL };\n+static const char * const builtin_remote_rm_usage[] = {\nREMOTE_RM_USAGE, NULL };\n+static const char * const builtin_remote_sethead_usage[] = {\nREMOTE_SETHEAD_USAGE, NULL };\n+static const char * const builtin_remote_show_usage[] = {\nREMOTE_SHOW_USAGE, NULL };\n+static const char * const builtin_remote_prune_usage[] = {\nREMOTE_PRUNE_USAGE, NULL };\n+static const char * const builtin_remote_update_usage[] = {\nREMOTE_UPDATE_USAGE, NULL };\n+\n #define GET_REF_STATES (1<<0)\n #define GET_HEAD_NAMES (1<<1)\n #define GET_PUSH_REF_STATES (1<<2)\n@@ -70,7 +87,6 @@ static int add(int argc, const char **argv)\n \tint i;\n\n \tstruct option options[] = {\n-\t\tOPT_GROUP(\"add specific options\"),\n \t\tOPT_BOOLEAN('f', \"fetch\", &fetch, \"fetch the remote branches\"),\n \t\tOPT_CALLBACK('t', \"track\", &track, \"branch\",\n \t\t\t\"branch(es) to track\", opt_parse_track),\n@@ -79,11 +95,11 @@ static int add(int argc, const char **argv)\n \t\tOPT_END()\n \t};\n\n-\targc = parse_options(argc, argv, NULL, options, builtin_remote_usage,\n+\targc = parse_options(argc, argv, NULL, options, builtin_remote_add_usage,\n \t\t\t     0);\n\n \tif (argc < 2)\n-\t\tusage_with_options(builtin_remote_usage, options);\n+\t\tusage_with_options(builtin_remote_add_usage, options);\n\n \tname = argv[0];\n \turl = argv[1];\n@@ -540,7 +556,7 @@ static int mv(int argc, const char **argv)\n \tint i;\n\n \tif (argc != 3)\n-\t\tusage_with_options(builtin_remote_usage, options);\n+\t\tusage_with_options(builtin_remote_rename_usage, options);\n\n \trename.old = argv[1];\n \trename.new = argv[2];\n@@ -681,7 +697,7 @@ static int rm(int argc, const char **argv)\n \tint i, result;\n\n \tif (argc != 2)\n-\t\tusage_with_options(builtin_remote_usage, options);\n+\t\tusage_with_options(builtin_remote_rm_usage, options);\n\n \tremote = remote_get(argv[1]);\n \tif (!remote)\n@@ -984,7 +1000,7 @@ static int show(int argc, const char **argv)\n \tstruct string_list info_list = { NULL, 0, 0, 0 };\n \tstruct show_info info;\n\n-\targc = parse_options(argc, argv, NULL, options, builtin_remote_usage,\n+\targc = parse_options(argc, argv, NULL, options, builtin_remote_show_usage,\n \t\t\t     0);\n\n \tif (argc < 1)\n@@ -1088,7 +1104,7 @@ static int set_head(int argc, const char **argv)\n \t\t\t    \"delete refs/remotes/<name>/HEAD\"),\n \t\tOPT_END()\n \t};\n-\targc = parse_options(argc, argv, NULL, options, builtin_remote_usage,\n+\targc = parse_options(argc, argv, NULL, options, builtin_remote_sethead_usage,\n \t\t\t     0);\n \tif (argc)\n \t\tstrbuf_addf(&buf, \"refs/remotes/%s/HEAD\", argv[0]);\n@@ -1114,7 +1130,7 @@ static int set_head(int argc, const char **argv)\n \t\tif (delete_ref(buf.buf, NULL, REF_NODEREF))\n \t\t\tresult |= error(\"Could not delete %s\", buf.buf);\n \t} else\n-\t\tusage_with_options(builtin_remote_usage, options);\n+\t\tusage_with_options(builtin_remote_sethead_usage, options);\n\n \tif (head_name) {\n \t\tunsigned char sha1[20];\n@@ -1138,16 +1154,15 @@ static int prune(int argc, const char **argv)\n {\n \tint dry_run = 0, result = 0;\n \tstruct option options[] = {\n-\t\tOPT_GROUP(\"prune specific options\"),\n \t\tOPT__DRY_RUN(&dry_run),\n \t\tOPT_END()\n \t};\n\n-\targc = parse_options(argc, argv, NULL, options, builtin_remote_usage,\n+\targc = parse_options(argc, argv, NULL, options, builtin_remote_prune_usage,\n \t\t\t     0);\n\n \tif (argc < 1)\n-\t\tusage_with_options(builtin_remote_usage, options);\n+\t\tusage_with_options(builtin_remote_prune_usage, options);\n\n \tfor (; argc; argc--, argv++)\n \t\tresult |= prune_remote(*argv, dry_run);\n@@ -1228,13 +1243,12 @@ static int update(int argc, const char **argv)\n \tstruct string_list list = { NULL, 0, 0, 0 };\n \tstatic const char *default_argv[] = { NULL, \"default\", NULL };\n \tstruct option options[] = {\n-\t\tOPT_GROUP(\"update specific options\"),\n \t\tOPT_BOOLEAN('p', \"prune\", &prune,\n \t\t\t    \"prune remotes after fetching\"),\n \t\tOPT_END()\n \t};\n\n-\targc = parse_options(argc, argv, NULL, options, builtin_remote_usage,\n+\targc = parse_options(argc, argv, NULL, options, builtin_remote_update_usage,\n \t\t\t     PARSE_OPT_KEEP_ARGV0);\n \tif (argc < 2) {\n \t\targc = 2;\n@@ -1334,7 +1348,6 @@ static int show_all(void)\n int cmd_remote(int argc, const char **argv, const char *prefix)\n {\n \tstruct option options[] = {\n-\t\tOPT__VERBOSE(&verbose),\n \t\tOPT_END()\n \t};\n \tint result;\n-- \n1.6.5.2.185.gb7fba.dirty\n"},{"id":"127722","messageId":"7vocn2klp8.fsf@alter.siamese.dyndns.org","threadId":"21631","inReplyTo":"32c343770911151343y1ed0aak83b43a4ab57eb6ab@mail.gmail.com","subject":"Re: [PATCH RFC] git remote: Separate usage strings for subcommands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-17T01:02:27Z","receivedAt":"2009-11-17T01:02:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> All usage strings are still only located at the top of file.  However,\n> separate usage string arrays have been created for each subcommand.\n>\n> Does this look like a sane way to structure the code?\n>\n> If anyone else should be added to the CC list, please let me know.\n>\n>\n>  builtin-remote.c |   57 +++++++++++++++++++++++++++++++++--------------------\n>  1 files changed, 35 insertions(+), 22 deletions(-)\n>\n> diff --git a/builtin-remote.c b/builtin-remote.c\n> index 0777dd7..ec65a4b 100644\n> --- a/builtin-remote.c\n> +++ b/builtin-remote.c\n> @@ -7,18 +7,35 @@\n>  #include \"run-command.h\"\n>  #include \"refs.h\"\n>\n> +#define REMOTE_BARE_USAGE\t\"git remote [-v | --verbose]\"\n> +#define REMOTE_ADD_USAGE\t\"git remote add [-t <branch>] [-m <master>]\n> [-f] [--mirror] <name> <url>\"\n> +#define REMOTE_RENAME_USAGE\t\"git remote rename <old> <new>\"\n> +#define REMOTE_RM_USAGE\t\t\"git remote rm <name>\"\n> +#define REMOTE_SETHEAD_USAGE\t\"git remote set-head <name> [-a | -d | <branch>]\"\n> +#define REMOTE_SHOW_USAGE\t\"git remote show [-n] <name>\"\n> +#define REMOTE_PRUNE_USAGE\t\"git remote prune [-n | --dry-run] <name>\"\n> +#define REMOTE_UPDATE_USAGE\t\"git remote [-v | --verbose] update [-p |\n> --prune] [group]\"\n> +\n>  static const char * const builtin_remote_usage[] = {\n> -\t\"git remote [-v | --verbose]\",\n> -\t\"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>\",\n> -\t\"git remote rename <old> <new>\",\n> -\t\"git remote rm <name>\",\n> -\t\"git remote set-head <name> [-a | -d | <branch>]\",\n> -\t\"git remote show [-n] <name>\",\n> -\t\"git remote prune [-n | --dry-run] <name>\",\n> -\t\"git remote [-v | --verbose] update [-p | --prune] [group]\",\n> +\tREMOTE_BARE_USAGE,\n> +\tREMOTE_ADD_USAGE,\n> +\tREMOTE_RENAME_USAGE,\n> +\tREMOTE_RM_USAGE,\n> +\tREMOTE_SETHEAD_USAGE,\n> +\tREMOTE_SHOW_USAGE,\n> +\tREMOTE_PRUNE_USAGE,\n> +\tREMOTE_UPDATE_USAGE,\n>  \tNULL\n>  };\n> ...\n> +static const char * const builtin_remote_add_usage[] = {\n> REMOTE_ADD_USAGE, NULL };\n> ...\n\nI am not sure about the value of reusing option string like this, and for\nall other subcommands the same comment applies.  For example, in the case\nof \"remote add -h\", you would use\n\n\"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>\"\n\nfrom REMOTE_ADD_USAGE, but ...\n\n> @@ -70,7 +87,6 @@ static int add(int argc, const char **argv)\n>  \tint i;\n>\n>  \tstruct option options[] = {\n> -\t\tOPT_GROUP(\"add specific options\"),\n>  \t\tOPT_BOOLEAN('f', \"fetch\", &fetch, \"fetch the remote branches\"),\n>  \t\tOPT_CALLBACK('t', \"track\", &track, \"branch\",\n>  \t\t\t\"branch(es) to track\", opt_parse_track),\n> @@ -79,11 +95,11 @@ static int add(int argc, const char **argv)\n>  \t\tOPT_END()\n>  \t};\n>\n> -\targc = parse_options(argc, argv, NULL, options, builtin_remote_usage,\n> +\targc = parse_options(argc, argv, NULL, options, builtin_remote_add_usage,\n>  \t\t\t     0);\n\n... the options list is used to reproduce the information in a major part\nof that string already.  So I would prefer builtin_remote_add_usage[] to\nbe something like:\n\n    \"git remote add [<options>...] <name> <url>\"\n\nIt is in line with a discussion we had earlier:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/129906/focus=130646\n"},{"id":"127723","messageId":"32c343770911161739g203a32d4p2b9f8ea6b39d900@mail.gmail.com","threadId":"21631","inReplyTo":"7vocn2klp8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH RFC] git remote: Separate usage strings for subcommands","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2009-11-17T01:39:09Z","receivedAt":"2009-11-17T01:39:09Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Mon, Nov 16, 2009 at 8:02 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Tim Henigan <tim.henigan@gmail.com> writes:\n>\n>> ...\n>> +static const char * const builtin_remote_add_usage[] = {\n>> REMOTE_ADD_USAGE, NULL };\n>> ...\n>\n> I am not sure about the value of reusing option string like this, and for\n> all other subcommands the same comment applies.  For example, in the case\n> of \"remote add -h\", you would use\n>\n> \"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>\"\n> from REMOTE_ADD_USAGE, but ...\n\nDo you object to the new #defines for the strings?  I added them since we now\nreally need to construct the usage string for 2 separate uses:\n  (1) When 'git remote -h' is invoked, we need to return the usage\nstring showing\n       all subcommands.\n  (2) When 'git remote <subcommand> -h' is invoked, we need to return the\n      usage for just that command.\n\nIt doesn't make sense to me to maintain separate strings for the two use cases.\n\n\n>> @@ -70,7 +87,6 @@ static int add(int argc, const char **argv)\n>>       int i;\n>>\n>>       struct option options[] = {\n>> -             OPT_GROUP(\"add specific options\"),\n>>               OPT_BOOLEAN('f', \"fetch\", &fetch, \"fetch the remote branches\"),\n>>               OPT_CALLBACK('t', \"track\", &track, \"branch\",\n>>                       \"branch(es) to track\", opt_parse_track),\n>> @@ -79,11 +95,11 @@ static int add(int argc, const char **argv)\n>>               OPT_END()\n>>       };\n>>\n>> -     argc = parse_options(argc, argv, NULL, options, builtin_remote_usage,\n>> +     argc = parse_options(argc, argv, NULL, options, builtin_remote_add_usage,\n>>                            0);\n>\n> ... the options list is used to reproduce the information in a major part\n> of that string already.  So I would prefer builtin_remote_add_usage[] to\n> be something like:\n>\n>    \"git remote add [<options>...] <name> <url>\"\n>\n> It is in line with a discussion we had earlier:\n>\n>  http://thread.gmane.org/gmane.comp.version-control.git/129906/focus=130646\n\nI planned to make this change in a separate (or maybe just a later\nversion) patch,\nbut first I wanted to be sure my other changes were sound.  I will send a new\nversion that includes this change.\n\nShould this change be made to the man page as well?\n\n\n>> @@ -1334,7 +1348,6 @@ static int show_all(void)\n>>  int cmd_remote(int argc, const char **argv, const char *prefix)\n>>  {\n>>  \tstruct option options[] = {\n>> -\t\tOPT__VERBOSE(&verbose),\n>>  \t\tOPT_END()\n>>  \t};\n>>  \tint result;\n\nI did find one problem with the above portion of the patch.  With this change\n'-v' was no longer recognized as an option.  I will remove this change in the\nnext version.  Keeping it means that '-v' will still be reported in the\n'git remote -h' usage string, but I don't see an easy way to remove the string\nwithout removing the feature.\n"}]}