{"thread":{"id":"38191","subject":"[PATCH v4] remote: add --fetch and --both options to set-url","startedAt":"2014-12-17T14:18:56Z","lastAt":"2014-12-17T22:31:34Z","messageCount":4,"participants":["Peter Wu","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"253765","messageId":"1418825936-18575-1-git-send-email-peter@lekensteyn.nl","threadId":"38191","inReplyTo":null,"subject":"[PATCH v4] remote: add --fetch and --both options to set-url","fromName":"Peter Wu","fromEmail":"peter@lekensteyn.nl","sentAt":"2014-12-17T14:18:56Z","receivedAt":"2014-12-17T14:18:56Z","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\nWhile '--both' could be implemented as '--fetch --push', it might also\nbe mistaken for \"--push overrides --fetch\". An option such as\n\"--only={fetch|push|both}\" was also considered, but it is longer than\nthe current options, makes --push redundant and brings the confusing\noption \"--only=both\". Options such as '--direction=...' is too long and\n'--dir=' is ambiguous (\"directory\"?). Thus, for brevity three new\noptions were introduced.\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---\nHi,\n\nThis is the fourth revision of the patch that allows for just setting the fetch\nURL. Eric wondered why '--fetch --push' is not used to describe the state\n'--both', so I added this to the commit message.\n\nThe t5505-remote.sh test still passes after this change (I was unable to run the\nfull test suite as it broke due to an unrelated gpg issue).\n\nKind regards,\nPeter\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 v4: incorporated documentation suggestion from Eric Sunshine;\n     Tried to make the code easier to follow by replacing\n     (modify_type == MODIFY_TYPE_FETCH) by\n     (modify_type & MODIFY_TYPE_FETCH && modify_type != MODIFY_TYPE_HISTORIC)\n     and added comments explaining why this is special (observed by Jeff King);\n     Fixed dangling else issue reported by Torsten Bögershausen;\n     Added considerations for other options to the commit message;\n     Rebased on v2.2.0-65-g9abc44b.\n---\n\n Documentation/git-remote.txt |  16 +++--\n builtin/remote.c             | 146 ++++++++++++++++++++++++++++---------------\n t/t5505-remote.sh            | 125 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 234 insertions(+), 53 deletions(-)\n\ndiff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\nindex cb103c8..09a4670 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,15 @@ 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+For historical reasons, if neither --fetch nor --push is specified then the\n+fetch URL is changed, as well as the push URL if this was not already set. This\n+behavior may change in the future.\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..1fa2fd7 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,39 @@ 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+/* The historic behavior behaves like --fetch, but does not touch the push URL\n+ * configuration (and thereby may appear to change the push URL too if this was\n+ * not set before).\n+ */\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 +1553,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 +1564,76 @@ 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-\t}\n+\tstrbuf_addf(&name_buf_fetch, \"remote.%s.url\", remotename);\n+\tstrbuf_addf(&name_buf_push, \"remote.%s.pushurl\", remotename);\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}\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/* 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+\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\t\t}\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\t\t}\n+\t\tif (!delete_mode && !matches)\n+\t\t\tdie(_(\"No such URL found: %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+\t\tregfree(&old_regex);\n+\t}\n \n-\tregfree(&old_regex);\n+\t/* Make the implicit push URL explicit if the fetch URL is modified,\n+\t * except when in the historic mode (then the pushurl configuration is\n+\t * not changed). */\n+\tif (modify_type & MODIFY_TYPE_FETCH &&\n+\t\t\tmodify_type != MODIFY_TYPE_HISTORIC &&\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-\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":"253768","messageId":"20141217143212.GA23085@peff.net","threadId":"38191","inReplyTo":"1418825936-18575-1-git-send-email-peter@lekensteyn.nl","subject":"Re: [PATCH v4] remote: add --fetch and --both options to set-url","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-17T14:32:13Z","receivedAt":"2014-12-17T14:32:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 17, 2014 at 03:18:56PM +0100, Peter Wu wrote:\n\n> This is the fourth revision of the patch that allows for just setting the fetch\n> URL. Eric wondered why '--fetch --push' is not used to describe the state\n> '--both', so I added this to the commit message.\n\nThanks, I think that explanation is very clear.\n\nThis version overall looks good to me.\n\n> The t5505-remote.sh test still passes after this change (I was unable to run the\n> full test suite as it broke due to an unrelated gpg issue).\n\nIt is it because you have gpg 2.1, and the patches to handle that are\nnot yet merged to master? Or is it because you are using the new patches\nand they are breaking things for your older gpg version?\n\nIf the former, that's fine. If the latter, it would be nice to see a\nreport of the breakage. :)\n\n-Peff\n"},{"id":"253769","messageId":"4526638.yURndRDM9N@al","threadId":"38191","inReplyTo":"20141217143212.GA23085@peff.net","subject":"Re: [PATCH v4] remote: add --fetch and --both options to set-url","fromName":"Peter Wu","fromEmail":"peter@lekensteyn.nl","sentAt":"2014-12-17T14:42:08Z","receivedAt":"2014-12-17T14:42:08Z","isPatch":true,"sender":{"key":"peter@lekensteyn.nl","avatar":"https://gravatar.com/avatar/efcf30db53f30b15e7c905ef37bd4dc44d87573bd92baf97cd707115d3fcd0c0?d=mp&s=160"},"body":"On Wednesday 17 December 2014 09:32:13 Jeff King wrote:\n> On Wed, Dec 17, 2014 at 03:18:56PM +0100, Peter Wu wrote:\n> \n> > This is the fourth revision of the patch that allows for just setting the fetch\n> > URL. Eric wondered why '--fetch --push' is not used to describe the state\n> > '--both', so I added this to the commit message.\n> \n> Thanks, I think that explanation is very clear.\n> \n> This version overall looks good to me.\n> \n> > The t5505-remote.sh test still passes after this change (I was unable to run the\n> > full test suite as it broke due to an unrelated gpg issue).\n> \n> It is it because you have gpg 2.1, and the patches to handle that are\n> not yet merged to master? Or is it because you are using the new patches\n> and they are breaking things for your older gpg version?\n> \n> If the former, that's fine. If the latter, it would be nice to see a\n> report of the breakage. :)\n\nRest assured, this is a gpg 2.1 issue :-)\n-- \nKind regards,\nPeter\nhttps://lekensteyn.nl\n"},{"id":"253781","messageId":"xmqqr3vx9ad5.fsf@gitster.dls.corp.google.com","threadId":"38191","inReplyTo":"1418825936-18575-1-git-send-email-peter@lekensteyn.nl","subject":"Re: [PATCH v4] remote: add --fetch and --both options to set-url","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-17T22:31:34Z","receivedAt":"2014-12-17T22:31:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Wu <peter@lekensteyn.nl> writes:\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\nOK.  In other words, for those without asymmetric configuration\nwithout a need to define pushURL, this default should be the most\nconvenient, as it does not have to fiddle with two variables.\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\n\"and/or\", I think.\n\n> operation (without --add or --delete), it will drop pushurl as the\n> implicit push URL is the (fetch) URL.\n\nI think this description is clear enough without \"(if exists)\" at\nthe end.\n\n> While '--both' could be implemented as '--fetch --push', it might also\n> be mistaken for \"--push overrides --fetch\". An option such as\n> \"--only={fetch|push|both}\" was also considered, but it is longer than\n> the current options, makes --push redundant and brings the confusing\n> option \"--only=both\". Options such as '--direction=...' is too long and\n> '--dir=' is ambiguous (\"directory\"?). Thus, for brevity three new\n> options were introduced.\n\nSounds sensible.\n\n> @@ -134,7 +134,15 @@ 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\nI am afraid that this part is confusing when read in the wider\ncontext of the document.\n\nThe first sentence of this section (outside the patch) is \"Changes\nURL remote points to\"; I expect that the most people would think\nthat it is talking about \"remote.*.url\" configuration, and the\nwording for \"--push\" in the original is also clear that it touches\n'remote.*.pushurl' instead.\n\nIn the updated text, you use words \"fetch URL\" and \"push URL\" to\nmean \"regardless of how these are represented by the configuration\nsystem, the URL to be used for fetching/pushing\".  In other words,\nin the updated vocabulary, 'remote.*.pushurl' is *NOT* called \"push\nURL\" (and 'remote.*.url' is not 'fetch URL').\n\nThat may be easier for new people who aren't familiar with the\nconfiguration system (read: I am saying that it may be a good idea\nin the longer term), but the phrasing does not make it clear that\nthey are not referring remote.*.{url,pushURL} variables.\n\nRephrasing these to \"URLs used for fetching\" vs \"URLs used for\npushing\" may make things a bit less confusion-prone.\n\nIn any case, we would also need to update Documentation/config.txt\nwhich has these:\n\n    remote.<name>.url::\n            The URL of a remote repository.  See linkgit:git-fetch[1] or\n            linkgit:git-push[1].\n\n    remote.<name>.pushurl::\n            The push URL of a remote repository.  See linkgit:git-push[1].\n\nIt may be sufficient to rephrase them like so:\n\n    remote.<name>.url::\n\tThe URL of a remote repository, used for fetching from it\n        and pushing into it unless a separate remote.<name>.pushURL\n        is defined.  See linkgit:git-fetch[1] and linkgit:git-push[1]\n\n    remote.<name>.pushurl::\n\tThe URL of a remote repository, used for pushing into it\n        (if undefined, remote.<name>.url is used to push into it).\n\tSee linkgit:git-push[1].\n\nperhaps?\n\n> +For historical reasons, if neither --fetch nor --push is specified then the\n> +fetch URL is changed, as well as the push URL if this was not already set. This\n> +behavior may change in the future.\n\nThis paragraph is unwarranted.  As you explained in the second\nparagraph in the log message, the traditional behaviour was a good\ndefault for majority of people and I do not think we saw any\ndemonstrated need to deprecate it.\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> +/* The historic behavior behaves like --fetch, but does not touch the push URL\n> + * configuration (and thereby may appear to change the push URL too if this was\n> + * not set before).\n> + */\n> +#define MODIFY_TYPE_HISTORIC    (MODIFY_TYPE_FETCH | (1 << 2))\n\n\t/*\n         * Our multi-line comment begins with a slash-asterisk\n         * without anything else on the remainder of the line\n         * and ends with an asterisk-slash, possibly indented\n         * and nothing else on it.\n         */\n\nPerhaps s/HISTORIC/TRADITIONAL/ or s/HISTORIC/LITERAL/ (the latter\nis for \"literally update 'URL' configuration for the named remote,\ndon't play with 'do we have pushURL?' games\").\n\n> +\t/* Make the implicit push URL explicit if the fetch URL is modified,\n> +\t * except when in the historic mode (then the pushurl configuration is\n> +\t * not changed). */\n\nLikewise.\n\nThanks.\n"}]}