[RFC] [PATCH] remote: add new --fetch option for set-url

[RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

git remote set-url knew about the '--push' option to update just the
pushurl, but it does not have a similar option for "update fetch URL and
leave whatever was in place for the push URL".

This patch adds support for a '--fetch' option which implements that use
case in a backwards compatible way: if neither --push nor --fetch are
given, then the push URL is modified too if it was not set before. This
is the case since the push URL is implicitly based on the fetch URL.

The documentation has also been updated and a missing '--push' option
is added to the 'git remote -h' command.

Tests are also added to verify the documented behavior.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
(please cc me, I am not subscribed)

Hi,

Now and then I hit this issue where I want to change the remote where
sources are fetched from without having to touch the push URL. Example:

    git remote add gh git@github.com:Lekensteyn/git.git
    # Avoid needing SSH for pulling from a repo, so change fetch URL
    git remote set-url https://github.com/Lekensteyn/git.git
    # Hmm, the fetch URL got changed too, let's fix that.
    git remote add --push gh git@github.com:Lekensteyn/git.git

After this patch, I can use:

    git remote add gh git@github.com:Lekensteyn/git.git
    git remote set-url --fetch https://github.com/Lekensteyn/git.git

rather than being forced into a specific order:

    git remote add gh https://github.com/Lekensteyn/git.git
    git remote set-url --push gh git@github.com:Lekensteyn/git.git

The functionality is implemented, but it might need a refactoring to
reduce duplicate code. Translation strings also need to be updated (the
current 'git remote set-url' strings are also not up to date).

Kind regards.
Peter
---
 Documentation/git-remote.txt |  12 +++--
 builtin/remote.c             | 106 +++++++++++++++++++++++++++++--------------
 t/t5505-remote.sh            |  53 ++++++++++++++++++++++
 3 files changed, 133 insertions(+), 38 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index cb103c8..7bb21a2 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,9 +15,9 @@ SYNOPSIS
 'git remote remove' <name>
 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
 'git remote set-branches' [--add] <name> <branch>...
-'git remote set-url' [--push] <name> <newurl> [<oldurl>]
-'git remote set-url --add' [--push] <name> <newurl>
-'git remote set-url --delete' [--push] <name> <url>
+'git remote set-url' [--fetch] [--push] <name> <newurl> [<oldurl>]
+'git remote set-url' [--fetch] [--push] '--add' <name> <newurl>
+'git remote set-url' [--fetch] [--push] '--delete' <name> <url>
 'git remote' [-v | --verbose] 'show' [-n] <name>...
 'git remote prune' [-n | --dry-run] <name>...
 'git remote' [-v | --verbose] 'update' [-p | --prune] [(<group> | <remote>)...]
@@ -134,7 +134,11 @@ Changes URL remote points to. Sets first URL remote points to matching
 regex <oldurl> (first URL if no <oldurl> is given) to <newurl>. If
 <oldurl> doesn't match any URL, error occurs and nothing is changed.
 +
-With '--push', push URLs are manipulated instead of fetch URLs.
+With '--push', push URLs are manipulated.
++
+With '--fetch', fetch URLs are manipulated. If neither '--push' nor '--fetch'
+are given, then (1) '--fetch' is assumed if no push URL is set in the
+configuration or (2) '--fetch --push' otherwise.
 +
 With '--add', instead of changing some URL, new URL is added.
 +
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..26caa6f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
 	N_("git remote prune [-n | --dry-run] <name>"),
 	N_("git remote [-v | --verbose] update [-p | --prune] [(<group> | <remote>)...]"),
 	N_("git remote set-branches [--add] <name> <branch>..."),
-	N_("git remote set-url [--push] <name> <newurl> [<oldurl>]"),
-	N_("git remote set-url --add <name> <newurl>"),
-	N_("git remote set-url --delete <name> <url>"),
+	N_("git remote set-url [--fetch] [--push] <name> <newurl> [<oldurl>]"),
+	N_("git remote set-url [--fetch] [--push] --add <name> <newurl>"),
+	N_("git remote set-url [--fetch] [--push] --delete <name> <url>"),
 	NULL
 };
 
@@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = {
 };
 
 static const char * const builtin_remote_seturl_usage[] = {
-	N_("git remote set-url [--push] <name> <newurl> [<oldurl>]"),
-	N_("git remote set-url --add <name> <newurl>"),
-	N_("git remote set-url --delete <name> <url>"),
+	N_("git remote set-url [--fetch] [--push] <name> <newurl> [<oldurl>]"),
+	N_("git remote set-url [--fetch] [--push] --add <name> <newurl>"),
+	N_("git remote set-url [--fetch] [--push] --delete <name> <url>"),
 	NULL
 };
 
@@ -1505,18 +1505,19 @@ static int set_branches(int argc, const char **argv)
 
 static int set_url(int argc, const char **argv)
 {
-	int i, push_mode = 0, add_mode = 0, delete_mode = 0;
+	int i, fetch_only = 0, push_only = 0, add_mode = 0, delete_mode = 0;
 	int matches = 0, negative_matches = 0;
 	const char *remotename = NULL;
 	const char *newurl = NULL;
 	const char *oldurl = NULL;
 	struct remote *remote;
 	regex_t old_regex;
-	const char **urlset;
-	int urlset_nr;
-	struct strbuf name_buf = STRBUF_INIT;
+	struct strbuf name_buf_fetch = STRBUF_INIT;
+	struct strbuf name_buf_push = STRBUF_INIT;
 	struct option options[] = {
-		OPT_BOOL('\0', "push", &push_mode,
+		OPT_BOOL('\0', "fetch", &fetch_only,
+			 N_("manipulate fetch URLs")),
+		OPT_BOOL('\0', "push", &push_only,
 			 N_("manipulate push URLs")),
 		OPT_BOOL('\0', "add", &add_mode,
 			 N_("add URL")),
@@ -1545,24 +1546,39 @@ static int set_url(int argc, const char **argv)
 		die(_("No such remote '%s'"), remotename);
 	remote = remote_get(remotename);
 
-	if (push_mode) {
-		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
-		urlset = remote->pushurl;
-		urlset_nr = remote->pushurl_nr;
-	} else {
-		strbuf_addf(&name_buf, "remote.%s.url", remotename);
-		urlset = remote->url;
-		urlset_nr = remote->url_nr;
-	}
+	strbuf_addf(&name_buf_fetch, "remote.%s.url", remotename);
+	strbuf_addf(&name_buf_push, "remote.%s.pushurl", remotename);
+
+	/* Backwards compatibility: do not copy fetch URL to push URL if push
+	 * URL is unset and --fetch and --push re missing */
+	if (!fetch_only && !push_only)
+		fetch_only = -1;
 
 	/* Special cases that add new entry. */
 	if ((!oldurl && !delete_mode) || add_mode) {
-		if (add_mode)
-			git_config_set_multivar(name_buf.buf, newurl,
-				"^$", 0);
-		else
-			git_config_set(name_buf.buf, newurl);
-		strbuf_release(&name_buf);
+		/* If --fetch was explicitly given, then ensure that the value
+		 * of push URL does not change by copying the fetch URL. */
+		if (fetch_only == 1 && !push_only &&
+				remote->pushurl_nr == 0 && remote->url_nr > 0)
+			for (i = 0; i < remote->url_nr; i++)
+				git_config_set_multivar(name_buf_push.buf,
+					remote->url[i], "^$", 0);
+
+		if (add_mode) {
+			if (fetch_only)
+				git_config_set_multivar(name_buf_fetch.buf,
+					newurl, "^$", 0);
+			if (push_only)
+				git_config_set_multivar(name_buf_push.buf,
+					newurl, "^$", 0);
+		} else {
+			if (fetch_only)
+				git_config_set(name_buf_fetch.buf, newurl);
+			if (push_only)
+				git_config_set(name_buf_push.buf, newurl);
+		}
+		strbuf_release(&name_buf_fetch);
+		strbuf_release(&name_buf_push);
 		return 0;
 	}
 
@@ -1570,22 +1586,44 @@ static int set_url(int argc, const char **argv)
 	if (regcomp(&old_regex, oldurl, REG_EXTENDED))
 		die(_("Invalid old URL pattern: %s"), oldurl);
 
-	for (i = 0; i < urlset_nr; i++)
-		if (!regexec(&old_regex, urlset[i], 0, NULL, 0))
+	for (i = 0; fetch_only && i < remote->url_nr; i++)
+		if (!regexec(&old_regex, remote->url[i], 0, NULL, 0))
+			matches++;
+		else
+			negative_matches++;
+	if (delete_mode && !negative_matches && fetch_only)
+		die(_("Will not delete all non-push URLs"));
+	for (i = 0; push_only && i < remote->pushurl_nr; i++)
+		if (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))
 			matches++;
 		else
 			negative_matches++;
 	if (!delete_mode && !matches)
 		die(_("No such URL found: %s"), oldurl);
-	if (delete_mode && !negative_matches && !push_mode)
-		die(_("Will not delete all non-push URLs"));
 
 	regfree(&old_regex);
 
-	if (!delete_mode)
-		git_config_set_multivar(name_buf.buf, newurl, oldurl, 0);
-	else
-		git_config_set_multivar(name_buf.buf, NULL, oldurl, 1);
+	/* If --fetch was explicitly given, then ensure that the value
+	 * of push URL does not change by copying the fetch URL. */
+	if (fetch_only == 1 && !push_only &&
+			remote->pushurl_nr == 0 && remote->url_nr > 0)
+		for (i = 0; i < remote->url_nr; i++)
+			git_config_set_multivar(name_buf_push.buf,
+				remote->url[i], "^$", 0);
+
+	if (!delete_mode) {
+		if (fetch_only)
+			git_config_set_multivar(name_buf_fetch.buf, newurl, oldurl, 0);
+		if (push_only)
+			git_config_set_multivar(name_buf_push.buf, newurl, oldurl, 0);
+	} else {
+		if (fetch_only)
+			git_config_set_multivar(name_buf_fetch.buf, NULL, oldurl, 1);
+		if (push_only)
+			git_config_set_multivar(name_buf_push.buf, NULL, oldurl, 1);
+	}
+	strbuf_release(&name_buf_fetch);
+	strbuf_release(&name_buf_push);
 	return 0;
 }
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index ac79dd9..a00e1cb 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -961,6 +961,59 @@ test_expect_success 'remote set-url --push zot' '
 	cmp expect actual
 '
 
+test_expect_success 'remote set-url --fetch zox' '
+	git remote set-url --fetch someremote zox &&
+	echo zox >expect &&
+	echo "YYY" >>expect &&
+	echo zot >>expect &&
+	git config --get-all remote.someremote.url >actual &&
+	echo "YYY" >>actual &&
+	git config --get-all remote.someremote.pushurl >>actual &&
+	cmp expect actual
+'
+
+test_expect_success 'remote set-url --fetch --push foo' '
+	git remote set-url --fetch --push someremote foo &&
+	echo foo >expect &&
+	echo "YYY" >>expect &&
+	echo foo >>expect &&
+	git config --get-all remote.someremote.pushurl >actual &&
+	echo "YYY" >>actual &&
+	git config --get-all remote.someremote.url >>actual &&
+	cmp expect actual
+'
+
+test_expect_success 'remote set-url --delete --push foo' '
+	git remote set-url --delete --push someremote foo &&
+	echo "YYY" >expect &&
+	echo foo >>expect &&
+	test_must_fail git config --get-all remote.someremote.pushurl >actual &&
+	echo "YYY" >actual &&
+	git config --get-all remote.someremote.url >>actual &&
+	cmp expect actual
+'
+
+test_expect_success 'remote set-url zot foo' '
+	git remote set-url someremote zot foo &&
+	echo "YYY" >expect &&
+	echo zot >>expect &&
+	test_must_fail git config --get-all remote.someremote.pushurl >actual &&
+	echo "YYY" >>actual &&
+	git config --get-all remote.someremote.url >>actual &&
+	cmp expect actual
+'
+
+test_expect_success 'remote set-url --push baz' '
+	git remote set-url --push someremote baz &&
+	echo baz >expect &&
+	echo "YYY" >>expect &&
+	echo zot >>expect &&
+	git config --get-all remote.someremote.pushurl >actual &&
+	echo "YYY" >>actual &&
+	git config --get-all remote.someremote.url >>actual &&
+	cmp expect actual
+'
+
 test_expect_success 'remote set-url --push qux zot' '
 	git remote set-url --push someremote qux zot &&
 	echo qux >expect &&
-- 
2.1.2

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Jeff King]

On Wed, Nov 19, 2014 at 04:18:02PM +0100, Peter Wu wrote:

> git remote set-url knew about the '--push' option to update just the
> pushurl, but it does not have a similar option for "update fetch URL and
> leave whatever was in place for the push URL".

Isn't that what:

  git remote set-url foo new-fetch-url

does already? It affects only the "url" setting, which is the de-facto
fetch setting (it is _also_ the push setting if there is no pushurl
defined).

You gave this example:

>     git remote add gh git@github.com:Lekensteyn/git.git
>     # Avoid needing SSH for pulling from a repo, so change fetch URL
>     git remote set-url https://github.com/Lekensteyn/git.git
>     # Hmm, the fetch URL got changed too, let's fix that.
>     git remote add --push gh git@github.com:Lekensteyn/git.git

But here you do not have a pushurl defined in the first place. So I
guess this is really just a shortcut for swapping the two, like:

  git remote set-url --push gh $(git config remote.gh.url)
  git remote set-url gh new-fetch-url

I dunno. I guess that is more convenient, but it seems like a lot of
code for a very marginal use case. But more importantly, I'm a little
worried that the presence of --fetch creates confusion about what
set-url without a --fetch or --push does. That is, it implies to me
that:

  git remote add gh old-url
  git remote set-url gh --push push-url
  git remote set-url gh new-url

would replace both the "url" _and_ "pushurl" values in the third step,
since we did not specify --fetch.  But it is in fact identical whether
you run it with "--fetch" or not.  That is, it creates a weirdly
non-orthogonal interface.

-Peff

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

[-- Attachment #1: Type: text/plain, Size: 3063 bytes --]

On Wednesday 19 November 2014 14:08:00 Jeff King wrote:
> On Wed, Nov 19, 2014 at 04:18:02PM +0100, Peter Wu wrote:
> 
> > git remote set-url knew about the '--push' option to update just the
> > pushurl, but it does not have a similar option for "update fetch URL and
> > leave whatever was in place for the push URL".
> 
> Isn't that what:
> 
>   git remote set-url foo new-fetch-url
> 
> does already? It affects only the "url" setting, which is the de-facto
> fetch setting (it is _also_ the push setting if there is no pushurl
> defined).

If pushurl is not set, then this will implicitly change the fetch URL
which is unwanted and fixed in this patch by introducing a --fetch
option.

> You gave this example:
> 
> >     git remote add gh git@github.com:Lekensteyn/git.git
> >     # Avoid needing SSH for pulling from a repo, so change fetch URL
> >     git remote set-url https://github.com/Lekensteyn/git.git
> >     # Hmm, the fetch URL got changed too, let's fix that.
> >     git remote add --push gh git@github.com:Lekensteyn/git.git
> 
> But here you do not have a pushurl defined in the first place. So I
> guess this is really just a shortcut for swapping the two, like:
> 
>   git remote set-url --push gh $(git config remote.gh.url)
>   git remote set-url gh new-fetch-url

Indeed, and not having a push URL is not uncommon (that is, always the
case when a new remote is added or just cloned). Compare the above
against this one:

    git remote set-url --fetch new-fetch-url

This is less verbose and is much more intuitive.

> I dunno. I guess that is more convenient, but it seems like a lot of
> code for a very marginal use case. But more importantly, I'm a little
> worried that the presence of --fetch creates confusion about what
> set-url without a --fetch or --push does. That is, it implies to me
> that:
> 
>   git remote add gh old-url
>   git remote set-url gh --push push-url
>   git remote set-url gh new-url
> 
> would replace both the "url" _and_ "pushurl" values in the third step,
> since we did not specify --fetch.  But it is in fact identical whether
> you run it with "--fetch" or not.  That is, it creates a weirdly
> non-orthogonal interface.

Step three currently only replaces the fetch URL as an explicit push URL
(remote.gh.pushurl) is set in step two (and therefore remote.gh.url does
not become the implicit push URL).

This might be a bug, but since it has been so long this way I was
worried that people actually rely on this behavior. The following sets
both url and pushurl (even if the URLs are equal):

    git remote set-url --push --fetch gh pushurl

The patch is not tiny, but also not overly large either even if it has
similar pieces of code duplicated. Care has been taken of to keep
backwards compatibility which has also increased the size.

By the way, in the old code there was a memleak as strbuf was not
released at the end of the function, isn't it? A new patch can be found
on the bottom, it fixes an issue in the tests (no change in other
parts).
-- 
Kind regards,
Peter
https://lekensteyn.nl

[-- Attachment #2: 0001-remote-add-new-fetch-option-for-set-url.patch --]
[-- Type: text/x-patch, Size: 10519 bytes --]

>From 6be164f68b4f625ac90f998ae4073e031ebed8ba Mon Sep 17 00:00:00 2001
From: Peter Wu <peter@lekensteyn.nl>
Date: Wed, 19 Nov 2014 15:57:40 +0100
Subject: [PATCHv2] remote: add new --fetch option for set-url

git remote set-url knew about the '--push' option to update just the
pushurl, but it does not have a similar option for "update fetch URL and
leave whatever was in place for the push URL".

This patch adds support for a '--fetch' option which implements that use
case in a backwards compatible way: if neither --push nor --fetch are
given, then the push URL is modified too if it was not set before. This
is the case since the push URL is implicitly based on the fetch URL.

The documentation has also been updated and a missing '--push' option
is added to the 'git remote -h' command.

Tests are also added to verify the documented behavior.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 Documentation/git-remote.txt |  12 +++--
 builtin/remote.c             | 106 +++++++++++++++++++++++++++++--------------
 t/t5505-remote.sh            |  54 ++++++++++++++++++++++
 3 files changed, 134 insertions(+), 38 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index cb103c8..7bb21a2 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,9 +15,9 @@ SYNOPSIS
 'git remote remove' <name>
 'git remote set-head' <name> (-a | --auto | -d | --delete | <branch>)
 'git remote set-branches' [--add] <name> <branch>...
-'git remote set-url' [--push] <name> <newurl> [<oldurl>]
-'git remote set-url --add' [--push] <name> <newurl>
-'git remote set-url --delete' [--push] <name> <url>
+'git remote set-url' [--fetch] [--push] <name> <newurl> [<oldurl>]
+'git remote set-url' [--fetch] [--push] '--add' <name> <newurl>
+'git remote set-url' [--fetch] [--push] '--delete' <name> <url>
 'git remote' [-v | --verbose] 'show' [-n] <name>...
 'git remote prune' [-n | --dry-run] <name>...
 'git remote' [-v | --verbose] 'update' [-p | --prune] [(<group> | <remote>)...]
@@ -134,7 +134,11 @@ Changes URL remote points to. Sets first URL remote points to matching
 regex <oldurl> (first URL if no <oldurl> is given) to <newurl>. If
 <oldurl> doesn't match any URL, error occurs and nothing is changed.
 +
-With '--push', push URLs are manipulated instead of fetch URLs.
+With '--push', push URLs are manipulated.
++
+With '--fetch', fetch URLs are manipulated. If neither '--push' nor '--fetch'
+are given, then (1) '--fetch' is assumed if no push URL is set in the
+configuration or (2) '--fetch --push' otherwise.
 +
 With '--add', instead of changing some URL, new URL is added.
 +
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..26caa6f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
 	N_("git remote prune [-n | --dry-run] <name>"),
 	N_("git remote [-v | --verbose] update [-p | --prune] [(<group> | <remote>)...]"),
 	N_("git remote set-branches [--add] <name> <branch>..."),
-	N_("git remote set-url [--push] <name> <newurl> [<oldurl>]"),
-	N_("git remote set-url --add <name> <newurl>"),
-	N_("git remote set-url --delete <name> <url>"),
+	N_("git remote set-url [--fetch] [--push] <name> <newurl> [<oldurl>]"),
+	N_("git remote set-url [--fetch] [--push] --add <name> <newurl>"),
+	N_("git remote set-url [--fetch] [--push] --delete <name> <url>"),
 	NULL
 };
 
@@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = {
 };
 
 static const char * const builtin_remote_seturl_usage[] = {
-	N_("git remote set-url [--push] <name> <newurl> [<oldurl>]"),
-	N_("git remote set-url --add <name> <newurl>"),
-	N_("git remote set-url --delete <name> <url>"),
+	N_("git remote set-url [--fetch] [--push] <name> <newurl> [<oldurl>]"),
+	N_("git remote set-url [--fetch] [--push] --add <name> <newurl>"),
+	N_("git remote set-url [--fetch] [--push] --delete <name> <url>"),
 	NULL
 };
 
@@ -1505,18 +1505,19 @@ static int set_branches(int argc, const char **argv)
 
 static int set_url(int argc, const char **argv)
 {
-	int i, push_mode = 0, add_mode = 0, delete_mode = 0;
+	int i, fetch_only = 0, push_only = 0, add_mode = 0, delete_mode = 0;
 	int matches = 0, negative_matches = 0;
 	const char *remotename = NULL;
 	const char *newurl = NULL;
 	const char *oldurl = NULL;
 	struct remote *remote;
 	regex_t old_regex;
-	const char **urlset;
-	int urlset_nr;
-	struct strbuf name_buf = STRBUF_INIT;
+	struct strbuf name_buf_fetch = STRBUF_INIT;
+	struct strbuf name_buf_push = STRBUF_INIT;
 	struct option options[] = {
-		OPT_BOOL('\0', "push", &push_mode,
+		OPT_BOOL('\0', "fetch", &fetch_only,
+			 N_("manipulate fetch URLs")),
+		OPT_BOOL('\0', "push", &push_only,
 			 N_("manipulate push URLs")),
 		OPT_BOOL('\0', "add", &add_mode,
 			 N_("add URL")),
@@ -1545,24 +1546,39 @@ static int set_url(int argc, const char **argv)
 		die(_("No such remote '%s'"), remotename);
 	remote = remote_get(remotename);
 
-	if (push_mode) {
-		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
-		urlset = remote->pushurl;
-		urlset_nr = remote->pushurl_nr;
-	} else {
-		strbuf_addf(&name_buf, "remote.%s.url", remotename);
-		urlset = remote->url;
-		urlset_nr = remote->url_nr;
-	}
+	strbuf_addf(&name_buf_fetch, "remote.%s.url", remotename);
+	strbuf_addf(&name_buf_push, "remote.%s.pushurl", remotename);
+
+	/* Backwards compatibility: do not copy fetch URL to push URL if push
+	 * URL is unset and --fetch and --push re missing */
+	if (!fetch_only && !push_only)
+		fetch_only = -1;
 
 	/* Special cases that add new entry. */
 	if ((!oldurl && !delete_mode) || add_mode) {
-		if (add_mode)
-			git_config_set_multivar(name_buf.buf, newurl,
-				"^$", 0);
-		else
-			git_config_set(name_buf.buf, newurl);
-		strbuf_release(&name_buf);
+		/* If --fetch was explicitly given, then ensure that the value
+		 * of push URL does not change by copying the fetch URL. */
+		if (fetch_only == 1 && !push_only &&
+				remote->pushurl_nr == 0 && remote->url_nr > 0)
+			for (i = 0; i < remote->url_nr; i++)
+				git_config_set_multivar(name_buf_push.buf,
+					remote->url[i], "^$", 0);
+
+		if (add_mode) {
+			if (fetch_only)
+				git_config_set_multivar(name_buf_fetch.buf,
+					newurl, "^$", 0);
+			if (push_only)
+				git_config_set_multivar(name_buf_push.buf,
+					newurl, "^$", 0);
+		} else {
+			if (fetch_only)
+				git_config_set(name_buf_fetch.buf, newurl);
+			if (push_only)
+				git_config_set(name_buf_push.buf, newurl);
+		}
+		strbuf_release(&name_buf_fetch);
+		strbuf_release(&name_buf_push);
 		return 0;
 	}
 
@@ -1570,22 +1586,44 @@ static int set_url(int argc, const char **argv)
 	if (regcomp(&old_regex, oldurl, REG_EXTENDED))
 		die(_("Invalid old URL pattern: %s"), oldurl);
 
-	for (i = 0; i < urlset_nr; i++)
-		if (!regexec(&old_regex, urlset[i], 0, NULL, 0))
+	for (i = 0; fetch_only && i < remote->url_nr; i++)
+		if (!regexec(&old_regex, remote->url[i], 0, NULL, 0))
+			matches++;
+		else
+			negative_matches++;
+	if (delete_mode && !negative_matches && fetch_only)
+		die(_("Will not delete all non-push URLs"));
+	for (i = 0; push_only && i < remote->pushurl_nr; i++)
+		if (!regexec(&old_regex, remote->pushurl[i], 0, NULL, 0))
 			matches++;
 		else
 			negative_matches++;
 	if (!delete_mode && !matches)
 		die(_("No such URL found: %s"), oldurl);
-	if (delete_mode && !negative_matches && !push_mode)
-		die(_("Will not delete all non-push URLs"));
 
 	regfree(&old_regex);
 
-	if (!delete_mode)
-		git_config_set_multivar(name_buf.buf, newurl, oldurl, 0);
-	else
-		git_config_set_multivar(name_buf.buf, NULL, oldurl, 1);
+	/* If --fetch was explicitly given, then ensure that the value
+	 * of push URL does not change by copying the fetch URL. */
+	if (fetch_only == 1 && !push_only &&
+			remote->pushurl_nr == 0 && remote->url_nr > 0)
+		for (i = 0; i < remote->url_nr; i++)
+			git_config_set_multivar(name_buf_push.buf,
+				remote->url[i], "^$", 0);
+
+	if (!delete_mode) {
+		if (fetch_only)
+			git_config_set_multivar(name_buf_fetch.buf, newurl, oldurl, 0);
+		if (push_only)
+			git_config_set_multivar(name_buf_push.buf, newurl, oldurl, 0);
+	} else {
+		if (fetch_only)
+			git_config_set_multivar(name_buf_fetch.buf, NULL, oldurl, 1);
+		if (push_only)
+			git_config_set_multivar(name_buf_push.buf, NULL, oldurl, 1);
+	}
+	strbuf_release(&name_buf_fetch);
+	strbuf_release(&name_buf_push);
 	return 0;
 }
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index ac79dd9..a1bd383 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -961,6 +961,60 @@ test_expect_success 'remote set-url --push zot' '
 	cmp expect actual
 '
 
+test_expect_success 'remote set-url --fetch zox' '
+	git remote set-url --fetch someremote zox &&
+	echo zox >expect &&
+	echo "YYY" >>expect &&
+	echo zot >>expect &&
+	git config --get-all remote.someremote.url >actual &&
+	echo "YYY" >>actual &&
+	git config --get-all remote.someremote.pushurl >>actual &&
+	cmp expect actual
+'
+
+test_expect_success 'remote set-url --fetch --push foo' '
+	git remote set-url --fetch --push someremote foo &&
+	echo foo >expect &&
+	echo "YYY" >>expect &&
+	echo foo >>expect &&
+	git config --get-all remote.someremote.pushurl >actual &&
+	echo "YYY" >>actual &&
+	git config --get-all remote.someremote.url >>actual &&
+	cmp expect actual
+'
+
+test_expect_success 'remote set-url --delete --push foo' '
+	git remote set-url --delete --push someremote foo &&
+	echo "YYY" >expect &&
+	echo foo >>expect &&
+	test_must_fail git config --get-all remote.someremote.pushurl >actual &&
+	echo "YYY" >actual &&
+	git config --get-all remote.someremote.url >>actual &&
+	cmp expect actual
+'
+
+test_expect_success 'remote set-url --push zot' '
+	git remote set-url --push someremote zot &&
+	echo zot >expect &&
+	echo "YYY" >>expect &&
+	echo foo >>expect &&
+	git config --get-all remote.someremote.pushurl >actual &&
+	echo "YYY" >>actual &&
+	git config --get-all remote.someremote.url >>actual &&
+	cmp expect actual
+'
+
+test_expect_success 'remote set-url --fetch baz foo' '
+	git remote set-url --fetch someremote baz foo &&
+	echo zot >expect &&
+	echo "YYY" >>expect &&
+	echo baz >>expect &&
+	git config --get-all remote.someremote.pushurl >actual &&
+	echo "YYY" >>actual &&
+	git config --get-all remote.someremote.url >>actual &&
+	cmp expect actual
+'
+
 test_expect_success 'remote set-url --push qux zot' '
 	git remote set-url --push someremote qux zot &&
 	echo qux >expect &&
-- 
2.1.2

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Jeff King]

On Wed, Nov 19, 2014 at 08:42:19PM +0100, Peter Wu wrote:

> >   git remote set-url --push gh $(git config remote.gh.url)
> >   git remote set-url gh new-fetch-url
> 
> Indeed, and not having a push URL is not uncommon (that is, always the
> case when a new remote is added or just cloned). Compare the above
> against this one:
> 
>     git remote set-url --fetch new-fetch-url
> 
> This is less verbose and is much more intuitive.

I agree your suggestion is a nicer way to do this. I'm just not sure
that this swapping is all that common an operation. If there were no
cost, I wouldn't mind. But I'm mostly concerned about the funny,
non-intuitive behavior of "git remote set-url foo" that is created by
this.

> > would replace both the "url" _and_ "pushurl" values in the third step,
> > since we did not specify --fetch.  But it is in fact identical whether
> > you run it with "--fetch" or not.  That is, it creates a weirdly
> > non-orthogonal interface.
> 
> Step three currently only replaces the fetch URL as an explicit push URL
> (remote.gh.pushurl) is set in step two (and therefore remote.gh.url does
> not become the implicit push URL).
> 
> This might be a bug, but since it has been so long this way I was
> worried that people actually rely on this behavior.

I don't think this is a bug. I think that "git fetch set-url" without
"--push" is a de-facto "--fetch" already. Which makes sense, as there
isn't a "--fetch" now (and the "--push" variant and "pushurl" grew after
the fact, so the "url" option serves double-duty as both the single url
and the "fetch" half).

And that's what makes the proposed interface funny. Omitting "--fetch"
is already a de-facto "--fetch", and sometimes the two behave the same,
and sometimes differently. Calling the option "--keep-push" would be a
more accurate description, but that is rather clunky.

> The patch is not tiny, but also not overly large either even if it has
> similar pieces of code duplicated. Care has been taken of to keep
> backwards compatibility which has also increased the size.

Yeah, sorry, I shouldn't have even mentioned patch size. I appreciate
your attention to backwards-compatibility, and the patch should be as
large as it needs to safely implement the desired behavior. It's really
the resulting interface that I'm a bit negative on (which can be related
to code size, in the sense that a simple interface can often be
implemented simply, but here we have to deal with historical corner
cases).

So I dunno. I do not think what you are proposing is the end of the
world, but it would be nice to resolve the inconsistency I mentioned.
Perhaps we can see what others think.

> By the way, in the old code there was a memleak as strbuf was not
> released at the end of the function, isn't it?

Yes. We are often quite lazy about such leaks in functions that are
known to return straight to the program exit, but it does not hurt to
clean up here.

-Peff

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I dunno. I guess that is more convenient, but it seems like a lot of
> code for a very marginal use case. But more importantly, I'm a little
> worried that the presence of --fetch creates confusion about what
> set-url without a --fetch or --push does. That is, it implies to me
> that:
>
>   git remote add gh old-url
>   git remote set-url gh --push push-url
>   git remote set-url gh new-url
>
> would replace both the "url" _and_ "pushurl" values in the third step,
> since we did not specify --fetch.  But it is in fact identical whether
> you run it with "--fetch" or not.  That is, it creates a weirdly
> non-orthogonal interface.

Yes, the semantics the updated code gives feel very strange.  I
wouldn't be able to write a three-line summary in the release notes
to advertise what good this new feature brings to users myself.

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

On Wednesday 19 November 2014 15:17:21 Jeff King wrote:
> On Wed, Nov 19, 2014 at 08:42:19PM +0100, Peter Wu wrote:
> >     git remote set-url --fetch new-fetch-url
> > 
> > This is less verbose and is much more intuitive.
> 
> I agree your suggestion is a nicer way to do this. I'm just not sure
> that this swapping is all that common an operation. If there were no
> cost, I wouldn't mind. But I'm mostly concerned about the funny,
> non-intuitive behavior of "git remote set-url foo" that is created by
> this.

It is not about swapping, but setting a new fetch while keeping the push
URL intact. Likely not common, but nice to have for the times it is
needed. I am sure there are other features which are not used a lot, but
still exist ;)

> > > would replace both the "url" _and_ "pushurl" values in the third step,
> > > since we did not specify --fetch.  But it is in fact identical whether
> > > you run it with "--fetch" or not.  That is, it creates a weirdly
> > > non-orthogonal interface.
> > 
> > Step three currently only replaces the fetch URL as an explicit push URL
> > (remote.gh.pushurl) is set in step two (and therefore remote.gh.url does
> > not become the implicit push URL).
> > 
> > This might be a bug, but since it has been so long this way I was
> > worried that people actually rely on this behavior.
> 
> I don't think this is a bug. I think that "git fetch set-url" without
> "--push" is a de-facto "--fetch" already. Which makes sense, as there
> isn't a "--fetch" now (and the "--push" variant and "pushurl" grew after
> the fact, so the "url" option serves double-duty as both the single url
> and the "fetch" half).
> 
> And that's what makes the proposed interface funny. Omitting "--fetch"
> is already a de-facto "--fetch", and sometimes the two behave the same,
> and sometimes differently. Calling the option "--keep-push" would be a
> more accurate description, but that is rather clunky.
 
Before this patch I did not even know that "git remote set-url name url"
would have different user-visible behavior depending on whether a
pushurl is set or not. In my opinion, the proposed functionality does
not make the interface more confusing. Instead, the new option establish
a behavior which is consistent with the existing '--push' name.

(Aside, I intended to name this option "--pull" which seemed more
natural given the opposite direction "--push", but decided to stay
consistent with the "git remote show" terminology.)

I think that your confusion is caused by the meaning of '--push' and
'--fetch'. These options form a group and are not as independent as
"--add". Something like "--change=[push|fetch|all]" would describe the
functionality better:

    git remote set-url --change=fetch gh fetchurl

But then the "--push" option becomes redundant.
-- 
Kind regards,
Peter
https://lekensteyn.nl

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

On Wednesday 19 November 2014 12:29:47 Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > I dunno. I guess that is more convenient, but it seems like a lot of
> > code for a very marginal use case. But more importantly, I'm a little
> > worried that the presence of --fetch creates confusion about what
> > set-url without a --fetch or --push does. That is, it implies to me
> > that:
> >
> >   git remote add gh old-url
> >   git remote set-url gh --push push-url
> >   git remote set-url gh new-url
> >
> > would replace both the "url" _and_ "pushurl" values in the third step,
> > since we did not specify --fetch.  But it is in fact identical whether
> > you run it with "--fetch" or not.  That is, it creates a weirdly
> > non-orthogonal interface.
> 
> Yes, the semantics the updated code gives feel very strange.  I
> wouldn't be able to write a three-line summary in the release notes
> to advertise what good this new feature brings to users myself.

What about:

    "git remote set-url" learned a new "--fetch" option which can be
    used to change the fetch URL while leaving the push URL intact.
    Useful to keep a ssh URL for push and change the fetch URL to https.

which is effectively the functionality I am using it for.
-- 
Kind regards,
Peter
https://lekensteyn.nl

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But here you do not have a pushurl defined in the first place. So I
> guess this is really just a shortcut for swapping the two, like:
>
>   git remote set-url --push gh $(git config remote.gh.url)
>   git remote set-url gh new-fetch-url

It seems that this swapping is only necessary because the repository
is set up in this way:

    $ browser github.com
    ... fork upstream to your own publishing repository ...
    $ git clone <your publish repo>
    ... oops, I am set up to fetch from myself ...
    $ git remote set-url --push mine <url for your publish repo>
    $ git remote set-url <url for your upstream repo>

If you are fetching from somebody else and then pushing into your
own publishing repository (i.e. fork of that upstream), why isn't
the sequence of event like this, instead?

    $ git clone $upstream
    $ browser github.com
    ... fork upstream to your own publishing repository ...
    $ git remote set-url --push mine <url for your publish repo>

Isn't this one of those bad workflows encouraged by GitHub, for
which you guys have to be punished ;-)?

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Junio C Hamano]

Peter Wu <peter@lekensteyn.nl> writes:

> On Wednesday 19 November 2014 12:29:47 Junio C Hamano wrote:
> ...
>> Yes, the semantics the updated code gives feel very strange.  I
>> wouldn't be able to write a three-line summary in the release notes
>> to advertise what good this new feature brings to users myself.
>
> What about:
>
>     "git remote set-url" learned a new "--fetch" option which can be
>     used to change the fetch URL while leaving the push URL intact.
>     Useful to keep a ssh URL for push and change the fetch URL to https.
>
> which is effectively the functionality I am using it for.

I do not think that clarifies the confusion-factor Peff has been
pointing out in this thread, unfortunately.  At least not to me.

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> But here you do not have a pushurl defined in the first place. So I
>> guess this is really just a shortcut for swapping the two, like:
>>
>>   git remote set-url --push gh $(git config remote.gh.url)
>>   git remote set-url gh new-fetch-url
>
> It seems that this swapping is only necessary because the repository
> is set up in this way:
>
>     $ browser github.com
>     ... fork upstream to your own publishing repository ...
>     $ git clone <your publish repo>
>     ... oops, I am set up to fetch from myself ...
>     $ git remote set-url --push mine <url for your publish repo>
>     $ git remote set-url <url for your upstream repo>
>
> If you are fetching from somebody else and then pushing into your
> own publishing repository (i.e. fork of that upstream), why isn't
> the sequence of event like this, instead?
>
>     $ git clone $upstream
>     $ browser github.com
>     ... fork upstream to your own publishing repository ...
>     $ git remote set-url --push mine <url for your publish repo>
>
> Isn't this one of those bad workflows encouraged by GitHub, for
> which you guys have to be punished ;-)?

I re-read the original and Peter is really accessing the same
repository, only over different transports.  Cloning via ssh:// but
later deciding to push via ssh:// but to fetch via https:// (or vice
versa; the important point is that clone was done over a transport
that is "wrong" for future fetching).

Of course, if you cloned via a wrong transport that is not suitable
for whatever reason for later fetching, you would need to have a way
to "swap", so the observation I made in the message I am following
up on does not apply to this thread at all.  Please scratch all of
the above.

Coming back to the topic, how common would this "oops, I cloned via
a wrong transport" be?  I am not opposed to giving a recovery method
for gotcha that does not happen very often, but if such an addition
adds undue confusion factor for people who use "set-url" for more
common cases, that would be a bad trade-off.

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

On Wednesday 19 November 2014 13:18:56 Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > Jeff King <peff@peff.net> writes:
> > If you are fetching from somebody else and then pushing into your
> > own publishing repository (i.e. fork of that upstream), why isn't
> > the sequence of event like this, instead?
> >
> >     $ git clone $upstream
> >     $ browser github.com
> >     ... fork upstream to your own publishing repository ...
> >     $ git remote set-url --push mine <url for your publish repo>
> >
> > Isn't this one of those bad workflows encouraged by GitHub, for
> > which you guys have to be punished ;-)?

For "forks", it usually goes like this:

    git clone $upstream
    ... realizes that is has a bug which I want to fix ...
    ... creates a new repo ...
    git remote rename origin upstream
    git remote add origin git@$personal_repo
    # "--fetch" is what I need
    git remote add --fetch https://$personal_repo

I often start by entering/copying the ssh URL which is what I need for
pushing. Later ssh-agent forget about my key and I realize that push
works fine over https, so would like to set that... only to observe that
is not possible in an straightforward way through 'git remote'.

> Coming back to the topic, how common would this "oops, I cloned via
> a wrong transport" be?  I am not opposed to giving a recovery method
> for gotcha that does not happen very often, but if such an addition
> adds undue confusion factor for people who use "set-url" for more
> common cases, that would be a bad trade-off.

Well, people rarely need to use 'git remote' except when, well, they
need to modify the remotes. Where does the confusion come from? I might
be biased now that I know the internals. Maybe the https/ssh case above
needs to be mentioned in the documentation? What do you think of the
updated documentation by the way?
-- 
Kind regards,
Peter
https://lekensteyn.nl

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

ping?

I asked around and the people who know of `git remote` fell in these two
categories:

 - those who know of this "bug" and then first set the fetch URL and
   then the push URL.
 - those who did not expect the current behavior.

The command 'git remote set-url NAME URL' reads as "set the URL(s) for
remote NAME to URL". Currently it means "set the fetch (and push) URL of
remote NAME to URL" (depending on whether pushurl is set).

I propose to add the option --fetch next to --push with the meaning "set
the fetch/push URL of remote NAME to URL". Then --fetch --push means
"set the fetch and push URL of remote NAME to URL". In a future git
version, this could be made the default option to avoid surprises (which
would be backwards incompatible though).

As for the changelog entry,

    The "git remote set-url" command now allows you to change just the
    fetch URL without modifying the push URL using the new --fetch
    option. For symmetry with the --push option.

("symmetry" in the eyes of the user, not how it is implemented in the
git config.)

Opinions?

On Wednesday 19 November 2014 22:28:35 Peter Wu wrote:
> On Wednesday 19 November 2014 13:18:56 Junio C Hamano wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> > > Jeff King <peff@peff.net> writes:
> > > If you are fetching from somebody else and then pushing into your
> > > own publishing repository (i.e. fork of that upstream), why isn't
> > > the sequence of event like this, instead?
> > >
> > >     $ git clone $upstream
> > >     $ browser github.com
> > >     ... fork upstream to your own publishing repository ...
> > >     $ git remote set-url --push mine <url for your publish repo>
> > >
> > > Isn't this one of those bad workflows encouraged by GitHub, for
> > > which you guys have to be punished ;-)?
> 
> For "forks", it usually goes like this:
> 
>     git clone $upstream
>     ... realizes that is has a bug which I want to fix ...
>     ... creates a new repo ...
>     git remote rename origin upstream
>     git remote add origin git@$personal_repo
>     # "--fetch" is what I need
>     git remote add --fetch https://$personal_repo
> 
> I often start by entering/copying the ssh URL which is what I need for
> pushing. Later ssh-agent forget about my key and I realize that push
> works fine over https, so would like to set that... only to observe that
> is not possible in an straightforward way through 'git remote'.
> 
> > Coming back to the topic, how common would this "oops, I cloned via
> > a wrong transport" be?  I am not opposed to giving a recovery method
> > for gotcha that does not happen very often, but if such an addition
> > adds undue confusion factor for people who use "set-url" for more
> > common cases, that would be a bad trade-off.
> 
> Well, people rarely need to use 'git remote' except when, well, they
> need to modify the remotes. Where does the confusion come from? I might
> be biased now that I know the internals. Maybe the https/ssh case above
> needs to be mentioned in the documentation? What do you think of the
> updated documentation by the way?

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Junio C Hamano]

Peter Wu <peter@lekensteyn.nl> writes:

> I propose to add the option --fetch next to --push with the meaning "set
> the fetch/push URL of remote NAME to URL". Then --fetch --push means
> "set the fetch and push URL of remote NAME to URL". 

What would (and "should") the configuration look like after you did
this?

	git remote set-url nick $url1
        git remote set-url nick --push $url2
        git remote set-url nick $url3

Whatever happens without your patch after the above is what the
current users (i.e. those who do not use the --fetch option) expect,
so if the behaviour does not change with your patch, then there is
one less incompatibilities to worry about.

A new option "--fetch" introducing a different behaviour is
perfectly fine; existing users who are not using it will not be
harmed by sudden behaviour change.

> In a future git
> version, this could be made the default option to avoid surprises (which
> would be backwards incompatible though).

I am not sure what you mean "by default".  If you mean "set both if
remote.nick.pushurl does not exist but otherwise update only
remote.nick.url", then the sequence

	git remote set-url nick $url1
        git remote set-url nick --push $url2
        git remote set-url nick $url3

would retain the current behaviour, so it probably is OK.

If you mean to always set remote.nick.url and remote.nick.pushurl
pointing at the same value when neither --fetch nor --push is given,
That would make the sequence behave quite different from what people
would expect, and you would need to devise a transition plan to
first start warning when the user did something that will behave
differently between the current version and the future version
without changing the behaviour, then switch the behaviour but keep
warning and finally remove the warning, or something like that.

And the above three-command sequence may not be the only case where
the change you are proposing may hurt existing users.

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

On Monday 24 November 2014 14:04:07 Junio C Hamano wrote:
> Peter Wu <peter@lekensteyn.nl> writes:
> 
> > I propose to add the option --fetch next to --push with the meaning "set
> > the fetch/push URL of remote NAME to URL". Then --fetch --push means
> > "set the fetch and push URL of remote NAME to URL". 
> 
> What would (and "should") the configuration look like after you did
> this?
> 
> 	git remote set-url nick $url1
>         git remote set-url nick --push $url2
>         git remote set-url nick $url3
> 
> Whatever happens without your patch after the above is what the
> current users (i.e. those who do not use the --fetch option) expect,
> so if the behaviour does not change with your patch, then there is
> one less incompatibilities to worry about.
> 
> A new option "--fetch" introducing a different behaviour is
> perfectly fine; existing users who are not using it will not be
> harmed by sudden behaviour change.

As stated before, I took care to avoid backwards incompatibilities. The
command will still work as expected by the users who are aware of this
particular behavior. What I am suggesting (and which is independent of
the patch) is to make the command have a more consistent behavior.
Either it should set the fetch URL, or both the fetch and push URL, but
not vary its behavior depending on whether a push URL is set or not.
That should make the behavior of the command more consistent.

> > In a future git version, this could be made the default option to
> > avoid surprises (which would be backwards incompatible though).
> 
> I am not sure what you mean "by default".  If you mean "set both if
> remote.nick.pushurl does not exist but otherwise update only
> remote.nick.url", then the sequence
> 
> 	git remote set-url nick $url1
>         git remote set-url nick --push $url2
>         git remote set-url nick $url3
> 
> would retain the current behaviour, so it probably is OK.
> 
> If you mean to always set remote.nick.url and remote.nick.pushurl
> pointing at the same value when neither --fetch nor --push is given,
> That would make the sequence behave quite different from what people
> would expect, and you would need to devise a transition plan to
> first start warning when the user did something that will behave
> differently between the current version and the future version
> without changing the behaviour, then switch the behaviour but keep
> warning and finally remove the warning, or something like that.
> 
> And the above three-command sequence may not be the only case where
> the change you are proposing may hurt existing users.

The "default" refers to the behavior of "git remote set-url" in absence
of "--push" and "--fetch" options. A transition period is expected (if
this idea is put forward). Since nobody seems to be bitten by this
option, I am not sure if it really adds much value to make this change
though.
-- 
Kind regards,
Peter
https://lekensteyn.nl

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Jeff King]

On Mon, Nov 24, 2014 at 11:16:03PM +0100, Peter Wu wrote:

> > A new option "--fetch" introducing a different behaviour is
> > perfectly fine; existing users who are not using it will not be
> > harmed by sudden behaviour change.
> 
> As stated before, I took care to avoid backwards incompatibilities. The
> command will still work as expected by the users who are aware of this
> particular behavior.

Right. My original complaint was only that "--fetch" is not as
orthogonal to "--push" (and an optionless set-url) as it could be. I
think the alternatives for going forward are basically:

  1. Name it something besides --fetch (but that's rather clunky).

  2. Migrate to new behavior, which is what is being discussed here.
     Probably needs a transition period?

  3. Live with it. Probably address the weirdness in the documentation.

  4. Do nothing, drop the patch.

I think I'd be OK with (3), with an appropriate documentation update.

-Peff

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

On Monday 24 November 2014 17:22:44 Jeff King wrote:
> On Mon, Nov 24, 2014 at 11:16:03PM +0100, Peter Wu wrote:
> 
> > > A new option "--fetch" introducing a different behaviour is
> > > perfectly fine; existing users who are not using it will not be
> > > harmed by sudden behaviour change.
> > 
> > As stated before, I took care to avoid backwards incompatibilities. The
> > command will still work as expected by the users who are aware of this
> > particular behavior.
> 
> Right. My original complaint was only that "--fetch" is not as
> orthogonal to "--push" (and an optionless set-url) as it could be. I
> think the alternatives for going forward are basically:
> 
>   1. Name it something besides --fetch (but that's rather clunky).

It is not orthogonal to --push in the config, but the behavior exposed
to the user is orthogonal unless I am missing something?

I can understand that --fetch sounds a bit weird, what about this
natural translation:

    "git remote: set the URL (only the fetch one) for NAME to URL"
    git remote set-url --only=fetch NAME URL

    "git remote: set the URL (only the push one) for NAME to URL"
    git remote set-url --only=push NAME URL
    (obsoletes --push)

    "git remote: set the URL (both) for NAME to URL"
    git remote set-url --only=both NAME URL
    (it would be nice if --only=both (weird!) can be removed in the
    future such that the option is more natural)

    "git remote: set the URL for NAME to URL"
    git remote set-url NAME URL
    (current behavior: YOU git guru knows what I do right?)


>   2. Migrate to new behavior, which is what is being discussed here.
>      Probably needs a transition period?

A transition period would also help to solicit feedback.

>   3. Live with it. Probably address the weirdness in the documentation.
> 
>   4. Do nothing, drop the patch.
> 
> I think I'd be OK with (3), with an appropriate documentation update.

I prefer 1 for now as it avoids the extra manual action I have to take
when changing URLs.

Peter

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Jeff King]

On Mon, Nov 24, 2014 at 11:47:30PM +0100, Peter Wu wrote:

> > Right. My original complaint was only that "--fetch" is not as
> > orthogonal to "--push" (and an optionless set-url) as it could be. I
> > think the alternatives for going forward are basically:
> > 
> >   1. Name it something besides --fetch (but that's rather clunky).
> 
> It is not orthogonal to --push in the config, but the behavior exposed
> to the user is orthogonal unless I am missing something?

My complaint is that you have three possible options to provide: --push,
--fetch, or no option at all. And "--fetch" sometimes behaves like no
option, and sometimes not. Which is the confusing/non-orthogonal part.

> I can understand that --fetch sounds a bit weird, what about this
> natural translation:
> 
>     "git remote: set the URL (only the fetch one) for NAME to URL"
>     git remote set-url --only=fetch NAME URL
> 
>     "git remote: set the URL (only the push one) for NAME to URL"
>     git remote set-url --only=push NAME URL
>     (obsoletes --push)
> 
>     "git remote: set the URL (both) for NAME to URL"
>     git remote set-url --only=both NAME URL
>     (it would be nice if --only=both (weird!) can be removed in the
>     future such that the option is more natural)
> 
>     "git remote: set the URL for NAME to URL"
>     git remote set-url NAME URL
>     (current behavior: YOU git guru knows what I do right?)

Yeah, I think that addresses my concern (because it explicitly leaves
no-option as a historical curiosity, and not as an implicit version of
"--both").

> >   3. Live with it. Probably address the weirdness in the documentation.
> > 
> >   4. Do nothing, drop the patch.
> > 
> > I think I'd be OK with (3), with an appropriate documentation update.
> 
> I prefer 1 for now as it avoids the extra manual action I have to take
> when changing URLs.

I'm not sure if I was clear on (3), but "live with it" was "live with
your original patch". Which I think you would also be happy with.

-Peff

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> My complaint is that you have three possible options to provide: --push,
> --fetch, or no option at all. And "--fetch" sometimes behaves like no
> option, and sometimes not. Which is the confusing/non-orthogonal part.
>
>> I can understand that --fetch sounds a bit weird, what about this
>> natural translation:
>> 
>>     "git remote: set the URL (only the fetch one) for NAME to URL"
>>     git remote set-url --only=fetch NAME URL
>> 
>>     "git remote: set the URL (only the push one) for NAME to URL"
>>     git remote set-url --only=push NAME URL
>>     (obsoletes --push)
>> 
>>     "git remote: set the URL (both) for NAME to URL"
>>     git remote set-url --only=both NAME URL
>>     (it would be nice if --only=both (weird!) can be removed in the
>>     future such that the option is more natural)
>> 
>>     "git remote: set the URL for NAME to URL"
>>     git remote set-url NAME URL
>>     (current behavior: YOU git guru knows what I do right?)
>
> Yeah, I think that addresses my concern (because it explicitly leaves
> no-option as a historical curiosity, and not as an implicit version of
> "--both").

Fine by me, too.

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

On Monday 24 November 2014 17:54:57 Jeff King wrote:
> On Mon, Nov 24, 2014 at 11:47:30PM +0100, Peter Wu wrote:
> > I can understand that --fetch sounds a bit weird, what about this
> > natural translation:
> > 
> >     "git remote: set the URL (only the fetch one) for NAME to URL"
> >     git remote set-url --only=fetch NAME URL
> > 
> >     "git remote: set the URL (only the push one) for NAME to URL"
> >     git remote set-url --only=push NAME URL
> >     (obsoletes --push)
> > 
> >     "git remote: set the URL (both) for NAME to URL"
> >     git remote set-url --only=both NAME URL
> >     (it would be nice if --only=both (weird!) can be removed in the
> >     future such that the option is more natural)
> > 
> >     "git remote: set the URL for NAME to URL"
> >     git remote set-url NAME URL
> >     (current behavior: YOU git guru knows what I do right?)
> 
> Yeah, I think that addresses my concern (because it explicitly leaves
> no-option as a historical curiosity, and not as an implicit version of
> "--both").

Ok, I will make a clear note about the default (without --only) behavior
having weird behavior for historical reasons. Are you really OK with
--only=both? It sounds a bit odd (mathematically speaking it is correct
as fetch and push are both partitions that form the whole set if you
ignore the historical behavior).

> > >   3. Live with it. Probably address the weirdness in the documentation.
> > > 
> > >   4. Do nothing, drop the patch.
> > > 
> > > I think I'd be OK with (3), with an appropriate documentation update.
> > 
> > I prefer 1 for now as it avoids the extra manual action I have to take
> > when changing URLs.
> 
> I'm not sure if I was clear on (3), but "live with it" was "live with
> your original patch". Which I think you would also be happy with.

Oh yes, I misunderstood this one ;)

What about the translations? Should I send a separate patch for that or
can I update all translations at once?
-- 
Kind regards,
Peter
https://lekensteyn.nl

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Jeff King]

On Tue, Nov 25, 2014 at 12:27:31AM +0100, Peter Wu wrote:

> On Monday 24 November 2014 17:54:57 Jeff King wrote:
> > On Mon, Nov 24, 2014 at 11:47:30PM +0100, Peter Wu wrote:
> > > I can understand that --fetch sounds a bit weird, what about this
> > > natural translation:
> > > 
> > >     "git remote: set the URL (only the fetch one) for NAME to URL"
> > >     git remote set-url --only=fetch NAME URL
> > > 
> > >     "git remote: set the URL (only the push one) for NAME to URL"
> > >     git remote set-url --only=push NAME URL
> > >     (obsoletes --push)
> > > 
> > >     "git remote: set the URL (both) for NAME to URL"
> > >     git remote set-url --only=both NAME URL
> > >     (it would be nice if --only=both (weird!) can be removed in the
> > >     future such that the option is more natural)
> > > 
> > >     "git remote: set the URL for NAME to URL"
> > >     git remote set-url NAME URL
> > >     (current behavior: YOU git guru knows what I do right?)
> > 
> > Yeah, I think that addresses my concern (because it explicitly leaves
> > no-option as a historical curiosity, and not as an implicit version of
> > "--both").
> 
> Ok, I will make a clear note about the default (without --only) behavior
> having weird behavior for historical reasons. Are you really OK with
> --only=both? It sounds a bit odd (mathematically speaking it is correct
> as fetch and push are both partitions that form the whole set if you
> ignore the historical behavior).

Maybe "--operation={push,fetch,both}" would be less odd (though
"--operation" is rather clunky, I could not think of a better word). It
is the conjunction of "--only" an "both" that makes little sense.

However, I think what removed the confusion for me in your --only=both
proposal was the presence of a "both" option, since it made it more
clear that is not what no-option means. So what about just "--push",
"--fetch", and "--both"? Explain the current behavior of no-options in
the documentation as a historical oddity.

That also gives us an easy path forward for changing the behavior.
During the transition period, people should use --push, --fetch, or
--both. Using no-options provides a warning. After a settling period,
the no-option behavior will switch to one of those (presumably --both),
and drop the warning.

You do not have to do the migration path if you don't want to. Adding
"--fetch" and "--both" scratches your itch and sets us up to migrate
later.

> What about the translations? Should I send a separate patch for that or
> can I update all translations at once?

You do not have to update the translations. When we near a release, the
l10n coordinator will run "make pot" to update po/git.pot with the
strings marked for translation, and then the translators will write
translations for the new strings. You are of course welcome to help with
the translation effort at that stage. :)

Details are in po/README.

-Peff

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> However, I think what removed the confusion for me in your --only=both
> proposal was the presence of a "both" option, since it made it more
> clear that is not what no-option means. So what about just "--push",
> "--fetch", and "--both"?

I think that is the set that is most sensible, at least
syntactically, among the ones I heard so far in this thread.

However, one thing still makes me wonder....

After doing "set-url --fetch" and nothing else, because the user
never said "--both" or "--push", does the user get a configuration
where "git push" fails?  Or does "set-url --fetch" still gives us
remote.nick.url and causes "git push" to also go there?

If that is the case, then did addition of "--fetch" achieve anything
to reduce confusion?

After doing "set-url --push" and nothing else, I suspect that having
remote.nick.pushURL alone without remote.nick.URL will make "git fetch"
to fail, which would be in line with my expectation.  I just expected
anything we do in the name of symmetry or consistency would work the
same/symmetric way, I cannot see how "set-url --fetch" would work to
make its effect symmetric to the "set-url --push" one.

Puzzled...

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Jeff King]

On Mon, Nov 24, 2014 at 08:55:13PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > However, I think what removed the confusion for me in your --only=both
> > proposal was the presence of a "both" option, since it made it more
> > clear that is not what no-option means. So what about just "--push",
> > "--fetch", and "--both"?
> 
> I think that is the set that is most sensible, at least
> syntactically, among the ones I heard so far in this thread.
> 
> However, one thing still makes me wonder....
> 
> After doing "set-url --fetch" and nothing else, because the user
> never said "--both" or "--push", does the user get a configuration
> where "git push" fails?  Or does "set-url --fetch" still gives us
> remote.nick.url and causes "git push" to also go there?

I think the latter. And that makes sense to me. Push falls back to fetch
at runtime. And you never set a push url, so that's what happens.

But it is not symmetric. We do not fall back fetch to push. We _could_,
but that is a separate issue (one for git-fetch, and not git-remote).
And I do not think it is one anybody is particularly asking for.

We could also stop making push fall back to fetch. But I think people
would find that irritating.

I dunno. I think there has always been an implicit "subordinate"
relationship between fetch and push URLs, with fetch being the "main"
one. Maybe that is so ingrained in me at this point that I do not see a
problem with the asymmetry.

-Peff

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

On Monday 24 November 2014 23:08:26 Jeff King wrote:
> However, I think what removed the confusion for me in your --only=both
> proposal was the presence of a "both" option, since it made it more
> clear that is not what no-option means. So what about just "--push",
> "--fetch", and "--both"? Explain the current behavior of no-options in
> the documentation as a historical oddity.

Ok, this sounds even better. I have dropped the --only part and made the
options --push, --fetch and --both disjoint (overriding each other). A
patch will follow soon. Maybe it should warn when you try to specify
both options though.

> That also gives us an easy path forward for changing the behavior.
> During the transition period, people should use --push, --fetch, or
> --both. Using no-options provides a warning. After a settling period,
> the no-option behavior will switch to one of those (presumably --both),
> and drop the warning.
> 
> You do not have to do the migration path if you don't want to. Adding
> "--fetch" and "--both" scratches your itch and sets us up to migrate
> later.

I have documented the historic behavior and mentioned that it is
/possible/ that the option --both becomes default in the future.

> > What about the translations? Should I send a separate patch for that or
> > can I update all translations at once?
> 
> You do not have to update the translations. When we near a release, the
> l10n coordinator will run "make pot" to update po/git.pot with the
> strings marked for translation, and then the translators will write
> translations for the new strings. You are of course welcome to help with
> the translation effort at that stage. 
> 
> Details are in po/README.

Well, it is not necessary the translations, but the format of them. The
format
"git remote set-url --delete <name> <url>" has changed to
"git remote set-url [--both | --fetch | --push] --delete <name> <url>"
for example. The old strings are still usable, so I wonder whether I can
make it easier for the i10n maintainer to recognize this change?
-- 
Kind regards,
Peter
https://lekensteyn.nl

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

On Monday 24 November 2014 21:19:16 Junio C Hamano wrote:
> On Mon, Nov 24, 2014 at 9:01 PM, Jeff King <peff@peff.net> wrote:
> > We could also stop making push fall back to fetch. But I think people
> > would find that irritating.

The common case is probably having the same fetch and push URL, so I
think that this should not be changed.

> > I dunno. I think there has always been an implicit "subordinate"
> > relationship between fetch and push URLs, with fetch being the "main"
> > one. Maybe that is so ingrained in me at this point that I do not see a
> > problem with the asymmetry.
> 
> I actually do not have problem with asymmetry/subordinate
> relationship myself, but then why are we adding --fetch to
> complement --push in the first place?
> 
> Or perhaps I am misunderstanding the suggested semantics
> of --both. That "subordinate" relationship really means that
> remote.nick.URL is the one that is used for both directions
> when pushURL is not set.
> 
> I misunderstood that --both would add identical value to both
> remote.nick.URL and remote.nick.pushURL, but that would
> break the implicit subordinate relationship. Is the suggested
> semantics of "set-url --both" to first delete remote.nick.pushURL
> if exists and then to set remote.nick.URL?
> 
> If that is what is being proposed, then I think it makes sense.

Yes, your last understanding is correct. For this feature, try to think
as the user who does not know about the configuration implementation.
That is why the --fetch option was proposed, earlier it did not make
sense to me why a --push option exists, but a --fetch option is missing.

Option '--both' will drop the push URL and result in an implicit
fallback to the fetch URL. It becomes slightly more hairy in the
presence of URL sets (using --add and --delete), but I have also tried
to make that act sensibly).
-- 
Kind regards,
Peter
https://lekensteyn.nl

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Philip Oakley]

From: "Peter Wu" <peter@lekensteyn.nl>
> Ok, I will make a clear note about the default (without --only) 
> behavior
> having weird behavior for historical reasons. Are you really OK with
> --only=both? It sounds a bit odd (mathematically speaking it is 
> correct
> as fetch and push are both partitions that form the whole set if you
> ignore the historical behavior).
>
How about :

s/--only/--direction/

or some suitable abbreviation (--dirn ?)
--
Philip 

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Peter Wu]

On Saturday 29 November 2014 13:31:18 Philip Oakley wrote:
> From: "Peter Wu" <peter@lekensteyn.nl>
> > Ok, I will make a clear note about the default (without --only) 
> > behavior
> > having weird behavior for historical reasons. Are you really OK with
> > --only=both? It sounds a bit odd (mathematically speaking it is 
> > correct
> > as fetch and push are both partitions that form the whole set if you
> > ignore the historical behavior).
> >
> How about :
> 
> s/--only/--direction/
> 
> or some suitable abbreviation (--dirn ?)

In the next version of the patch I went for three separate options,
--fetch, --push and --both:
http://article.gmane.org/gmane.comp.version-control.git/260213
(Juno, Jeff: ping?)

The option --direction=<fetch|push|both> is a bit longer and --dirn can
be mistaken for "directory N".
-- 
Kind regards,
Peter
https://lekensteyn.nl

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[Junio C Hamano]

Peter Wu <peter@lekensteyn.nl> writes:

> On Saturday 29 November 2014 13:31:18 Philip Oakley wrote:
>> From: "Peter Wu" <peter@lekensteyn.nl>
>> > Ok, I will make a clear note about the default (without --only) 
>> > behavior
>> > having weird behavior for historical reasons. Are you really OK with
>> > --only=both? It sounds a bit odd (mathematically speaking it is 
>> > correct
>> > as fetch and push are both partitions that form the whole set if you
>> > ignore the historical behavior).
>> >
>> How about :
>> 
>> s/--only/--direction/
>> 
>> or some suitable abbreviation (--dirn ?)
>
> In the next version of the patch I went for three separate options,
> --fetch, --push and --both:
> http://article.gmane.org/gmane.comp.version-control.git/260213
> (Juno, Jeff: ping?)
>
> The option --direction=<fetch|push|both> is a bit longer and --dirn can
> be mistaken for "directory N".

If we have to have three variants, --{fetch,push,both} would be the
easiest to understand among the possibilities listed above, I would
think.