[PATCH 0/3] Be more careful when prunning

Hello,

The first patch is not that big a deal, but it's better if we're
freeing the refspecs, we might as well free all of them.

The second patch teaches get_stale_heads to use the user-provided
refspecs instead of the ones in the config. For example, running

    git fetch --prune origin refs/heads/master:refs/heads/master

doesn't remove the other branches anymore. For a more interesting (and
believable) example, let's take

    git fetch --prune origin refs/heads/b/*:refs/heads/b/*

because you want to prune the refs inside the b/ namespace
only. Currently git will delete all the refs that aren't under that
namespace. With the second patch applied, git won't remove any refs
outside the b/ namespace.

What is probably the most usual case is covered by the third patch,
which pretends that a "refs/tags/*:refs/tags/*" refspec was given on
the command-line.

Cheers,
   cmn

Carlos Martín Nieto (3):
      fetch: free all the additional refspecs
      fetch: honor the user-provided refspecs when pruning refs
      fetch: treat --tags like refs/tags/*:refs/tags/* when pruning

 builtin/fetch.c  |   19 ++++++++++---
 builtin/remote.c |    2 +-
 remote.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 remote.h         |    3 +-
 4 files changed, 84 insertions(+), 14 deletions(-)

-- 
1.7.5.2.354.g349bf

If the user gave us refspecs on the command line, we should use those
when deciding whether to prune a ref instead of relying on the
refspecs in the config.

Previously, running

    git fetch --prune origin refs/heads/master:refs/remotes/origin/master

would delete every other tag under the origin namespace because we
were using the refspec to filter the available refs but using the
configured refspec to figure out if a ref had been deleted on the
remote. This is clearly the wrong thing to do.

Teach get_stale_heads about user-provided refspecs and use them if
they're available.

Signed-off-by: Carlos Martín Nieto <redacted>
---
 builtin/fetch.c  |    6 ++--
 builtin/remote.c |    2 +-
 remote.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 remote.h         |    3 +-
 4 files changed, 73 insertions(+), 12 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 30b485e..b937d71 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -505,10 +505,10 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	return ret;
 }
 
-static int prune_refs(struct transport *transport, struct ref *ref_map)
+static int prune_refs(struct transport *transport, struct refspec *refs, int n, struct ref *ref_map)
 {
 	int result = 0;
-	struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);
+	struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map, refs, n);
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)\n")
 		: _("   (%s has become dangling)\n");

@@ -700,7 +700,7 @@ static int do_fetch(struct transport *transport,
 		return 1;
 	}
 	if (prune)
-		prune_refs(transport, ref_map);
+		prune_refs(transport, refs, ref_count, ref_map);
 	free_refs(ref_map);
 
 	/* if neither --no-tags nor --tags was specified, do automated tag

diff --git a/builtin/remote.c b/builtin/remote.c
index b25dfb4..91a2148 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c

@@ -349,7 +349,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
 		else
 			string_list_append(&states->tracked, abbrev_branch(ref->name));
 	}
-	stale_refs = get_stale_heads(states->remote, fetch_map);
+	stale_refs = get_stale_heads(states->remote, fetch_map, NULL, 0);
 	for (ref = stale_refs; ref; ref = ref->next) {
 		struct string_list_item *item =
 			string_list_append(&states->stale, abbrev_branch(ref->name));

diff --git a/remote.c b/remote.c
index 7840d2f..72a26d3 100644
--- a/remote.c
+++ b/remote.c

@@ -1684,26 +1684,84 @@ struct stale_heads_info {
 	struct remote *remote;
 	struct string_list *ref_names;
 	struct ref **stale_refs_tail;
+	struct refspec *refs;
+	int ref_count;
 };
 
+/*
+ * Find a refspec to a remote's
+ * Returns 0 on success, -1 if it couldn't find a the refspec
+ */
+static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)
+{
+	int i;
+	struct refspec *refspec;
+
+	for (i = 0; i < ref_count; ++i) {
+		refspec = &refs[i];
+
+		/*
+		 * No '*' means that it must match exactly. If it does
+		 * have it, try to match it against the pattern. If
+		 * the refspec matches, store the ref name as it would
+		 * appear in the server in query->src.
+		 */
+		if (!strchr(refspec->dst, '*')) {
+			if (!strcmp(query->dst, refspec->dst)) {
+				query->src = xstrdup(refspec->src);
+				return 0;
+			}
+		} else {
+			if (match_name_with_pattern(refspec->dst, query->dst,
+						    refspec->src, &query->src)) {
+				return 0;
+			}
+		}
+	}
+
+	return -1;
+}
+
 static int get_stale_heads_cb(const char *refname,
 	const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct stale_heads_info *info = cb_data;
 	struct refspec refspec;
+	int ret;
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
-	if (!remote_find_tracking(info->remote, &refspec)) {
-		if (!((flags & REF_ISSYMREF) ||
-		    string_list_has_string(info->ref_names, refspec.src))) {
-			struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
-			hashcpy(ref->new_sha1, sha1);
-		}
+
+	/*
+	 * If the user speicified refspecs on the command line, we
+	 * should only use those to check. Otherwise, look in the
+	 * remote's configuration for the branch.
+	 */
+	if (info->ref_count)
+		ret = find_in_refs(info->refs, info->ref_count, &refspec);
+	else
+		ret = remote_find_tracking(info->remote, &refspec);
+
+	/* No matches */
+	if (ret)
+		return 0;
+
+	/*
+	 * If we did find a suitable refspec and it's not a symref and
+	 * it's not in the list of refs that currently exist in that
+	 * remote we consider it to be stale.
+	 */
+	if (!((flags & REF_ISSYMREF) ||
+	      string_list_has_string(info->ref_names, refspec.src))) {
+		struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
+		hashcpy(ref->new_sha1, sha1);
 	}
+
+	free(refspec.src);
 	return 0;
 }
 
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
+struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map,
+			    struct refspec *refs, int ref_count)
 {
 	struct ref *ref, *stale_refs = NULL;
 	struct string_list ref_names = STRING_LIST_INIT_NODUP;

@@ -1711,6 +1769,8 @@ struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
 	info.remote = remote;
 	info.ref_names = &ref_names;
 	info.stale_refs_tail = &stale_refs;
+	info.refs = refs;
+	info.ref_count = ref_count;
 	for (ref = fetch_map; ref; ref = ref->next)
 		string_list_append(&ref_names, ref->name);
 	sort_string_list(&ref_names);

diff --git a/remote.h b/remote.h
index 9a30a9d..2f753a0 100644
--- a/remote.h
+++ b/remote.h

@@ -164,6 +164,7 @@ struct ref *guess_remote_head(const struct ref *head,
 			      int all);
 
 /* Return refs which no longer exist on remote */
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map);
+struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map,
+			    struct refspec *refs, int ref_count);
 
 #endif

-- 
1.7.5.2.354.g349bf

If --tags is specified, add that refspec to the list given to prune_refs
so it knows to treat it as a filter on what refs to should consider
for prunning. This way

    git fetch --prune --tags origin

only prunes tags and doesn't delete the branch refs.

Signed-off-by: Carlos Martín Nieto <redacted>
---
 builtin/fetch.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b937d71..94b2bd3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -699,8 +699,17 @@ static int do_fetch(struct transport *transport,
 		free_refs(ref_map);
 		return 1;
 	}
-	if (prune)
+	if (prune) {
+		/* If --tags was specified, we need to tell prune_refs
+		 * that we're filtering the refs from the remote */
+		if (tags == TAGS_SET) {
+			const char * tags_refspec = "refs/tags/*:refs/tags/*";
+			refs = xrealloc(refs, (ref_count + 1) * sizeof(struct refspec));
+			refs[ref_count] = *parse_fetch_refspec(1, &tags_refspec);
+			ref_count++;
+		}
 		prune_refs(transport, refs, ref_count, ref_map);
+	}
 	free_refs(ref_map);
 
 	/* if neither --no-tags nor --tags was specified, do automated tag

-- 
1.7.5.2.354.g349bf

Signed-off-by: Carlos Martín Nieto <redacted>
---
 builtin/fetch.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7a4e41c..30b485e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -883,7 +883,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
 	atexit(unlock_pack);
 	refspec = parse_fetch_refspec(ref_nr, refs);
 	exit_code = do_fetch(transport, refspec, ref_nr);
-	free(refspec);
+	free_refspec(ref_nr, refspec);
 	transport_disconnect(transport);
 	transport = NULL;
 	return exit_code;

-- 
1.7.5.2.354.g349bf

On Thu, Oct 06, 2011 at 22:19:42 +0200, Carlos Martín Nieto wrote:

The first patch is not that big a deal, but it's better if we're
freeing the refspecs, we might as well free all of them.

The second patch teaches get_stale_heads to use the user-provided
refspecs instead of the ones in the config. For example, running

    git fetch --prune origin refs/heads/master:refs/heads/master

doesn't remove the other branches anymore. For a more interesting (and
believable) example, let's take

    git fetch --prune origin refs/heads/b/*:refs/heads/b/*

because you want to prune the refs inside the b/ namespace
only. Currently git will delete all the refs that aren't under that
namespace. With the second patch applied, git won't remove any refs
outside the b/ namespace.

What is probably the most usual case is covered by the third patch,
which pretends that a "refs/tags/*:refs/tags/*" refspec was given on
the command-line.

I applied the patches to current master (7f41b6b) and got a segfault
with:

    git fetch -p -t origin master

It does not happen with master.

Backtrace:

(gdb) bt
#0  0x00007ffff7395d18 in __strchr_sse42 () from /lib64/libc.so.6
#1  0x00000000004b2d39 in find_in_refs (query=0x7fffffffdb90, ref_count=2, refs=<optimized out>) at remote.c:1709
#2  get_stale_heads_cb (refname=0x7a8f31 "refs/heads/a/branch/name", sha1=0x7a8f09 "\367\343\375C٩\223u\305OG\233)z\347X\370\333\325", <incomplete sequence \335>, flags=0, cb_data=0x7fffffffdc50) at remote.c:1740
#3  0x00000000004adf19 in do_for_each_ref (submodule=<optimized out>, base=0x4ea1c2 "", fn=0x4b2ca0 <get_stale_heads_cb>, trim=0, flags=0, cb_data=0x7fffffffdc50) at refs.c:684
#4  0x00000000004b4249 in get_stale_heads (remote=<optimized out>, fetch_map=<optimized out>, refs=<optimized out>, ref_count=<optimized out>) at remote.c:1777
#5  0x0000000000426cfb in prune_refs (ref_map=<optimized out>, n=<optimized out>, refs=<optimized out>, transport=0x78e040) at builtin/fetch.c:511
#6  do_fetch (ref_count=<optimized out>, refs=<optimized out>, transport=0x78e040) at builtin/fetch.c:711
#7  fetch_one (remote=<optimized out>, argc=<optimized out>, argv=<optimized out>) at builtin/fetch.c:894
#8  0x0000000000427550 in cmd_fetch (argc=2, argv=0x7fffffffe070, prefix=0x0) at builtin/fetch.c:955
#9  0x0000000000405084 in run_builtin (argv=0x7fffffffe070, argc=5, p=0x731b08) at git.c:308
#10 handle_internal_command (argc=5, argv=0x7fffffffe070) at git.c:466
#11 0x000000000040448b in run_argv (argv=0x7fffffffdf10, argcp=0x7fffffffdf1c) at git.c:512
#12 main (argc=5, argv=0x7fffffffe070) at git.c:585

--Ben

On Thu, 2011-10-06 at 16:51 -0400, Ben Boeckel wrote:

On Thu, Oct 06, 2011 at 22:19:42 +0200, Carlos Martín Nieto wrote:

quoted

The first patch is not that big a deal, but it's better if we're
freeing the refspecs, we might as well free all of them.

The second patch teaches get_stale_heads to use the user-provided
refspecs instead of the ones in the config. For example, running

    git fetch --prune origin refs/heads/master:refs/heads/master

doesn't remove the other branches anymore. For a more interesting (and
believable) example, let's take

    git fetch --prune origin refs/heads/b/*:refs/heads/b/*

because you want to prune the refs inside the b/ namespace
only. Currently git will delete all the refs that aren't under that
namespace. With the second patch applied, git won't remove any refs
outside the b/ namespace.

What is probably the most usual case is covered by the third patch,
which pretends that a "refs/tags/*:refs/tags/*" refspec was given on
the command-line.

I applied the patches to current master (7f41b6b) and got a segfault
with:

    git fetch -p -t origin master

It does not happen with master.

I thought I'd got rid of those problems. Thanks for noticing. I'll
investigate.

Backtrace:

(gdb) bt
#0  0x00007ffff7395d18 in __strchr_sse42 () from /lib64/libc.so.6
#1  0x00000000004b2d39 in find_in_refs (query=0x7fffffffdb90, ref_count=2, refs=<optimized out>) at remote.c:1709
#2  get_stale_heads_cb (refname=0x7a8f31 "refs/heads/a/branch/name", sha1=0x7a8f09 "\367\343\375C٩\223u\305OG\233)z\347X\370\333\325", <incomplete sequence \335>, flags=0, cb_data=0x7fffffffdc50) at remote.c:1740
#3  0x00000000004adf19 in do_for_each_ref (submodule=<optimized out>, base=0x4ea1c2 "", fn=0x4b2ca0 <get_stale_heads_cb>, trim=0, flags=0, cb_data=0x7fffffffdc50) at refs.c:684
#4  0x00000000004b4249 in get_stale_heads (remote=<optimized out>, fetch_map=<optimized out>, refs=<optimized out>, ref_count=<optimized out>) at remote.c:1777
#5  0x0000000000426cfb in prune_refs (ref_map=<optimized out>, n=<optimized out>, refs=<optimized out>, transport=0x78e040) at builtin/fetch.c:511
#6  do_fetch (ref_count=<optimized out>, refs=<optimized out>, transport=0x78e040) at builtin/fetch.c:711
#7  fetch_one (remote=<optimized out>, argc=<optimized out>, argv=<optimized out>) at builtin/fetch.c:894
#8  0x0000000000427550 in cmd_fetch (argc=2, argv=0x7fffffffe070, prefix=0x0) at builtin/fetch.c:955
#9  0x0000000000405084 in run_builtin (argv=0x7fffffffe070, argc=5, p=0x731b08) at git.c:308
#10 handle_internal_command (argc=5, argv=0x7fffffffe070) at git.c:466
#11 0x000000000040448b in run_argv (argv=0x7fffffffdf10, argcp=0x7fffffffdf1c) at git.c:512
#12 main (argc=5, argv=0x7fffffffe070) at git.c:585

--Ben
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,

The first patch is not that big a deal, but it's better if we're
freeing the refspecs, we might as well free all of them.

The second patch teaches get_stale_heads to use the user-provided
refspecs instead of the ones in the config. For example, running

    git fetch --prune origin refs/heads/master:refs/heads/master

doesn't remove the other branches anymore. For a more interesting (and
believable) example, let's take

    git fetch --prune origin refs/heads/b/*:refs/heads/b/*

because you want to prune the refs inside the b/ namespace
only. Currently git will delete all the refs that aren't under that
namespace. With the second patch applied, git won't remove any refs
outside the b/ namespace.

What is probably the most usual case is covered by the third patch,
which pretends that a "refs/tags/*:refs/tags/*" refspec was given on
the command-line.

Version 1 assumed that a refspec would have its dst filled
automatically. This is not the case and was fixed in the second patch.

Cheers,
   cmn

Carlos Martín Nieto (3):
      fetch: free all the additional refspecs
      fetch: honor the user-provided refspecs when pruning refs
      fetch: treat --tags like refs/tags/*:refs/tags/* when pruning

 builtin/fetch.c  |   19 ++++++++++---
 builtin/remote.c |    2 +-
 remote.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 remote.h         |    3 +-
 4 files changed, 84 insertions(+), 14 deletions(-)

-- 
1.7.5.2.354.g349bf

Signed-off-by: Carlos Martín Nieto <redacted>
---
 builtin/fetch.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7a4e41c..30b485e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -883,7 +883,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
 	atexit(unlock_pack);
 	refspec = parse_fetch_refspec(ref_nr, refs);
 	exit_code = do_fetch(transport, refspec, ref_nr);
-	free(refspec);
+	free_refspec(ref_nr, refspec);
 	transport_disconnect(transport);
 	transport = NULL;
 	return exit_code;

-- 
1.7.5.2.354.g349bf

If --tags is specified, add that refspec to the list given to prune_refs
so it knows to treat it as a filter on what refs to should consider
for prunning. This way

    git fetch --prune --tags origin

only prunes tags and doesn't delete the branch refs.

Signed-off-by: Carlos Martín Nieto <redacted>
---
 builtin/fetch.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b937d71..94b2bd3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -699,8 +699,17 @@ static int do_fetch(struct transport *transport,
 		free_refs(ref_map);
 		return 1;
 	}
-	if (prune)
+	if (prune) {
+		/* If --tags was specified, we need to tell prune_refs
+		 * that we're filtering the refs from the remote */
+		if (tags == TAGS_SET) {
+			const char * tags_refspec = "refs/tags/*:refs/tags/*";
+			refs = xrealloc(refs, (ref_count + 1) * sizeof(struct refspec));
+			refs[ref_count] = *parse_fetch_refspec(1, &tags_refspec);
+			ref_count++;
+		}
 		prune_refs(transport, refs, ref_count, ref_map);
+	}
 	free_refs(ref_map);
 
 	/* if neither --no-tags nor --tags was specified, do automated tag

-- 
1.7.5.2.354.g349bf

If the user gave us refspecs on the command line, we should use those
when deciding whether to prune a ref instead of relying on the
refspecs in the config.

Previously, running

    git fetch --prune origin refs/heads/master:refs/remotes/origin/master

would delete every other tag under the origin namespace because we
were using the refspec to filter the available refs but using the
configured refspec to figure out if a ref had been deleted on the
remote. This is clearly the wrong thing to do.

Teach get_stale_heads about user-provided refspecs and use them if
they're available.

Signed-off-by: Carlos Martín Nieto <redacted>
---
 builtin/fetch.c  |    6 ++--
 builtin/remote.c |    2 +-
 remote.c         |   78 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 remote.h         |    3 +-
 4 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 30b485e..b937d71 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -505,10 +505,10 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	return ret;
 }
 
-static int prune_refs(struct transport *transport, struct ref *ref_map)
+static int prune_refs(struct transport *transport, struct refspec *refs, int n, struct ref *ref_map)
 {
 	int result = 0;
-	struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);
+	struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map, refs, n);
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)\n")
 		: _("   (%s has become dangling)\n");

@@ -700,7 +700,7 @@ static int do_fetch(struct transport *transport,
 		return 1;
 	}
 	if (prune)
-		prune_refs(transport, ref_map);
+		prune_refs(transport, refs, ref_count, ref_map);
 	free_refs(ref_map);
 
 	/* if neither --no-tags nor --tags was specified, do automated tag

diff --git a/builtin/remote.c b/builtin/remote.c
index b25dfb4..91a2148 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c

@@ -349,7 +349,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
 		else
 			string_list_append(&states->tracked, abbrev_branch(ref->name));
 	}
-	stale_refs = get_stale_heads(states->remote, fetch_map);
+	stale_refs = get_stale_heads(states->remote, fetch_map, NULL, 0);
 	for (ref = stale_refs; ref; ref = ref->next) {
 		struct string_list_item *item =
 			string_list_append(&states->stale, abbrev_branch(ref->name));

diff --git a/remote.c b/remote.c
index 7840d2f..28f7917 100644
--- a/remote.c
+++ b/remote.c

@@ -1684,26 +1684,88 @@ struct stale_heads_info {
 	struct remote *remote;
 	struct string_list *ref_names;
 	struct ref **stale_refs_tail;
+	struct refspec *refs;
+	int ref_count;
 };
 
+/*
+ * Find a refspec to a remote's
+ * Returns 0 on success, -1 if it couldn't find a the refspec
+ */
+static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)
+{
+	int i;
+	struct refspec *refspec;
+
+	for (i = 0; i < ref_count; ++i) {
+		refspec = &refs[i];
+
+		/* No dst means it can't be used for prunning. */
+		if (!refspec->dst)
+			continue;
+
+		/*
+		 * No '*' means that it must match exactly. If it does
+		 * have it, try to match it against the pattern. If
+		 * the refspec matches, store the ref name as it would
+		 * appear in the server in query->src.
+		 */
+		if (!strchr(refspec->dst, '*')) {
+			if (!strcmp(query->dst, refspec->dst)) {
+				query->src = xstrdup(refspec->src);
+				return 0;
+			}
+		} else {
+			if (match_name_with_pattern(refspec->dst, query->dst,
+						    refspec->src, &query->src)) {
+				return 0;
+			}
+		}
+	}
+
+	return -1;
+}
+
 static int get_stale_heads_cb(const char *refname,
 	const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct stale_heads_info *info = cb_data;
 	struct refspec refspec;
+	int ret;
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
-	if (!remote_find_tracking(info->remote, &refspec)) {
-		if (!((flags & REF_ISSYMREF) ||
-		    string_list_has_string(info->ref_names, refspec.src))) {
-			struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
-			hashcpy(ref->new_sha1, sha1);
-		}
+
+	/*
+	 * If the user speicified refspecs on the command line, we
+	 * should only use those to check. Otherwise, look in the
+	 * remote's configuration for the branch.
+	 */
+	if (info->ref_count)
+		ret = find_in_refs(info->refs, info->ref_count, &refspec);
+	else
+		ret = remote_find_tracking(info->remote, &refspec);
+
+	/* No matches */
+	if (ret)
+		return 0;
+
+	/*
+	 * If we did find a suitable refspec and it's not a symref and
+	 * it's not in the list of refs that currently exist in that
+	 * remote we consider it to be stale.
+	 */
+	if (!((flags & REF_ISSYMREF) ||
+	      string_list_has_string(info->ref_names, refspec.src))) {
+		struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
+		hashcpy(ref->new_sha1, sha1);
 	}
+
+	free(refspec.src);
 	return 0;
 }
 
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
+struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map,
+			    struct refspec *refs, int ref_count)
 {
 	struct ref *ref, *stale_refs = NULL;
 	struct string_list ref_names = STRING_LIST_INIT_NODUP;

@@ -1711,6 +1773,8 @@ struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
 	info.remote = remote;
 	info.ref_names = &ref_names;
 	info.stale_refs_tail = &stale_refs;
+	info.refs = refs;
+	info.ref_count = ref_count;
 	for (ref = fetch_map; ref; ref = ref->next)
 		string_list_append(&ref_names, ref->name);
 	sort_string_list(&ref_names);

diff --git a/remote.h b/remote.h
index 9a30a9d..2f753a0 100644
--- a/remote.h
+++ b/remote.h

@@ -164,6 +164,7 @@ struct ref *guess_remote_head(const struct ref *head,
 			      int all);
 
 /* Return refs which no longer exist on remote */
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map);
+struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map,
+			    struct refspec *refs, int ref_count);
 
 #endif

-- 
1.7.5.2.354.g349bf

On Thu, Oct 06, 2011 at 11:21:46PM +0200, Carlos Martín Nieto wrote:

If the user gave us refspecs on the command line, we should use those
when deciding whether to prune a ref instead of relying on the
refspecs in the config.

Previously, running

    git fetch --prune origin refs/heads/master:refs/remotes/origin/master

would delete every other tag under the origin namespace because we

I assume you mean s/tag/branch/ in the last line?

---
 builtin/fetch.c  |    6 ++--
 builtin/remote.c |    2 +-
 remote.c         |   78 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 remote.h         |    3 +-
 4 files changed, 77 insertions(+), 12 deletions(-)

Tests?

 static int get_stale_heads_cb(const char *refname,
 	const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct stale_heads_info *info = cb_data;
 	struct refspec refspec;
+	int ret;
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
-	if (!remote_find_tracking(info->remote, &refspec)) {
-		if (!((flags & REF_ISSYMREF) ||
-		    string_list_has_string(info->ref_names, refspec.src))) {
-			struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
-			hashcpy(ref->new_sha1, sha1);
-		}
+
+	/*
+	 * If the user speicified refspecs on the command line, we
+	 * should only use those to check. Otherwise, look in the
+	 * remote's configuration for the branch.
+	 */
+	if (info->ref_count)
+		ret = find_in_refs(info->refs, info->ref_count, &refspec);
+	else
+		ret = remote_find_tracking(info->remote, &refspec);

Minor typo in the comment. But more importantly, this feels like a very
low-level place to be thinking about things like what the user gave us
on the command line.

Shouldn't get_stale_heads not get a remote at all, and just get a set of
refspecs? Those should be the minimal information it needs to get its
answer, right?

-Peff

On Thu, Oct 06, 2011 at 11:21:47PM +0200, Carlos Martín Nieto wrote:

I don't think we can realloc refs here. It's passed into do_fetch. When
we realloc it, the old pointer value will be invalid. But when we return
from do_fetch, the caller (fetch_one) will still have that old value,
and will call free() on it.

Instead, you have to make a whole new list, copy the old values in, add
your new one, and then free the result.

-Peff

On Fri, 2011-10-07 at 12:26 -0400, Jeff King wrote:

On Thu, Oct 06, 2011 at 11:21:46PM +0200, Carlos Martín Nieto wrote:

quoted

If the user gave us refspecs on the command line, we should use those
when deciding whether to prune a ref instead of relying on the
refspecs in the config.

Previously, running

    git fetch --prune origin refs/heads/master:refs/remotes/origin/master

would delete every other tag under the origin namespace because we

I assume you mean s/tag/branch/ in the last line?

Yeah, maybe ref would be better?

quoted

---
 builtin/fetch.c  |    6 ++--
 builtin/remote.c |    2 +-
 remote.c         |   78 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 remote.h         |    3 +-
 4 files changed, 77 insertions(+), 12 deletions(-)

Tests?

Good point.

quoted

 static int get_stale_heads_cb(const char *refname,
 	const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct stale_heads_info *info = cb_data;
 	struct refspec refspec;
+	int ret;
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
-	if (!remote_find_tracking(info->remote, &refspec)) {
-		if (!((flags & REF_ISSYMREF) ||
-		    string_list_has_string(info->ref_names, refspec.src))) {
-			struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
-			hashcpy(ref->new_sha1, sha1);
-		}
+
+	/*
+	 * If the user speicified refspecs on the command line, we
+	 * should only use those to check. Otherwise, look in the
+	 * remote's configuration for the branch.
+	 */
+	if (info->ref_count)
+		ret = find_in_refs(info->refs, info->ref_count, &refspec);
+	else
+		ret = remote_find_tracking(info->remote, &refspec);

Minor typo in the comment. But more importantly, this feels like a very
low-level place to be thinking about things like what the user gave us
on the command line.

Shouldn't get_stale_heads not get a remote at all, and just get a set of
refspecs? Those should be the minimal information it needs to get its
answer, right?

OK, so take a step back and figure out what we want the rules to be
before we call get_stale_heads. It does sound like a more elegant
approach. I was trying to disrupt the callers as little as possible, but
then again, there's only two. Will change.

   cmn

On Fri, Oct 07, 2011 at 06:37:13PM +0200, Carlos Martín Nieto wrote:

quoted

I assume you mean s/tag/branch/ in the last line?

Yeah, maybe ref would be better?

Yeah, agreed.

quoted

Tests?

Good point.

It sounds like you already have a reproduction recipe for this, and for
the --tags thing in the next commit.

OK, so take a step back and figure out what we want the rules to be
before we call get_stale_heads. It does sound like a more elegant
approach. I was trying to disrupt the callers as little as possible, but
then again, there's only two. Will change.

Yeah. Sometimes we try hard to make a minimal patch, because it makes it
easier to review. At the same time, I think it's more important to make
the code clean if it needs it. Especially when there aren't many callers
to disrupt.

-Peff

On Fri, 2011-10-07 at 12:33 -0400, Jeff King wrote:

On Thu, Oct 06, 2011 at 11:21:47PM +0200, Carlos Martín Nieto wrote:

quoted

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b937d71..94b2bd3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -699,8 +699,17 @@ static int do_fetch(struct transport *transport,
 		free_refs(ref_map);
 		return 1;
 	}
-	if (prune)
+	if (prune) {
+		/* If --tags was specified, we need to tell prune_refs
+		 * that we're filtering the refs from the remote */
+		if (tags == TAGS_SET) {
+			const char * tags_refspec = "refs/tags/*:refs/tags/*";
+			refs = xrealloc(refs, (ref_count + 1) * sizeof(struct refspec));
+			refs[ref_count] = *parse_fetch_refspec(1, &tags_refspec);
+			ref_count++;
+		}
 		prune_refs(transport, refs, ref_count, ref_map);
+	}

I don't think we can realloc refs here. It's passed into do_fetch. When
we realloc it, the old pointer value will be invalid. But when we return
from do_fetch, the caller (fetch_one) will still have that old value,
and will call free() on it.

Yes, you're right. I guess it's been working by luck and generous amount
of memory.

Instead, you have to make a whole new list, copy the old values in, add
your new one, and then free the result.

Will do.

   cmn