{"thread":{"id":"16365","subject":"[PATCH 1/3] git-remote: match usage string with the manual pages","startedAt":"2008-11-17T11:15:49Z","lastAt":"2008-11-18T00:56:12Z","messageCount":6,"participants":["crquan@gmail.com","Junio C Hamano","rae l"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"96032","messageId":"1226920551-28303-1-git-send-email-crquan@gmail.com","threadId":"16365","inReplyTo":null,"subject":"[PATCH 1/3] git-remote: match usage string with the manual pages","fromName":"","fromEmail":"crquan@gmail.com","sentAt":"2008-11-17T11:15:49Z","receivedAt":"2008-11-17T11:15:49Z","isPatch":true,"sender":{"key":"crquan@gmail.com","avatar":"https://gravatar.com/avatar/8689a8a26f5d1c7515ca8226258710684dac5c5208b84a9e16a086377f2d3ded?d=mp&s=160"},"body":"From: Cheng Renquan <crquan@gmail.com>\n\nSigned-off-by: Cheng Renquan <crquan@gmail.com>\n---\n builtin-remote.c |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 71696b5..d032f25 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -8,12 +8,12 @@\n #include \"refs.h\"\n \n static const char * const builtin_remote_usage[] = {\n-\t\"git remote\",\n-\t\"git remote add <name> <url>\",\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 show <name>\",\n-\t\"git remote prune <name>\",\n+\t\"git remote show [-n] <name>\",\n+\t\"git remote prune [-n | --dry-run] <name>\",\n \t\"git remote update [group]\",\n \tNULL\n };\n-- \n1.6.0.2\n"},{"id":"96033","messageId":"1226920551-28303-2-git-send-email-crquan@gmail.com","threadId":"16365","inReplyTo":"1226920551-28303-1-git-send-email-crquan@gmail.com","subject":"[PATCH 2/3] git-remote: add verbose mode to git remote update","fromName":"","fromEmail":"crquan@gmail.com","sentAt":"2008-11-17T11:15:50Z","receivedAt":"2008-11-17T11:15:50Z","isPatch":true,"sender":{"key":"crquan@gmail.com","avatar":"https://gravatar.com/avatar/8689a8a26f5d1c7515ca8226258710684dac5c5208b84a9e16a086377f2d3ded?d=mp&s=160"},"body":"From: Cheng Renquan <crquan@gmail.com>\n\nPass the verbose mode parameter to the underlying fetch command.\n\n  $ ./git remote -v update\n  Updating origin\n  From git://git.kernel.org/pub/scm/git/git\n   = [up to date]      html       -> origin/html\n   = [up to date]      maint      -> origin/maint\n   = [up to date]      man        -> origin/man\n   = [up to date]      master     -> origin/master\n   = [up to date]      next       -> origin/next\n   = [up to date]      pu         -> origin/pu\n   = [up to date]      todo       -> origin/todo\n\nSigned-off-by: Cheng Renquan <crquan@gmail.com>\n---\n builtin-remote.c |   20 ++++++++++++++------\n 1 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex d032f25..fff9920 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -14,7 +14,7 @@ static const char * const builtin_remote_usage[] = {\n \t\"git remote rm <name>\",\n \t\"git remote show [-n] <name>\",\n \t\"git remote prune [-n | --dry-run] <name>\",\n-\t\"git remote update [group]\",\n+\t\"git remote update [-v | --verbose] [group]\",\n \tNULL\n };\n \n@@ -40,9 +40,13 @@ static int opt_parse_track(const struct option *opt, const char *arg, int not)\n \treturn 0;\n }\n \n-static int fetch_remote(const char *name)\n+static int fetch_remote(const char *name, const char *url)\n {\n-\tconst char *argv[] = { \"fetch\", name, NULL };\n+\tconst char *argv[] = { \"fetch\", name, NULL, NULL };\n+\tif (verbose) {\n+\t\targv[1] = \"-v\";\n+\t\targv[2] = name;\n+\t}\n \tprintf(\"Updating %s\\n\", name);\n \tif (run_command_v_opt(argv, RUN_GIT_CMD))\n \t\treturn error(\"Could not fetch %s\", name);\n@@ -117,7 +121,7 @@ static int add(int argc, const char **argv)\n \t\t\treturn 1;\n \t}\n \n-\tif (fetch && fetch_remote(name))\n+\tif (fetch && fetch_remote(name, url))\n \t\treturn 1;\n \n \tif (master) {\n@@ -769,8 +773,12 @@ static int prune(int argc, const char **argv)\n static int get_one_remote_for_update(struct remote *remote, void *priv)\n {\n \tstruct string_list *list = priv;\n+\n \tif (!remote->skip_default_update)\n-\t\tstring_list_append(xstrdup(remote->name), list);\n+\t\tstring_list_append(remote->name, list)->util =\n+\t\t\tremote->url_nr > 0\n+\t\t\t? (void *)remote->url[remote->url_nr-1] : NULL;\n+\n \treturn 0;\n }\n \n@@ -818,7 +826,7 @@ static int update(int argc, const char **argv)\n \t\tresult = for_each_remote(get_one_remote_for_update, &list);\n \n \tfor (i = 0; i < list.nr; i++)\n-\t\tresult |= fetch_remote(list.items[i].string);\n+\t\tresult |= fetch_remote(list.items[i].string, list.items[i].util);\n \n \t/* all names were strdup()ed or strndup()ed */\n \tlist.strdup_strings = 1;\n-- \n1.6.0.2\n"},{"id":"96034","messageId":"1226920551-28303-3-git-send-email-crquan@gmail.com","threadId":"16365","inReplyTo":"1226920551-28303-2-git-send-email-crquan@gmail.com","subject":"[PATCH 3/3] git-remote: simplifying get_one_entry","fromName":"","fromEmail":"crquan@gmail.com","sentAt":"2008-11-17T11:15:51Z","receivedAt":"2008-11-17T11:15:51Z","isPatch":true,"sender":{"key":"crquan@gmail.com","avatar":"https://gravatar.com/avatar/8689a8a26f5d1c7515ca8226258710684dac5c5208b84a9e16a086377f2d3ded?d=mp&s=160"},"body":"From: Cheng Renquan <crquan@gmail.com>\n\nThe loop for remote->url_nr is really useless,\nset to the last one directly is better.\n\nSigned-off-by: Cheng Renquan <crquan@gmail.com>\n---\n builtin-remote.c |   10 +++-------\n 1 files changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex fff9920..59d69a5 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -839,13 +839,9 @@ static int get_one_entry(struct remote *remote, void *priv)\n {\n \tstruct string_list *list = priv;\n \n-\tif (remote->url_nr > 0) {\n-\t\tint i;\n-\n-\t\tfor (i = 0; i < remote->url_nr; i++)\n-\t\t\tstring_list_append(remote->name, list)->util = (void *)remote->url[i];\n-\t} else\n-\t\tstring_list_append(remote->name, list)->util = NULL;\n+\tstring_list_append(remote->name, list)->util =\n+\t\tremote->url_nr > 0\n+\t\t? (void *)remote->url[remote->url_nr-1] : NULL;\n \n \treturn 0;\n }\n-- \n1.6.0.2\n"},{"id":"96044","messageId":"7vljviwbav.fsf@gitster.siamese.dyndns.org","threadId":"16365","inReplyTo":"1226920551-28303-2-git-send-email-crquan@gmail.com","subject":"Re: [PATCH 2/3] git-remote: add verbose mode to git remote update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-17T16:46:00Z","receivedAt":"2008-11-17T16:46:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"crquan@gmail.com writes:\n\n> From: Cheng Renquan <crquan@gmail.com>\n>\n> Pass the verbose mode parameter to the underlying fetch command.\n> \n>   $ ./git remote -v update\n>   Updating origin\n>   From git://git.kernel.org/pub/scm/git/git\n>    = [up to date]      html       -> origin/html\n> ...\n> -\t\"git remote update [group]\",\n> +\t\"git remote update [-v | --verbose] [group]\",\n>  \tNULL\n>  };\n \nHmm, ok.  I do not think \"git remote update -v\" would work, though.\n\n> @@ -40,9 +40,13 @@ static int opt_parse_track(const struct option *opt, const char *arg, int not)\n>  \treturn 0;\n>  }\n>  \n> -static int fetch_remote(const char *name)\n> +static int fetch_remote(const char *name, const char *url)\n>  {\n> -\tconst char *argv[] = { \"fetch\", name, NULL };\n> +\tconst char *argv[] = { \"fetch\", name, NULL, NULL };\n> +\tif (verbose) {\n> +\t\targv[1] = \"-v\";\n> +\t\targv[2] = name;\n> +\t}\n>  \tprintf(\"Updating %s\\n\", name);\n>  \tif (run_command_v_opt(argv, RUN_GIT_CMD))\n>  \t\treturn error(\"Could not fetch %s\", name);\n\nI do not think this new parameter \"url\" is used anywhere in this function;\nplease drop the change in the function signature.  That would make your\nchange to \"add()\", \"get_one_remote_for_update()\", and \"update()\" all\nunnecessary.\n\n> @@ -769,8 +773,12 @@ static int prune(int argc, const char **argv)\n>  static int get_one_remote_for_update(struct remote *remote, void *priv)\n>  {\n>  \tstruct string_list *list = priv;\n> +\n>  \tif (!remote->skip_default_update)\n> -\t\tstring_list_append(xstrdup(remote->name), list);\n> +\t\tstring_list_append(remote->name, list)->util =\n> +\t\t\tremote->url_nr > 0\n> +\t\t\t? (void *)remote->url[remote->url_nr-1] : NULL;\n> +\n>  \treturn 0;\n>  }\n>  \n\nI notice that you dropped xstrdup() without explanation.  While I think it\nis a valid leak fix, that should be done as a separate commit (shown\nbelow).\n\nBy the way, you fixed mismatch between the documentation and usage string\nin an earlier patch, but you broke it yourself ;-).  Please fix it up.\n\nThanks.\n\n-- >8 --\nSubject: builtin-remote.c: plug a small memory leak in get_one_remote_for_updates()\n\nWe know that the string pointed at by remote->name won't change.  It can\nbe borrowed as the key in the string_list without copying.  Other parts of\nexisting code such as get_one_entry() already rely on this fact.\n\nNoticed by Cheng Renquan.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-remote.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex d032f25..14774e3 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -770,7 +770,7 @@ static int get_one_remote_for_update(struct remote *remote, void *priv)\n {\n \tstruct string_list *list = priv;\n \tif (!remote->skip_default_update)\n-\t\tstring_list_append(xstrdup(remote->name), list);\n+\t\tstring_list_append(remote->name, list);\n \treturn 0;\n }\n \n-- \n1.6.0.4.772.g2ebfe\n"},{"id":"96046","messageId":"7vabbywb75.fsf@gitster.siamese.dyndns.org","threadId":"16365","inReplyTo":"1226920551-28303-3-git-send-email-crquan@gmail.com","subject":"Re: [PATCH 3/3] git-remote: simplifying get_one_entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-17T16:48:14Z","receivedAt":"2008-11-17T16:48:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"crquan@gmail.com writes:\n\n> From: Cheng Renquan <crquan@gmail.com>\n>\n> The loop for remote->url_nr is really useless, set to the last one\n> directly is better.\n\nIs it really useless?  Be more descriptive.\n\n> -\tif (remote->url_nr > 0) {\n> -\t\tint i;\n> -\n> -\t\tfor (i = 0; i < remote->url_nr; i++)\n> -\t\t\tstring_list_append(remote->name, list)->util = (void *)remote->url[i];\n> -\t} else\n> -\t\tstring_list_append(remote->name, list)->util = NULL;\n> +\tstring_list_append(remote->name, list)->util =\n> +\t\tremote->url_nr > 0\n> +\t\t? (void *)remote->url[remote->url_nr-1] : NULL;\n\nWhen you have more than one URL associated with the remote (this makes\nsense only for pushing), the current code adds that many string_list_item\nto the list, each holding the URL.  \"git remote -v\" shows all of them.\n\nYour change instead creates only one string_list_item and hold the last\nURL.  Doesn't it make show_all() to show only one URL for the remote?\n"},{"id":"96068","messageId":"91b13c310811171656v54363993rfe0f149e7d1da0b0@mail.gmail.com","threadId":"16365","inReplyTo":"7vabbywb75.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] git-remote: simplifying get_one_entry","fromName":"rae l","fromEmail":"crquan@gmail.com","sentAt":"2008-11-18T00:56:12Z","receivedAt":"2008-11-18T00:56:12Z","isPatch":true,"sender":{"key":"crquan@gmail.com","avatar":"https://gravatar.com/avatar/8689a8a26f5d1c7515ca8226258710684dac5c5208b84a9e16a086377f2d3ded?d=mp&s=160"},"body":"On Tue, Nov 18, 2008 at 12:48 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> crquan@gmail.com writes:\n>\n>> From: Cheng Renquan <crquan@gmail.com>\n>>\n>> The loop for remote->url_nr is really useless, set to the last one\n>> directly is better.\n>\n> Is it really useless?  Be more descriptive.\n>\n>> -     if (remote->url_nr > 0) {\n>> -             int i;\n>> -\n>> -             for (i = 0; i < remote->url_nr; i++)\n>> -                     string_list_append(remote->name, list)->util = (void *)remote->url[i];\n>> -     } else\n>> -             string_list_append(remote->name, list)->util = NULL;\n>> +     string_list_append(remote->name, list)->util =\n>> +             remote->url_nr > 0\n>> +             ? (void *)remote->url[remote->url_nr-1] : NULL;\n>\n> When you have more than one URL associated with the remote (this makes\n> sense only for pushing), the current code adds that many string_list_item\n> to the list, each holding the URL.  \"git remote -v\" shows all of them.\n>\n> Your change instead creates only one string_list_item and hold the last\n> URL.  Doesn't it make show_all() to show only one URL for the remote?\nSorry, this patch is totally wrong, I will regenerate the other two and resend.\n\nThanks for your patience.\n\n-- \nCheng Renquan, Shenzhen, China\nSteven Wright  - \"Cross country skiing is great if you live in a small country.\"\n"}]}