git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCHv4 0/5] Be more careful when prunning
@ 2011-10-15  5:04 Carlos Martín Nieto
  2011-10-15  5:04 ` [PATCH 1/5] fetch: free all the additional refspecs Carlos Martín Nieto
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf

Hello,

This version covers Junio's concerns about code and functionality
duplication with the third patch. The order of arguments for
get_stale_heads is not changed anymore in the fourth patch.

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 introduces expected failures for the features that
this series fixes.

The third patch is new and moves the logic out of remote_find_tracking
into a query_refspecs function. remote_find_tracking and
apply_refspecs are both changed to be wrappers around this
function. This way, the logic is in one function instead of three
different ones.

The fourth patch changes prune_refs and get_stale_heads so the caller
has to decide which refspecs are the appropriate ones to use. 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 fifth patch,
which pretends that a "refs/tags/*:refs/tags/*" refspec was given on
the command-line. That fixes the

    git fetch --prune --tags origin

case. The non-tag refs are kept now.

Cheers,
   cmn

Carlos Martín Nieto (5):
  fetch: free all the additional refspecs
  t5510: add tests for fetch --prune
  remote: separate out the remote_find_tracking logic into
    query_refspecs
  fetch: honor the user-provided refspecs when pruning refs
  fetch: treat --tags like refs/tags/*:refs/tags/* when pruning

 builtin/fetch.c  |   33 ++++++++++++++---
 builtin/remote.c |    3 +-
 remote.c         |  106 +++++++++++++++++++++++++++++------------------------
 remote.h         |    2 +-
 t/t5510-fetch.sh |   50 +++++++++++++++++++++++++
 5 files changed, 139 insertions(+), 55 deletions(-)

-- 
1.7.5.2.354.g349bf

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/5] fetch: free all the additional refspecs
  2011-10-15  5:04 [PATCHv4 0/5] Be more careful when prunning Carlos Martín Nieto
@ 2011-10-15  5:04 ` Carlos Martín Nieto
  2011-10-15  5:04 ` [PATCH 2/5] t5510: add tests for fetch --prune Carlos Martín Nieto
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fetch.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e422ced..605d1bf 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -918,7 +918,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] 8+ messages in thread

* [PATCH 2/5] t5510: add tests for fetch --prune
  2011-10-15  5:04 [PATCHv4 0/5] Be more careful when prunning Carlos Martín Nieto
  2011-10-15  5:04 ` [PATCH 1/5] fetch: free all the additional refspecs Carlos Martín Nieto
@ 2011-10-15  5:04 ` Carlos Martín Nieto
  2011-10-15  5:04 ` [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs Carlos Martín Nieto
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf

The failures will be fixed in later commits.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5510-fetch.sh |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 7e433b1..8b5e925 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -76,6 +76,56 @@ test_expect_success "fetch test for-merge" '
 	cut -f -2 .git/FETCH_HEAD >actual &&
 	test_cmp expected actual'
 
+test_expect_success 'fetch --prune on its own works as expected' '
+	cd "$D" &&
+	git clone . prune &&
+	cd prune &&
+	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+
+	git fetch --prune origin &&
+	test_must_fail git rev-parse origin/extrabranch
+'
+
+test_expect_failure 'fetch --prune with a branch name keeps branches' '
+	cd "$D" &&
+	git clone . prune-branch &&
+	cd prune-branch &&
+	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+
+	git fetch --prune origin master &&
+	git rev-parse origin/extrabranch
+'
+
+test_expect_failure 'fetch --prune with a namespace keeps other namespaces' '
+	cd "$D" &&
+	git clone . prune-namespace &&
+	cd prune-namespace &&
+
+	git fetch --prune origin refs/heads/a/*:refs/remotes/origin/a/* &&
+	git rev-parse origin/master
+'
+
+test_expect_failure 'fetch --prune --tags does not delete the remote-tracking branches' '
+	cd "$D" &&
+	git clone . prune-tags &&
+	cd prune-tags &&
+	git fetch origin refs/heads/master:refs/tags/sometag &&
+
+	git fetch --prune --tags origin &&
+	git rev-parse origin/master &&
+	test_must_fail git rev-parse somebranch
+'
+
+test_expect_failure 'fetch --prune --tags with branch does not delete other remote-tracking branches' '
+	cd "$D" &&
+	git clone . prune-tags-branch &&
+	cd prune-tags-branch &&
+	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+
+	git fetch --prune --tags origin master &&
+	git rev-parse origin/extrabranch
+'
+
 test_expect_success 'fetch tags when there is no tags' '
 
     cd "$D" &&
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs
  2011-10-15  5:04 [PATCHv4 0/5] Be more careful when prunning Carlos Martín Nieto
  2011-10-15  5:04 ` [PATCH 1/5] fetch: free all the additional refspecs Carlos Martín Nieto
  2011-10-15  5:04 ` [PATCH 2/5] t5510: add tests for fetch --prune Carlos Martín Nieto
@ 2011-10-15  5:04 ` Carlos Martín Nieto
  2011-10-16  4:45   ` Junio C Hamano
  2011-10-15  5:04 ` [PATCH 4/5] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
  2011-10-15  5:04 ` [PATCH 5/5] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto
  4 siblings, 1 reply; 8+ messages in thread
From: Carlos Martín Nieto @ 2011-10-15  5:04 UTC (permalink / raw
  To: git; +Cc: Jeff King, Junio C Hamano, mathstuf

Move the body of remote_find_tracking to a new function query_refspecs
which does the same (find a refspec that matches and apply the
transformation) but explicitely wants the list of refspecs.

Make remote_find_tracking and apply_refspecs use query_refspecs.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 remote.c |   70 ++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 34 insertions(+), 36 deletions(-)

diff --git a/remote.c b/remote.c
index b8ecfa5..e94c6d2 100644
--- a/remote.c
+++ b/remote.c
@@ -828,59 +828,57 @@ static int match_name_with_pattern(const char *key, const char *name,
 	return ret;
 }
 
-char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
-		     const char *name)
+static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query)
 {
 	int i;
-	char *ret = NULL;
-	for (i = 0; i < nr_refspec; i++) {
-		struct refspec *refspec = refspecs + i;
-		if (refspec->pattern) {
-			if (match_name_with_pattern(refspec->src, name,
-						    refspec->dst, &ret))
-				return ret;
-		} else if (!strcmp(refspec->src, name))
-			return strdup(refspec->dst);
-	}
-	return NULL;
-}
+	int find_src = query->src == NULL;
 
-int remote_find_tracking(struct remote *remote, struct refspec *refspec)
-{
-	int find_src = refspec->src == NULL;
-	char *needle, **result;
-	int i;
+	if (find_src && !query->dst)
+		return error("query_refspecs: need either src or dst");
 
-	if (find_src) {
-		if (!refspec->dst)
-			return error("find_tracking: need either src or dst");
-		needle = refspec->dst;
-		result = &refspec->src;
-	} else {
-		needle = refspec->src;
-		result = &refspec->dst;
-	}
+	for (i = 0; i < ref_count; i++) {
+		struct refspec *refspec = &refs[i];
+		const char *key = find_src ? refspec->dst : refspec->src;
+		const char *value = find_src ? refspec->src : refspec->dst;
+		const char *needle = find_src ? query->dst : query->src;
+		char **result = find_src ? &query->src : &query->dst;
 
-	for (i = 0; i < remote->fetch_refspec_nr; i++) {
-		struct refspec *fetch = &remote->fetch[i];
-		const char *key = find_src ? fetch->dst : fetch->src;
-		const char *value = find_src ? fetch->src : fetch->dst;
-		if (!fetch->dst)
+		if (!refspec->dst)
 			continue;
-		if (fetch->pattern) {
+		if (refspec->pattern) {
 			if (match_name_with_pattern(key, needle, value, result)) {
-				refspec->force = fetch->force;
+				query->force = refspec->force;
 				return 0;
 			}
 		} else if (!strcmp(needle, key)) {
 			*result = xstrdup(value);
-			refspec->force = fetch->force;
+			query->force = refspec->force;
 			return 0;
 		}
 	}
+
 	return -1;
 }
 
+char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
+		     const char *name)
+{
+	struct refspec query;
+
+	memset(&query, 0x0, sizeof(struct refspec));
+	query.src = (char *) name;
+
+	if (query_refspecs(refspecs, nr_refspec, &query))
+		return NULL;
+
+	return query.dst;
+}
+
+int remote_find_tracking(struct remote *remote, struct refspec *refspec)
+{
+	return query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);
+}
+
 static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
 		const char *name)
 {
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/5] fetch: honor the user-provided refspecs when pruning refs
  2011-10-15  5:04 [PATCHv4 0/5] Be more careful when prunning Carlos Martín Nieto
                   ` (2 preceding siblings ...)
  2011-10-15  5:04 ` [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs Carlos Martín Nieto
@ 2011-10-15  5:04 ` Carlos Martín Nieto
  2011-10-15  5:04 ` [PATCH 5/5] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto
  4 siblings, 0 replies; 8+ messages in thread
From: Carlos Martín Nieto @ 2011-10-15  5:04 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 ref 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.

Change prune_refs and get_stale_heads to simply accept a list of
references and a list of refspecs. The caller of either function needs
to decide what refspecs should be used to decide whether a ref is
stale.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 builtin/fetch.c  |   12 ++++++++----
 builtin/remote.c |    3 ++-
 remote.c         |   36 ++++++++++++++++++++++++------------
 remote.h         |    2 +-
 t/t5510-fetch.sh |    4 ++--
 5 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 605d1bf..e295d97 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -540,10 +540,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 refspec *refs, int ref_count, 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(refs, ref_count, ref_map);
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)\n")
 		: _("   (%s has become dangling)\n");
@@ -734,8 +734,12 @@ static int do_fetch(struct transport *transport,
 		free_refs(ref_map);
 		return 1;
 	}
-	if (prune)
-		prune_refs(transport, ref_map);
+	if (prune) {
+		if (ref_count)
+			prune_refs(refs, ref_count, ref_map);
+		else
+			prune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, 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 f2a9c26..de52367 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -349,7 +349,8 @@ 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,
+				     states->remote->fetch_refspec_nr, fetch_map);
 	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 e94c6d2..069b1ff 100644
--- a/remote.c
+++ b/remote.c
@@ -1679,36 +1679,48 @@ struct ref *guess_remote_head(const struct ref *head,
 }
 
 struct stale_heads_info {
-	struct remote *remote;
 	struct string_list *ref_names;
 	struct ref **stale_refs_tail;
+	struct refspec *refs;
+	int ref_count;
 };
 
 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;
-	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);
-		}
+	struct refspec query;
+	memset(&query, 0, sizeof(struct refspec));
+	query.dst = (char *)refname;
+
+	if (query_refspecs(info->refs, info->ref_count, &query))
+	    return 0; /* No matches */
+
+
+	/*
+	 * 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, query.src))) {
+		struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
+		hashcpy(ref->new_sha1, sha1);
 	}
+
+	free(query.src);
 	return 0;
 }
 
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
+struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fetch_map)
 {
 	struct ref *ref, *stale_refs = NULL;
 	struct string_list ref_names = STRING_LIST_INIT_NODUP;
 	struct stale_heads_info info;
-	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..f2541b5 100644
--- a/remote.h
+++ b/remote.h
@@ -164,6 +164,6 @@ 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 refspec *refs, int ref_count, struct ref *fetch_map);
 
 #endif
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 8b5e925..581049b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -86,7 +86,7 @@ test_expect_success 'fetch --prune on its own works as expected' '
 	test_must_fail git rev-parse origin/extrabranch
 '
 
-test_expect_failure 'fetch --prune with a branch name keeps branches' '
+test_expect_success 'fetch --prune with a branch name keeps branches' '
 	cd "$D" &&
 	git clone . prune-branch &&
 	cd prune-branch &&
@@ -96,7 +96,7 @@ test_expect_failure 'fetch --prune with a branch name keeps branches' '
 	git rev-parse origin/extrabranch
 '
 
-test_expect_failure 'fetch --prune with a namespace keeps other namespaces' '
+test_expect_success 'fetch --prune with a namespace keeps other namespaces' '
 	cd "$D" &&
 	git clone . prune-namespace &&
 	cd prune-namespace &&
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 5/5] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning
  2011-10-15  5:04 [PATCHv4 0/5] Be more careful when prunning Carlos Martín Nieto
                   ` (3 preceding siblings ...)
  2011-10-15  5:04 ` [PATCH 4/5] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
@ 2011-10-15  5:04 ` Carlos Martín Nieto
  4 siblings, 0 replies; 8+ messages in thread
From: Carlos Martín Nieto @ 2011-10-15  5:04 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  |   23 +++++++++++++++++++++--
 t/t5510-fetch.sh |    4 ++--
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e295d97..9d481f8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -735,10 +735,29 @@ static int do_fetch(struct transport *transport,
 		return 1;
 	}
 	if (prune) {
-		if (ref_count)
+		/* If --tags was specified, pretend the user gave us the canonical tags refspec */
+		if (tags == TAGS_SET) {
+			const char *tags_str = "refs/tags/*:refs/tags/*";
+			struct refspec *tags_refspec, *refspec;
+
+			/* Copy the refspec and add the tags to it */
+			refspec = xcalloc(ref_count + 1, sizeof(struct refspec));
+			tags_refspec = parse_fetch_refspec(1, &tags_str);
+			memcpy(refspec, refs, ref_count * sizeof(struct refspec));
+			memcpy(&refspec[ref_count], tags_refspec, sizeof(struct refspec));
+			ref_count++;
+
+			prune_refs(refspec, ref_count, ref_map);
+
+			ref_count--;
+			/* The rest of the strings belong to fetch_one */
+			free_refspec(1, tags_refspec);
+			free(refspec);
+		} else if (ref_count) {
 			prune_refs(refs, ref_count, ref_map);
-		else
+		} else {
 			prune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);
+		}
 	}
 	free_refs(ref_map);
 
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 581049b..e0af4c4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -105,7 +105,7 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '
 	git rev-parse origin/master
 '
 
-test_expect_failure 'fetch --prune --tags does not delete the remote-tracking branches' '
+test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '
 	cd "$D" &&
 	git clone . prune-tags &&
 	cd prune-tags &&
@@ -116,7 +116,7 @@ test_expect_failure 'fetch --prune --tags does not delete the remote-tracking br
 	test_must_fail git rev-parse somebranch
 '
 
-test_expect_failure 'fetch --prune --tags with branch does not delete other remote-tracking branches' '
+test_expect_success 'fetch --prune --tags with branch does not delete other remote-tracking branches' '
 	cd "$D" &&
 	git clone . prune-tags-branch &&
 	cd prune-tags-branch &&
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs
  2011-10-15  5:04 ` [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs Carlos Martín Nieto
@ 2011-10-16  4:45   ` Junio C Hamano
  2011-10-16  5:10     ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2011-10-16  4:45 UTC (permalink / raw
  To: Carlos Martín Nieto; +Cc: git, Jeff King, mathstuf

Carlos Martín Nieto <cmn@elego.de> writes:

> Move the body of remote_find_tracking to a new function query_refspecs
> which does the same (find a refspec that matches and apply the
> transformation) but explicitly wants the list of refspecs.
>
> Make remote_find_tracking and apply_refspecs use query_refspecs.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>  remote.c |   70 ++++++++++++++++++++++++++++++-------------------------------
>  1 files changed, 34 insertions(+), 36 deletions(-)

Looks very sensible, especially knowing what you want to do in the next
patch ;-).

Thanks.

>
> diff --git a/remote.c b/remote.c
> index b8ecfa5..e94c6d2 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -828,59 +828,57 @@ static int match_name_with_pattern(const char *key, const char *name,
>  	return ret;
>  }
>  
> -char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
> -		     const char *name)
> +static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query)
>  {
>  	int i;
> -	char *ret = NULL;
> -	for (i = 0; i < nr_refspec; i++) {
> -		struct refspec *refspec = refspecs + i;
> -		if (refspec->pattern) {
> -			if (match_name_with_pattern(refspec->src, name,
> -						    refspec->dst, &ret))
> -				return ret;
> -		} else if (!strcmp(refspec->src, name))
> -			return strdup(refspec->dst);
> -	}
> -	return NULL;
> -}
> +	int find_src = query->src == NULL;
>  
> -int remote_find_tracking(struct remote *remote, struct refspec *refspec)
> -{
> -	int find_src = refspec->src == NULL;
> -	char *needle, **result;
> -	int i;
> +	if (find_src && !query->dst)
> +		return error("query_refspecs: need either src or dst");
>  
> -	if (find_src) {
> -		if (!refspec->dst)
> -			return error("find_tracking: need either src or dst");
> -		needle = refspec->dst;
> -		result = &refspec->src;
> -	} else {
> -		needle = refspec->src;
> -		result = &refspec->dst;
> -	}
> +	for (i = 0; i < ref_count; i++) {
> +		struct refspec *refspec = &refs[i];
> +		const char *key = find_src ? refspec->dst : refspec->src;
> +		const char *value = find_src ? refspec->src : refspec->dst;
> +		const char *needle = find_src ? query->dst : query->src;
> +		char **result = find_src ? &query->src : &query->dst;
>  
> -	for (i = 0; i < remote->fetch_refspec_nr; i++) {
> -		struct refspec *fetch = &remote->fetch[i];
> -		const char *key = find_src ? fetch->dst : fetch->src;
> -		const char *value = find_src ? fetch->src : fetch->dst;
> -		if (!fetch->dst)
> +		if (!refspec->dst)
>  			continue;
> -		if (fetch->pattern) {
> +		if (refspec->pattern) {
>  			if (match_name_with_pattern(key, needle, value, result)) {
> -				refspec->force = fetch->force;
> +				query->force = refspec->force;
>  				return 0;
>  			}
>  		} else if (!strcmp(needle, key)) {
>  			*result = xstrdup(value);
> -			refspec->force = fetch->force;
> +			query->force = refspec->force;
>  			return 0;
>  		}
>  	}
> +
>  	return -1;
>  }
>  
> +char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
> +		     const char *name)
> +{
> +	struct refspec query;
> +
> +	memset(&query, 0x0, sizeof(struct refspec));
> +	query.src = (char *) name;
> +
> +	if (query_refspecs(refspecs, nr_refspec, &query))
> +		return NULL;
> +
> +	return query.dst;
> +}
> +
> +int remote_find_tracking(struct remote *remote, struct refspec *refspec)
> +{
> +	return query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);
> +}
> +
>  static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
>  		const char *name)
>  {

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs
  2011-10-16  4:45   ` Junio C Hamano
@ 2011-10-16  5:10     ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2011-10-16  5:10 UTC (permalink / raw
  To: git, Daniel Barkalow; +Cc: Carlos Martín Nieto, Jeff King, mathstuf

Junio C Hamano <gitster@pobox.com> writes:

> Carlos Martín Nieto <cmn@elego.de> writes:
>
>> Move the body of remote_find_tracking to a new function query_refspecs
>> which does the same (find a refspec that matches and apply the
>> transformation) but explicitly wants the list of refspecs.
>>
>> Make remote_find_tracking and apply_refspecs use query_refspecs.
>>
>> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
>> ---
>>  remote.c |   70 ++++++++++++++++++++++++++++++-------------------------------
>>  1 files changed, 34 insertions(+), 36 deletions(-)
>
> Looks very sensible, especially knowing what you want to do in the next
> patch ;-).
>
> Thanks.

I notice that before this update, passing a refspec[] with an element that
lacks its dst side to apply_refspecs() would have segfaulted but with this
update such an element in the refspec[] will simply be ignored.

This helper function was added in 72ff894 (Allow helper to map private ref
names into normal names, 2009-11-18) for transport-helper's use. I am sure
the change will not introduce a regression due to this skippage (the code
without this patch would have simply crashed with such an input anyway),
but I thought people involved in the transport layer may want to know.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2011-10-16  5:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-15  5:04 [PATCHv4 0/5] Be more careful when prunning Carlos Martín Nieto
2011-10-15  5:04 ` [PATCH 1/5] fetch: free all the additional refspecs Carlos Martín Nieto
2011-10-15  5:04 ` [PATCH 2/5] t5510: add tests for fetch --prune Carlos Martín Nieto
2011-10-15  5:04 ` [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs Carlos Martín Nieto
2011-10-16  4:45   ` Junio C Hamano
2011-10-16  5:10     ` Junio C Hamano
2011-10-15  5:04 ` [PATCH 4/5] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
2011-10-15  5:04 ` [PATCH 5/5] fetch: treat --tags like refs/tags/*:refs/tags/* when pruning Carlos Martín Nieto

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).