patch, 2 partsFix remote_is_configured()

A surprising behavior triggered the bug report in https://github.com/git-for-windows/git/issues/888: the mere existence of the config setting "remote.origin.prune" (in this instance, configured via ~/.gitconfig so that it applies to all repositories) fooled `git remote rename <source> <target>` into believing that the <target> remote is already there.

This patch pair demonstrates the problem, and then fixes it (along with potential similar problems, such as setting an HTTP proxy for remotes of a given name via ~/.gitconfig).

Johannes Schindelin (2):
  remote rename: demonstrate a bogus "remote exists" bug
  Be more careful when determining whether a remote was configured

 remote.c          | 9 ++++++++-
 remote.h          | 2 +-
 t/t5505-remote.sh | 9 +++++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

base-commit: d7dffce1cebde29a0c4b309a79e4345450bf352a
Published-As: https://github.com/dscho/git/releases/tag/rename-remote-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rename-remote-v1

Some users like to set `remote.origin.prune = true` in their ~/.gitconfig so that all of their repositories use that default.

However, our code is ill-prepared for this, mistaking that single entry to mean that there is already a remote of the name "origin", even if there is not.

This patch adds a test case demonstrating this issue.

Reported by Andrew Arnott.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5505-remote.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

One of the really nice features of the ~/.gitconfig file is that users can override defaults by their own preferred settings for all of their repositories.

One such default that some users like to override is whether the "origin" remote gets auto-pruned or not. The user would simply call

	git config --global remote.origin.prune true

and from now on all "origin" remotes would be pruned automatically when fetching into the local repository.

There is just one catch: now Git thinks that the "origin" remote is configured, as it does not discern between having a remote whose fetch (and/or push) URL and refspec is set, and merely having preemptively-configured, global flags for specific remotes.

Let's fix this by telling Git that a remote is not configured unless any fetch/push URL or refspect is configured explicitly.

As a special exception, we deem a remote configured also when *only* the "vcs" setting is configured. The commit a31eeae27f (remote: use remote_is_configured() for add and rename, 2016-02-16) specifically extended our test suite to verify this, so it is safe to assume that there has been at least one user with a legitimate use case for this.

This fixes https://github.com/git-for-windows/git/issues/888

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c          | 9 ++++++++-
 remote.h          | 2 +-
 t/t5505-remote.sh | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

On Tue, Jan 17, 2017 at 10:19:24PM +0100, Johannes Schindelin wrote:

Hmm. Old versions of GitHub for Windows used to set fetch refspecs in the system gitconfig, for a similar purpose to what you want to do with remote.origin.prune.

I notice here that setting a refspec _does_ define a remote. Is there a reason you drew the line there, and not at, say, whether it has a URL?

-Peff

Jeff King <peff@peff.net> writes:

"Not configured unless any URL or refspec is configured" means that if URL is there, even if there is no refspec, then it is a remote definition, right?

But I think "what does it mean to define a remote" is a question that you are asking, and that is not necessary to answer within the scope of this topic.

I do agree that honoring .prune is nonsense unless refspecs are defined, but the question "does it make sense to allow prune?" is different from "is it configured?". Your "you can set .proxy and other useful things, it is just .prune does not make sense" is a quite appropriate statement to illustrate the difference.

Perhaps instead of adding "is it configured?" flag that is too broadly named and has too narrow meaning, would it make more sense to introduce "int can_prune(struct remote *remote)" that looks at the remote refspecs?

Hi Junio,

On Tue, 17 Jan 2017, Junio C Hamano wrote:

That ("can we prune?") is not the question we need to ask when determining whether we can rename a remote to a new name.

Ciao, Johannes

Hi Peff,

On Tue, 17 Jan 2017, Jeff King wrote:

I want to err on the side of caution. That's why.

Ciao, Johannes

On Wed, Jan 18, 2017 at 01:34:28PM +0100, Johannes Schindelin wrote:

I guess I just don't see why changing the behavior with respect to "prune" or "proxy" is any less conservative than changing the one for "refspec". Both can make some real-world cases work, and both can cause breakage in some possible real-world cases. If we are going to change anything, it seems like we should at least aim for a simple and consistent rule (since users have to know which keys can be put in ~/.gitconfig and which cannot).

I can think of one alternative approach that might be easier for users to understand, and that we already use elsewhere (e.g., with "http.*" config): have a set of "default" remote keys (e.g., just "remote.key") that git falls back to when the remote.*.key isn't set. Then your use case becomes something like:

  [remote]
  prune = true

That's not _exactly_ the same, as it applies to all remotes, not just "origin" (other remotes can override it, but would need to do so explicitly). But I have a suspicion that may actually be what users want.

-Peff

Hi Peff,

On Wed, 18 Jan 2017, Jeff King wrote:

Let's take a step back and consider the problem I try to solve, okay?

The problem is that `git remote rename <old> <new>` refuses to do its job if it detects a configured `<new>`. And it detects a configured `<new>` even in cases where there is not *really* any remote configured.

The example use case is to configure `remote.origin.prune = true` in ~/.gitconfig, i.e. changing Git's default for all "origin" remotes in all of the user's repositories.

Now, the *real* fix would be to detect whether the remote was "configured" in the current repository, or in ~/.gitconfig. But that may not even be desirable, as we would want a more general fix for the question: can `git remote` rename a given remote to a new name, i.e. is that new name already taken?

And if you try to answer that last question, you will probably pick the same set of keys for which you assume that remote.<name>.<key> really configures a remote or not.

Originally, I would even have put the "vcs" into that set, as I could see a legitimate use case for users to configure "remote.svn.vcs = vcs" in ~/.gitconfig. But the regression test suite specifically tests for that case, and I trust that there was a good reason, even if Thomas did not describe that good reason in the commit message nor in any reply to this patch pair.

So how can things go wrong?

Let's take your example that the user may have specified refspecs (or prune, or proxy) for a URL via "remote.<url>.fetch", and that `git rename` incorrectly allows that as a new remote name. You know what? Let's do a real code review here, not just a patch glance-over. Let's test this and *know* whether it can be a problem:

	git remote rename origin https://github.com/git/git
	fatal: 'https://github.com/git/git' is not a valid remote name

A ha! It is *not* possible to hit that case because `git remote rename` already complains much earlier about the new name not being a valid name.

So let's see what could go wrong with another example you mentioned, that the proxy may not be used because we changed the logic of remote_is_configured(). But the only user of the remote proxy settings is in http_init() and reads:

        if (remote && remote->http_proxy)
                curl_http_proxy = xstrdup(remote->http_proxy);

        if (remote)
                var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);

It does not even test whether the remote is configured! So maybe the caller does? Nope, the only caller of http_init() that passes a remote is remote-curl's cmd_main() function, and the relevant part reads:

        remote = remote_get(argv[1]);

        if (argc > 2) {
                end_url_with_slash(&url, argv[2]);
        } else {
                end_url_with_slash(&url, remote->url[0]);
        }

        http_init(remote, url.buf, 0);

This code also does not care whether the remote "is configured" or not.

So maybe there are any other downsides with callers of remote_is_configured()?

There is one caller in builtin/fetch.c's add_remote_or_group() which clearly is covered (we need a URL, unless the vcs is configured).

All other callers are in builtin/remote.c:

- in add(), to test whether we can add a new remote,
- in mv(), to test whether the remote to rename is configured,
- in mv(), to test whether the new name is already taken,
- in rm(), to test whether the remote exists,
- in set_remote_branches(), to test whether the remote to be changed
  exists,
- in get_url(), to test whether the remote exists, and
- in set_url(), to test whether the remote exists.

It appears pretty obvious that in all of these cases, the suggested patch still makes sense and does not introduce any nasty surprise.

Sure. That'll be another patch series, though. And another contributor.

Ciao, Johannes

On Wed, Jan 18, 2017 at 05:22:40PM +0100, Johannes Schindelin wrote:

OK. Though after reading your message over several times, I think I may have confused things by raising two separate issues.

So let me re-state my issues here for clarity:

  1. I'm concerned that a setting for remote.<url>.X would fail to apply
     to a bare "git fetch <url>" if "X" is considered as "not really"
     configuring a remote.

  2. I'm concerned that splitting the remote.*.X keys into two classes:
     "really configures" and "does not really configure" creates a
     confusing interface for users. Some keys are OK to set in your
     ~/.gitconfig and others are not.

Let's talk about concern 1 first. Based on your analysis in this message, it looks like it _isn't_ a problem with your patch (because is_configured is never used for applying values, only for add/del/rename types of operations in remote.c).

So good. Thanks for addressing my concern. And that makes your "conservative" comment make more sense; the idea is that you are not breaking anything, but just loosening selectively for some values of "X".

I think there are still some weird corner cases even for "prune", though. E.g., try:

  git init
  git remote add foo whatever
  git config remote.foo.prune true
  git config remote.other.prune false

So now we have:

	[remote "foo"]
		url = whatever
		fetch = +refs/heads/*:refs/remotes/foo/*
		prune = true
	[remote "other"]
		prune = false

Now try "git remote rename foo other". With current versions of git, it's disallowed. With your patch, I get:

	[remote "other"]
		url = whatever
		prune = true
	[remote "other"]
		prune = false
		fetch = +refs/heads/*:refs/remotes/other/*

The old value of remote.other.prune is overriding the one we tried to move into place.

In your motivating example, the old value is in ~/.gitconfig, so it automatically takes lower precedence, and everything just works (I think it would also just work if "other" had been defined originally _before_ foo in the config file).

I think you could fix it by having git-remote teach the "not really" config values (like "prune") to overwrite any existing value when rewriting the config. I think this just needs the multi_replace flag set when calling git_config_set_multivar().

That raises questions about what should happen when multi-value keys like "refspec" would be set (if we were to add them to the "not really" set). Should they be overwritten, or merged? And in that sense, your patch lets you punt on those issues.

I still think my second concern is valid. It's made worse by your patch (if only because everything was disallowed before, and now some things are and some things aren't). If this is a first step towards a final state where the rules are simple, then starting conservatively makes sense. And until we get there, the answer "yes, it should, but nobody has worked out the semantics yet; care to make a patch?" is an OK one. But it sounds like you do not ever want to loosen the "refspec" case.

I don't think that's ideal, but at the very least if that's the end state, the list of "OK to use in ~/.gitconfig" keys should probably be documented.

Reading your message, though, I still wonder if we can do better...

I'd add to this that "git remote add <new>" should work in a similar way (that was the one that I think people often ran into with remote.origin.fetch refspecs).

I think _this_ is a much better way of framing the problem. It is not about which keys are set, but about _where_ they are set. IOW, a reasonable rule would be: if there is any remote.*.X in the repo config, then git-remote should consider it a configured repo. And otherwise, no matter what is in ~/.gitconfig or elsewhere, git-remote should proceed as if it doesn't exist (and repo-level config can take precedence over config defined elsewhere).

I.e., something like this:

Jeff King <peff@peff.net> writes:

I am not "you" you are addressing to, but I think tying it to where the variable came from makes quite sense.

Because it makes it no longer possible to just inspect the configured result to answer "is the remote configured?", introduction of the configured field also needs to be preserved from the original by Dscho, so does reading from historical non-config sources like $GIT_DIR/remotes/*, which are by definition per-repository thing.

IOW, with this tweak (and not setting ->configured based on what keys are set), I think Dscho's patch makes sense.

On Thu, Jan 19, 2017 at 12:12:47PM -0800, Junio C Hamano wrote:

Yeah, worry if that wasn't clear: the hunk I posted was a just a partial. The actual thing I built and ran against the test suite was exactly as you described.

-Peff

Hi Peff,

On Thu, 19 Jan 2017, Jeff King wrote:

Heh. After skimming the first three paragraphs of your mail, I had a similar idea and implemented it. It works great!

v2 coming right up, Johannes

On Tue, Jan 17, 2017 at 10:18:58PM +0100, Johannes Schindelin wrote:

I thought it was intentional that any config "created" the remote, even without a url field. E.g., you can set:

  [remote "https://example.com/foo/git"]
  proxy = localhost:1234

and then a bare "git fetch https://example.com/foo/git" will respect it.

I admit that "prune" is probably useless without a fetch refspec, though.

Note that I don't disagree that the rule "it's not a remote unless it has a url" would be a lot saner. But I have a feeling you may be breaking some existing setups.

I grepped around in the list but I couldn't find any relevant discussion. So maybe I just dreamed it.

-Peff

A surprising behavior triggered the bug report in https://github.com/git-for-windows/git/issues/888: the mere existence of the config setting "remote.origin.prune" (in this instance, configured via ~/.gitconfig so that it applies to all repositories) fooled `git remote rename <source> <target>` into believing that the <target> remote is already there.

This patch pair demonstrates the problem, and then fixes it (along with potential similar problems, such as setting an HTTP proxy for remotes of a given name via ~/.gitconfig).

Changes since v1:

 - overhauled the strategy to fix the problem: instead of looking at the
   config key to determine whether it configures a remote or not, we now
   look at the config_source: if the remote setting was configured in
   .git/config, `git remote <command>` must not overwrite it. If it was
   configured elsewhere, `git remote` cannot overwrite any setting, as
   it only touches .git/config.

Johannes Schindelin (2):
  remote rename: demonstrate a bogus "remote exists" bug
  Be more careful when determining whether a remote was configured

 builtin/fetch.c   |  2 +-
 builtin/remote.c  | 14 +++++++-------
 remote.c          | 12 ++++++++++--
 remote.h          |  4 ++--
 t/t5505-remote.sh |  7 +++++++
 5 files changed, 27 insertions(+), 12 deletions(-)

base-commit: ffac48d093d4b518a0cc0e8bf1b7cb53e0c3d7a2
Published-As: https://github.com/dscho/git/releases/tag/rename-remote-v2
Fetch-It-Via: git fetch https://github.com/dscho/git rename-remote-v2

Interdiff vs v1:

 diff --git a/builtin/fetch.c b/builtin/fetch.c
 index f1570e3464..b5ad09d046 100644
 --- a/builtin/fetch.c
 +++ b/builtin/fetch.c
 @@ -1177,7 +1177,7 @@ static int add_remote_or_group(const char *name, struct string_list *list)
  	git_config(get_remote_group, &g);
  	if (list->nr == prev_nr) {
  		struct remote *remote = remote_get(name);
 -		if (!remote_is_configured(remote))
 +		if (!remote_is_configured(remote, 0))
  			return 0;
  		string_list_append(list, remote->name);
  	}
 diff --git a/builtin/remote.c b/builtin/remote.c
 index e52cf3925b..5339ed6ad1 100644
 --- a/builtin/remote.c
 +++ b/builtin/remote.c
 @@ -186,7 +186,7 @@ static int add(int argc, const char **argv)
  	url = argv[1];
  
  	remote = remote_get(name);
 -	if (remote_is_configured(remote))
 +	if (remote_is_configured(remote, 1))
  		die(_("remote %s already exists."), name);
  
  	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
 @@ -618,14 +618,14 @@ static int mv(int argc, const char **argv)
  	rename.remote_branches = &remote_branches;
  
  	oldremote = remote_get(rename.old);
 -	if (!remote_is_configured(oldremote))
 +	if (!remote_is_configured(oldremote, 1))
  		die(_("No such remote: %s"), rename.old);
  
  	if (!strcmp(rename.old, rename.new) && oldremote->origin != REMOTE_CONFIG)
  		return migrate_file(oldremote);
  
  	newremote = remote_get(rename.new);
 -	if (remote_is_configured(newremote))
 +	if (remote_is_configured(newremote, 1))
  		die(_("remote %s already exists."), rename.new);
  
  	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
 @@ -753,7 +753,7 @@ static int rm(int argc, const char **argv)
  		usage_with_options(builtin_remote_rm_usage, options);
  
  	remote = remote_get(argv[1]);
 -	if (!remote_is_configured(remote))
 +	if (!remote_is_configured(remote, 1))
  		die(_("No such remote: %s"), argv[1]);
  
  	known_remotes.to_delete = remote;
 @@ -1415,7 +1415,7 @@ static int set_remote_branches(const char *remotename, const char **branches,
  	strbuf_addf(&key, "remote.%s.fetch", remotename);
  
  	remote = remote_get(remotename);
 -	if (!remote_is_configured(remote))
 +	if (!remote_is_configured(remote, 1))
  		die(_("No such remote '%s'"), remotename);
  
  	if (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {
 @@ -1469,7 +1469,7 @@ static int get_url(int argc, const char **argv)
  	remotename = argv[0];
  
  	remote = remote_get(remotename);
 -	if (!remote_is_configured(remote))
 +	if (!remote_is_configured(remote, 1))
  		die(_("No such remote '%s'"), remotename);
  
  	url_nr = 0;
 @@ -1537,7 +1537,7 @@ static int set_url(int argc, const char **argv)
  		oldurl = newurl;
  
  	remote = remote_get(remotename);
 -	if (!remote_is_configured(remote))
 +	if (!remote_is_configured(remote, 1))
  		die(_("No such remote '%s'"), remotename);
  
  	if (push_mode) {
 diff --git a/remote.c b/remote.c
 index 298f2f93fa..8524135de4 100644
 --- a/remote.c
 +++ b/remote.c
 @@ -255,7 +255,7 @@ static void read_remotes_file(struct remote *remote)
  
  	if (!f)
  		return;
 -	remote->configured = 1;
 +	remote->configured_in_repo = 1;
  	remote->origin = REMOTE_REMOTES;
  	while (strbuf_getline(&buf, f) != EOF) {
  		const char *v;
 @@ -290,7 +290,7 @@ static void read_branches_file(struct remote *remote)
  		return;
  	}
  
 -	remote->configured = 1;
 +	remote->configured_in_repo = 1;
  	remote->origin = REMOTE_BRANCHES;
  
  	/*
 @@ -373,6 +373,8 @@ static int handle_config(const char *key, const char *value, void *cb)
  	}
  	remote = make_remote(name, namelen);
  	remote->origin = REMOTE_CONFIG;
 +	if (current_config_scope() == CONFIG_SCOPE_REPO)
 +		remote->configured_in_repo = 1;
  	if (!strcmp(subkey, "mirror"))
  		remote->mirror = git_config_bool(key, value);
  	else if (!strcmp(subkey, "skipdefaultupdate"))
 @@ -386,25 +388,21 @@ static int handle_config(const char *key, const char *value, void *cb)
  		if (git_config_string(&v, key, value))
  			return -1;
  		add_url(remote, v);
 -		remote->configured = 1;
  	} else if (!strcmp(subkey, "pushurl")) {
  		const char *v;
  		if (git_config_string(&v, key, value))
  			return -1;
  		add_pushurl(remote, v);
 -		remote->configured = 1;
  	} else if (!strcmp(subkey, "push")) {
  		const char *v;
  		if (git_config_string(&v, key, value))
  			return -1;
  		add_push_refspec(remote, v);
 -		remote->configured = 1;
  	} else if (!strcmp(subkey, "fetch")) {
  		const char *v;
  		if (git_config_string(&v, key, value))
  			return -1;
  		add_fetch_refspec(remote, v);
 -		remote->configured = 1;
  	} else if (!strcmp(subkey, "receivepack")) {
  		const char *v;
  		if (git_config_string(&v, key, value))
 @@ -433,7 +431,6 @@ static int handle_config(const char *key, const char *value, void *cb)
  		return git_config_string((const char **)&remote->http_proxy_authmethod,
  					 key, value);
  	} else if (!strcmp(subkey, "vcs")) {
 -		remote->configured = 1;
  		return git_config_string(&remote->foreign_vcs, key, value);
  	}
  	return 0;
 @@ -721,9 +718,13 @@ struct remote *pushremote_get(const char *name)
  	return remote_get_1(name, pushremote_for_branch);
  }
  
 -int remote_is_configured(struct remote *remote)
 +int remote_is_configured(struct remote *remote, int in_repo)
  {
 -	return remote && remote->configured;
 +	if (!remote)
 +		return 0;
 +	if (in_repo)
 +		return remote->configured_in_repo;
 +	return !!remote->origin;
  }
  
  int for_each_remote(each_remote_fn fn, void *priv)
 diff --git a/remote.h b/remote.h
 index 7e6c8067bb..a5bbbe0ef9 100644
 --- a/remote.h
 +++ b/remote.h
 @@ -15,7 +15,7 @@ struct remote {
  	struct hashmap_entry ent;  /* must be first */
  
  	const char *name;
 -	int origin, configured;
 +	int origin, configured_in_repo;
  
  	const char *foreign_vcs;
  
 @@ -60,7 +60,7 @@ struct remote {
  
  struct remote *remote_get(const char *name);
  struct remote *pushremote_get(const char *name);
 -int remote_is_configured(struct remote *remote);
 +int remote_is_configured(struct remote *remote, int in_repo);
  
  typedef int each_remote_fn(struct remote *remote, void *priv);
  int for_each_remote(each_remote_fn fn, void *priv);
 diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
 index 09c9823002..ba46e86de0 100755
 --- a/t/t5505-remote.sh
 +++ b/t/t5505-remote.sh
 @@ -766,11 +766,9 @@ test_expect_success 'rename a remote with name prefix of other remote' '
  
  test_expect_success 'rename succeeds with existing remote.<target>.prune' '
  	git clone one four.four &&
 -	(
 -		cd four.four &&
 -		git config remote.upstream.prune true &&
 -		git remote rename origin upstream
 -	)
 +	test_when_finished git config --global --unset remote.upstream.prune &&
 +	git config --global remote.upstream.prune true &&
 +	git -C four.four remote rename origin upstream
  '
  
  cat >remotes_origin <<EOF

Some users like to set `remote.origin.prune = true` in their ~/.gitconfig so that all of their repositories use that default.

However, our code is ill-prepared for this, mistaking that single entry to mean that there is already a remote of the name "origin", even if there is not.

This patch adds a test case demonstrating this issue.

Reported by Andrew Arnott.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5505-remote.sh | 7 +++++++
 1 file changed, 7 insertions(+)

One of the really nice features of the ~/.gitconfig file is that users can override defaults by their own preferred settings for all of their repositories.

One such default that some users like to override is whether the "origin" remote gets auto-pruned or not. The user would simply call

	git config --global remote.origin.prune true

and from now on all "origin" remotes would be pruned automatically when fetching into the local repository.

There is just one catch: now Git thinks that the "origin" remote is configured, even if the repository config has no [remote "origin"] section at all, as it does not realize that the "prune" setting was configured globally and that there really is no "origin" remote configured in this repository.

That is a problem e.g. when renaming a remote to a new name, when Git may be fooled into thinking that there is already a remote of that new name.

Let's fix this by paying more attention to *where* the remote settings came from: if they are configured in the local repository config, we must not overwrite them. If they were configured elsewhere, we cannot overwrite them to begin with, as we only write the repository config.

There is only one caller of remote_is_configured() (in `git fetch`) that may want to take remotes into account even if they were configured outside the repository config; all other callers essentially try to prevent the Git command from overwriting settings in the repository config.

To accommodate that fact, the remote_is_configured() function now requires a parameter that states whether the caller is interested in all remotes, or only in those that were configured in the repository config.

Many thanks to Jeff King whose tireless review helped with settling for nothing less than the current strategy.

This fixes https://github.com/git-for-windows/git/issues/888

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fetch.c   |  2 +-
 builtin/remote.c  | 14 +++++++-------
 remote.c          | 12 ++++++++++--
 remote.h          |  4 ++--
 t/t5505-remote.sh |  2 +-
 5 files changed, 21 insertions(+), 13 deletions(-)

On Thu, Jan 19, 2017 at 10:20:02PM +0100, Johannes Schindelin wrote:

Just to make sure I understand the issue, the problem is that:

  git config --global remote.foo.url whatever
  git fetch --multiple foo bar

would not work without this part of the patch?

I'm trying to figure out why "fetch --multiple" wouldn't just take a url in the first place. I guess it is because multiple fetch is useless without refspecs (since otherwise you're just writing to FETCH_HEAD, which gets immediately overwritten). I wonder if that test really should be:

Hi Peff,

On Thu, 19 Jan 2017, Jeff King wrote:

Please note that it is legitimate to fetch from foreign vcs, in which case the fetch refspecs may not be required (or even make sense).

Sure, it would avoid that. But that parameter makes semantic sense: some callers may want to have all remotes, even those configured via the command-line, and others really are only interested in knowing whether the current repository config already has settings for a remote of the given name.

You are welcome ;-)

Ciao, Johannes

Jeff King <peff@peff.net> writes:

This is probably a tangent, if FETCH_HEAD is overwritten, wouldn't that be a bug in the implementation of --multiple? I somehow thought we had an option to tell second and subsequent "fetch" to append to FETCH_HEAD instead of overwriting it.

On Thu, Jan 19, 2017 at 01:45:29PM -0800, Junio C Hamano wrote:

Maybe. I was just speculating on the reason.

I just disabled that check and tried it with two local repos:

  git init
  git fetch --multiple /path/to/one /path/to/two
  cat .git/FETCH_HEAD

and indeed it does seem to append.

The logic comes from 9c4a036b3 (Teach the --all option to 'git fetch', 2009-11-09), but it does not seem to give any reasoning. Nor could I find anything on the mailing list. <shrug>

-Peff