[PATCH 0/3] Be more careful when prunning

[PATCH 0/3] Be more careful when prunning

[Carlos Martín Nieto]

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

[PATCH 1/3] fetch: free all the additional refspecs

[Carlos Martín Nieto]

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 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

[PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs

[Carlos Martín Nieto]

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 <cmn@elego.de>
---
 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

[PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning

[Carlos Martín Nieto]

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 <cmn@elego.de>
---
 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

Re: [PATCH 0/3] Be more careful when prunning

[Ben Boeckel]

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

Re: [PATCH 0/3] Be more careful when prunning

[Carlos Martín Nieto]

[-- Attachment #1: Type: text/plain, Size: 3136 bytes --]

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:
> > 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
> 



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCHv2 0/3] Be more careful when prunning

[Carlos Martín Nieto]

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

[PATCH 1/3] fetch: free all the additional refspecs

[Carlos Martín Nieto]

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 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

[PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs

[Carlos Martín Nieto]

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 <cmn@elego.de>
---
 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

Re: [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs

[Jeff King]

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

Re: [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs

[Carlos Martín Nieto]

[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]

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:
> 
> > 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?

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

Good point.

> 
> >  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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs

[Jeff King]

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

> > I assume you mean s/tag/branch/ in the last line?
> 
> Yeah, maybe ref would be better?

Yeah, agreed.

> > 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

[PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning

[Carlos Martín Nieto]

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 <cmn@elego.de>
---
 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

Re: [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning

[Jeff King]

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

> 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.

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

-Peff

Re: [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning

[Carlos Martín Nieto]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

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:
> 
> > 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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]