* [PATCH 0/3] Be more careful when prunning
@ 2011-10-06 20:19 Carlos Martín Nieto
2011-10-06 20:19 ` [PATCH 1/3] fetch: free all the additional refspecs Carlos Martín Nieto
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 20:19 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] fetch: free all the additional refspecs
2011-10-06 20:19 [PATCH 0/3] Be more careful when prunning Carlos Martín Nieto
@ 2011-10-06 20:19 ` Carlos Martín Nieto
2011-10-06 20:19 ` [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 20:19 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs
2011-10-06 20:19 [PATCH 0/3] Be more careful when prunning Carlos Martín Nieto
2011-10-06 20:19 ` [PATCH 1/3] fetch: free all the additional refspecs Carlos Martín Nieto
@ 2011-10-06 20:19 ` Carlos Martín Nieto
2011-10-06 20:19 ` [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto
2011-10-06 20:51 ` [PATCH 0/3] Be more careful when prunning Ben Boeckel
3 siblings, 0 replies; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 20:19 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning
2011-10-06 20:19 [PATCH 0/3] Be more careful when prunning Carlos Martín Nieto
2011-10-06 20:19 ` [PATCH 1/3] fetch: free all the additional refspecs Carlos Martín Nieto
2011-10-06 20:19 ` [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
@ 2011-10-06 20:19 ` Carlos Martín Nieto
2011-10-06 20:51 ` [PATCH 0/3] Be more careful when prunning Ben Boeckel
3 siblings, 0 replies; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 20:19 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Be more careful when prunning
2011-10-06 20:19 [PATCH 0/3] Be more careful when prunning Carlos Martín Nieto
` (2 preceding siblings ...)
2011-10-06 20:19 ` [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto
@ 2011-10-06 20:51 ` Ben Boeckel
2011-10-06 20:58 ` Carlos Martín Nieto
2011-10-06 21:21 ` [PATCHv2 " Carlos Martín Nieto
3 siblings, 2 replies; 15+ messages in thread
From: Ben Boeckel @ 2011-10-06 20:51 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git, Jeff King, Junio C Hamano
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Be more careful when prunning
2011-10-06 20:51 ` [PATCH 0/3] Be more careful when prunning Ben Boeckel
@ 2011-10-06 20:58 ` Carlos Martín Nieto
2011-10-06 21:21 ` [PATCHv2 " Carlos Martín Nieto
1 sibling, 0 replies; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 20:58 UTC (permalink / raw)
To: mathstuf; +Cc: git, Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCHv2 0/3] Be more careful when prunning
2011-10-06 20:51 ` [PATCH 0/3] Be more careful when prunning Ben Boeckel
2011-10-06 20:58 ` Carlos Martín Nieto
@ 2011-10-06 21:21 ` Carlos Martín Nieto
2011-10-06 21:21 ` [PATCH 1/3] fetch: free all the additional refspecs Carlos Martín Nieto
` (2 more replies)
1 sibling, 3 replies; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 21:21 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] fetch: free all the additional refspecs
2011-10-06 21:21 ` [PATCHv2 " Carlos Martín Nieto
@ 2011-10-06 21:21 ` Carlos Martín Nieto
2011-10-06 21:21 ` [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
2011-10-06 21:21 ` [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto
2 siblings, 0 replies; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 21:21 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs
2011-10-06 21:21 ` [PATCHv2 " Carlos Martín Nieto
2011-10-06 21:21 ` [PATCH 1/3] fetch: free all the additional refspecs Carlos Martín Nieto
@ 2011-10-06 21:21 ` Carlos Martín Nieto
2011-10-07 16:26 ` Jeff King
2011-10-06 21:21 ` [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto
2 siblings, 1 reply; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 21:21 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning
2011-10-06 21:21 ` [PATCHv2 " Carlos Martín Nieto
2011-10-06 21:21 ` [PATCH 1/3] fetch: free all the additional refspecs Carlos Martín Nieto
2011-10-06 21:21 ` [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
@ 2011-10-06 21:21 ` Carlos Martín Nieto
2011-10-07 16:33 ` Jeff King
2 siblings, 1 reply; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 21:21 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, mathstuf
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs
2011-10-06 21:21 ` [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
@ 2011-10-07 16:26 ` Jeff King
2011-10-07 16:37 ` Carlos Martín Nieto
0 siblings, 1 reply; 15+ messages in thread
From: Jeff King @ 2011-10-07 16:26 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git, Junio C Hamano, mathstuf
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning
2011-10-06 21:21 ` [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto
@ 2011-10-07 16:33 ` Jeff King
2011-10-07 16:40 ` Carlos Martín Nieto
0 siblings, 1 reply; 15+ messages in thread
From: Jeff King @ 2011-10-07 16:33 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git, Junio C Hamano, mathstuf
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs
2011-10-07 16:26 ` Jeff King
@ 2011-10-07 16:37 ` Carlos Martín Nieto
2011-10-07 16:39 ` Jeff King
0 siblings, 1 reply; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-07 16:37 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, mathstuf
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs
2011-10-07 16:37 ` Carlos Martín Nieto
@ 2011-10-07 16:39 ` Jeff King
0 siblings, 0 replies; 15+ messages in thread
From: Jeff King @ 2011-10-07 16:39 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git, Junio C Hamano, mathstuf
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning
2011-10-07 16:33 ` Jeff King
@ 2011-10-07 16:40 ` Carlos Martín Nieto
0 siblings, 0 replies; 15+ messages in thread
From: Carlos Martín Nieto @ 2011-10-07 16:40 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, mathstuf
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2011-10-07 16:40 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-06 20:19 [PATCH 0/3] Be more careful when prunning Carlos Martín Nieto
2011-10-06 20:19 ` [PATCH 1/3] fetch: free all the additional refspecs Carlos Martín Nieto
2011-10-06 20:19 ` [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
2011-10-06 20:19 ` [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto
2011-10-06 20:51 ` [PATCH 0/3] Be more careful when prunning Ben Boeckel
2011-10-06 20:58 ` Carlos Martín Nieto
2011-10-06 21:21 ` [PATCHv2 " Carlos Martín Nieto
2011-10-06 21:21 ` [PATCH 1/3] fetch: free all the additional refspecs Carlos Martín Nieto
2011-10-06 21:21 ` [PATCH 2/3] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
2011-10-07 16:26 ` Jeff King
2011-10-07 16:37 ` Carlos Martín Nieto
2011-10-07 16:39 ` Jeff King
2011-10-06 21:21 ` [PATCH 3/3] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto
2011-10-07 16:33 ` Jeff King
2011-10-07 16:40 ` Carlos Martín Nieto
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.