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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 9, 2018, 03:15 UTC
Message-ID: <xmqqin1754c5.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <b910987d3aa778b39b6e27a75fe685a4797b8422.1541730538.git.liu.denton@gmail.com>

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

Show 15 quoted lines

> This adds the --save-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-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

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.

> +With `--save-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 or if any push URLs are defined because behavior would be ambiguous.

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.

Show 15 quoted lines

> diff --git a/builtin/remote.c b/builtin/remote.c
> index f7edf7f2cb..0eaec7ef38 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-push <name> <url>"),
>  	NULL
>  };

Needs update?

Show 11 quoted lines

> @@ -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-push <name> <url>"),
>  	NULL
>  };

Needs update?

> +		if (save_push) {
> +			if (remote->url_nr != 1 || remote->pushurl_nr != 0)
> +				die(_("--save-push can only be used when one url and no pushurl is defined"), remotename);

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.