Re: [PATCH v3] remote: add --save-to-push option to git remote set-url

[Junio C Hamano <gitster@pobox.com>]

Denton Liu <liu.denton@gmail.com> writes:

> This adds the --save-to-push option to `git remote set-url` such that
> when executed, we move the remote.*.url to remote.*.pushurl and set
> remote.*.url to the given url argument.
>
> For example, if we have the following config:
>
> 	[remote "origin"]
> 		url = git@github.com:git/git.git
>
> `git remote set-url --save-to-push origin https://github.com/git/git.git`
> would change the config to the following:
>
> 	[remote "origin"]
> 		url = https://github.com/git/git.git
> 		pushurl = git@github.com:git/git.git
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> On Fri, Nov 09, 2018 at 12:15:22PM +0900, Junio C Hamano wrote:
>> This sounds more like "saving to push" (i.e. what you are saving is
>> the existing "url" and the "push" is a shorthand for "pushURL",
>> which is the location the old value of "url" is aved to), not "save
>> (the) push(URL)".  So if adding this option makes sense, I would say
>> "--save-to-push" (or even "--save-to-pushURL") may be a more
>> appropriate name for it.
>> 
>
> My original intention was for it to mean "save push behavior" but I
> agree with you that it's unclear so I'm changing it to --save-to-push.
>
>> Ambigous in what way?  You asked the current URL to be saved as a
>> pushURL, so existing pushURL destinations should not come into play,
>> I would think.  If there are more than one URL (not pushURL), on the
>> other hand, I think you have a bigger problem (where would "git fetch"
>> fetch from, and how would these multiple URLs are prevented from
>> trashing refs/remotes/$remote/* with each other's refs?), so
>> stopping the operation before "set-url" makes the problem worse is
>> probably a good idea, but I think that is true with or without this
>> new option.
>> 
>
>> I _think_ in the future (if this option turns out to be widely used)
>> people may ask for this condition to be loosened somewhat, but it is
>> relatively easy to start restrictive and then to loosen later, so I
>> think this is OK for now.
>> 
>
> I agree, there's no reason why we shouldn't allow appending to the push
> URLs if one already exists so I removed that removed that restriction.
> ---
>  Documentation/git-remote.txt |  5 +++++
>  builtin/remote.c             | 26 +++++++++++++++++++++-----
>  t/t5505-remote.sh            | 11 +++++++++++
>  3 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index 0cad37fb81..8f9d700252 100644
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
> @@ -19,6 +19,7 @@ SYNOPSIS
>  '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 --save-to-push' <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>)...]
> @@ -155,6 +156,10 @@ With `--delete`, instead of changing existing URLs, all URLs matching
>  regex <url> are deleted for remote <name>.  Trying to delete all
>  non-push URLs is an error.
>  +
> +With `--save-to-push`, the current URL is saved into the push URL before
> +setting the URL to <url>. Note that this command will not work if more than one
> +URL is defined because the behavior would be ambiguous.
> ++
>  Note that the push URL and the fetch URL, even though they can
>  be set differently, must still refer to the same place.  What you
>  pushed to the push URL should be what you would see if you
> diff --git a/builtin/remote.c b/builtin/remote.c
> index f7edf7f2cb..3249c3face 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -24,8 +24,9 @@ static const char * const builtin_remote_usage[] = {
>  	N_("git remote set-branches [--add] <name> <branch>..."),
>  	N_("git remote get-url [--push] [--all] <name>"),
>  	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 --add [--push] <name> <newurl>"),
> +	N_("git remote set-url --delete [--push] <name> <url>"),
> +	N_("git remote set-url --save-to-push <name> <url>"),
>  	NULL
>  };
>  
> @@ -77,8 +78,9 @@ static const char * const builtin_remote_geturl_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 --add [--push] <name> <newurl>"),
> +	N_("git remote set-url --delete [--push] <name> <url>"),
> +	N_("git remote set-url --save-to-push <name> <url>"),
>  	NULL
>  };
>  
> @@ -1519,7 +1521,7 @@ static int get_url(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, push_mode = 0, save_to_push = 0, add_mode = 0, delete_mode = 0;
>  	int matches = 0, negative_matches = 0;
>  	const char *remotename = NULL;
>  	const char *newurl = NULL;
> @@ -1532,6 +1534,8 @@ static int set_url(int argc, const char **argv)
>  	struct option options[] = {
>  		OPT_BOOL('\0', "push", &push_mode,
>  			 N_("manipulate push URLs")),
> +		OPT_BOOL('\0', "save-to-push", &save_to_push,
> +			 N_("change fetching URL behavior")),
>  		OPT_BOOL('\0', "add", &add_mode,
>  			 N_("add URL")),
>  		OPT_BOOL('\0', "delete", &delete_mode,
> @@ -1543,6 +1547,8 @@ static int set_url(int argc, const char **argv)
>  
>  	if (add_mode && delete_mode)
>  		die(_("--add --delete doesn't make sense"));
> +	if (save_to_push && (push_mode || add_mode || delete_mode))
> +		die(_("--save-to-push cannot be used with other options"));
>  
>  	if (argc < 3 || argc > 4 || ((add_mode || delete_mode) && argc != 3))
>  		usage_with_options(builtin_remote_seturl_usage, options);
> @@ -1564,6 +1570,16 @@ static int set_url(int argc, const char **argv)
>  		urlset = remote->pushurl;
>  		urlset_nr = remote->pushurl_nr;
>  	} else {
> +		if (save_to_push) {
> +			if (remote->url_nr != 1)
> +				die(_("--save-to-push can only be used when only one url is defined"), remotename);

Unused parameter "remotename" is fed to die().  I'll drop this.

> +
> +			strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
> +			git_config_set_multivar(name_buf.buf,
> +			      remote->url[0], "^$", 0);
> +			strbuf_reset(&name_buf);
> +		}
> +
>  		strbuf_addf(&name_buf, "remote.%s.url", remotename);
>  		urlset = remote->url;
>  		urlset_nr = remote->url_nr;
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index d2a2cdd453..434c1f828a 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -1194,6 +1194,17 @@ test_expect_success 'remote set-url --delete baz' '
>  	cmp expect actual
>  '
>  
> +test_expect_success 'remote set-url --save-to-push bbb' '
> +	git remote set-url --save-to-push someremote bbb &&
> +	echo bbb >expect &&
> +	echo "YYY" >>expect &&
> +	echo ccc >>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 'extra args: setup' '
>  	# add a dummy origin so that this does not trigger failure
>  	git remote add origin .