{"thread":{"id":"38043","subject":"[PATCH v3] remote: add --fetch and --both options to set-url","startedAt":"2014-11-25T11:48:26Z","lastAt":"2014-12-17T14:28:00Z","messageCount":7,"participants":["Peter Wu","Eric Sunshine","Jeff King","Torsten Bögershausen"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"252513","messageId":"1416916106-19892-1-git-send-email-peter@lekensteyn.nl","threadId":"38043","inReplyTo":null,"subject":"[PATCH v3] remote: add --fetch and --both options to set-url","fromName":"Peter Wu","fromEmail":"peter@lekensteyn.nl","sentAt":"2014-11-25T11:48:26Z","receivedAt":"2014-11-25T11:48:26Z","isPatch":true,"sender":{"key":"peter@lekensteyn.nl","avatar":"https://gravatar.com/avatar/efcf30db53f30b15e7c905ef37bd4dc44d87573bd92baf97cd707115d3fcd0c0?d=mp&s=160"},"body":"git remote set-url knew about the '--push' option to update just the\npushurl, but it does not have a similar option for \"update fetch URL and\nleave whatever was in place for the push URL\".\n\nThis patch adds support for a '--fetch' option which implements that use\ncase in a backwards compatible way: if no --both, --push or --fetch\noptions are given, then the push URL is modified too if it was not set\nbefore. This is the case since the push URL is implicitly based on the\nfetch URL.\n\nA '--both' option is added to make the command independent of previous\npushurl settings. For the --add and --delete set operations, it will\nalways set the push and/ or the fetch URLs. For the primary mode of\noperation (without --add or --delete), it will drop pushurl as the\nimplicit push URL is the (fetch) URL.\n\nThe documentation has also been updated and a missing '--push' option\nis added to the 'git remote -h' command.\n\nTests are also added to verify the documented behavior.\n\nSigned-off-by: Peter Wu <peter@lekensteyn.nl>\n---\n\n v2: fixed test case\n v3: added --both option, changed --fetch --push behavior, added more tests for\n     --add/--delete cases, refactored to reduce duplication (not special-casing\n     add_mode without oldurl, just skip initially setting oldurl).\n\nTranslators note: the help text gained more translatable strings and some\nstrings got additional options.\n---\n Documentation/git-remote.txt |  17 ++++--\n builtin/remote.c             | 140 +++++++++++++++++++++++++++----------------\n t/t5505-remote.sh            | 125 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 228 insertions(+), 54 deletions(-)\n\ndiff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\nindex cb103c8..bdd0305 100644\n--- a/Documentation/git-remote.txt\n+++ b/Documentation/git-remote.txt\n@@ -15,9 +15,9 @@ SYNOPSIS\n 'git remote remove' <name>\n 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)\n 'git remote set-branches' [--add] <name> <branch>...\n-'git remote set-url' [--push] <name> <newurl> [<oldurl>]\n-'git remote set-url --add' [--push] <name> <newurl>\n-'git remote set-url --delete' [--push] <name> <url>\n+'git remote set-url' [--both | --fetch | --push] <name> <newurl> [<oldurl>]\n+'git remote set-url' [--both | --fetch | --push] '--add' <name> <newurl>\n+'git remote set-url' [--both | --fetch | --push] '--delete' <name> <url>\n 'git remote' [-v | --verbose] 'show' [-n] <name>...\n 'git remote prune' [-n | --dry-run] <name>...\n 'git remote' [-v | --verbose] 'update' [-p | --prune] [(<group> | <remote>)...]\n@@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote points to matching\n regex <oldurl> (first URL if no <oldurl> is given) to <newurl>. If\n <oldurl> doesn't match any URL, error occurs and nothing is changed.\n +\n-With '--push', push URLs are manipulated instead of fetch URLs.\n+With '--both', both the fetch and push URLs are manipulated.\n++\n+With '--fetch', only fetch URLs are manipulated.\n++\n+With '--push', only push URLs are manipulated.\n++\n+If none of the '--both', '--fetch' or --push' options are given, then\n+'--both' applies only if no push URL was set before. Otherwise '--fetch'\n+is assumed for historical reasons. This default may change in the\n+future to '--both' to avoid surprises depending on the configuration.\n +\n With '--add', instead of changing some URL, new URL is added.\n +\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..a250b23 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {\n \tN_(\"git remote prune [-n | --dry-run] <name>\"),\n \tN_(\"git remote [-v | --verbose] update [-p | --prune] [(<group> | <remote>)...]\"),\n \tN_(\"git remote set-branches [--add] <name> <branch>...\"),\n-\tN_(\"git remote set-url [--push] <name> <newurl> [<oldurl>]\"),\n-\tN_(\"git remote set-url --add <name> <newurl>\"),\n-\tN_(\"git remote set-url --delete <name> <url>\"),\n+\tN_(\"git remote set-url [--both | --fetch | --push] <name> <newurl> [<oldurl>]\"),\n+\tN_(\"git remote set-url [--both | --fetch | --push] --add <name> <newurl>\"),\n+\tN_(\"git remote set-url [--both | --fetch | --push] --delete <name> <url>\"),\n \tNULL\n };\n \n@@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = {\n };\n \n static const char * const builtin_remote_seturl_usage[] = {\n-\tN_(\"git remote set-url [--push] <name> <newurl> [<oldurl>]\"),\n-\tN_(\"git remote set-url --add <name> <newurl>\"),\n-\tN_(\"git remote set-url --delete <name> <url>\"),\n+\tN_(\"git remote set-url [--both | --fetch | --push] <name> <newurl> [<oldurl>]\"),\n+\tN_(\"git remote set-url [--both | --fetch | --push] --add <name> <newurl>\"),\n+\tN_(\"git remote set-url [--both | --fetch | --push] --delete <name> <url>\"),\n \tNULL\n };\n \n@@ -1503,21 +1503,35 @@ static int set_branches(int argc, const char **argv)\n \treturn set_remote_branches(argv[0], argv + 1, add_mode);\n }\n \n+#define MODIFY_TYPE_FETCH       (1 << 0)\n+#define MODIFY_TYPE_PUSH        (1 << 1)\n+#define MODIFY_TYPE_BOTH        (MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH)\n+#define MODIFY_TYPE_HISTORIC    (MODIFY_TYPE_FETCH | (1 << 2))\n+\n static int set_url(int argc, const char **argv)\n {\n-\tint i, push_mode = 0, add_mode = 0, delete_mode = 0;\n+\tint i, add_mode = 0, delete_mode = 0;\n+\tint modify_type = MODIFY_TYPE_HISTORIC;\n \tint matches = 0, negative_matches = 0;\n \tconst char *remotename = NULL;\n \tconst char *newurl = NULL;\n \tconst char *oldurl = NULL;\n \tstruct remote *remote;\n \tregex_t old_regex;\n-\tconst char **urlset;\n-\tint urlset_nr;\n-\tstruct strbuf name_buf = STRBUF_INIT;\n+\tstruct strbuf name_buf_fetch = STRBUF_INIT;\n+\tstruct strbuf name_buf_push = STRBUF_INIT;\n \tstruct option options[] = {\n-\t\tOPT_BOOL('\\0', \"push\", &push_mode,\n-\t\t\t N_(\"manipulate push URLs\")),\n+\t\tOPT_GROUP(\"\"),\n+\t\tOPT_SET_INT('\\0', \"fetch\", &modify_type,\n+\t\t\tN_(\"manipulate just fetch URLs\"),\n+\t\t\tMODIFY_TYPE_FETCH),\n+\t\tOPT_SET_INT('\\0', \"push\", &modify_type,\n+\t\t\tN_(\"manipulate just push URLs\"),\n+\t\t\tMODIFY_TYPE_PUSH),\n+\t\tOPT_SET_INT('\\0', \"both\", &modify_type,\n+\t\t\tN_(\"manipulate both push and fetch URLs\"),\n+\t\t\tMODIFY_TYPE_BOTH),\n+\t\tOPT_GROUP(\"\"),\n \t\tOPT_BOOL('\\0', \"add\", &add_mode,\n \t\t\t N_(\"add URL\")),\n \t\tOPT_BOOL('\\0', \"delete\", &delete_mode,\n@@ -1535,7 +1549,8 @@ static int set_url(int argc, const char **argv)\n \n \tremotename = argv[1];\n \tnewurl = argv[2];\n-\tif (argc > 3)\n+\t/* The old URL is only meaningful for the primary non-set operation. */\n+\tif (argc > 3 && !add_mode && !delete_mode)\n \t\toldurl = argv[3];\n \n \tif (delete_mode)\n@@ -1545,47 +1560,72 @@ static int set_url(int argc, const char **argv)\n \t\tdie(_(\"No such remote '%s'\"), remotename);\n \tremote = remote_get(remotename);\n \n-\tif (push_mode) {\n-\t\tstrbuf_addf(&name_buf, \"remote.%s.pushurl\", remotename);\n-\t\turlset = remote->pushurl;\n-\t\turlset_nr = remote->pushurl_nr;\n-\t} else {\n-\t\tstrbuf_addf(&name_buf, \"remote.%s.url\", remotename);\n-\t\turlset = remote->url;\n-\t\turlset_nr = remote->url_nr;\n+\tstrbuf_addf(&name_buf_fetch, \"remote.%s.url\", remotename);\n+\tstrbuf_addf(&name_buf_push, \"remote.%s.pushurl\", remotename);\n+\n+\tif (oldurl && !add_mode) {\n+\t\t/* Old URL specified, or deletion. Demand that one matches. */\n+\t\tif (regcomp(&old_regex, oldurl, REG_EXTENDED))\n+\t\t\tdie(_(\"Invalid old URL pattern: %s\"), oldurl);\n+\n+\t\tif (modify_type & MODIFY_TYPE_FETCH)\n+\t\t\tfor (i = 0; i < remote->url_nr; i++)\n+\t\t\t\tif (!regexec(&old_regex, remote->url[i], 0, NULL, 0))\n+\t\t\t\t\tmatches++;\n+\t\t\t\telse\n+\t\t\t\t\tnegative_matches++;\n+\t\tif (delete_mode && !negative_matches && modify_type & MODIFY_TYPE_FETCH)\n+\t\t\tdie(_(\"Will not delete all non-push URLs\"));\n+\t\tif (modify_type & MODIFY_TYPE_PUSH)\n+\t\t\tfor (i = 0; i < remote->pushurl_nr; i++)\n+\t\t\t\tif (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))\n+\t\t\t\t\tmatches++;\n+\t\t\t\telse\n+\t\t\t\t\tnegative_matches++;\n+\t\tif (!delete_mode && !matches)\n+\t\t\tdie(_(\"No such URL found: %s\"), oldurl);\n+\n+\t\tregfree(&old_regex);\n \t}\n \n-\t/* Special cases that add new entry. */\n-\tif ((!oldurl && !delete_mode) || add_mode) {\n-\t\tif (add_mode)\n-\t\t\tgit_config_set_multivar(name_buf.buf, newurl,\n-\t\t\t\t\"^$\", 0);\n-\t\telse\n-\t\t\tgit_config_set(name_buf.buf, newurl);\n-\t\tstrbuf_release(&name_buf);\n-\t\treturn 0;\n+\t/* If --fetch was explicitly given, then ensure that the push\n+\t * URL does not change by copying the fetch URL to it. */\n+\tif (modify_type == MODIFY_TYPE_FETCH &&\n+\t\t\tremote->pushurl_nr == 0 && remote->url_nr > 0)\n+\t\tfor (i = 0; i < remote->url_nr; i++)\n+\t\t\tgit_config_set_multivar(name_buf_push.buf,\n+\t\t\t\tremote->url[i], \"^$\", 0);\n+\n+\t/* Set the new entry value (not a --add or --delete operation). */\n+\tif (!add_mode && !delete_mode && !oldurl) {\n+\t\tif (modify_type & MODIFY_TYPE_FETCH)\n+\t\t\tgit_config_set(name_buf_fetch.buf, newurl);\n+\t\t/* URLs will be the same, so remove pushurl. */\n+\t\tif (modify_type == MODIFY_TYPE_BOTH)\n+\t\t\tgit_config_set(name_buf_push.buf, NULL);\n+\t\telse if (modify_type == MODIFY_TYPE_PUSH)\n+\t\t\tgit_config_set(name_buf_push.buf, newurl);\n+\n+\t\tgoto cleanup_ok;\n \t}\n \n-\t/* Old URL specified. Demand that one matches. */\n-\tif (regcomp(&old_regex, oldurl, REG_EXTENDED))\n-\t\tdie(_(\"Invalid old URL pattern: %s\"), oldurl);\n-\n-\tfor (i = 0; i < urlset_nr; i++)\n-\t\tif (!regexec(&old_regex, urlset[i], 0, NULL, 0))\n-\t\t\tmatches++;\n-\t\telse\n-\t\t\tnegative_matches++;\n-\tif (!delete_mode && !matches)\n-\t\tdie(_(\"No such URL found: %s\"), oldurl);\n-\tif (delete_mode && !negative_matches && !push_mode)\n-\t\tdie(_(\"Will not delete all non-push URLs\"));\n-\n-\tregfree(&old_regex);\n-\n-\tif (!delete_mode)\n-\t\tgit_config_set_multivar(name_buf.buf, newurl, oldurl, 0);\n-\telse\n-\t\tgit_config_set_multivar(name_buf.buf, NULL, oldurl, 1);\n+\t/* Set operations (--add, --delete) or change request (oldurl given). */\n+\tif (delete_mode) {\n+\t\tif (modify_type & MODIFY_TYPE_FETCH)\n+\t\t\tgit_config_set_multivar(name_buf_fetch.buf, NULL, oldurl, 1);\n+\t\tif (modify_type & MODIFY_TYPE_PUSH)\n+\t\t\tgit_config_set_multivar(name_buf_push.buf, NULL, oldurl, 1);\n+\t} else {\n+\t\tif (add_mode) /* Do not replace oldurl, but add a new one. */\n+\t\t\toldurl = \"^$\";\n+\t\tif (modify_type & MODIFY_TYPE_FETCH)\n+\t\t\tgit_config_set_multivar(name_buf_fetch.buf, newurl, oldurl, 0);\n+\t\tif (modify_type & MODIFY_TYPE_PUSH)\n+\t\t\tgit_config_set_multivar(name_buf_push.buf, newurl, oldurl, 0);\n+\t}\n+cleanup_ok:\n+\tstrbuf_release(&name_buf_fetch);\n+\tstrbuf_release(&name_buf_push);\n \treturn 0;\n }\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex ac79dd9..390281a 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -961,6 +961,59 @@ test_expect_success 'remote set-url --push zot' '\n \tcmp expect actual\n '\n \n+test_expect_success 'remote set-url --fetch zox' '\n+\tgit remote set-url --fetch someremote zox &&\n+\techo zox >expect &&\n+\techo \"YYY\" >>expect &&\n+\techo zot >>expect &&\n+\tgit config --get-all remote.someremote.url >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.pushurl >>actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'remote set-url --both foo' '\n+\tgit remote set-url --both someremote foo &&\n+\techo \"YYY\" >expect &&\n+\techo foo >>expect &&\n+\ttest_must_fail git config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'remote set-url --delete --push foo' '\n+\tgit remote set-url --delete --push someremote foo &&\n+\techo \"YYY\" >expect &&\n+\techo foo >>expect &&\n+\ttest_must_fail git config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'remote set-url --push zot' '\n+\tgit remote set-url --push someremote zot &&\n+\techo zot >expect &&\n+\techo \"YYY\" >>expect &&\n+\techo foo >>expect &&\n+\tgit config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'remote set-url --fetch baz foo' '\n+\tgit remote set-url --fetch someremote baz foo &&\n+\techo zot >expect &&\n+\techo \"YYY\" >>expect &&\n+\techo baz >>expect &&\n+\tgit config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n test_expect_success 'remote set-url --push qux zot' '\n \tgit remote set-url --push someremote qux zot &&\n \techo qux >expect &&\n@@ -1091,6 +1144,78 @@ test_expect_success 'remote set-url --delete baz' '\n \tcmp expect actual\n '\n \n+test_expect_success 'remote set-url --fetch --add bar' '\n+\tgit remote set-url --fetch --add someremote bar &&\n+\techo ccc >expect &&\n+\techo \"YYY\" >>expect &&\n+\techo ccc >>expect &&\n+\techo bar >>expect &&\n+\tgit config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'remote set-url --both --add foo' '\n+\tgit remote set-url --both --add someremote foo &&\n+\techo ccc >expect &&\n+\techo foo >>expect &&\n+\techo \"YYY\" >>expect &&\n+\techo ccc >>expect &&\n+\techo bar >>expect &&\n+\techo foo >>expect &&\n+\tgit config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'remote set-url --both --delete ccc' '\n+\tgit remote set-url --both --delete someremote ccc &&\n+\techo foo >expect &&\n+\techo \"YYY\" >>expect &&\n+\techo bar >>expect &&\n+\techo foo >>expect &&\n+\tgit config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'remote set-url --fetch --delete bar' '\n+\tgit remote set-url --fetch --delete someremote bar &&\n+\techo foo >expect &&\n+\techo \"YYY\" >>expect &&\n+\techo foo >>expect &&\n+\tgit config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'remote set-url --push --add baz' '\n+\tgit remote set-url --push --add someremote baz &&\n+\techo foo >expect &&\n+\techo baz >>expect &&\n+\techo \"YYY\" >>expect &&\n+\techo foo >>expect &&\n+\tgit config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n+test_expect_success 'remote set-url --push --delete foo' '\n+\tgit remote set-url --push --delete someremote foo &&\n+\techo baz >expect &&\n+\techo \"YYY\" >>expect &&\n+\techo foo >>expect &&\n+\tgit config --get-all remote.someremote.pushurl >actual &&\n+\techo \"YYY\" >>actual &&\n+\tgit config --get-all remote.someremote.url >>actual &&\n+\tcmp expect actual\n+'\n+\n test_expect_success 'extra args: setup' '\n \t# add a dummy origin so that this does not trigger failure\n \tgit remote add origin .\n-- \n2.1.3\n"},{"id":"252543","messageId":"CAPig+cS1rFpFO4YsNEi2Fjzfj+qH7Oat+PR+0GUo32=MFTNRiA@mail.gmail.com","threadId":"38043","inReplyTo":"1416916106-19892-1-git-send-email-peter@lekensteyn.nl","subject":"Re: [PATCH v3] remote: add --fetch and --both options to set-url","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-11-25T22:09:04Z","receivedAt":"2014-11-25T22:09:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 25, 2014 at 6:48 AM, Peter Wu <peter@lekensteyn.nl> wrote:\n> git remote set-url knew about the '--push' option to update just the\n> pushurl, but it does not have a similar option for \"update fetch URL and\n> leave whatever was in place for the push URL\".\n>\n> This patch adds support for a '--fetch' option which implements that use\n> case in a backwards compatible way: if no --both, --push or --fetch\n> options are given, then the push URL is modified too if it was not set\n> before. This is the case since the push URL is implicitly based on the\n> fetch URL.\n>\n> A '--both' option is added to make the command independent of previous\n> pushurl settings. For the --add and --delete set operations, it will\n> always set the push and/ or the fetch URLs. For the primary mode of\n> operation (without --add or --delete), it will drop pushurl as the\n> implicit push URL is the (fetch) URL.\n\nI've read (though perhaps not fully digested) the other email thread\nwhich led up to this version of the patch, as well as the\ndocumentation update in this patch, but I still don't understand the\nneed for the --both option. Intuitively, I would expect that\nspecifying --fetch and --push on the command line would have the same\neffect as the proposed --both option. Thus, why is a separate (and\nexclusive) --both option needed? Is it to reduce typing? What am I\nmissing?\n\nMore below.\n\n> The documentation has also been updated and a missing '--push' option\n> is added to the 'git remote -h' command.\n>\n> Tests are also added to verify the documented behavior.\n>\n> Signed-off-by: Peter Wu <peter@lekensteyn.nl>\n> ---\n>\n>  v2: fixed test case\n>  v3: added --both option, changed --fetch --push behavior, added more tests for\n>      --add/--delete cases, refactored to reduce duplication (not special-casing\n>      add_mode without oldurl, just skip initially setting oldurl).\n>\n> Translators note: the help text gained more translatable strings and some\n> strings got additional options.\n> ---\n> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\n> index cb103c8..bdd0305 100644\n> --- a/Documentation/git-remote.txt\n> +++ b/Documentation/git-remote.txt\n> @@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote points to matching\n>  regex <oldurl> (first URL if no <oldurl> is given) to <newurl>. If\n>  <oldurl> doesn't match any URL, error occurs and nothing is changed.\n>  +\n> -With '--push', push URLs are manipulated instead of fetch URLs.\n> +With '--both', both the fetch and push URLs are manipulated.\n> ++\n> +With '--fetch', only fetch URLs are manipulated.\n> ++\n> +With '--push', only push URLs are manipulated.\n> ++\n> +If none of the '--both', '--fetch' or --push' options are given, then\n> +'--both' applies only if no push URL was set before. Otherwise '--fetch'\n> +is assumed for historical reasons. This default may change in the\n> +future to '--both' to avoid surprises depending on the configuration.\n\nThis explanation is somewhat tortuous. Assuming that the --both option\nis superfluous, perhaps this could be explained more clearly along\nthese lines:\n\n--- 8< ---\n`--fetch` changes fetch URLs, and --push changes push URLs. Specified\ntogether, both fetch and push URLs are changed.\n\nFor historical reasons, if neither --fetch nor --push is specified,\nthen the fetch URL is changed, as well as the push URL if not already\nset.\n--- 8< ---\n\n>  +\n>  With '--add', instead of changing some URL, new URL is added.\n>  +\n"},{"id":"252548","messageId":"2086201.pfQdnB4m0z@al","threadId":"38043","inReplyTo":"CAPig+cS1rFpFO4YsNEi2Fjzfj+qH7Oat+PR+0GUo32=MFTNRiA@mail.gmail.com","subject":"Re: [PATCH v3] remote: add --fetch and --both options to set-url","fromName":"Peter Wu","fromEmail":"peter@lekensteyn.nl","sentAt":"2014-11-25T22:53:56Z","receivedAt":"2014-11-25T22:53:56Z","isPatch":true,"sender":{"key":"peter@lekensteyn.nl","avatar":"https://gravatar.com/avatar/efcf30db53f30b15e7c905ef37bd4dc44d87573bd92baf97cd707115d3fcd0c0?d=mp&s=160"},"body":"On Tuesday 25 November 2014 17:09:04 Eric Sunshine wrote:\n> On Tue, Nov 25, 2014 at 6:48 AM, Peter Wu <peter@lekensteyn.nl> wrote:\n> > git remote set-url knew about the '--push' option to update just the\n> > pushurl, but it does not have a similar option for \"update fetch URL and\n> > leave whatever was in place for the push URL\".\n> >\n> > This patch adds support for a '--fetch' option which implements that use\n> > case in a backwards compatible way: if no --both, --push or --fetch\n> > options are given, then the push URL is modified too if it was not set\n> > before. This is the case since the push URL is implicitly based on the\n> > fetch URL.\n> >\n> > A '--both' option is added to make the command independent of previous\n> > pushurl settings. For the --add and --delete set operations, it will\n> > always set the push and/ or the fetch URLs. For the primary mode of\n> > operation (without --add or --delete), it will drop pushurl as the\n> > implicit push URL is the (fetch) URL.\n> \n> I've read (though perhaps not fully digested) the other email thread\n> which led up to this version of the patch, as well as the\n> documentation update in this patch, but I still don't understand the\n> need for the --both option. Intuitively, I would expect that\n> specifying --fetch and --push on the command line would have the same\n> effect as the proposed --both option. Thus, why is a separate (and\n> exclusive) --both option needed? Is it to reduce typing? What am I\n> missing?\n\nThe initial version just added --fetch and let --fetch --push behave\nlike the current --both. Here you can see the most recent discussion\nleading to the --both option:\nhttp://www.spinics.net/lists/git/msg242336.html\n\nThere was an overlapping discussion about the confusing historic\nbehavior (default vs. --fetch vs. --push --fetch), and an alternative\n(less verbose form) of --push --fetch. The reason that --both is\nintroduced probably has something to do with it being less verbose.\nI am not too attached to either option by the way.\n\n> > The documentation has also been updated and a missing '--push' option\n> > is added to the 'git remote -h' command.\n> >\n> > Tests are also added to verify the documented behavior.\n> >\n> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>\n> > ---\n> >\n> >  v2: fixed test case\n> >  v3: added --both option, changed --fetch --push behavior, added more tests for\n> >      --add/--delete cases, refactored to reduce duplication (not special-casing\n> >      add_mode without oldurl, just skip initially setting oldurl).\n> >\n> > Translators note: the help text gained more translatable strings and some\n> > strings got additional options.\n> > ---\n> > diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\n> > index cb103c8..bdd0305 100644\n> > --- a/Documentation/git-remote.txt\n> > +++ b/Documentation/git-remote.txt\n> > @@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote points to matching\n> >  regex <oldurl> (first URL if no <oldurl> is given) to <newurl>. If\n> >  <oldurl> doesn't match any URL, error occurs and nothing is changed.\n> >  +\n> > -With '--push', push URLs are manipulated instead of fetch URLs.\n> > +With '--both', both the fetch and push URLs are manipulated.\n> > ++\n> > +With '--fetch', only fetch URLs are manipulated.\n> > ++\n> > +With '--push', only push URLs are manipulated.\n> > ++\n> > +If none of the '--both', '--fetch' or --push' options are given, then\n> > +'--both' applies only if no push URL was set before. Otherwise '--fetch'\n> > +is assumed for historical reasons. This default may change in the\n> > +future to '--both' to avoid surprises depending on the configuration.\n> \n> This explanation is somewhat tortuous. Assuming that the --both option\n> is superfluous, perhaps this could be explained more clearly along\n> these lines:\n> \n> --- 8< ---\n> `--fetch` changes fetch URLs, and --push changes push URLs. Specified\n> together, both fetch and push URLs are changed.\n> \n> For historical reasons, if neither --fetch nor --push is specified,\n> then the fetch URL is changed, as well as the push URL if not already\n> set.\n> --- 8< ---\n\nExcellent, short and concise. If this path is taken, I would still add\nthe note about the future change.\n\nI am also interested in opinions about the suggested default behavior.\nShould it become --both (--push --fetch)? In that case '--both' will be\na pretty useless option in the future (when it becomes the default).\n\n> >  +\n> >  With '--add', instead of changing some URL, new URL is added.\n> >  +\n"},{"id":"253046","messageId":"20141204093750.GD27455@peff.net","threadId":"38043","inReplyTo":"1416916106-19892-1-git-send-email-peter@lekensteyn.nl","subject":"Re: [PATCH v3] remote: add --fetch and --both options to set-url","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-04T09:37:50Z","receivedAt":"2014-12-04T09:37:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 25, 2014 at 12:48:26PM +0100, Peter Wu wrote:\n\n> git remote set-url knew about the '--push' option to update just the\n> pushurl, but it does not have a similar option for \"update fetch URL and\n> leave whatever was in place for the push URL\".\n> \n> This patch adds support for a '--fetch' option which implements that use\n> case in a backwards compatible way: if no --both, --push or --fetch\n> options are given, then the push URL is modified too if it was not set\n> before. This is the case since the push URL is implicitly based on the\n> fetch URL.\n> \n> A '--both' option is added to make the command independent of previous\n> pushurl settings. For the --add and --delete set operations, it will\n> always set the push and/ or the fetch URLs. For the primary mode of\n> operation (without --add or --delete), it will drop pushurl as the\n> implicit push URL is the (fetch) URL.\n> \n> The documentation has also been updated and a missing '--push' option\n> is added to the 'git remote -h' command.\n> \n> Tests are also added to verify the documented behavior.\n> \n> Signed-off-by: Peter Wu <peter@lekensteyn.nl>\n> ---\n\nSorry for the slowness in reviewing. The design of this version makes\nsense to me (not surprising, I guess, since it was in direct response to\nmy comments).\n\nI didn't see anything glaringly wrong in the implementation, though I\ndid find it a little hard to follow, because of this:\n\n> +#define MODIFY_TYPE_FETCH       (1 << 0)\n> +#define MODIFY_TYPE_PUSH        (1 << 1)\n> +#define MODIFY_TYPE_BOTH        (MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH)\n> +#define MODIFY_TYPE_HISTORIC    (MODIFY_TYPE_FETCH | (1 << 2))\n\nWhen reading through the code, the distinction between\n\n  modify_type & MODIFY_TYPE_FETCH\n\nand\n\n  modify_type == MODIFY_TYPE_FETCH\n\nis significant, because the former matches HISTORIC, while the latter\ndoes not. I imagine that a distinct bit value for HISTORIC would make\nthings a bit more verbose (you would have to add an extra OR in many\nplaces), but I wonder if it would make the code easier to follow (one of\nthe things I wanted to check was that HISTORIC does the same thing that\nit always did, and it is very hard to follow the HISTORIC behavior\nreading the code linearly).\n\nI dunno. I don't insist; just noting a difficulty I had while reading\nit.  Maybe you went down that route already during development and found\nit more painful.\n\n-Peff\n"},{"id":"253760","messageId":"54915607.9020304@web.de","threadId":"38043","inReplyTo":"1416916106-19892-1-git-send-email-peter@lekensteyn.nl","subject":"Re: [PATCH v3] remote: add --fetch and --both options to set-url","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-12-17T10:08:07Z","receivedAt":"2014-12-17T10:08:07Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 11/25/2014 12:48 PM, Peter Wu wrote:\n> git remote set-url knew about the '--push' option to update just the\n> pushurl, but it does not have a similar option for \"update fetch URL and\n> leave whatever was in place for the push URL\".\n>\n> This patch adds support for a '--fetch' option which implements that use\n> case in a backwards compatible way: if no --both, --push or --fetch\n> options are given, then the push URL is modified too if it was not set\n> before. This is the case since the push URL is implicitly based on the\n> fetch URL.\n>\n> A '--both' option is added to make the command independent of previous\n> pushurl settings. For the --add and --delete set operations, it will\n> always set the push and/ or the fetch URLs. For the primary mode of\n> operation (without --add or --delete), it will drop pushurl as the\n> implicit push URL is the (fetch) URL.\n>\n> The documentation has also been updated and a missing '--push' option\n> is added to the 'git remote -h' command.\n>\n> Tests are also added to verify the documented behavior.\n>\n> Signed-off-by: Peter Wu <peter@lekensteyn.nl>\n> ---\n>\n>   v2: fixed test case\n>   v3: added --both option, changed --fetch --push behavior, added more tests for\n>       --add/--delete cases, refactored to reduce duplication (not special-casing\n>       add_mode without oldurl, just skip initially setting oldurl).\n>\n> Translators note: the help text gained more translatable strings and some\n> strings got additional options.\n> ---\n>   Documentation/git-remote.txt |  17 ++++--\n>   builtin/remote.c             | 140 +++++++++++++++++++++++++++----------------\n>   t/t5505-remote.sh            | 125 ++++++++++++++++++++++++++++++++++++++\n>   3 files changed, 228 insertions(+), 54 deletions(-)\n>\n> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\n> index cb103c8..bdd0305 100644\n> --- a/Documentation/git-remote.txt\n> +++ b/Documentation/git-remote.txt\n> @@ -15,9 +15,9 @@ SYNOPSIS\n>   'git remote remove' <name>\n>   'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)\n>   'git remote set-branches' [--add] <name> <branch>...\n> -'git remote set-url' [--push] <name> <newurl> [<oldurl>]\n> -'git remote set-url --add' [--push] <name> <newurl>\n> -'git remote set-url --delete' [--push] <name> <url>\n> +'git remote set-url' [--both | --fetch | --push] <name> <newurl> [<oldurl>]\n> +'git remote set-url' [--both | --fetch | --push] '--add' <name> <newurl>\n> +'git remote set-url' [--both | --fetch | --push] '--delete' <name> <url>\n>   'git remote' [-v | --verbose] 'show' [-n] <name>...\n>   'git remote prune' [-n | --dry-run] <name>...\n>   'git remote' [-v | --verbose] 'update' [-p | --prune] [(<group> | <remote>)...]\n> @@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote points to matching\n>   regex <oldurl> (first URL if no <oldurl> is given) to <newurl>. If\n>   <oldurl> doesn't match any URL, error occurs and nothing is changed.\n>   +\n> -With '--push', push URLs are manipulated instead of fetch URLs.\n> +With '--both', both the fetch and push URLs are manipulated.\n> ++\n> +With '--fetch', only fetch URLs are manipulated.\n> ++\n> +With '--push', only push URLs are manipulated.\n> ++\n> +If none of the '--both', '--fetch' or --push' options are given, then\n> +'--both' applies only if no push URL was set before. Otherwise '--fetch'\n> +is assumed for historical reasons. This default may change in the\n> +future to '--both' to avoid surprises depending on the configuration.\n>   +\n>   With '--add', instead of changing some URL, new URL is added.\n>   +\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 7f28f92..a250b23 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {\n>   \tN_(\"git remote prune [-n | --dry-run] <name>\"),\n>   \tN_(\"git remote [-v | --verbose] update [-p | --prune] [(<group> | <remote>)...]\"),\n>   \tN_(\"git remote set-branches [--add] <name> <branch>...\"),\n> -\tN_(\"git remote set-url [--push] <name> <newurl> [<oldurl>]\"),\n> -\tN_(\"git remote set-url --add <name> <newurl>\"),\n> -\tN_(\"git remote set-url --delete <name> <url>\"),\n> +\tN_(\"git remote set-url [--both | --fetch | --push] <name> <newurl> [<oldurl>]\"),\n> +\tN_(\"git remote set-url [--both | --fetch | --push] --add <name> <newurl>\"),\n> +\tN_(\"git remote set-url [--both | --fetch | --push] --delete <name> <url>\"),\n>   \tNULL\n>   };\n>   \n> @@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = {\n>   };\n>   \n>   static const char * const builtin_remote_seturl_usage[] = {\n> -\tN_(\"git remote set-url [--push] <name> <newurl> [<oldurl>]\"),\n> -\tN_(\"git remote set-url --add <name> <newurl>\"),\n> -\tN_(\"git remote set-url --delete <name> <url>\"),\n> +\tN_(\"git remote set-url [--both | --fetch | --push] <name> <newurl> [<oldurl>]\"),\n> +\tN_(\"git remote set-url [--both | --fetch | --push] --add <name> <newurl>\"),\n> +\tN_(\"git remote set-url [--both | --fetch | --push] --delete <name> <url>\"),\n>   \tNULL\n>   };\n>   \n> @@ -1503,21 +1503,35 @@ static int set_branches(int argc, const char **argv)\n>   \treturn set_remote_branches(argv[0], argv + 1, add_mode);\n>   }\n>   \n> +#define MODIFY_TYPE_FETCH       (1 << 0)\n> +#define MODIFY_TYPE_PUSH        (1 << 1)\n> +#define MODIFY_TYPE_BOTH        (MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH)\n> +#define MODIFY_TYPE_HISTORIC    (MODIFY_TYPE_FETCH | (1 << 2))\n> +\n>   static int set_url(int argc, const char **argv)\n>   {\n> -\tint i, push_mode = 0, add_mode = 0, delete_mode = 0;\n> +\tint i, add_mode = 0, delete_mode = 0;\n> +\tint modify_type = MODIFY_TYPE_HISTORIC;\n>   \tint matches = 0, negative_matches = 0;\n>   \tconst char *remotename = NULL;\n>   \tconst char *newurl = NULL;\n>   \tconst char *oldurl = NULL;\n>   \tstruct remote *remote;\n>   \tregex_t old_regex;\n> -\tconst char **urlset;\n> -\tint urlset_nr;\n> -\tstruct strbuf name_buf = STRBUF_INIT;\n> +\tstruct strbuf name_buf_fetch = STRBUF_INIT;\n> +\tstruct strbuf name_buf_push = STRBUF_INIT;\n>   \tstruct option options[] = {\n> -\t\tOPT_BOOL('\\0', \"push\", &push_mode,\n> -\t\t\t N_(\"manipulate push URLs\")),\n> +\t\tOPT_GROUP(\"\"),\n> +\t\tOPT_SET_INT('\\0', \"fetch\", &modify_type,\n> +\t\t\tN_(\"manipulate just fetch URLs\"),\n> +\t\t\tMODIFY_TYPE_FETCH),\n> +\t\tOPT_SET_INT('\\0', \"push\", &modify_type,\n> +\t\t\tN_(\"manipulate just push URLs\"),\n> +\t\t\tMODIFY_TYPE_PUSH),\n> +\t\tOPT_SET_INT('\\0', \"both\", &modify_type,\n> +\t\t\tN_(\"manipulate both push and fetch URLs\"),\n> +\t\t\tMODIFY_TYPE_BOTH),\n> +\t\tOPT_GROUP(\"\"),\n>   \t\tOPT_BOOL('\\0', \"add\", &add_mode,\n>   \t\t\t N_(\"add URL\")),\n>   \t\tOPT_BOOL('\\0', \"delete\", &delete_mode,\n> @@ -1535,7 +1549,8 @@ static int set_url(int argc, const char **argv)\n>   \n>   \tremotename = argv[1];\n>   \tnewurl = argv[2];\n> -\tif (argc > 3)\n> +\t/* The old URL is only meaningful for the primary non-set operation. */\n> +\tif (argc > 3 && !add_mode && !delete_mode)\n>   \t\toldurl = argv[3];\n>   \n>   \tif (delete_mode)\n> @@ -1545,47 +1560,72 @@ static int set_url(int argc, const char **argv)\n>   \t\tdie(_(\"No such remote '%s'\"), remotename);\n>   \tremote = remote_get(remotename);\n>   \n> -\tif (push_mode) {\n> -\t\tstrbuf_addf(&name_buf, \"remote.%s.pushurl\", remotename);\n> -\t\turlset = remote->pushurl;\n> -\t\turlset_nr = remote->pushurl_nr;\n> -\t} else {\n> -\t\tstrbuf_addf(&name_buf, \"remote.%s.url\", remotename);\n> -\t\turlset = remote->url;\n> -\t\turlset_nr = remote->url_nr;\n> +\tstrbuf_addf(&name_buf_fetch, \"remote.%s.url\", remotename);\n> +\tstrbuf_addf(&name_buf_push, \"remote.%s.pushurl\", remotename);\n> +\n> +\tif (oldurl && !add_mode) {\n> +\t\t/* Old URL specified, or deletion. Demand that one matches. */\n> +\t\tif (regcomp(&old_regex, oldurl, REG_EXTENDED))\n> +\t\t\tdie(_(\"Invalid old URL pattern: %s\"), oldurl);\n> +\n> +\t\tif (modify_type & MODIFY_TYPE_FETCH)\n> +\t\t\tfor (i = 0; i < remote->url_nr; i++)\n> +\t\t\t\tif (!regexec(&old_regex, remote->url[i], 0, NULL, 0))\n> +\t\t\t\t\tmatches++;\n> +\t\t\t\telse\n> +\t\t\t\t\tnegative_matches++;\n> +\t\tif (delete_mode && !negative_matches && modify_type & MODIFY_TYPE_FETCH)\n> +\t\t\tdie(_(\"Will not delete all non-push URLs\"));\n> +\t\tif (modify_type & MODIFY_TYPE_PUSH)\n> +\t\t\tfor (i = 0; i < remote->pushurl_nr; i++)\n> +\t\t\t\tif (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))\n> +\t\t\t\t\tmatches++;\n> +\t\t\t\telse\n> +\t\t\t\t\tnegative_matches++;\nThere are 2 warning  \"dangling else\", line 1570 and 1578\nI think we should use:\n\n\t\t\tfor (i = 0; i < remote->pushurl_nr; i++) {\n\t\t\t\tif (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))\n\t\t\t\t\tmatches++;\n\t\t\t\telse\n\t\t\t\t\tnegative_matches++;\n\t\t\t}\n"},{"id":"253766","messageId":"5257945.aZPNBJ6Uhr@al","threadId":"38043","inReplyTo":"54915607.9020304@web.de","subject":"Re: [PATCH v3] remote: add --fetch and --both options to set-url","fromName":"Peter Wu","fromEmail":"peter@lekensteyn.nl","sentAt":"2014-12-17T14:20:58Z","receivedAt":"2014-12-17T14:20:58Z","isPatch":true,"sender":{"key":"peter@lekensteyn.nl","avatar":"https://gravatar.com/avatar/efcf30db53f30b15e7c905ef37bd4dc44d87573bd92baf97cd707115d3fcd0c0?d=mp&s=160"},"body":"On Wednesday 17 December 2014 11:08:07 Torsten Bögershausen wrote:\n> On 11/25/2014 12:48 PM, Peter Wu wrote:\n> > git remote set-url knew about the '--push' option to update just the\n> > pushurl, but it does not have a similar option for \"update fetch URL and\n> > leave whatever was in place for the push URL\".\n> >\n> > This patch adds support for a '--fetch' option which implements that use\n> > case in a backwards compatible way: if no --both, --push or --fetch\n> > options are given, then the push URL is modified too if it was not set\n> > before. This is the case since the push URL is implicitly based on the\n> > fetch URL.\n> >\n> > A '--both' option is added to make the command independent of previous\n> > pushurl settings. For the --add and --delete set operations, it will\n> > always set the push and/ or the fetch URLs. For the primary mode of\n> > operation (without --add or --delete), it will drop pushurl as the\n> > implicit push URL is the (fetch) URL.\n> >\n> > The documentation has also been updated and a missing '--push' option\n> > is added to the 'git remote -h' command.\n> >\n> > Tests are also added to verify the documented behavior.\n> >\n> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>\n> > ---\n> >\n> >   v2: fixed test case\n> >   v3: added --both option, changed --fetch --push behavior, added more tests for\n> >       --add/--delete cases, refactored to reduce duplication (not special-casing\n> >       add_mode without oldurl, just skip initially setting oldurl).\n> >\n> > Translators note: the help text gained more translatable strings and some\n> > strings got additional options.\n> > ---\n> >   Documentation/git-remote.txt |  17 ++++--\n> >   builtin/remote.c             | 140 +++++++++++++++++++++++++++----------------\n> >   t/t5505-remote.sh            | 125 ++++++++++++++++++++++++++++++++++++++\n> >   3 files changed, 228 insertions(+), 54 deletions(-)\n> >\n> > diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\n> > index cb103c8..bdd0305 100644\n> > --- a/Documentation/git-remote.txt\n> > +++ b/Documentation/git-remote.txt\n> > @@ -15,9 +15,9 @@ SYNOPSIS\n> >   'git remote remove' <name>\n> >   'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)\n> >   'git remote set-branches' [--add] <name> <branch>...\n> > -'git remote set-url' [--push] <name> <newurl> [<oldurl>]\n> > -'git remote set-url --add' [--push] <name> <newurl>\n> > -'git remote set-url --delete' [--push] <name> <url>\n> > +'git remote set-url' [--both | --fetch | --push] <name> <newurl> [<oldurl>]\n> > +'git remote set-url' [--both | --fetch | --push] '--add' <name> <newurl>\n> > +'git remote set-url' [--both | --fetch | --push] '--delete' <name> <url>\n> >   'git remote' [-v | --verbose] 'show' [-n] <name>...\n> >   'git remote prune' [-n | --dry-run] <name>...\n> >   'git remote' [-v | --verbose] 'update' [-p | --prune] [(<group> | <remote>)...]\n> > @@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote points to matching\n> >   regex <oldurl> (first URL if no <oldurl> is given) to <newurl>. If\n> >   <oldurl> doesn't match any URL, error occurs and nothing is changed.\n> >   +\n> > -With '--push', push URLs are manipulated instead of fetch URLs.\n> > +With '--both', both the fetch and push URLs are manipulated.\n> > ++\n> > +With '--fetch', only fetch URLs are manipulated.\n> > ++\n> > +With '--push', only push URLs are manipulated.\n> > ++\n> > +If none of the '--both', '--fetch' or --push' options are given, then\n> > +'--both' applies only if no push URL was set before. Otherwise '--fetch'\n> > +is assumed for historical reasons. This default may change in the\n> > +future to '--both' to avoid surprises depending on the configuration.\n> >   +\n> >   With '--add', instead of changing some URL, new URL is added.\n> >   +\n> > diff --git a/builtin/remote.c b/builtin/remote.c\n> > index 7f28f92..a250b23 100644\n> > --- a/builtin/remote.c\n> > +++ b/builtin/remote.c\n> > @@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {\n> >   \tN_(\"git remote prune [-n | --dry-run] <name>\"),\n> >   \tN_(\"git remote [-v | --verbose] update [-p | --prune] [(<group> | <remote>)...]\"),\n> >   \tN_(\"git remote set-branches [--add] <name> <branch>...\"),\n> > -\tN_(\"git remote set-url [--push] <name> <newurl> [<oldurl>]\"),\n> > -\tN_(\"git remote set-url --add <name> <newurl>\"),\n> > -\tN_(\"git remote set-url --delete <name> <url>\"),\n> > +\tN_(\"git remote set-url [--both | --fetch | --push] <name> <newurl> [<oldurl>]\"),\n> > +\tN_(\"git remote set-url [--both | --fetch | --push] --add <name> <newurl>\"),\n> > +\tN_(\"git remote set-url [--both | --fetch | --push] --delete <name> <url>\"),\n> >   \tNULL\n> >   };\n> >   \n> > @@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = {\n> >   };\n> >   \n> >   static const char * const builtin_remote_seturl_usage[] = {\n> > -\tN_(\"git remote set-url [--push] <name> <newurl> [<oldurl>]\"),\n> > -\tN_(\"git remote set-url --add <name> <newurl>\"),\n> > -\tN_(\"git remote set-url --delete <name> <url>\"),\n> > +\tN_(\"git remote set-url [--both | --fetch | --push] <name> <newurl> [<oldurl>]\"),\n> > +\tN_(\"git remote set-url [--both | --fetch | --push] --add <name> <newurl>\"),\n> > +\tN_(\"git remote set-url [--both | --fetch | --push] --delete <name> <url>\"),\n> >   \tNULL\n> >   };\n> >   \n> > @@ -1503,21 +1503,35 @@ static int set_branches(int argc, const char **argv)\n> >   \treturn set_remote_branches(argv[0], argv + 1, add_mode);\n> >   }\n> >   \n> > +#define MODIFY_TYPE_FETCH       (1 << 0)\n> > +#define MODIFY_TYPE_PUSH        (1 << 1)\n> > +#define MODIFY_TYPE_BOTH        (MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH)\n> > +#define MODIFY_TYPE_HISTORIC    (MODIFY_TYPE_FETCH | (1 << 2))\n> > +\n> >   static int set_url(int argc, const char **argv)\n> >   {\n> > -\tint i, push_mode = 0, add_mode = 0, delete_mode = 0;\n> > +\tint i, add_mode = 0, delete_mode = 0;\n> > +\tint modify_type = MODIFY_TYPE_HISTORIC;\n> >   \tint matches = 0, negative_matches = 0;\n> >   \tconst char *remotename = NULL;\n> >   \tconst char *newurl = NULL;\n> >   \tconst char *oldurl = NULL;\n> >   \tstruct remote *remote;\n> >   \tregex_t old_regex;\n> > -\tconst char **urlset;\n> > -\tint urlset_nr;\n> > -\tstruct strbuf name_buf = STRBUF_INIT;\n> > +\tstruct strbuf name_buf_fetch = STRBUF_INIT;\n> > +\tstruct strbuf name_buf_push = STRBUF_INIT;\n> >   \tstruct option options[] = {\n> > -\t\tOPT_BOOL('\\0', \"push\", &push_mode,\n> > -\t\t\t N_(\"manipulate push URLs\")),\n> > +\t\tOPT_GROUP(\"\"),\n> > +\t\tOPT_SET_INT('\\0', \"fetch\", &modify_type,\n> > +\t\t\tN_(\"manipulate just fetch URLs\"),\n> > +\t\t\tMODIFY_TYPE_FETCH),\n> > +\t\tOPT_SET_INT('\\0', \"push\", &modify_type,\n> > +\t\t\tN_(\"manipulate just push URLs\"),\n> > +\t\t\tMODIFY_TYPE_PUSH),\n> > +\t\tOPT_SET_INT('\\0', \"both\", &modify_type,\n> > +\t\t\tN_(\"manipulate both push and fetch URLs\"),\n> > +\t\t\tMODIFY_TYPE_BOTH),\n> > +\t\tOPT_GROUP(\"\"),\n> >   \t\tOPT_BOOL('\\0', \"add\", &add_mode,\n> >   \t\t\t N_(\"add URL\")),\n> >   \t\tOPT_BOOL('\\0', \"delete\", &delete_mode,\n> > @@ -1535,7 +1549,8 @@ static int set_url(int argc, const char **argv)\n> >   \n> >   \tremotename = argv[1];\n> >   \tnewurl = argv[2];\n> > -\tif (argc > 3)\n> > +\t/* The old URL is only meaningful for the primary non-set operation. */\n> > +\tif (argc > 3 && !add_mode && !delete_mode)\n> >   \t\toldurl = argv[3];\n> >   \n> >   \tif (delete_mode)\n> > @@ -1545,47 +1560,72 @@ static int set_url(int argc, const char **argv)\n> >   \t\tdie(_(\"No such remote '%s'\"), remotename);\n> >   \tremote = remote_get(remotename);\n> >   \n> > -\tif (push_mode) {\n> > -\t\tstrbuf_addf(&name_buf, \"remote.%s.pushurl\", remotename);\n> > -\t\turlset = remote->pushurl;\n> > -\t\turlset_nr = remote->pushurl_nr;\n> > -\t} else {\n> > -\t\tstrbuf_addf(&name_buf, \"remote.%s.url\", remotename);\n> > -\t\turlset = remote->url;\n> > -\t\turlset_nr = remote->url_nr;\n> > +\tstrbuf_addf(&name_buf_fetch, \"remote.%s.url\", remotename);\n> > +\tstrbuf_addf(&name_buf_push, \"remote.%s.pushurl\", remotename);\n> > +\n> > +\tif (oldurl && !add_mode) {\n> > +\t\t/* Old URL specified, or deletion. Demand that one matches. */\n> > +\t\tif (regcomp(&old_regex, oldurl, REG_EXTENDED))\n> > +\t\t\tdie(_(\"Invalid old URL pattern: %s\"), oldurl);\n> > +\n> > +\t\tif (modify_type & MODIFY_TYPE_FETCH)\n> > +\t\t\tfor (i = 0; i < remote->url_nr; i++)\n> > +\t\t\t\tif (!regexec(&old_regex, remote->url[i], 0, NULL, 0))\n> > +\t\t\t\t\tmatches++;\n> > +\t\t\t\telse\n> > +\t\t\t\t\tnegative_matches++;\n> > +\t\tif (delete_mode && !negative_matches && modify_type & MODIFY_TYPE_FETCH)\n> > +\t\t\tdie(_(\"Will not delete all non-push URLs\"));\n> > +\t\tif (modify_type & MODIFY_TYPE_PUSH)\n> > +\t\t\tfor (i = 0; i < remote->pushurl_nr; i++)\n> > +\t\t\t\tif (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))\n> > +\t\t\t\t\tmatches++;\n> > +\t\t\t\telse\n> > +\t\t\t\t\tnegative_matches++;\n> There are 2 warning  \"dangling else\", line 1570 and 1578\n> I think we should use:\n> \n> \t\t\tfor (i = 0; i < remote->pushurl_nr; i++) {\n> \t\t\t\tif (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))\n> \t\t\t\t\tmatches++;\n> \t\t\t\telse\n> \t\t\t\t\tnegative_matches++;\n> \t\t\t}\n> \n\nGood catch, strange enough I did not get any warnings from my compiler\nor Clang's static analyzer. I have submitted a new patch which treats\nthe accumulated comments.\n-- \nKind regards,\nPeter\nhttps://lekensteyn.nl\n"},{"id":"253767","messageId":"20141217142800.GA20766@peff.net","threadId":"38043","inReplyTo":"5257945.aZPNBJ6Uhr@al","subject":"Re: [PATCH v3] remote: add --fetch and --both options to set-url","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-17T14:28:00Z","receivedAt":"2014-12-17T14:28:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 17, 2014 at 03:20:58PM +0100, Peter Wu wrote:\n\n> > There are 2 warning  \"dangling else\", line 1570 and 1578\n> > I think we should use:\n> > \n> > \t\t\tfor (i = 0; i < remote->pushurl_nr; i++) {\n> > \t\t\t\tif (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))\n> > \t\t\t\t\tmatches++;\n> > \t\t\t\telse\n> > \t\t\t\t\tnegative_matches++;\n> > \t\t\t}\n> > \n> \n> Good catch, strange enough I did not get any warnings from my compiler\n> or Clang's static analyzer. I have submitted a new patch which treats\n> the accumulated comments.\n\nYeah, gcc does not seem to care, but compiling with clang (3.5.0 here)\ndoes notice it.\n\nC's parser matches what your indentation indicates, so it is just a\nstyle fixup, not a bug (but one I agree is worth addressing).\n\n-Peff\n"}]}