Merge branch 'mh/fetch-tags-in-addition-to-normal-refs'

The "--tags" option to "git fetch" used to be literally a synonym to
a "refs/tags/*:refs/tags/*" refspec, which meant that (1) as an
explicit refspec given from the command line, it silenced the lazy
"git fetch" default that is configured, and (2) also as an explicit
refspec given from the command line, it interacted with "--prune"
to remove any tag that the remote we are fetching from does not
have.

This demotes it to an option; with it, we fetch all tags in
addition to what would be fetched without the option, and it does
not interact with the decision "--prune" makes to see what
remote-tracking refs the local has are missing the remote
counterpart.

* mh/fetch-tags-in-addition-to-normal-refs: (23 commits)
  fetch: improve the error messages emitted for conflicting refspecs
  handle_duplicate(): mark error message for translation
  ref_remote_duplicates(): extract a function handle_duplicate()
  ref_remove_duplicates(): simplify loop logic
  t5536: new test of refspec conflicts when fetching
  ref_remove_duplicates(): avoid redundant bisection
  git-fetch.txt: improve description of tag auto-following
  fetch-options.txt: simplify ifdef/ifndef/endif usage
  fetch, remote: properly convey --no-prune options to subprocesses
  builtin/remote.c:update(): use struct argv_array
  builtin/remote.c: reorder function definitions
  query_refspecs(): move some constants out of the loop
  fetch --prune: prune only based on explicit refspecs
  fetch --tags: fetch tags *in addition to* other stuff
  fetch: only opportunistically update references based on command line
  get_expanded_map(): avoid memory leak
  get_expanded_map(): add docstring
  builtin/fetch.c: reorder function definitions
  get_ref_map(): rename local variables
  api-remote.txt: correct section "struct refspec"
  ...

Documentation/config.txt

@@ -2087,8 +2087,8 @@ remote.<name>.vcs::
 
 remote.<name>.prune::
 	When set to true, fetching from this remote by default will also
-	remove any remote-tracking branches which no longer exist on the
+	remove any remote-tracking references that no longer exist on the
-	remote (as if the `--prune` option was give on the command line).
+	remote (as if the `--prune` option was given on the command line).
 	Overrides `fetch.prune` settings, if any.
 
 remotes.<group>::

Documentation/fetch-options.txt

@@ -41,17 +41,20 @@ ifndef::git-pull[]
 
 -p::
 --prune::
-	After fetching, remove any remote-tracking branches which
+	After fetching, remove any remote-tracking references that no
-	no longer exist	on the remote.
+	longer exist on the remote.  Tags are not subject to pruning
+	if they are fetched only because of the default tag
+	auto-following or due to a --tags option.  However, if tags
+	are fetched due to an explicit refspec (either on the command
+	line or in the remote configuration, for example if the remote
+	was cloned with the --mirror option), then they are also
+	subject to pruning.
 endif::git-pull[]
 
-ifdef::git-pull[]
---no-tags::
-endif::git-pull[]
 ifndef::git-pull[]
 -n::
---no-tags::
 endif::git-pull[]
+--no-tags::
 	By default, tags that point at objects that are downloaded
 	from the remote repository are fetched and stored locally.
 	This option disables this automatic tag following. The default
@@ -61,11 +64,12 @@ endif::git-pull[]
 ifndef::git-pull[]
 -t::
 --tags::
-	This is a short-hand for giving `refs/tags/*:refs/tags/*`
+	Fetch all tags from the remote (i.e., fetch remote tags
-	refspec from the command line, to ask all tags to be fetched
+	`refs/tags/*` into local tags with the same name), in addition
-	and stored locally.  Because this acts as an explicit
+	to whatever else would otherwise be fetched.  Using this
-	refspec, the default refspecs (configured with the
+	option alone does not subject tags to pruning, even if --prune
-	remote.$name.fetch variable) are overridden and not used.
+	is used (though tags may be pruned anyway if they are also the
+	destination of an explicit refspec; see '--prune').
 
 --recurse-submodules[=yes|on-demand|no]::
 	This option controls if and under what conditions new commits of

Documentation/git-fetch.txt

@@ -24,13 +24,13 @@ The ref names and their object names of fetched refs are stored
 in `.git/FETCH_HEAD`.  This information is left for a later merge
 operation done by 'git merge'.
 
-When <refspec> stores the fetched result in remote-tracking branches,
+By default, tags are auto-followed.  This means that when fetching
-the tags that point at these branches are automatically
+from a remote, any tags on the remote that point to objects that exist
-followed.  This is done by first fetching from the remote using
+in the local repository are fetched.  The effect is to fetch tags that
-the given <refspec>s, and if the repository has objects that are
+point at branches that you are interested in.  This default behavior
-pointed by remote tags that it does not yet have, then fetch
+can be changed by using the --tags or --no-tags options, by
-those missing tags.  If the other end has tags that point at
+configuring remote.<name>.tagopt, or by using a refspec that fetches
-branches you are not interested in, you will not get them.
+tags explicitly.
 
 'git fetch' can fetch from either a single named repository,
 or from several repositories at once if <group> is given and

Documentation/technical/api-remote.txt

@@ -58,16 +58,16 @@ default remote, given the current branch and configuration.
 struct refspec
 --------------
 
-A struct refspec holds the parsed interpretation of a refspec. If it
+A struct refspec holds the parsed interpretation of a refspec.  If it
-will force updates (starts with a '+'), force is true. If it is a
+will force updates (starts with a '+'), force is true.  If it is a
-pattern (sides end with '*') pattern is true. src and dest are the two
+pattern (sides end with '*') pattern is true.  src and dest are the
-sides (if a pattern, only the part outside of the wildcards); if there
+two sides (including '*' characters if present); if there is only one
-is only one side, it is src, and dst is NULL; if sides exist but are
+side, it is src, and dst is NULL; if sides exist but are empty (i.e.,
-empty (i.e., the refspec either starts or ends with ':'), the
+the refspec either starts or ends with ':'), the corresponding side is
-corresponding side is "".
+"".
 
-This parsing can be done to an array of strings to give an array of
+An array of strings can be parsed into an array of struct refspecs
-struct refpsecs with parse_ref_spec().
+using parse_fetch_refspec() or parse_push_refspec().
 
 remote_find_tracking(), given a remote and a struct refspec with
 either src or dst filled out, will fill out the other such that the

builtin/fetch.c

@@ -160,48 +160,156 @@ static void add_merge_config(struct ref **head,
 	}
 }
 
+static int add_existing(const char *refname, const unsigned char *sha1,
+			int flag, void *cbdata)
+{
+	struct string_list *list = (struct string_list *)cbdata;
+	struct string_list_item *item = string_list_insert(list, refname);
+	item->util = xmalloc(20);
+	hashcpy(item->util, sha1);
+	return 0;
+}
+
+static int will_fetch(struct ref **head, const unsigned char *sha1)
+{
+	struct ref *rm = *head;
+	while (rm) {
+		if (!hashcmp(rm->old_sha1, sha1))
+			return 1;
+		rm = rm->next;
+	}
+	return 0;
+}
+
 static void find_non_local_tags(struct transport *transport,
 			struct ref **head,
-			struct ref ***tail);
+			struct ref ***tail)
+{
+	struct string_list existing_refs = STRING_LIST_INIT_DUP;
+	struct string_list remote_refs = STRING_LIST_INIT_NODUP;
+	const struct ref *ref;
+	struct string_list_item *item = NULL;
+
+	for_each_ref(add_existing, &existing_refs);
+	for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
+		if (prefixcmp(ref->name, "refs/tags/"))
+			continue;
+
+		/*
+		 * The peeled ref always follows the matching base
+		 * ref, so if we see a peeled ref that we don't want
+		 * to fetch then we can mark the ref entry in the list
+		 * as one to ignore by setting util to NULL.
+		 */
+		if (!suffixcmp(ref->name, "^{}")) {
+			if (item && !has_sha1_file(ref->old_sha1) &&
+			    !will_fetch(head, ref->old_sha1) &&
+			    !has_sha1_file(item->util) &&
+			    !will_fetch(head, item->util))
+				item->util = NULL;
+			item = NULL;
+			continue;
+		}
+
+		/*
+		 * If item is non-NULL here, then we previously saw a
+		 * ref not followed by a peeled reference, so we need
+		 * to check if it is a lightweight tag that we want to
+		 * fetch.
+		 */
+		if (item && !has_sha1_file(item->util) &&
+		    !will_fetch(head, item->util))
+			item->util = NULL;
+
+		item = NULL;
+
+		/* skip duplicates and refs that we already have */
+		if (string_list_has_string(&remote_refs, ref->name) ||
+		    string_list_has_string(&existing_refs, ref->name))
+			continue;
+
+		item = string_list_insert(&remote_refs, ref->name);
+		item->util = (void *)ref->old_sha1;
+	}
+	string_list_clear(&existing_refs, 1);
+
+	/*
+	 * We may have a final lightweight tag that needs to be
+	 * checked to see if it needs fetching.
+	 */
+	if (item && !has_sha1_file(item->util) &&
+	    !will_fetch(head, item->util))
+		item->util = NULL;
+
+	/*
+	 * For all the tags in the remote_refs string list,
+	 * add them to the list of refs to be fetched
+	 */
+	for_each_string_list_item(item, &remote_refs) {
+		/* Unless we have already decided to ignore this item... */
+		if (item->util)
+		{
+			struct ref *rm = alloc_ref(item->string);
+			rm->peer_ref = alloc_ref(item->string);
+			hashcpy(rm->old_sha1, item->util);
+			**tail = rm;
+			*tail = &rm->next;
+		}
+	}
+
+	string_list_clear(&remote_refs, 0);
+}
 
 static struct ref *get_ref_map(struct transport *transport,
-			       struct refspec *refs, int ref_count, int tags,
+			       struct refspec *refspecs, int refspec_count,
-			       int *autotags)
+			       int tags, int *autotags)
 {
 	int i;
 	struct ref *rm;
 	struct ref *ref_map = NULL;
 	struct ref **tail = &ref_map;
 
+	/* opportunistically-updated references: */
+	struct ref *orefs = NULL, **oref_tail = &orefs;
+
 	const struct ref *remote_refs = transport_get_remote_refs(transport);
 
-	if (ref_count || tags == TAGS_SET) {
+	if (refspec_count) {
-		struct ref **old_tail;
+		for (i = 0; i < refspec_count; i++) {
-
+			get_fetch_map(remote_refs, &refspecs[i], &tail, 0);
-		for (i = 0; i < ref_count; i++) {
+			if (refspecs[i].dst && refspecs[i].dst[0])
-			get_fetch_map(remote_refs, &refs[i], &tail, 0);
-			if (refs[i].dst && refs[i].dst[0])
 				*autotags = 1;
 		}
-		/* Merge everything on the command line, but not --tags */
+		/* Merge everything on the command line (but not --tags) */
 		for (rm = ref_map; rm; rm = rm->next)
 			rm->fetch_head_status = FETCH_HEAD_MERGE;
-		if (tags == TAGS_SET)
-			get_fetch_map(remote_refs, tag_refspec, &tail, 0);
 
 		/*
-		 * For any refs that we happen to be fetching via command-line
+		 * For any refs that we happen to be fetching via
-		 * arguments, take the opportunity to update their configured
+		 * command-line arguments, the destination ref might
-		 * counterparts. However, we do not want to mention these
+		 * have been missing or have been different than the
-		 * entries in FETCH_HEAD at all, as they would simply be
+		 * remote-tracking ref that would be derived from the
-		 * duplicates of existing entries.
+		 * configured refspec.  In these cases, we want to
+		 * take the opportunity to update their configured
+		 * remote-tracking reference.  However, we do not want
+		 * to mention these entries in FETCH_HEAD at all, as
+		 * they would simply be duplicates of existing
+		 * entries, so we set them FETCH_HEAD_IGNORE below.
+		 *
+		 * We compute these entries now, based only on the
+		 * refspecs specified on the command line.  But we add
+		 * them to the list following the refspecs resulting
+		 * from the tags option so that one of the latter,
+		 * which has FETCH_HEAD_NOT_FOR_MERGE, is not removed
+		 * by ref_remove_duplicates() in favor of one of these
+		 * opportunistic entries with FETCH_HEAD_IGNORE.
 		 */
-		old_tail = tail;
 		for (i = 0; i < transport->remote->fetch_refspec_nr; i++)
 			get_fetch_map(ref_map, &transport->remote->fetch[i],
-				      &tail, 1);
+				      &oref_tail, 1);
-		for (rm = *old_tail; rm; rm = rm->next)
+
-			rm->fetch_head_status = FETCH_HEAD_IGNORE;
+		if (tags == TAGS_SET)
+			get_fetch_map(remote_refs, tag_refspec, &tail, 0);
 	} else {
 		/* Use the defaults */
 		struct remote *remote = transport->remote;
@@ -238,11 +346,21 @@ static struct ref *get_ref_map(struct transport *transport,
 			tail = &ref_map->next;
 		}
 	}
-	if (tags == TAGS_DEFAULT && *autotags)
-		find_non_local_tags(transport, &ref_map, &tail);
-	ref_remove_duplicates(ref_map);
 
-	return ref_map;
+	if (tags == TAGS_SET)
+		/* also fetch all tags */
+		get_fetch_map(remote_refs, tag_refspec, &tail, 0);
+	else if (tags == TAGS_DEFAULT && *autotags)
+		find_non_local_tags(transport, &ref_map, &tail);
+
+	/* Now append any refs to be updated opportunistically: */
+	*tail = orefs;
+	for (rm = orefs; rm; rm = rm->next) {
+		rm->fetch_head_status = FETCH_HEAD_IGNORE;
+		tail = &rm->next;
+	}
+
+	return ref_remove_duplicates(ref_map);
 }
 
 #define STORE_REF_ERROR_OTHER 1
@@ -612,106 +730,6 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
 	return result;
 }
 
-static int add_existing(const char *refname, const unsigned char *sha1,
-			int flag, void *cbdata)
-{
-	struct string_list *list = (struct string_list *)cbdata;
-	struct string_list_item *item = string_list_insert(list, refname);
-	item->util = xmalloc(20);
-	hashcpy(item->util, sha1);
-	return 0;
-}
-
-static int will_fetch(struct ref **head, const unsigned char *sha1)
-{
-	struct ref *rm = *head;
-	while (rm) {
-		if (!hashcmp(rm->old_sha1, sha1))
-			return 1;
-		rm = rm->next;
-	}
-	return 0;
-}
-
-static void find_non_local_tags(struct transport *transport,
-			struct ref **head,
-			struct ref ***tail)
-{
-	struct string_list existing_refs = STRING_LIST_INIT_DUP;
-	struct string_list remote_refs = STRING_LIST_INIT_NODUP;
-	const struct ref *ref;
-	struct string_list_item *item = NULL;
-
-	for_each_ref(add_existing, &existing_refs);
-	for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
-		if (prefixcmp(ref->name, "refs/tags/"))
-			continue;
-
-		/*
-		 * The peeled ref always follows the matching base
-		 * ref, so if we see a peeled ref that we don't want
-		 * to fetch then we can mark the ref entry in the list
-		 * as one to ignore by setting util to NULL.
-		 */
-		if (!suffixcmp(ref->name, "^{}")) {
-			if (item && !has_sha1_file(ref->old_sha1) &&
-			    !will_fetch(head, ref->old_sha1) &&
-			    !has_sha1_file(item->util) &&
-			    !will_fetch(head, item->util))
-				item->util = NULL;
-			item = NULL;
-			continue;
-		}
-
-		/*
-		 * If item is non-NULL here, then we previously saw a
-		 * ref not followed by a peeled reference, so we need
-		 * to check if it is a lightweight tag that we want to
-		 * fetch.
-		 */
-		if (item && !has_sha1_file(item->util) &&
-		    !will_fetch(head, item->util))
-			item->util = NULL;
-
-		item = NULL;
-
-		/* skip duplicates and refs that we already have */
-		if (string_list_has_string(&remote_refs, ref->name) ||
-		    string_list_has_string(&existing_refs, ref->name))
-			continue;
-
-		item = string_list_insert(&remote_refs, ref->name);
-		item->util = (void *)ref->old_sha1;
-	}
-	string_list_clear(&existing_refs, 1);
-
-	/*
-	 * We may have a final lightweight tag that needs to be
-	 * checked to see if it needs fetching.
-	 */
-	if (item && !has_sha1_file(item->util) &&
-	    !will_fetch(head, item->util))
-		item->util = NULL;
-
-	/*
-	 * For all the tags in the remote_refs string list,
-	 * add them to the list of refs to be fetched
-	 */
-	for_each_string_list_item(item, &remote_refs) {
-		/* Unless we have already decided to ignore this item... */
-		if (item->util)
-		{
-			struct ref *rm = alloc_ref(item->string);
-			rm->peer_ref = alloc_ref(item->string);
-			hashcpy(rm->old_sha1, item->util);
-			**tail = rm;
-			*tail = &rm->next;
-		}
-	}
-
-	string_list_clear(&remote_refs, 0);
-}
-
 static void check_not_current_branch(struct ref *ref_map)
 {
 	struct branch *current_branch = branch_get(NULL);
@@ -831,30 +849,16 @@ static int do_fetch(struct transport *transport,
 	}
 	if (prune) {
 		/*
-		 * If --tags was specified, pretend that the user gave us
+		 * We only prune based on refspecs specified
-		 * the canonical tags refspec
+		 * explicitly (via command line or configuration); we
+		 * don't care whether --tags was specified.
 		 */
-		if (tags == TAGS_SET) {
+		if (ref_count) {
-			const char *tags_str = "refs/tags/*:refs/tags/*";
-			struct refspec *tags_refspec, *refspec;
-
-			/* Copy the refspec and add the tags to it */
-			refspec = xcalloc(ref_count + 1, sizeof(struct refspec));
-			tags_refspec = parse_fetch_refspec(1, &tags_str);
-			memcpy(refspec, refs, ref_count * sizeof(struct refspec));
-			memcpy(&refspec[ref_count], tags_refspec, sizeof(struct refspec));
-			ref_count++;
-
-			prune_refs(refspec, ref_count, ref_map);
-
-			ref_count--;
-			/* The rest of the strings belong to fetch_one */
-			free_refspec(1, tags_refspec);
-			free(refspec);
-		} else if (ref_count) {
 			prune_refs(refs, ref_count, ref_map);
 		} else {
-			prune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);
+			prune_refs(transport->remote->fetch,
+				   transport->remote->fetch_refspec_nr,
+				   ref_map);
 		}
 	}
 	free_refs(ref_map);
@@ -930,8 +934,8 @@ static void add_options_to_argv(struct argv_array *argv)
 {
 	if (dry_run)
 		argv_array_push(argv, "--dry-run");
-	if (prune > 0)
+	if (prune != -1)
-		argv_array_push(argv, "--prune");
+		argv_array_push(argv, prune ? "--prune" : "--no-prune");
 	if (update_head_ok)
 		argv_array_push(argv, "--update-head-ok");
 	if (force)

builtin/remote.c

@@ -6,6 +6,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "refs.h"
+#include "argv-array.h"
 
 static const char * const builtin_remote_usage[] = {
 	N_("git remote [-v | --verbose]"),
@@ -77,9 +78,6 @@ static const char * const builtin_remote_seturl_usage[] = {
 
 static int verbose;
 
-static int show_all(void);
-static int prune_remote(const char *remote, int dry_run);
-
 static inline int postfixcmp(const char *string, const char *postfix)
 {
 	int len1 = strlen(string), len2 = strlen(postfix);
@@ -1084,6 +1082,64 @@ static int show_push_info_item(struct string_list_item *item, void *cb_data)
 	return 0;
 }
 
+static int get_one_entry(struct remote *remote, void *priv)
+{
+	struct string_list *list = priv;
+	struct strbuf url_buf = STRBUF_INIT;
+	const char **url;
+	int i, url_nr;
+
+	if (remote->url_nr > 0) {
+		strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]);
+		string_list_append(list, remote->name)->util =
+				strbuf_detach(&url_buf, NULL);
+	} else
+		string_list_append(list, remote->name)->util = NULL;
+	if (remote->pushurl_nr) {
+		url = remote->pushurl;
+		url_nr = remote->pushurl_nr;
+	} else {
+		url = remote->url;
+		url_nr = remote->url_nr;
+	}
+	for (i = 0; i < url_nr; i++)
+	{
+		strbuf_addf(&url_buf, "%s (push)", url[i]);
+		string_list_append(list, remote->name)->util =
+				strbuf_detach(&url_buf, NULL);
+	}
+
+	return 0;
+}
+
+static int show_all(void)
+{
+	struct string_list list = STRING_LIST_INIT_NODUP;
+	int result;
+
+	list.strdup_strings = 1;
+	result = for_each_remote(get_one_entry, &list);
+
+	if (!result) {
+		int i;
+
+		sort_string_list(&list);
+		for (i = 0; i < list.nr; i++) {
+			struct string_list_item *item = list.items + i;
+			if (verbose)
+				printf("%s\t%s\n", item->string,
+					item->util ? (const char *)item->util : "");
+			else {
+				if (i && !strcmp((item - 1)->string, item->string))
+					continue;
+				printf("%s\n", item->string);
+			}
+		}
+	}
+	string_list_clear(&list, 1);
+	return result;
+}
+
 static int show(int argc, const char **argv)
 {
 	int no_query = 0, result = 0, query_flag = 0;
@@ -1246,26 +1302,6 @@ static int set_head(int argc, const char **argv)
 	return result;
 }
 
-static int prune(int argc, const char **argv)
-{
-	int dry_run = 0, result = 0;
-	struct option options[] = {
-		OPT__DRY_RUN(&dry_run, N_("dry run")),
-		OPT_END()
-	};
-
-	argc = parse_options(argc, argv, NULL, options, builtin_remote_prune_usage,
-			     0);
-
-	if (argc < 1)
-		usage_with_options(builtin_remote_prune_usage, options);
-
-	for (; argc; argc--, argv++)
-		result |= prune_remote(*argv, dry_run);
-
-	return result;
-}
-
 static int prune_remote(const char *remote, int dry_run)
 {
 	int result = 0, i;
@@ -1304,6 +1340,26 @@ static int prune_remote(const char *remote, int dry_run)
 	return result;
 }
 
+static int prune(int argc, const char **argv)
+{
+	int dry_run = 0, result = 0;
+	struct option options[] = {
+		OPT__DRY_RUN(&dry_run, N_("dry run")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, NULL, options, builtin_remote_prune_usage,
+			     0);
+
+	if (argc < 1)
+		usage_with_options(builtin_remote_prune_usage, options);
+
+	for (; argc; argc--, argv++)
+		result |= prune_remote(*argv, dry_run);
+
+	return result;
+}
+
 static int get_remote_default(const char *key, const char *value, void *priv)
 {
 	if (strcmp(key, "remotes.default") == 0) {
@@ -1315,42 +1371,42 @@ static int get_remote_default(const char *key, const char *value, void *priv)
 
 static int update(int argc, const char **argv)
 {
-	int i, prune = 0;
+	int i, prune = -1;
 	struct option options[] = {
 		OPT_BOOL('p', "prune", &prune,
 			 N_("prune remotes after fetching")),
 		OPT_END()
 	};
-	const char **fetch_argv;
+	struct argv_array fetch_argv = ARGV_ARRAY_INIT;
-	int fetch_argc = 0;
 	int default_defined = 0;
-
+	int retval;
-	fetch_argv = xmalloc(sizeof(char *) * (argc+5));
 
 	argc = parse_options(argc, argv, NULL, options, builtin_remote_update_usage,
 			     PARSE_OPT_KEEP_ARGV0);
 
-	fetch_argv[fetch_argc++] = "fetch";
+	argv_array_push(&fetch_argv, "fetch");
 
-	if (prune)
+	if (prune != -1)
-		fetch_argv[fetch_argc++] = "--prune";
+		argv_array_push(&fetch_argv, prune ? "--prune" : "--no-prune");
 	if (verbose)
-		fetch_argv[fetch_argc++] = "-v";
+		argv_array_push(&fetch_argv, "-v");
-	fetch_argv[fetch_argc++] = "--multiple";
+	argv_array_push(&fetch_argv, "--multiple");
 	if (argc < 2)
-		fetch_argv[fetch_argc++] = "default";
+		argv_array_push(&fetch_argv, "default");
 	for (i = 1; i < argc; i++)
-		fetch_argv[fetch_argc++] = argv[i];
+		argv_array_push(&fetch_argv, argv[i]);
 
-	if (strcmp(fetch_argv[fetch_argc-1], "default") == 0) {
+	if (strcmp(fetch_argv.argv[fetch_argv.argc-1], "default") == 0) {
 		git_config(get_remote_default, &default_defined);
-		if (!default_defined)
+		if (!default_defined) {
-			fetch_argv[fetch_argc-1] = "--all";
+			argv_array_pop(&fetch_argv);
+			argv_array_push(&fetch_argv, "--all");
+		}
 	}
 
-	fetch_argv[fetch_argc] = NULL;
+	retval = run_command_v_opt(fetch_argv.argv, RUN_GIT_CMD);
-
+	argv_array_clear(&fetch_argv);
-	return run_command_v_opt(fetch_argv, RUN_GIT_CMD);
+	return retval;
 }
 
 static int remove_all_fetch_refspecs(const char *remote, const char *key)
@@ -1505,64 +1561,6 @@ static int set_url(int argc, const char **argv)
 	return 0;
 }
 
-static int get_one_entry(struct remote *remote, void *priv)
-{
-	struct string_list *list = priv;
-	struct strbuf url_buf = STRBUF_INIT;
-	const char **url;
-	int i, url_nr;
-
-	if (remote->url_nr > 0) {
-		strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]);
-		string_list_append(list, remote->name)->util =
-				strbuf_detach(&url_buf, NULL);
-	} else
-		string_list_append(list, remote->name)->util = NULL;
-	if (remote->pushurl_nr) {
-		url = remote->pushurl;
-		url_nr = remote->pushurl_nr;
-	} else {
-		url = remote->url;
-		url_nr = remote->url_nr;
-	}
-	for (i = 0; i < url_nr; i++)
-	{
-		strbuf_addf(&url_buf, "%s (push)", url[i]);
-		string_list_append(list, remote->name)->util =
-				strbuf_detach(&url_buf, NULL);
-	}
-
-	return 0;
-}
-
-static int show_all(void)
-{
-	struct string_list list = STRING_LIST_INIT_NODUP;
-	int result;
-
-	list.strdup_strings = 1;
-	result = for_each_remote(get_one_entry, &list);
-
-	if (!result) {
-		int i;
-
-		sort_string_list(&list);
-		for (i = 0; i < list.nr; i++) {
-			struct string_list_item *item = list.items + i;
-			if (verbose)
-				printf("%s\t%s\n", item->string,
-					item->util ? (const char *)item->util : "");
-			else {
-				if (i && !strcmp((item - 1)->string, item->string))
-					continue;
-				printf("%s\n", item->string);
-			}
-		}
-	}
-	string_list_clear(&list, 1);
-	return result;
-}
-
 int cmd_remote(int argc, const char **argv, const char *prefix)
 {
 	struct option options[] = {

git-pull.sh

@@ -172,7 +172,7 @@ error_on_no_merge_candidates () {
 	do
 		case "$opt" in
 		-t|--t|--ta|--tag|--tags)
-			echo "Fetching tags only, you probably meant:"
+			echo "It doesn't make sense to pull all tags; you probably meant:"
 			echo "  git fetch --tags"
 			exit 1
 		esac

remote.c

@@ -745,35 +745,66 @@ int for_each_remote(each_remote_fn fn, void *priv)
 	return result;
 }
 
-void ref_remove_duplicates(struct ref *ref_map)
+static void handle_duplicate(struct ref *ref1, struct ref *ref2)
+{
+	if (strcmp(ref1->name, ref2->name)) {
+		if (ref1->fetch_head_status != FETCH_HEAD_IGNORE &&
+		    ref2->fetch_head_status != FETCH_HEAD_IGNORE) {
+			die(_("Cannot fetch both %s and %s to %s"),
+			    ref1->name, ref2->name, ref2->peer_ref->name);
+		} else if (ref1->fetch_head_status != FETCH_HEAD_IGNORE &&
+			   ref2->fetch_head_status == FETCH_HEAD_IGNORE) {
+			warning(_("%s usually tracks %s, not %s"),
+				ref2->peer_ref->name, ref2->name, ref1->name);
+		} else if (ref1->fetch_head_status == FETCH_HEAD_IGNORE &&
+			   ref2->fetch_head_status == FETCH_HEAD_IGNORE) {
+			die(_("%s tracks both %s and %s"),
+			    ref2->peer_ref->name, ref1->name, ref2->name);
+		} else {
+			/*
+			 * This last possibility doesn't occur because
+			 * FETCH_HEAD_IGNORE entries always appear at
+			 * the end of the list.
+			 */
+			die(_("Internal error"));
+		}
+	}
+	free(ref2->peer_ref);
+	free(ref2);
+}
+
+struct ref *ref_remove_duplicates(struct ref *ref_map)
 {
 	struct string_list refs = STRING_LIST_INIT_NODUP;
-	struct string_list_item *item = NULL;
+	struct ref *retval = NULL;
-	struct ref *prev = NULL, *next = NULL;
+	struct ref **p = &retval;
-	for (; ref_map; prev = ref_map, ref_map = next) {
-		next = ref_map->next;
-		if (!ref_map->peer_ref)
-			continue;
 
-		item = string_list_lookup(&refs, ref_map->peer_ref->name);
+	while (ref_map) {
-		if (item) {
+		struct ref *ref = ref_map;
-			if (strcmp(((struct ref *)item->util)->name,
+
-				   ref_map->name))
+		ref_map = ref_map->next;
-				die("%s tracks both %s and %s",
+		ref->next = NULL;
-				    ref_map->peer_ref->name,
+
-				    ((struct ref *)item->util)->name,
+		if (!ref->peer_ref) {
-				    ref_map->name);
+			*p = ref;
-			prev->next = ref_map->next;
+			p = &ref->next;
-			free(ref_map->peer_ref);
+		} else {
-			free(ref_map);
+			struct string_list_item *item =
-			ref_map = prev; /* skip this; we freed it */
+				string_list_insert(&refs, ref->peer_ref->name);
-			continue;
+
+			if (item->util) {
+				/* Entry already existed */
+				handle_duplicate((struct ref *)item->util, ref);
+			} else {
+				*p = ref;
+				p = &ref->next;
+				item->util = ref;
+			}
 		}
-
-		item = string_list_insert(&refs, ref_map->peer_ref->name);
-		item->util = ref_map;
 	}
+
 	string_list_clear(&refs, 0);
+	return retval;
 }
 
 int remote_has_url(struct remote *remote, const char *url)
@@ -825,6 +856,8 @@ static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *q
 {
 	int i;
 	int find_src = !query->src;
+	const char *needle = find_src ? query->dst : query->src;
+	char **result = find_src ? &query->src : &query->dst;
 
 	if (find_src && !query->dst)
 		return error("query_refspecs: need either src or dst");
@@ -833,8 +866,6 @@ static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *q
 		struct refspec *refspec = &refs[i];
 		const char *key = find_src ? refspec->dst : refspec->src;
 		const char *value = find_src ? refspec->src : refspec->dst;
-		const char *needle = find_src ? query->dst : query->src;
-		char **result = find_src ? &query->src : &query->dst;
 
 		if (!refspec->dst)
 			continue;
@@ -1553,6 +1584,13 @@ static int ignore_symref_update(const char *refname)
 	return (flag & REF_ISSYMREF);
 }
 
+/*
+ * Create and return a list of (struct ref) consisting of copies of
+ * each remote_ref that matches refspec.  refspec must be a pattern.
+ * Fill in the copies' peer_ref to describe the local tracking refs to
+ * which they map.  Omit any references that would map to an existing
+ * local symbolic ref.
+ */
 static struct ref *get_expanded_map(const struct ref *remote_refs,
 				    const struct refspec *refspec)
 {
@@ -1560,9 +1598,9 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 	struct ref *ret = NULL;
 	struct ref **tail = &ret;
 
-	char *expn_name;
-
 	for (ref = remote_refs; ref; ref = ref->next) {
+		char *expn_name = NULL;
+
 		if (strchr(ref->name, '^'))
 			continue; /* a dereference item */
 		if (match_name_with_pattern(refspec->src, ref->name,
@@ -1571,12 +1609,12 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 			struct ref *cpy = copy_ref(ref);
 
 			cpy->peer_ref = alloc_ref(expn_name);
-			free(expn_name);
 			if (refspec->force)
 				cpy->peer_ref->force = 1;
 			*tail = cpy;
 			tail = &cpy->next;
 		}
+		free(expn_name);
 	}
 
 	return ret;

remote.h

@@ -149,9 +149,13 @@ int resolve_remote_symref(struct ref *ref, struct ref *list);
 int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);
 
 /*
- * Removes and frees any duplicate refs in the map.
+ * Remove and free all but the first of any entries in the input list
+ * that map the same remote reference to the same local reference.  If
+ * there are two entries that map different remote references to the
+ * same local reference, emit an error message and die.  Return a
+ * pointer to the head of the resulting list.
  */
-void ref_remove_duplicates(struct ref *ref_map);
+struct ref *ref_remove_duplicates(struct ref *ref_map);
 
 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);

t/t5510-fetch.sh

@@ -88,7 +88,7 @@ test_expect_success 'fetch --prune on its own works as expected' '
 	cd "$D" &&
 	git clone . prune &&
 	cd prune &&
-	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+	git update-ref refs/remotes/origin/extrabranch master &&
 
 	git fetch --prune origin &&
 	test_must_fail git rev-parse origin/extrabranch
@@ -98,7 +98,7 @@ test_expect_success 'fetch --prune with a branch name keeps branches' '
 	cd "$D" &&
 	git clone . prune-branch &&
 	cd prune-branch &&
-	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+	git update-ref refs/remotes/origin/extrabranch master &&
 
 	git fetch --prune origin master &&
 	git rev-parse origin/extrabranch
@@ -113,25 +113,45 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '
 	git rev-parse origin/master
 '
 
-test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '
+test_expect_success 'fetch --prune --tags prunes branches but not tags' '
 	cd "$D" &&
 	git clone . prune-tags &&
 	cd prune-tags &&
-	git fetch origin refs/heads/master:refs/tags/sometag &&
+	git tag sometag master &&
+	# Create what looks like a remote-tracking branch from an earlier
+	# fetch that has since been deleted from the remote:
+	git update-ref refs/remotes/origin/fake-remote master &&
 
 	git fetch --prune --tags origin &&
 	git rev-parse origin/master &&
-	test_must_fail git rev-parse somebranch
+	test_must_fail git rev-parse origin/fake-remote &&
+	git rev-parse sometag
 '
 
-test_expect_success 'fetch --prune --tags with branch does not delete other remote-tracking branches' '
+test_expect_success 'fetch --prune --tags with branch does not prune other things' '
 	cd "$D" &&
 	git clone . prune-tags-branch &&
 	cd prune-tags-branch &&
-	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+	git tag sometag master &&
+	git update-ref refs/remotes/origin/extrabranch master &&
 
 	git fetch --prune --tags origin master &&
-	git rev-parse origin/extrabranch
+	git rev-parse origin/extrabranch &&
+	git rev-parse sometag
+'
+
+test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' '
+	cd "$D" &&
+	git clone . prune-tags-refspec &&
+	cd prune-tags-refspec &&
+	git tag sometag master &&
+	git update-ref refs/remotes/origin/foo/otherbranch master &&
+	git update-ref refs/remotes/origin/extrabranch master &&
+
+	git fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* &&
+	test_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&
+	git rev-parse origin/extrabranch &&
+	git rev-parse sometag
 '
 
 test_expect_success 'fetch tags when there is no tags' '

t/t5515/fetch.br-unconfig_--tags_.._.git

@@ -1,4 +1,5 @@
 # br-unconfig --tags ../.git
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../

t/t5515/fetch.master_--tags_.._.git

@@ -1,4 +1,5 @@
 # master --tags ../.git
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../

t/t5525-fetch-tagopt.sh

@@ -8,7 +8,8 @@ setup_clone () {
 	git clone --mirror . $1 &&
 	git remote add remote_$1 $1 &&
 	(cd $1 &&
-	git tag tag_$1)
+	git tag tag_$1 &&
+	git branch branch_$1)
 }
 
 test_expect_success setup '
@@ -21,21 +22,33 @@ test_expect_success setup '
 
 test_expect_success "fetch with tagopt=--no-tags does not get tag" '
 	git fetch remote_one &&
-	test_must_fail git show-ref tag_one
+	test_must_fail git show-ref tag_one &&
+	git show-ref remote_one/branch_one
 	'
 
 test_expect_success "fetch --tags with tagopt=--no-tags gets tag" '
+	(
+		cd one &&
+		git branch second_branch_one
+	) &&
 	git fetch --tags remote_one &&
-	git show-ref tag_one
+	git show-ref tag_one &&
+	git show-ref remote_one/second_branch_one
 	'
 
 test_expect_success "fetch --no-tags with tagopt=--tags does not get tag" '
 	git fetch --no-tags remote_two &&
-	test_must_fail git show-ref tag_two
+	test_must_fail git show-ref tag_two &&
+	git show-ref remote_two/branch_two
 	'
 
 test_expect_success "fetch with tagopt=--tags gets tag" '
+	(
+		cd two &&
+		git branch second_branch_two
+	) &&
 	git fetch remote_two &&
-	git show-ref tag_two
+	git show-ref tag_two &&
+	git show-ref remote_two/second_branch_two
 	'
 test_done

t/t5536-fetch-conflicts.sh

@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='fetch handles conflicting refspecs correctly'
+
+. ./test-lib.sh
+
+D=$(pwd)
+
+setup_repository () {
+	git init "$1" && (
+		cd "$1" &&
+		git config remote.origin.url "$D" &&
+		shift &&
+		for refspec in "$@"
+		do
+			git config --add remote.origin.fetch "$refspec"
+		done
+	)
+}
+
+verify_stderr () {
+	cat >expected &&
+	# We're not interested in the error
+	# "fatal: The remote end hung up unexpectedly":
+	grep -E '^(fatal|warning):' <error | grep -v 'hung up' >actual | sort &&
+	test_cmp expected actual
+}
+
+test_expect_success 'setup' '
+	git commit --allow-empty -m "Initial" &&
+	git branch branch1 &&
+	git tag tag1 &&
+	git commit --allow-empty -m "First" &&
+	git branch branch2 &&
+	git tag tag2
+'
+
+test_expect_success 'fetch with no conflict' '
+	setup_repository ok "+refs/heads/*:refs/remotes/origin/*" && (
+		cd ok &&
+		git fetch origin
+	)
+'
+
+test_expect_success 'fetch conflict: config vs. config' '
+	setup_repository ccc \
+		"+refs/heads/branch1:refs/remotes/origin/branch1" \
+		"+refs/heads/branch2:refs/remotes/origin/branch1" && (
+		cd ccc &&
+		test_must_fail git fetch origin 2>error &&
+		verify_stderr <<-\EOF
+		fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1
+		EOF
+	)
+'
+
+test_expect_success 'fetch duplicate: config vs. config' '
+	setup_repository dcc \
+		"+refs/heads/*:refs/remotes/origin/*" \
+		"+refs/heads/branch1:refs/remotes/origin/branch1" && (
+		cd dcc &&
+		git fetch origin
+	)
+'
+
+test_expect_success 'fetch conflict: arg overrides config' '
+	setup_repository aoc \
+		"+refs/heads/*:refs/remotes/origin/*" && (
+		cd aoc &&
+		git fetch origin refs/heads/branch2:refs/remotes/origin/branch1
+	)
+'
+
+test_expect_success 'fetch conflict: arg vs. arg' '
+	setup_repository caa && (
+		cd caa &&
+		test_must_fail git fetch origin \
+			refs/heads/*:refs/remotes/origin/* \
+			refs/heads/branch2:refs/remotes/origin/branch1 2>error &&
+		verify_stderr <<-\EOF
+		fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1
+		EOF
+	)
+'
+
+test_expect_success 'fetch conflict: criss-cross args' '
+	setup_repository xaa \
+		"+refs/heads/*:refs/remotes/origin/*" && (
+		cd xaa &&
+		git fetch origin \
+			refs/heads/branch1:refs/remotes/origin/branch2 \
+			refs/heads/branch2:refs/remotes/origin/branch1 2>error &&
+		verify_stderr <<-\EOF
+		warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2
+		warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1
+		EOF
+	)
+'
+
+test_done