{"thread":{"id":"12469","subject":"[PATCH] remote: fix \"update [group...]\"","startedAt":"2008-03-04T00:30:24Z","lastAt":"2008-03-04T11:23:53Z","messageCount":4,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"70861","messageId":"alpine.LSU.1.00.0803040029440.22527@racer.site","threadId":"12469","inReplyTo":null,"subject":"[PATCH] remote: fix \"update [group...]\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-04T00:30:24Z","receivedAt":"2008-03-04T00:30:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe rewrite in C inadvertently broke updating with remote groups: when you\npass parameters to \"git remote update\", it used to look up \"remotes.<group>\"\nfor every parameter, and interpret the value as a list of remotes to update.\n\nAlso, no parameter, or a single parameter \"default\" should update all\nremotes that have not been marked with \"skipDefaultUpdate\".\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tI only realised the breakage today, sorry.\n\n builtin-remote.c  |   53 ++++++++++++++++++++++++++++++++------\n t/t5505-remote.sh |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 116 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex fc01bcc..4d2ca16 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -477,24 +477,59 @@ cleanup_states:\n \treturn result;\n }\n \n-static int update_one(struct remote *remote, void *priv)\n+static int get_one_remote_for_update(struct remote *remote, void *priv)\n {\n+\tstruct path_list *list = priv;\n \tif (!remote->skip_default_update)\n-\t\treturn fetch_remote(remote->name);\n+\t\tpath_list_append(xstrdup(remote->name), list);\n+\treturn 0;\n+}\n+\n+struct remote_group {\n+\tconst char *name;\n+\tstruct path_list *list;\n+} remote_group;\n+\n+static int get_remote_group(const char *key, const char *value)\n+{\n+\tif (!prefixcmp(key, \"remotes.\") &&\n+\t\t\t!strcmp(key + 8, remote_group.name)) {\n+\t\tchar *space = strchr(value, ' ');\n+\t\twhile (space) {\n+\t\t\tpath_list_append(xstrndup(value, space - value),\n+\t\t\t\t\tremote_group.list);\n+\t\t\tvalue = space + 1;\n+\t\t\tspace = strchr(value, ' ');\n+\t\t}\n+\t\tpath_list_append(xstrdup(value), remote_group.list);\n+\t}\n+\n \treturn 0;\n }\n \n static int update(int argc, const char **argv)\n {\n-\tint i;\n+\tint i, result = 0;\n+\tstruct path_list list = { NULL, 0, 0, 0 };\n \n-\tif (argc < 2)\n-\t\treturn for_each_remote(update_one, NULL);\n+\tif (argc < 2 || (argc == 2 && !strcmp(argv[1], \"default\")))\n+\t\tresult = for_each_remote(get_one_remote_for_update, &list);\n+\telse {\n+\t\tremote_group.list = &list;\n+\t\tfor (i = 1; i < argc; i++) {\n+\t\t\tremote_group.name = argv[i];\n+\t\t\tresult = git_config(get_remote_group);\n+\t\t}\n+\t}\n+\tif (result)\n+\t\treturn result;\n \n-\tfor (i = 1; i < argc; i++)\n-\t\tif (fetch_remote(argv[i]))\n-\t\t\treturn 1;\n-\treturn 0;\n+\tfor (i = 0; i < list.nr; i++)\n+\t\tresult |= fetch_remote(list.items[i].path);\n+\tlist.strdup_paths = 1;\n+\tpath_list_clear(&list, 0);\n+\n+\treturn result;\n }\n \n static int get_one_entry(struct remote *remote, void *priv)\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 0a25c8b..b9757c0 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -10,10 +10,12 @@ setup_repository () {\n \tgit init &&\n \t>file &&\n \tgit add file &&\n+\ttest_tick &&\n \tgit commit -m \"Initial\" &&\n \tgit checkout -b side &&\n \t>elif &&\n \tgit add elif &&\n+\ttest_tick &&\n \tgit commit -m \"Second\" &&\n \tgit checkout master\n \t)\n@@ -113,6 +115,7 @@ test_expect_success 'show' '\n \t git branch -d -r origin/master &&\n \t (cd ../one &&\n \t  echo 1 > file &&\n+\t  test_tick &&\n \t  git commit -m update file) &&\n \t git remote show origin > output &&\n \t git diff expect output)\n@@ -144,4 +147,73 @@ test_expect_success 'add --mirror && prune' '\n \t git rev-parse --verify refs/heads/side)\n '\n \n+cat > one/expect << EOF\n+  apis/master\n+  apis/side\n+  drosophila/another\n+  drosophila/master\n+  drosophila/side\n+EOF\n+\n+test_expect_success 'update' '\n+\n+\t(cd one &&\n+\t git remote add drosophila ../two &&\n+\t git remote add apis ../mirror &&\n+\t git remote update &&\n+\t git branch -r > output &&\n+\t git diff expect output)\n+\n+'\n+\n+cat > one/expect << EOF\n+  drosophila/another\n+  drosophila/master\n+  drosophila/side\n+  manduca/master\n+  manduca/side\n+  megaloprepus/master\n+  megaloprepus/side\n+EOF\n+\n+test_expect_success 'update with arguments' '\n+\n+\t(cd one &&\n+\t for b in $(git branch -r)\n+\t do\n+\t\tgit branch -r -d $b || break\n+\t done &&\n+\t git remote add manduca ../mirror &&\n+\t git remote add megaloprepus ../mirror &&\n+\t git config remotes.phobaeticus \"drosophila megaloprepus\" &&\n+\t git config remotes.titanus manduca &&\n+\t git remote update phobaeticus titanus &&\n+\t git branch -r > output &&\n+\t git diff expect output)\n+\n+'\n+\n+cat > one/expect << EOF\n+  apis/master\n+  apis/side\n+  manduca/master\n+  manduca/side\n+  megaloprepus/master\n+  megaloprepus/side\n+EOF\n+\n+test_expect_success 'update default' '\n+\n+\t(cd one &&\n+\t for b in $(git branch -r)\n+\t do\n+\t\tgit branch -r -d $b || break\n+\t done &&\n+\t git config remote.drosophila.skipDefaultUpdate true &&\n+\t git remote update default &&\n+\t git branch -r > output &&\n+\t git diff expect output)\n+\n+'\n+\n test_done\n-- \n1.5.4.3.542.g98d7c\n\n"},{"id":"70866","messageId":"7v4pbne013.fsf@gitster.siamese.dyndns.org","threadId":"12469","inReplyTo":"alpine.LSU.1.00.0803040029440.22527@racer.site","subject":"Re: [PATCH] remote: fix \"update [group...]\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-04T01:43:52Z","receivedAt":"2008-03-04T01:43:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> The rewrite in C inadvertently broke updating with remote groups: when you\n> pass parameters to \"git remote update\", it used to look up \"remotes.<group>\"\n> for every parameter, and interpret the value as a list of remotes to update.\n>\n> Also, no parameter, or a single parameter \"default\" should update all\n> remotes that have not been marked with \"skipDefaultUpdate\".\n\nMy reading of \"sub update_remote {}\" seems to indicate there still are\nsome differences.  What the scripted version did was:\n\n   - Without extra parameter, update_remote() is run with \"default\";\n   - With parameters, update_remote() is run for each of them;\n\n  Then inside update_remote():\n\n   - grab \"remotes.$name\" configuration; if exists, then it is split using\n     \"split(' ')\", which is different from split(/ /).\n\n     Compare:\n          my @a = split(/ /, \" A quick\\t brown fox  \");\n          my @b = split(' ', \" A quick\\t brown fox  \");\n\t  print \"<\" . join(\"><\", @a), \">\\n\";\n\t  print \"<\" . join(\"><\", @b), \">\\n\";\n     The latter returns ('A', 'quick', 'brown', 'fox').\n\n   - If no \"remotes.$name\" configuration exists, and if the name is\n     \"default\", then grab all remotes that do not have\n     remote.$foo.skipDefaultUpdate set to true.\n\n   - Otherwise complain and die.\n\n  Then fetch each of them.\n\n> +static int get_one_remote_for_update(struct remote *remote, void *priv)\n>  {\n> +\tstruct path_list *list = priv;\n>  \tif (!remote->skip_default_update)\n> +\t\tpath_list_append(xstrdup(remote->name), list);\n> +\treturn 0;\n> +}\n\nThis is called when no extra parameter is given or the name is default.\nIt behaves differently from the scripted version if remote.default\nconfiguration exists, doesn't it?\n\nI suspect this is a regression to a useful feature.  The user can specify\nan explicit list of remotes to fetch from when he says \"update default\" or\njust \"update\", without having to say skipdefaultupdate for all the usually\nuninteresting remotes.\n\n> +struct remote_group {\n> +\tconst char *name;\n> +\tstruct path_list *list;\n> +} remote_group;\n> +\n> +static int get_remote_group(const char *key, const char *value)\n> +{\n> +\tif (!prefixcmp(key, \"remotes.\") &&\n> +\t\t\t!strcmp(key + 8, remote_group.name)) {\n> +\t\tchar *space = strchr(value, ' ');\n> +\t\twhile (space) {\n> +\t\t\tpath_list_append(xstrndup(value, space - value),\n> +\t\t\t\t\tremote_group.list);\n> +\t\t\tvalue = space + 1;\n> +\t\t\tspace = strchr(value, ' ');\n> +\t\t}\n> +\t\tpath_list_append(xstrdup(value), remote_group.list);\n> +\t}\n> +\n\nThis does not mimick the magic \"split(' ', $conf)\" in the scripted\nversion.  We probably need a library function to parse whitespace\nseparated list of tokens, which may help other config parsers (color?  I\nwas too lazy to check).\n\n>  static int update(int argc, const char **argv)\n>  {\n> +\tint i, result = 0;\n> +\tstruct path_list list = { NULL, 0, 0, 0 };\n>  \n> +\tif (argc < 2 || (argc == 2 && !strcmp(argv[1], \"default\")))\n> +\t\tresult = for_each_remote(get_one_remote_for_update, &list);\n> +\telse {\n> +\t\tremote_group.list = &list;\n> +\t\tfor (i = 1; i < argc; i++) {\n> +\t\t\tremote_group.name = argv[i];\n> +\t\t\tresult = git_config(get_remote_group);\n> +\t\t}\n> +\t}\n> +\tif (result)\n> +\t\treturn result;\n>  \n> +\tfor (i = 0; i < list.nr; i++)\n> +\t\tresult |= fetch_remote(list.items[i].path);\n> +\tlist.strdup_paths = 1;\n> +\tpath_list_clear(&list, 0);\n\nThis setting of strdup_paths after the fact and causing clear to free\nthings that were allocated while strdup_paths were set to 0 made me go\n\"Huh?\", which means it needs explanation or code clarification.\n\n> +\n> +\treturn result;\n\nThe scripted one did not stop fetching upon error and this does not\nregress it, but it reports error, which I think is a good change.\n"},{"id":"70868","messageId":"alpine.LSU.1.00.0803040208060.22527@racer.site","threadId":"12469","inReplyTo":"7v4pbne013.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote: fix \"update [group...]\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-04T02:09:14Z","receivedAt":"2008-03-04T02:09:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 3 Mar 2008, Junio C Hamano wrote:\n\n> My reading of \"sub update_remote {}\" seems to indicate there still are\n> some differences.\n\nI will take care of your comments tomorrow (i.e. after sleeping), and yes, \nyou are correct, I missed the fact that remotes.default has a special \nmeaning.\n\nWill fix,\nDscho\n"},{"id":"70923","messageId":"alpine.LSU.1.00.0803041120300.22527@racer.site","threadId":"12469","inReplyTo":"7v4pbne013.fsf@gitster.siamese.dyndns.org","subject":"[PATCH v2] remote: fix \"update [group...]\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-04T11:23:53Z","receivedAt":"2008-03-04T11:23:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe rewrite in C inadvertently broke updating with remote groups: when you\npass parameters to \"git remote update\", it used to look up \"remotes.<group>\"\nfor every parameter, and interpret the value as a list of remotes to update.\n\nAlso, no parameter, or a single parameter \"default\" should update all\nremotes that have not been marked with \"skipDefaultUpdate\".\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tOn Mon, 3 Mar 2008, Junio C Hamano wrote:\n\n\t> > +static int get_one_remote_for_update([...]\n\t>\n\t> This is called when no extra parameter is given or the name is \n\t> default. It behaves differently from the scripted version if \n\t> remote.default configuration exists, doesn't it?\n\n\tremotes.default, yes.  Fixed.\n\n\t> > +static int get_remote_group(const char *key, const char *value)\n\t> > +{\n\t> > +\tif (!prefixcmp(key, \"remotes.\") &&\n\t> > +\t\t\t!strcmp(key + 8, remote_group.name)) {\n\t> > +\t\tchar *space = strchr(value, ' ');\n\t> > +\t\twhile (space) {\n\t> > +\t\t\tpath_list_append(xstrndup(value, space - value),\n\t> > +\t\t\t\t\tremote_group.list);\n\t> > +\t\t\tvalue = space + 1;\n\t> > +\t\t\tspace = strchr(value, ' ');\n\t> > +\t\t}\n\t> > +\t\tpath_list_append(xstrdup(value), remote_group.list);\n\t> > +\t}\n\t> > +\n\t> \n\t> This does not mimick the magic \"split(' ', $conf)\" in the scripted\n\t> version.\n\n\tI was not aware of that Perl feature.  Fixed.\n\n\t> > +\tlist.strdup_paths = 1;\n\t> > +\tpath_list_clear(&list, 0);\n\t> \n\t> This setting of strdup_paths after the fact and causing clear to \n\t> free things that were allocated while strdup_paths were set to 0 \n\t> made me go \"Huh?\", which means it needs explanation or code \n\t> clarification.\n\n\tFixed with a comment.\n\n\t> > +\treturn result;\n\t> \n\t> The scripted one did not stop fetching upon error and this does \n\t> not regress it, but it reports error, which I think is a good \n\t> change.\n\n\tI overlooked that regression, but yes, it was out of a habit to \n\texpect programs to fail when some part of them fails.\n\n builtin-remote.c  |   59 +++++++++++++++++++++++++++++-----\n t/t5505-remote.sh |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 142 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex fc01bcc..aa90cc9 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -477,24 +477,65 @@ cleanup_states:\n \treturn result;\n }\n \n-static int update_one(struct remote *remote, void *priv)\n+static int get_one_remote_for_update(struct remote *remote, void *priv)\n {\n+\tstruct path_list *list = priv;\n \tif (!remote->skip_default_update)\n-\t\treturn fetch_remote(remote->name);\n+\t\tpath_list_append(xstrdup(remote->name), list);\n+\treturn 0;\n+}\n+\n+struct remote_group {\n+\tconst char *name;\n+\tstruct path_list *list;\n+} remote_group;\n+\n+static int get_remote_group(const char *key, const char *value)\n+{\n+\tif (!prefixcmp(key, \"remotes.\") &&\n+\t\t\t!strcmp(key + 8, remote_group.name)) {\n+\t\t/* split list by white space */\n+\t\tint space = strcspn(value, \" \\t\\n\");\n+\t\twhile (*value) {\n+\t\t\tif (space > 1)\n+\t\t\t\tpath_list_append(xstrndup(value, space),\n+\t\t\t\t\t\tremote_group.list);\n+\t\t\tvalue += space + (value[space] != '\\0');\n+\t\t\tspace = strcspn(value, \" \\t\\n\");\n+\t\t}\n+\t}\n+\n \treturn 0;\n }\n \n static int update(int argc, const char **argv)\n {\n-\tint i;\n+\tint i, result = 0;\n+\tstruct path_list list = { NULL, 0, 0, 0 };\n+\tstatic const char *default_argv[] = { NULL, \"default\", NULL };\n \n-\tif (argc < 2)\n-\t\treturn for_each_remote(update_one, NULL);\n+\tif (argc < 2) {\n+\t\targc = 2;\n+\t\targv = default_argv;\n+\t}\n \n-\tfor (i = 1; i < argc; i++)\n-\t\tif (fetch_remote(argv[i]))\n-\t\t\treturn 1;\n-\treturn 0;\n+\tremote_group.list = &list;\n+\tfor (i = 1; i < argc; i++) {\n+\t\tremote_group.name = argv[i];\n+\t\tresult = git_config(get_remote_group);\n+\t}\n+\n+\tif (!result && !list.nr  && argc == 2 && !strcmp(argv[1], \"default\"))\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].path);\n+\n+\t/* all names were strdup()ed or strndup()ed */\n+\tlist.strdup_paths = 1;\n+\tpath_list_clear(&list, 0);\n+\n+\treturn result;\n }\n \n static int get_one_entry(struct remote *remote, void *priv)\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 0a25c8b..f45ea68 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -10,10 +10,12 @@ setup_repository () {\n \tgit init &&\n \t>file &&\n \tgit add file &&\n+\ttest_tick &&\n \tgit commit -m \"Initial\" &&\n \tgit checkout -b side &&\n \t>elif &&\n \tgit add elif &&\n+\ttest_tick &&\n \tgit commit -m \"Second\" &&\n \tgit checkout master\n \t)\n@@ -113,6 +115,7 @@ test_expect_success 'show' '\n \t git branch -d -r origin/master &&\n \t (cd ../one &&\n \t  echo 1 > file &&\n+\t  test_tick &&\n \t  git commit -m update file) &&\n \t git remote show origin > output &&\n \t git diff expect output)\n@@ -144,4 +147,93 @@ test_expect_success 'add --mirror && prune' '\n \t git rev-parse --verify refs/heads/side)\n '\n \n+cat > one/expect << EOF\n+  apis/master\n+  apis/side\n+  drosophila/another\n+  drosophila/master\n+  drosophila/side\n+EOF\n+\n+test_expect_success 'update' '\n+\n+\t(cd one &&\n+\t git remote add drosophila ../two &&\n+\t git remote add apis ../mirror &&\n+\t git remote update &&\n+\t git branch -r > output &&\n+\t git diff expect output)\n+\n+'\n+\n+cat > one/expect << EOF\n+  drosophila/another\n+  drosophila/master\n+  drosophila/side\n+  manduca/master\n+  manduca/side\n+  megaloprepus/master\n+  megaloprepus/side\n+EOF\n+\n+test_expect_success 'update with arguments' '\n+\n+\t(cd one &&\n+\t for b in $(git branch -r)\n+\t do\n+\t\tgit branch -r -d $b || break\n+\t done &&\n+\t git remote add manduca ../mirror &&\n+\t git remote add megaloprepus ../mirror &&\n+\t git config remotes.phobaeticus \"drosophila megaloprepus\" &&\n+\t git config remotes.titanus manduca &&\n+\t git remote update phobaeticus titanus &&\n+\t git branch -r > output &&\n+\t git diff expect output)\n+\n+'\n+\n+cat > one/expect << EOF\n+  apis/master\n+  apis/side\n+  manduca/master\n+  manduca/side\n+  megaloprepus/master\n+  megaloprepus/side\n+EOF\n+\n+test_expect_success 'update default' '\n+\n+\t(cd one &&\n+\t for b in $(git branch -r)\n+\t do\n+\t\tgit branch -r -d $b || break\n+\t done &&\n+\t git config remote.drosophila.skipDefaultUpdate true &&\n+\t git remote update default &&\n+\t git branch -r > output &&\n+\t git diff expect output)\n+\n+'\n+\n+cat > one/expect << EOF\n+  drosophila/another\n+  drosophila/master\n+  drosophila/side\n+EOF\n+\n+test_expect_success 'update default (overridden, with funny whitespace)' '\n+\n+\t(cd one &&\n+\t for b in $(git branch -r)\n+\t do\n+\t\tgit branch -r -d $b || break\n+\t done &&\n+\t git config remotes.default \"$(printf \"\\t drosophila  \\n\")\" &&\n+\t git remote update default &&\n+\t git branch -r > output &&\n+\t git diff expect output)\n+\n+'\n+\n test_done\n-- \n1.5.4.3.570.g97e7d\n\n"}]}