From f166db26af524278b42caac8f092d8de5e3e9c29 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 30 Oct 2013 06:32:56 +0100 Subject: get_expanded_map(): add docstring Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- remote.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'remote.c') diff --git a/remote.c b/remote.c index e9fedfa918..bef81cdb6c 100644 --- a/remote.c +++ b/remote.c @@ -1553,6 +1553,13 @@ static int ignore_symref_update(const char *refname) return (flag & REF_ISSYMREF); } +/* + * Create and return a list of (struct ref) consisting of copies of + * each remote_ref that matches refspec. refspec must be a pattern. + * Fill in the copies' peer_ref to describe the local tracking refs to + * which they map. Omit any references that would map to an existing + * local symbolic ref. + */ static struct ref *get_expanded_map(const struct ref *remote_refs, const struct refspec *refspec) { -- cgit v1.3-5-g9baa From e31a17f7416a85ee8c3260f8c8b7519ff7ea82b6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 30 Oct 2013 06:32:57 +0100 Subject: get_expanded_map(): avoid memory leak The old code could leak *expn_name if match_name_with_pattern() succeeded but ignore_symref_update() returned true. So make sure that *expn_name is freed in any case. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- remote.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index bef81cdb6c..a83dc90318 100644 --- a/remote.c +++ b/remote.c @@ -1567,9 +1567,9 @@ static struct ref *get_expanded_map(const struct ref *remote_refs, struct ref *ret = NULL; struct ref **tail = &ret; - char *expn_name; - for (ref = remote_refs; ref; ref = ref->next) { + char *expn_name = NULL; + if (strchr(ref->name, '^')) continue; /* a dereference item */ if (match_name_with_pattern(refspec->src, ref->name, @@ -1578,12 +1578,12 @@ static struct ref *get_expanded_map(const struct ref *remote_refs, struct ref *cpy = copy_ref(ref); cpy->peer_ref = alloc_ref(expn_name); - free(expn_name); if (refspec->force) cpy->peer_ref->force = 1; *tail = cpy; tail = &cpy->next; } + free(expn_name); } return ret; -- cgit v1.3-5-g9baa From 049bff8f0ec722dbd4ca9582df7891c920e41165 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 30 Oct 2013 06:33:01 +0100 Subject: query_refspecs(): move some constants out of the loop Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- remote.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index a83dc90318..4d45f5dc72 100644 --- a/remote.c +++ b/remote.c @@ -825,6 +825,8 @@ static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *q { int i; int find_src = !query->src; + const char *needle = find_src ? query->dst : query->src; + char **result = find_src ? &query->src : &query->dst; if (find_src && !query->dst) return error("query_refspecs: need either src or dst"); @@ -833,8 +835,6 @@ static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *q struct refspec *refspec = &refs[i]; const char *key = find_src ? refspec->dst : refspec->src; const char *value = find_src ? refspec->src : refspec->dst; - const char *needle = find_src ? query->dst : query->src; - char **result = find_src ? &query->src : &query->dst; if (!refspec->dst) continue; -- cgit v1.3-5-g9baa From 09ea1f8e0e85dd65307b29219ef1f5a3958eb918 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 30 Oct 2013 06:33:07 +0100 Subject: ref_remove_duplicates(): avoid redundant bisection The old code called string_list_lookup(), and if that failed called string_list_insert(), thus doing the bisection search through the string list twice in the latter code path. Instead, just call string_list_insert() right away. If an entry for that peer reference name already existed, then its util pointer is always non-NULL. Of course this doesn't change the fact that the repeated string_list_insert() calls make the function scale like O(N^2) if the input reference list is not already approximately sorted. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- remote.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index 4d45f5dc72..1fb87de01c 100644 --- a/remote.c +++ b/remote.c @@ -750,13 +750,15 @@ void ref_remove_duplicates(struct ref *ref_map) struct string_list refs = STRING_LIST_INIT_NODUP; struct string_list_item *item = NULL; struct ref *prev = NULL, *next = NULL; + for (; ref_map; prev = ref_map, ref_map = next) { next = ref_map->next; if (!ref_map->peer_ref) continue; - item = string_list_lookup(&refs, ref_map->peer_ref->name); - if (item) { + item = string_list_insert(&refs, ref_map->peer_ref->name); + if (item->util) { + /* Entry already existed */ if (strcmp(((struct ref *)item->util)->name, ref_map->name)) die("%s tracks both %s and %s", @@ -767,11 +769,9 @@ void ref_remove_duplicates(struct ref *ref_map) free(ref_map->peer_ref); free(ref_map); ref_map = prev; /* skip this; we freed it */ - continue; + } else { + item->util = ref_map; } - - item = string_list_insert(&refs, ref_map->peer_ref->name); - item->util = ref_map; } string_list_clear(&refs, 0); } -- cgit v1.3-5-g9baa From b9afe6654db2bfb776db933f832e7e03052adf98 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 30 Oct 2013 06:33:09 +0100 Subject: ref_remove_duplicates(): simplify loop logic Change the loop body into the more straightforward * remove item from the front of the old list * if necessary, add it to the tail of the new list and return a pointer to the new list (even though it is currently always the same as the input argument, because the first element in the list is currently never deleted). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch.c | 4 +--- remote.c | 52 +++++++++++++++++++++++++++++++--------------------- remote.h | 8 ++++++-- 3 files changed, 38 insertions(+), 26 deletions(-) (limited to 'remote.c') diff --git a/builtin/fetch.c b/builtin/fetch.c index 5ddb9af05c..3d978eb58e 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -360,9 +360,7 @@ static struct ref *get_ref_map(struct transport *transport, tail = &rm->next; } - ref_remove_duplicates(ref_map); - - return ref_map; + return ref_remove_duplicates(ref_map); } #define STORE_REF_ERROR_OTHER 1 diff --git a/remote.c b/remote.c index 1fb87de01c..f803990760 100644 --- a/remote.c +++ b/remote.c @@ -745,35 +745,45 @@ int for_each_remote(each_remote_fn fn, void *priv) return result; } -void ref_remove_duplicates(struct ref *ref_map) +struct ref *ref_remove_duplicates(struct ref *ref_map) { struct string_list refs = STRING_LIST_INIT_NODUP; - struct string_list_item *item = NULL; - struct ref *prev = NULL, *next = NULL; + struct ref *retval = NULL; + struct ref **p = &retval; - for (; ref_map; prev = ref_map, ref_map = next) { - next = ref_map->next; - if (!ref_map->peer_ref) - continue; + while (ref_map) { + struct ref *ref = ref_map; + + ref_map = ref_map->next; + ref->next = NULL; - item = string_list_insert(&refs, ref_map->peer_ref->name); - if (item->util) { - /* Entry already existed */ - if (strcmp(((struct ref *)item->util)->name, - ref_map->name)) - die("%s tracks both %s and %s", - ref_map->peer_ref->name, - ((struct ref *)item->util)->name, - ref_map->name); - prev->next = ref_map->next; - free(ref_map->peer_ref); - free(ref_map); - ref_map = prev; /* skip this; we freed it */ + if (!ref->peer_ref) { + *p = ref; + p = &ref->next; } else { - item->util = ref_map; + struct string_list_item *item = + string_list_insert(&refs, ref->peer_ref->name); + + if (item->util) { + /* Entry already existed */ + if (strcmp(((struct ref *)item->util)->name, + ref->name)) + die("%s tracks both %s and %s", + ref->peer_ref->name, + ((struct ref *)item->util)->name, + ref->name); + free(ref->peer_ref); + free(ref); + } else { + *p = ref; + p = &ref->next; + item->util = ref; + } } } + string_list_clear(&refs, 0); + return retval; } int remote_has_url(struct remote *remote, const char *url) diff --git a/remote.h b/remote.h index 131130a611..c07eb9950a 100644 --- a/remote.h +++ b/remote.h @@ -149,9 +149,13 @@ int resolve_remote_symref(struct ref *ref, struct ref *list); int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1); /* - * Removes and frees any duplicate refs in the map. + * Remove and free all but the first of any entries in the input list + * that map the same remote reference to the same local reference. If + * there are two entries that map different remote references to the + * same local reference, emit an error message and die. Return a + * pointer to the head of the resulting list. */ -void ref_remove_duplicates(struct ref *ref_map); +struct ref *ref_remove_duplicates(struct ref *ref_map); int valid_fetch_refspec(const char *refspec); struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec); -- cgit v1.3-5-g9baa From df02ebdac8bdf0edd30e85c9902d177dccfac276 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 30 Oct 2013 06:33:10 +0100 Subject: ref_remote_duplicates(): extract a function handle_duplicate() It will become more complex in a moment. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- remote.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index f803990760..4bed101da6 100644 --- a/remote.c +++ b/remote.c @@ -745,6 +745,15 @@ int for_each_remote(each_remote_fn fn, void *priv) return result; } +static void handle_duplicate(struct ref *ref1, struct ref *ref2) +{ + if (strcmp(ref1->name, ref2->name)) + die("%s tracks both %s and %s", + ref2->peer_ref->name, ref1->name, ref2->name); + free(ref2->peer_ref); + free(ref2); +} + struct ref *ref_remove_duplicates(struct ref *ref_map) { struct string_list refs = STRING_LIST_INIT_NODUP; @@ -766,14 +775,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map) if (item->util) { /* Entry already existed */ - if (strcmp(((struct ref *)item->util)->name, - ref->name)) - die("%s tracks both %s and %s", - ref->peer_ref->name, - ((struct ref *)item->util)->name, - ref->name); - free(ref->peer_ref); - free(ref); + handle_duplicate((struct ref *)item->util, ref); } else { *p = ref; p = &ref->next; -- cgit v1.3-5-g9baa From 76ea6717fe7dda28966f586e09e02b7b0d5b76cf Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 30 Oct 2013 06:33:11 +0100 Subject: handle_duplicate(): mark error message for translation Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index 4bed101da6..0848faffa4 100644 --- a/remote.c +++ b/remote.c @@ -748,7 +748,7 @@ int for_each_remote(each_remote_fn fn, void *priv) static void handle_duplicate(struct ref *ref1, struct ref *ref2) { if (strcmp(ref1->name, ref2->name)) - die("%s tracks both %s and %s", + die(_("%s tracks both %s and %s"), ref2->peer_ref->name, ref1->name, ref2->name); free(ref2->peer_ref); free(ref2); -- cgit v1.3-5-g9baa From f096e6e826678c29e4bfde4d9d1ae1df79074ce3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 30 Oct 2013 06:33:12 +0100 Subject: fetch: improve the error messages emitted for conflicting refspecs If we find two refspecs that want to update the same local reference, emit an error message that is more informative based on whether one of the conflicting refspecs is an opportunistic update during a fetch with explicit command-line refspecs. And especially, do not die if an opportunistic reference update conflicts with an express wish of the user; rather, just emit a warning and skip the opportunistic reference update. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- remote.c | 25 ++++++++++++++++++++++--- t/t5536-fetch-conflicts.sh | 14 +++++++++----- 2 files changed, 31 insertions(+), 8 deletions(-) (limited to 'remote.c') diff --git a/remote.c b/remote.c index 0848faffa4..dc56619d23 100644 --- a/remote.c +++ b/remote.c @@ -747,9 +747,28 @@ int for_each_remote(each_remote_fn fn, void *priv) static void handle_duplicate(struct ref *ref1, struct ref *ref2) { - if (strcmp(ref1->name, ref2->name)) - die(_("%s tracks both %s and %s"), - ref2->peer_ref->name, ref1->name, ref2->name); + if (strcmp(ref1->name, ref2->name)) { + if (ref1->fetch_head_status != FETCH_HEAD_IGNORE && + ref2->fetch_head_status != FETCH_HEAD_IGNORE) { + die(_("Cannot fetch both %s and %s to %s"), + ref1->name, ref2->name, ref2->peer_ref->name); + } else if (ref1->fetch_head_status != FETCH_HEAD_IGNORE && + ref2->fetch_head_status == FETCH_HEAD_IGNORE) { + warning(_("%s usually tracks %s, not %s"), + ref2->peer_ref->name, ref2->name, ref1->name); + } else if (ref1->fetch_head_status == FETCH_HEAD_IGNORE && + ref2->fetch_head_status == FETCH_HEAD_IGNORE) { + die(_("%s tracks both %s and %s"), + ref2->peer_ref->name, ref1->name, ref2->name); + } else { + /* + * This last possibility doesn't occur because + * FETCH_HEAD_IGNORE entries always appear at + * the end of the list. + */ + die(_("Internal error")); + } + } free(ref2->peer_ref); free(ref2); } diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh index 679c53e044..6c5d3a4ce0 100755 --- a/t/t5536-fetch-conflicts.sh +++ b/t/t5536-fetch-conflicts.sh @@ -22,7 +22,7 @@ verify_stderr () { cat >expected && # We're not interested in the error # "fatal: The remote end hung up unexpectedly": - grep -v "hung up" actual && + grep -E '^(fatal|warning):' actual | sort && test_cmp expected actual } @@ -49,7 +49,7 @@ test_expect_success 'fetch conflict: config vs. config' ' cd ccc && test_must_fail git fetch origin 2>error && verify_stderr <<-\EOF - fatal: refs/remotes/origin/branch1 tracks both refs/heads/branch1 and refs/heads/branch2 + fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1 EOF ) ' @@ -78,18 +78,22 @@ test_expect_success 'fetch conflict: arg vs. arg' ' refs/heads/*:refs/remotes/origin/* \ refs/heads/branch2:refs/remotes/origin/branch1 2>error && verify_stderr <<-\EOF - fatal: refs/remotes/origin/branch1 tracks both refs/heads/branch1 and refs/heads/branch2 + fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1 EOF ) ' -test_expect_failure 'fetch conflict: criss-cross args' ' +test_expect_success 'fetch conflict: criss-cross args' ' setup_repository xaa \ "+refs/heads/*:refs/remotes/origin/*" && ( cd xaa && git fetch origin \ refs/heads/branch1:refs/remotes/origin/branch2 \ - refs/heads/branch2:refs/remotes/origin/branch1 + refs/heads/branch2:refs/remotes/origin/branch1 2>error && + verify_stderr <<-\EOF + warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2 + warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1 + EOF ) ' -- cgit v1.3-5-g9baa