[RFC] remote: add --fetch option to git remote set-url
diff mbox series

Message ID 1d1b0fe85ddd89cf8172e730e8886d5b4a9ea7eb.1540627720.git.liu.denton@gmail.com
State New
Headers show
Series
  • [RFC] remote: add --fetch option to git remote set-url
Related show

Commit Message

Denton Liu Oct. 27, 2018, 8:09 a.m. UTC
This adds the --fetch 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 --fetch 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>
Signed-off-by: Filip Francetic <f.francet@hotmail.com>
---
 builtin/remote.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

Comments

Junio C Hamano Oct. 29, 2018, 5:57 a.m. UTC | #1
Denton Liu <liu.denton@gmail.com> writes:

> This adds the --fetch 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.
>

I suspect this is a horrible idea from end-user's point of view.
"set-url --push" is used to SET pushURL instead of setting URL and
does not MOVE anything.  Why should the end user expect and remember
"set-url --fetch" works very differently?  

If there is a need for a "--move-URL-to-pushURL-and-set-pushURL"
short-hand to avoid having to use two commands

	git remote set-url --push $(git remote --get-url origin) origin
	git remote set-url $there origin

it should not be called "--fetch", which has a strong connotation of
being an opposite of existing "--push", but something else.  And
then we need to ask ourselves if we also need such a short-hand to
"--move-pushURL-to-URL-and-set-URL" operation.  The answer to the
last question would help us decide if (1) this combined operation is
a good idea to begin with and (2) what is the good name for such an
operation.

Assuming that the short-hand operation is a good idea in the first
place, without deciding what the externally visible good name for it
is, let's read on.

> +	/*
> +	 * If add_mode, we will be appending to remote.*.url so we shouldn't move the urls over.
> +	 * If pushurls exist, we don't need to move the urls over to pushurl.
> +	 */
> +	move_fetch_to_push = fetch_mode && !add_mode && !remote->pushurl_nr;

Should this kind of "the user asked for --fetch, but sometimes it is
not appropriate to honor that request" be done silently like this?

Earlier you had a check like this:

> +	if (push_mode && fetch_mode)
> +		die(_("--push --fetch doesn't make sense"));

If a request to "--fetch" is ignored when "--add" is given, for
example, shouldn't the combination also be diagnosed as "not making
sense, we'd ignore your wish to use the --fetch option"?  Similarly
for the case where there already is pushurl defined for the remote.

This is a different tangent on the same line, but it could be that
the user wants to have two (or more) pushURLs because the user wants
to push to two remotes at the same time with "git push this-remote",
so silently ignoring "--force" may not be the right thing in the
first place.  We may instead need to make the value of URL to an
extra pushURL entry (if we had one, we now have two).
Denton Liu Oct. 30, 2018, 7:56 a.m. UTC | #2
On Mon, Oct 29, 2018 at 02:57:28PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > This adds the --fetch 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.
> >
> 
> I suspect this is a horrible idea from end-user's point of view.
> "set-url --push" is used to SET pushURL instead of setting URL and
> does not MOVE anything.  Why should the end user expect and remember
> "set-url --fetch" works very differently?  
>

I agree, `--fetch` is a terrible name for this. Perhaps a better name
would be something like `--fetch-behavior` or `--keep-push` so that the
behaviour is more transparent for the end-user. Either way, I think we
can decide on the name later.

> If there is a need for a "--move-URL-to-pushURL-and-set-pushURL"
> short-hand to avoid having to use two commands
> 
> 	git remote set-url --push $(git remote --get-url origin) origin
> 	git remote set-url $there origin
> 
> it should not be called "--fetch", which has a strong connotation of
> being an opposite of existing "--push", but something else.  And
> then we need to ask ourselves if we also need such a short-hand to
> "--move-pushURL-to-URL-and-set-URL" operation.  The answer to the
> last question would help us decide if (1) this combined operation is
> a good idea to begin with and (2) what is the good name for such an
> operation.
> 
> Assuming that the short-hand operation is a good idea in the first
> place, without deciding what the externally visible good name for it
> is, let's read on.
> 
> > +	/*
> > +	 * If add_mode, we will be appending to remote.*.url so we shouldn't move the urls over.
> > +	 * If pushurls exist, we don't need to move the urls over to pushurl.
> > +	 */
> > +	move_fetch_to_push = fetch_mode && !add_mode && !remote->pushurl_nr;
> 
> Should this kind of "the user asked for --fetch, but sometimes it is
> not appropriate to honor that request" be done silently like this?
> 
> Earlier you had a check like this:
> 
> > +	if (push_mode && fetch_mode)
> > +		die(_("--push --fetch doesn't make sense"));
> 
> If a request to "--fetch" is ignored when "--add" is given, for
> example, shouldn't the combination also be diagnosed as "not making
> sense, we'd ignore your wish to use the --fetch option"?  Similarly
> for the case where there already is pushurl defined for the remote.
> 
> This is a different tangent on the same line, but it could be that
> the user wants to have two (or more) pushURLs because the user wants
> to push to two remotes at the same time with "git push this-remote",
> so silently ignoring "--force" may not be the right thing in the
> first place.  We may instead need to make the value of URL to an
> extra pushURL entry (if we had one, we now have two).
>

Perhaps I should motivate the use-case for this option. There have been
times when I've had the URL set to something but no pushURL. I've
wanted to only change the fetching URL and keep the pushing URL the same
but unfortunately, there's no way to do that because of the url/pushurl
split is set up.

My implementation of --fetch is supposed to emulate what would happen if
git were implemented with fetchurl/pushurl instead. Does the patch make
more sense in this context?

Please let me know if you think that the concept behind this patch is a
good idea. Thanks!
Junio C Hamano Oct. 30, 2018, 10:11 a.m. UTC | #3
Denton Liu <liu.denton@gmail.com> writes:

> On Mon, Oct 29, 2018 at 02:57:28PM +0900, Junio C Hamano wrote:
> ...
>> Earlier you had a check like this:
>> 
>> > +	if (push_mode && fetch_mode)
>> > +		die(_("--push --fetch doesn't make sense"));
>> 
>> If a request to "--fetch" is ignored when "--add" is given, for
>> example, shouldn't the combination also be diagnosed as "not making
>> sense, we'd ignore your wish to use the --fetch option"?  Similarly
>> for the case where there already is pushurl defined for the remote.

Clarification.  Here I am suggesting that a part of the logic in the
earlier assignment to move_fetch_to_push should become a similar
call to die(), detecting a competing and unsatisfiable wish, rather
than getting silently ignored.

>> This is a different tangent on the same line, but it could be that
>> the user wants to have two (or more) pushURLs because the user wants
>> to push to two remotes at the same time with "git push this-remote",
>> so silently ignoring "--force" may not be the right thing in the

Correction.  s/--force/--fetch/ was what I meant here.

>> first place.  We may instead need to make the value of URL to an
>> extra pushURL entry (if we had one, we now have two).

Also, additionally, since there is no use to have two or more URL,
because unlike "git push $there" that can push to two places,
fetching from two places into the same set of remote-tracking
branches would not make sense, --move-pushURL-to-URL-and-set-pushURL
operation that is the symmetry of what the patch under discussion
proposes should fail instead of creating an extra URL entry, breaking
an apparent symmetry.

> Perhaps I should motivate the use-case for this option. There have been
> times when I've had the URL set to something but no pushURL. I've
> wanted to only change the fetching URL and keep the pushing URL the same

URL (plus optionally pushURL) split was done because most everybody
were fetching from and pushing to the same place; it would not have
made any sense to have fetchURL and pushURL that are separate, as
that would have forced everybody to have both, when majority of the
users would have to set them to the same value.

Quite honestly, tweaking URL and/or pushURL is not something you'd
do every three months or more frequently, so I do not particularly
feel sympathetic to the cause of this patch, which would allow
setting one to the value that happens to be set to the other one
while setting an arbitrary new value to the other one.  Once the
user's need deviates from that single niche pattern, the user needs
to update both, and setting these two independently is quite simple
and straight-forward in the first place.  And that same simple pattern
to set two independently can be used without learning this new option.

> My implementation of --fetch is supposed to emulate what would happen if
> git were implemented with fetchurl/pushurl instead. Does the patch make
> more sense in this context?

Hmph.  As a system that has fetchURL and pushURL independently and
forces everybody to set both does not make much sense in the first
place, the patch making sense in that context is not a very strong
reason to support it.

> Please let me know if you think that the concept behind this patch is a
> good idea. Thanks!

If I don't then I do not have to let you know, then ;-) 

I do not particularly think it is a horrible idea, but I do not see
that it would be a feature that helps particuarly wide audience.

Patch
diff mbox series

diff --git a/builtin/remote.c b/builtin/remote.c
index f7edf7f2c..fcf1220c6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -23,9 +23,9 @@  static const char * const builtin_remote_usage[] = {
 	N_("git remote [-v | --verbose] update [-p | --prune] [(<group> | <remote>)...]"),
 	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 [--push|--fetch] <name> <newurl> [<oldurl>]"),
+	N_("git remote set-url --add [--push|--fetch] <name> <newurl>"),
+	N_("git remote set-url --delete [--push|--fetch] <name> <url>"),
 	NULL
 };
 
@@ -76,9 +76,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 [--push|--fetch] <name> <newurl> [<oldurl>]"),
+	N_("git remote set-url --add [--push|--fetch] <name> <newurl>"),
+	N_("git remote set-url --delete [--push|--fetch] <name> <url>"),
 	NULL
 };
 
@@ -1519,7 +1519,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, fetch_mode = 0, add_mode = 0, delete_mode = 0, move_fetch_to_push = 0;
 	int matches = 0, negative_matches = 0;
 	const char *remotename = NULL;
 	const char *newurl = NULL;
@@ -1532,6 +1532,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', "fetch", &fetch_mode,
+			 N_("manipulate fetch URLs")),
 		OPT_BOOL('\0', "add", &add_mode,
 			 N_("add URL")),
 		OPT_BOOL('\0', "delete", &delete_mode,
@@ -1543,6 +1545,8 @@  static int set_url(int argc, const char **argv)
 
 	if (add_mode && delete_mode)
 		die(_("--add --delete doesn't make sense"));
+	if (push_mode && fetch_mode)
+		die(_("--push --fetch doesn't make sense"));
 
 	if (argc < 3 || argc > 4 || ((add_mode || delete_mode) && argc != 3))
 		usage_with_options(builtin_remote_seturl_usage, options);
@@ -1559,18 +1563,40 @@  static int set_url(int argc, const char **argv)
 	if (!remote_is_configured(remote, 1))
 		die(_("No such remote '%s'"), remotename);
 
+	/*
+	 * If add_mode, we will be appending to remote.*.url so we shouldn't move the urls over.
+	 * If pushurls exist, we don't need to move the urls over to pushurl.
+	 */
+	move_fetch_to_push = fetch_mode && !add_mode && !remote->pushurl_nr;
+
 	if (push_mode) {
 		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
 		urlset = remote->pushurl;
 		urlset_nr = remote->pushurl_nr;
 	} else {
+		if (move_fetch_to_push) {
+			strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
+			for (i = 0; i < remote->url_nr; i++) {
+				git_config_set_multivar(name_buf.buf, remote->url[i],
+						"^$", 0);
+			}
+			strbuf_reset(&name_buf);
+		}
+
 		strbuf_addf(&name_buf, "remote.%s.url", remotename);
 		urlset = remote->url;
 		urlset_nr = remote->url_nr;
 	}
 
+	/* Empty fetch URLs if they are being replaced */
+	if (move_fetch_to_push) {
+		for (i = 0; i < remote->url_nr; i++) {
+			git_config_set_multivar(name_buf.buf, NULL, remote->url[i], 1);
+		}
+	}
+
 	/* Special cases that add new entry. */
-	if ((!oldurl && !delete_mode) || add_mode) {
+	if ((!oldurl && !delete_mode) || move_fetch_to_push || add_mode) {
 		if (add_mode)
 			git_config_set_multivar(name_buf.buf, newurl,
 						       "^$", 0);