From 3b0820045a68fa84707768528f3af4520fbf3bf0 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:36 +0200 Subject: [PATCH 01/15] t5500: add tests of error output for missing refs If "git fetch-pack" is called with reference names that do not exist on the remote, then it should emit an error message error: no such remote ref refs/heads/xyzzy This is currently broken if *only* missing references are passed to "git fetch-pack". Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index e80a2af348..6fa1ceffee 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -397,4 +397,34 @@ test_expect_success 'test duplicate refs from stdin' ' test_cmp expect actual ' +test_expect_success 'set up tests of missing reference' ' + cat >expect-error <<-\EOF + error: no such remote ref refs/heads/xyzzy + EOF +' + +test_expect_failure 'test lonely missing ref' ' + ( + cd client && + test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy + ) >/dev/null 2>error-m && + test_cmp expect-error error-m +' + +test_expect_success 'test missing ref after existing' ' + ( + cd client && + test_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy + ) >/dev/null 2>error-em && + test_cmp expect-error error-em +' + +test_expect_success 'test missing ref before existing' ' + ( + cd client && + test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A + ) >/dev/null 2>error-me && + test_cmp expect-error error-me +' + test_done From 8db43d2935811319a1d427be4da2723f68516e62 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:37 +0200 Subject: [PATCH 02/15] t5500: add tests of fetch-pack --all --depth=N $URL $REF Document some bugs in "git fetch-pack": 1. If "git fetch-pack" is called with "--all", "--depth", and an explicit existing non-tag reference to fetch, then it falsely reports that the reference was not found, even though it was fetched correctly. 2. If "git fetch-pack" is called with "--all", "--depth", and an explicit existing tag reference to fetch, then it segfaults in filter_refs() because return_refs is used without having been initialized. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t5500-fetch-pack.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 6fa1ceffee..15d277f158 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -427,4 +427,19 @@ test_expect_success 'test missing ref before existing' ' test_cmp expect-error error-me ' +test_expect_failure 'test --all, --depth, and explicit head' ' + ( + cd client && + git fetch-pack --no-progress --all --depth=1 .. refs/heads/A + ) >out-adh 2>error-adh +' + +test_expect_failure 'test --all, --depth, and explicit tag' ' + git tag OLDTAG refs/heads/B~5 && + ( + cd client && + git fetch-pack --no-progress --all --depth=1 .. refs/tags/OLDTAG + ) >out-adt 2>error-adt +' + test_done From 07c19e72c5f07ac687e87e49cbb4e128a8b26629 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:38 +0200 Subject: [PATCH 03/15] Rename static function fetch_pack() to http_fetch_pack() Avoid confusion with the non-static function of the same name from fetch-pack.h. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- http-walker.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http-walker.c b/http-walker.c index 51a906e9e3..1516c5eb29 100644 --- a/http-walker.c +++ b/http-walker.c @@ -396,7 +396,7 @@ static int fetch_indices(struct walker *walker, struct alt_base *repo) return ret; } -static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned char *sha1) +static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigned char *sha1) { struct packed_git *target; int ret; @@ -524,7 +524,7 @@ static int fetch(struct walker *walker, unsigned char *sha1) if (!fetch_object(walker, altbase, sha1)) return 0; while (altbase) { - if (!fetch_pack(walker, altbase, sha1)) + if (!http_fetch_pack(walker, altbase, sha1)) return 0; fetch_alternates(walker, data->alt->base); altbase = altbase->next; From 63c694534bbe93a188ed74396a1a3ace0b24d6ad Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:39 +0200 Subject: [PATCH 04/15] fetch_pack(): reindent function decl and defn Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 8 ++++---- fetch-pack.h | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index fdda36f149..30990c0a83 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -1082,10 +1082,10 @@ static int compare_heads(const void *a, const void *b) struct ref *fetch_pack(struct fetch_pack_args *my_args, int fd[], struct child_process *conn, const struct ref *ref, - const char *dest, - int nr_heads, - char **heads, - char **pack_lockfile) + const char *dest, + int nr_heads, + char **heads, + char **pack_lockfile) { struct stat st; struct ref *ref_cpy; diff --git a/fetch-pack.h b/fetch-pack.h index 7c2069c972..1dbe90fa43 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -18,11 +18,11 @@ struct fetch_pack_args { }; struct ref *fetch_pack(struct fetch_pack_args *args, - int fd[], struct child_process *conn, - const struct ref *ref, - const char *dest, - int nr_heads, - char **heads, - char **pack_lockfile); + int fd[], struct child_process *conn, + const struct ref *ref, + const char *dest, + int nr_heads, + char **heads, + char **pack_lockfile); #endif From 8bee93dd24731a1d2ef7c82d484a893cf68b6572 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:40 +0200 Subject: [PATCH 05/15] Change fetch_pack() and friends to take string_list arguments Instead of juggling (sometimes called ), pass around the list of references to be sought in a single string_list variable called "sought". Future commits will make more use of string_list functionality. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 88 ++++++++++++++++++++------------------------ fetch-pack.h | 5 ++- transport.c | 18 ++++----- 3 files changed, 51 insertions(+), 60 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 30990c0a83..df81995eab 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -525,27 +525,27 @@ static void mark_recent_complete_commits(unsigned long cutoff) } } -static void filter_refs(struct ref **refs, int nr_match, char **match) +static void filter_refs(struct ref **refs, struct string_list *sought) { struct ref **return_refs; struct ref *newlist = NULL; struct ref **newtail = &newlist; struct ref *ref, *next; struct ref *fastarray[32]; - int match_pos; + int sought_pos; - if (nr_match && !args.fetch_all) { - if (ARRAY_SIZE(fastarray) < nr_match) - return_refs = xcalloc(nr_match, sizeof(struct ref *)); + if (sought->nr && !args.fetch_all) { + if (ARRAY_SIZE(fastarray) < sought->nr) + return_refs = xcalloc(sought->nr, sizeof(struct ref *)); else { return_refs = fastarray; - memset(return_refs, 0, sizeof(struct ref *) * nr_match); + memset(return_refs, 0, sizeof(struct ref *) * sought->nr); } } else return_refs = NULL; - match_pos = 0; + sought_pos = 0; for (ref = *refs; ref; ref = next) { next = ref->next; if (!memcmp(ref->name, "refs/", 5) && @@ -560,17 +560,17 @@ static void filter_refs(struct ref **refs, int nr_match, char **match) } else { int cmp = -1; - while (match_pos < nr_match) { - cmp = strcmp(ref->name, match[match_pos]); + while (sought_pos < sought->nr) { + cmp = strcmp(ref->name, sought->items[sought_pos].string); if (cmp < 0) /* definitely do not have it */ break; else if (cmp == 0) { /* definitely have it */ - match[match_pos][0] = '\0'; - return_refs[match_pos] = ref; + sought->items[sought_pos].string[0] = '\0'; + return_refs[sought_pos] = ref; break; } else /* might have it; keep looking */ - match_pos++; + sought_pos++; } if (!cmp) continue; /* we will link it later */ @@ -580,7 +580,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match) if (!args.fetch_all) { int i; - for (i = 0; i < nr_match; i++) { + for (i = 0; i < sought->nr; i++) { ref = return_refs[i]; if (ref) { *newtail = ref; @@ -599,7 +599,7 @@ static void mark_alternate_complete(const struct ref *ref, void *unused) mark_complete(NULL, ref->old_sha1, 0, NULL); } -static int everything_local(struct ref **refs, int nr_match, char **match) +static int everything_local(struct ref **refs, struct string_list *sought) { struct ref *ref; int retval; @@ -650,7 +650,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match) } } - filter_refs(refs, nr_match, match); + filter_refs(refs, sought); for (retval = 1, ref = *refs; ref ; ref = ref->next) { const unsigned char *remote = ref->old_sha1; @@ -781,8 +781,7 @@ static int get_pack(int xd[2], char **pack_lockfile) static struct ref *do_fetch_pack(int fd[2], const struct ref *orig_ref, - int nr_match, - char **match, + struct string_list *sought, char **pack_lockfile) { struct ref *ref = copy_ref_list(orig_ref); @@ -839,7 +838,7 @@ static struct ref *do_fetch_pack(int fd[2], agent_len, agent_feature); } - if (everything_local(&ref, nr_match, match)) { + if (everything_local(&ref, sought)) { packet_flush(fd[1]); goto all_done; } @@ -859,16 +858,16 @@ static struct ref *do_fetch_pack(int fd[2], return ref; } -static int remove_duplicates(int nr_heads, char **heads) +static int remove_duplicates(struct string_list *sought) { int src, dst; - if (!nr_heads) + if (!sought->nr) return 0; - for (src = dst = 1; src < nr_heads; src++) - if (strcmp(heads[src], heads[dst-1])) - heads[dst++] = heads[src]; + for (src = dst = 1; src < sought->nr; src++) + if (strcmp(sought->items[src].string, sought->items[dst-1].string)) + sought->items[dst++] = sought->items[src]; return dst; } @@ -922,8 +921,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) int i, ret; struct ref *ref = NULL; const char *dest = NULL; - int alloc_heads = 0, nr_heads = 0; - char **heads = NULL; + struct string_list sought = STRING_LIST_INIT_DUP; int fd[2]; char *pack_lockfile = NULL; char **pack_lockfile_ptr = NULL; @@ -1000,9 +998,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) * Copy refs from cmdline to growable list, then append any * refs from the standard input: */ - ALLOC_GROW(heads, argc - i, alloc_heads); for (; i < argc; i++) - heads[nr_heads++] = xstrdup(argv[i]); + string_list_append(&sought, xstrdup(argv[i])); if (args.stdin_refs) { if (args.stateless_rpc) { /* in stateless RPC mode we use pkt-line to read @@ -1015,17 +1012,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) break; if (line[n-1] == '\n') n--; - ALLOC_GROW(heads, nr_heads + 1, alloc_heads); - heads[nr_heads++] = xmemdupz(line, n); + string_list_append(&sought, xmemdupz(line, n)); } } else { /* read from stdin one ref per line, until EOF */ struct strbuf line = STRBUF_INIT; - while (strbuf_getline(&line, stdin, '\n') != EOF) { - ALLOC_GROW(heads, nr_heads + 1, alloc_heads); - heads[nr_heads++] = strbuf_detach(&line, NULL); - } + while (strbuf_getline(&line, stdin, '\n') != EOF) + string_list_append(&sought, strbuf_detach(&line, NULL)); strbuf_release(&line); } } @@ -1042,7 +1036,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) get_remote_heads(fd[0], &ref, 0, NULL); ref = fetch_pack(&args, fd, conn, ref, dest, - nr_heads, heads, pack_lockfile_ptr); + &sought, pack_lockfile_ptr); if (pack_lockfile) { printf("lock %s\n", pack_lockfile); fflush(stdout); @@ -1053,17 +1047,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) ref = NULL; ret = !ref; - if (!ret && nr_heads) { + if (!ret && sought.nr) { /* If the heads to pull were given, we should have * consumed all of them by matching the remote. * Otherwise, 'git fetch remote no-such-ref' would * silently succeed without issuing an error. */ - for (i = 0; i < nr_heads; i++) - if (heads[i] && heads[i][0]) { - error("no such remote ref %s", heads[i]); + for (i = 0; i < sought.nr; i++) { + char *s = sought.items[i].string; + if (s && s[0]) { + error("no such remote ref %s", s); ret = 1; } + } } while (ref) { printf("%s %s\n", @@ -1074,17 +1070,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) return ret; } -static int compare_heads(const void *a, const void *b) -{ - return strcmp(*(const char **)a, *(const char **)b); -} - struct ref *fetch_pack(struct fetch_pack_args *my_args, int fd[], struct child_process *conn, const struct ref *ref, const char *dest, - int nr_heads, - char **heads, + struct string_list *sought, char **pack_lockfile) { struct stat st; @@ -1098,16 +1088,16 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args, st.st_mtime = 0; } - if (heads && nr_heads) { - qsort(heads, nr_heads, sizeof(*heads), compare_heads); - nr_heads = remove_duplicates(nr_heads, heads); + if (sought->nr) { + sort_string_list(sought); + remove_duplicates(sought); } if (!ref) { packet_flush(fd[1]); die("no matching remote head"); } - ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile); + ref_cpy = do_fetch_pack(fd, ref, sought, pack_lockfile); if (args.depth > 0) { struct cache_time mtime; diff --git a/fetch-pack.h b/fetch-pack.h index 1dbe90fa43..a6a8a73311 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -1,6 +1,8 @@ #ifndef FETCH_PACK_H #define FETCH_PACK_H +#include "string-list.h" + struct fetch_pack_args { const char *uploadpack; int unpacklimit; @@ -21,8 +23,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args, int fd[], struct child_process *conn, const struct ref *ref, const char *dest, - int nr_heads, - char **heads, + struct string_list *sought, char **pack_lockfile); #endif diff --git a/transport.c b/transport.c index 1811b500d9..a847bf31e2 100644 --- a/transport.c +++ b/transport.c @@ -518,8 +518,8 @@ static int fetch_refs_via_pack(struct transport *transport, int nr_heads, struct ref **to_fetch) { struct git_transport_data *data = transport->data; - char **heads = xmalloc(nr_heads * sizeof(*heads)); - char **origh = xmalloc(nr_heads * sizeof(*origh)); + struct string_list orig_sought = STRING_LIST_INIT_DUP; + struct string_list sought = STRING_LIST_INIT_NODUP; const struct ref *refs; char *dest = xstrdup(transport->url); struct fetch_pack_args args; @@ -537,8 +537,10 @@ static int fetch_refs_via_pack(struct transport *transport, args.no_progress = !transport->progress; args.depth = data->options.depth; - for (i = 0; i < nr_heads; i++) - origh[i] = heads[i] = xstrdup(to_fetch[i]->name); + for (i = 0; i < nr_heads; i++) { + string_list_append(&orig_sought, to_fetch[i]->name); + string_list_append(&sought, orig_sought.items[orig_sought.nr - 1].string); + } if (!data->got_remote_heads) { connect_setup(transport, 0, 0); @@ -548,7 +550,7 @@ static int fetch_refs_via_pack(struct transport *transport, refs = fetch_pack(&args, data->fd, data->conn, refs_tmp ? refs_tmp : transport->remote_refs, - dest, nr_heads, heads, &transport->pack_lockfile); + dest, &sought, &transport->pack_lockfile); close(data->fd[0]); close(data->fd[1]); if (finish_connect(data->conn)) @@ -558,10 +560,8 @@ static int fetch_refs_via_pack(struct transport *transport, free_refs(refs_tmp); - for (i = 0; i < nr_heads; i++) - free(origh[i]); - free(origh); - free(heads); + string_list_clear(&sought, 0); + string_list_clear(&orig_sought, 0); free(dest); return (refs ? 0 : -1); } From 382a96711470489ea6e4e277accd3afc46526bfc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:41 +0200 Subject: [PATCH 06/15] filter_refs(): do not check the same sought_pos twice Once a match has been found at sought_pos, the entry is zeroed and no future attempts will match that entry. So increment sought_pos to avoid checking against the zeroed-out entry during the next iteration. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index df81995eab..63d455ffef 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -565,8 +565,8 @@ static void filter_refs(struct ref **refs, struct string_list *sought) if (cmp < 0) /* definitely do not have it */ break; else if (cmp == 0) { /* definitely have it */ - sought->items[sought_pos].string[0] = '\0'; return_refs[sought_pos] = ref; + sought->items[sought_pos++].string[0] = '\0'; break; } else /* might have it; keep looking */ From 4c58f13ba67f9dbe67a351de61703a2db20712cc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:42 +0200 Subject: [PATCH 07/15] fetch_pack(): update sought->nr to reflect number of unique entries fetch_pack() removes duplicates from the "sought" list, thereby shrinking the list. But previously, the caller was not informed about the shrinkage. This would cause a spurious error message to be emitted by cmd_fetch_pack() if "git fetch-pack" is called with duplicate refnames. Instead, remove duplicates using string_list_remove_duplicates(), which adjusts sought->nr to reflect the new length of the list. The last test of t5500 inexplicably *required* "git fetch-pack" to fail when fetching a list of references that contains duplicates; i.e., it insisted on the buggy behavior. So change the test to expect the correct behavior. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 15 +-------------- t/t5500-fetch-pack.sh | 2 +- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 63d455ffef..6cd734a9cb 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -858,19 +858,6 @@ static struct ref *do_fetch_pack(int fd[2], return ref; } -static int remove_duplicates(struct string_list *sought) -{ - int src, dst; - - if (!sought->nr) - return 0; - - for (src = dst = 1; src < sought->nr; src++) - if (strcmp(sought->items[src].string, sought->items[dst-1].string)) - sought->items[dst++] = sought->items[src]; - return dst; -} - static int fetch_pack_config(const char *var, const char *value, void *cb) { if (strcmp(var, "fetch.unpacklimit") == 0) { @@ -1090,7 +1077,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args, if (sought->nr) { sort_string_list(sought); - remove_duplicates(sought); + string_list_remove_duplicates(sought, 0); } if (!ref) { diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 15d277f158..acd41e8576 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -391,7 +391,7 @@ test_expect_success 'fetch mixed refs from cmdline and stdin' ' test_expect_success 'test duplicate refs from stdin' ' ( cd client && - test_must_fail git fetch-pack --stdin --no-progress .. <../input.dup + git fetch-pack --stdin --no-progress .. <../input.dup ) >output && cut -d " " -f 2 actual && test_cmp expect actual From 4ba159996f6c1b0d6dd0a2a8bd9d6f5b342a4aa5 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:43 +0200 Subject: [PATCH 08/15] filter_refs(): delete matched refs from sought list Remove any references that are available from the remote from the sought list (rather than overwriting their names with NUL characters, as previously). Mark matching entries by writing a non-NULL pointer to string_list_item::util during the iteration, then use filter_string_list() later to filter out the entries that have been marked. Document this aspect of fetch_pack() in a comment in the header file. (More documentation is obviously still needed.) Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 23 +++++++++++++++-------- fetch-pack.h | 7 +++++++ transport.c | 10 +++------- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 6cd734a9cb..12ba009c12 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -525,6 +525,16 @@ static void mark_recent_complete_commits(unsigned long cutoff) } } +static int non_matching_ref(struct string_list_item *item, void *unused) +{ + if (item->util) { + item->util = NULL; + return 0; + } + else + return 1; +} + static void filter_refs(struct ref **refs, struct string_list *sought) { struct ref **return_refs; @@ -566,7 +576,7 @@ static void filter_refs(struct ref **refs, struct string_list *sought) break; else if (cmp == 0) { /* definitely have it */ return_refs[sought_pos] = ref; - sought->items[sought_pos++].string[0] = '\0'; + sought->items[sought_pos++].util = "matched"; break; } else /* might have it; keep looking */ @@ -590,6 +600,7 @@ static void filter_refs(struct ref **refs, struct string_list *sought) } if (return_refs != fastarray) free(return_refs); + filter_string_list(sought, 0, non_matching_ref, NULL); } *refs = newlist; } @@ -1040,13 +1051,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) * Otherwise, 'git fetch remote no-such-ref' would * silently succeed without issuing an error. */ - for (i = 0; i < sought.nr; i++) { - char *s = sought.items[i].string; - if (s && s[0]) { - error("no such remote ref %s", s); - ret = 1; - } - } + for (i = 0; i < sought.nr; i++) + error("no such remote ref %s", sought.items[i].string); + ret = 1; } while (ref) { printf("%s %s\n", diff --git a/fetch-pack.h b/fetch-pack.h index a6a8a73311..cb148719bf 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -19,6 +19,13 @@ struct fetch_pack_args { stateless_rpc:1; }; +/* + * sought contains the full names of remote references that should be + * updated from. On return, the names that were found on the remote + * will have been removed from the list. The util members of the + * string_list_items are used internally; they must be NULL on entry + * (and will be NULL on exit). + */ struct ref *fetch_pack(struct fetch_pack_args *args, int fd[], struct child_process *conn, const struct ref *ref, diff --git a/transport.c b/transport.c index a847bf31e2..9932f402df 100644 --- a/transport.c +++ b/transport.c @@ -518,8 +518,7 @@ static int fetch_refs_via_pack(struct transport *transport, int nr_heads, struct ref **to_fetch) { struct git_transport_data *data = transport->data; - struct string_list orig_sought = STRING_LIST_INIT_DUP; - struct string_list sought = STRING_LIST_INIT_NODUP; + struct string_list sought = STRING_LIST_INIT_DUP; const struct ref *refs; char *dest = xstrdup(transport->url); struct fetch_pack_args args; @@ -537,10 +536,8 @@ static int fetch_refs_via_pack(struct transport *transport, args.no_progress = !transport->progress; args.depth = data->options.depth; - for (i = 0; i < nr_heads; i++) { - string_list_append(&orig_sought, to_fetch[i]->name); - string_list_append(&sought, orig_sought.items[orig_sought.nr - 1].string); - } + for (i = 0; i < nr_heads; i++) + string_list_append(&sought, to_fetch[i]->name); if (!data->got_remote_heads) { connect_setup(transport, 0, 0); @@ -561,7 +558,6 @@ static int fetch_refs_via_pack(struct transport *transport, free_refs(refs_tmp); string_list_clear(&sought, 0); - string_list_clear(&orig_sought, 0); free(dest); return (refs ? 0 : -1); } From 5096e4875303038ffc34af96c28cf2a2b7e103a6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:44 +0200 Subject: [PATCH 09/15] filter_refs(): build refs list as we go Instead of temporarily storing matched refs to temporary array "return_refs", simply append them to newlist as we go. This changes the order of references in newlist to strictly sorted if "--all" and "--depth" and named references are all specified, but that usage is broken anyway (see the last two tests in t5500). This changes the last test in t5500 from segfaulting into just emitting a spurious error (this will be fixed in a moment). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 12ba009c12..4e94aa4635 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -537,24 +537,11 @@ static int non_matching_ref(struct string_list_item *item, void *unused) static void filter_refs(struct ref **refs, struct string_list *sought) { - struct ref **return_refs; struct ref *newlist = NULL; struct ref **newtail = &newlist; struct ref *ref, *next; - struct ref *fastarray[32]; int sought_pos; - if (sought->nr && !args.fetch_all) { - if (ARRAY_SIZE(fastarray) < sought->nr) - return_refs = xcalloc(sought->nr, sizeof(struct ref *)); - else { - return_refs = fastarray; - memset(return_refs, 0, sizeof(struct ref *) * sought->nr); - } - } - else - return_refs = NULL; - sought_pos = 0; for (ref = *refs; ref; ref = next) { next = ref->next; @@ -575,8 +562,10 @@ static void filter_refs(struct ref **refs, struct string_list *sought) if (cmp < 0) /* definitely do not have it */ break; else if (cmp == 0) { /* definitely have it */ - return_refs[sought_pos] = ref; sought->items[sought_pos++].util = "matched"; + *newtail = ref; + ref->next = NULL; + newtail = &ref->next; break; } else /* might have it; keep looking */ @@ -588,20 +577,8 @@ static void filter_refs(struct ref **refs, struct string_list *sought) free(ref); } - if (!args.fetch_all) { - int i; - for (i = 0; i < sought->nr; i++) { - ref = return_refs[i]; - if (ref) { - *newtail = ref; - ref->next = NULL; - newtail = &ref->next; - } - } - if (return_refs != fastarray) - free(return_refs); + if (!args.fetch_all) filter_string_list(sought, 0, non_matching_ref, NULL); - } *refs = newlist; } From f537cfa7501de5e4dcdac55a95515e9a2ff880e3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:45 +0200 Subject: [PATCH 10/15] filter_refs(): simplify logic Simplify flow within loop: first decide whether to keep the reference, then keep/free it. This makes it clearer that each ref has exactly two possible destinies, and removes duplication of the code for appending the reference to the linked list. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 43 +++++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 4e94aa4635..056ccb8667 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -544,37 +544,36 @@ static void filter_refs(struct ref **refs, struct string_list *sought) sought_pos = 0; for (ref = *refs; ref; ref = next) { + int keep = 0; next = ref->next; if (!memcmp(ref->name, "refs/", 5) && check_refname_format(ref->name + 5, 0)) ; /* trash */ else if (args.fetch_all && - (!args.depth || prefixcmp(ref->name, "refs/tags/") )) { + (!args.depth || prefixcmp(ref->name, "refs/tags/"))) + keep = 1; + else { + while (sought_pos < sought->nr) { + int cmp = strcmp(ref->name, sought->items[sought_pos].string); + if (cmp < 0) + break; /* definitely do not have it */ + else if (cmp == 0) { + keep = 1; /* definitely have it */ + sought->items[sought_pos++].util = "matched"; + break; + } + else + sought_pos++; /* might have it; keep looking */ + } + } + + if (keep) { *newtail = ref; ref->next = NULL; newtail = &ref->next; - continue; + } else { + free(ref); } - else { - int cmp = -1; - while (sought_pos < sought->nr) { - cmp = strcmp(ref->name, sought->items[sought_pos].string); - if (cmp < 0) /* definitely do not have it */ - break; - else if (cmp == 0) { /* definitely have it */ - sought->items[sought_pos++].util = "matched"; - *newtail = ref; - ref->next = NULL; - newtail = &ref->next; - break; - } - else /* might have it; keep looking */ - sought_pos++; - } - if (!cmp) - continue; /* we will link it later */ - } - free(ref); } if (!args.fetch_all) From 7418f1a0371fd935de6cbe718bbdd567070e9d17 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:46 +0200 Subject: [PATCH 11/15] cmd_fetch_pack(): return early if finish_connect() fails This simplifies the logic without changing the behavior. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 056ccb8667..fb2a423b51 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -1018,10 +1018,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) close(fd[0]); close(fd[1]); if (finish_connect(conn)) - ref = NULL; - ret = !ref; + return 1; - if (!ret && sought.nr) { + ret = !ref; + if (ref && sought.nr) { /* If the heads to pull were given, we should have * consumed all of them by matching the remote. * Otherwise, 'git fetch remote no-such-ref' would From 778e7543d20531bec93d5c9965f32541e78b8e34 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:47 +0200 Subject: [PATCH 12/15] fetch-pack: report missing refs even if no existing refs were received This fixes a test in t5500. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 2 +- t/t5500-fetch-pack.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index fb2a423b51..3d388b53e3 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -1021,7 +1021,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) return 1; ret = !ref; - if (ref && sought.nr) { + if (sought.nr) { /* If the heads to pull were given, we should have * consumed all of them by matching the remote. * Otherwise, 'git fetch remote no-such-ref' would diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index acd41e8576..894d945bdd 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -403,7 +403,7 @@ test_expect_success 'set up tests of missing reference' ' EOF ' -test_expect_failure 'test lonely missing ref' ' +test_expect_success 'test lonely missing ref' ' ( cd client && test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy From b285668dd23952b96d716aac769c0ed1080d11d3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:48 +0200 Subject: [PATCH 13/15] cmd_fetch_pack(): simplify computation of return value Set the final value at initialization rather than initializing it then sometimes changing it. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 3d388b53e3..42078e5cd5 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -1020,17 +1020,16 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) if (finish_connect(conn)) return 1; - ret = !ref; - if (sought.nr) { - /* If the heads to pull were given, we should have - * consumed all of them by matching the remote. - * Otherwise, 'git fetch remote no-such-ref' would - * silently succeed without issuing an error. - */ - for (i = 0; i < sought.nr; i++) - error("no such remote ref %s", sought.items[i].string); - ret = 1; - } + ret = !ref || sought.nr; + + /* + * If the heads to pull were given, we should have consumed + * all of them by matching the remote. Otherwise, 'git fetch + * remote no-such-ref' would silently succeed without issuing + * an error. + */ + for (i = 0; i < sought.nr; i++) + error("no such remote ref %s", sought.items[i].string); while (ref) { printf("%s %s\n", sha1_to_hex(ref->old_sha1), ref->name); From 5f0fc64513c8cb638e0d6010005d8e86c9dbca3a Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Sun, 9 Sep 2012 08:19:49 +0200 Subject: [PATCH 14/15] fetch-pack: eliminate spurious error messages It used to be that if "--all", "--depth", and also explicit references were sought, then the explicit references were not handled correctly in filter_refs() because the "--all --depth" code took precedence over the explicit reference handling, and the explicit references were never noted as having been found. So check for explicitly sought references before proceeding to the "--all --depth" logic. This fixes two test cases in t5500. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 10 +++++----- t/t5500-fetch-pack.sh | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 42078e5cd5..e6443986b8 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -549,9 +549,6 @@ static void filter_refs(struct ref **refs, struct string_list *sought) if (!memcmp(ref->name, "refs/", 5) && check_refname_format(ref->name + 5, 0)) ; /* trash */ - else if (args.fetch_all && - (!args.depth || prefixcmp(ref->name, "refs/tags/"))) - keep = 1; else { while (sought_pos < sought->nr) { int cmp = strcmp(ref->name, sought->items[sought_pos].string); @@ -567,6 +564,10 @@ static void filter_refs(struct ref **refs, struct string_list *sought) } } + if (! keep && args.fetch_all && + (!args.depth || prefixcmp(ref->name, "refs/tags/"))) + keep = 1; + if (keep) { *newtail = ref; ref->next = NULL; @@ -576,8 +577,7 @@ static void filter_refs(struct ref **refs, struct string_list *sought) } } - if (!args.fetch_all) - filter_string_list(sought, 0, non_matching_ref, NULL); + filter_string_list(sought, 0, non_matching_ref, NULL); *refs = newlist; } diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 894d945bdd..6322e8ade8 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -427,14 +427,14 @@ test_expect_success 'test missing ref before existing' ' test_cmp expect-error error-me ' -test_expect_failure 'test --all, --depth, and explicit head' ' +test_expect_success 'test --all, --depth, and explicit head' ' ( cd client && git fetch-pack --no-progress --all --depth=1 .. refs/heads/A ) >out-adh 2>error-adh ' -test_expect_failure 'test --all, --depth, and explicit tag' ' +test_expect_success 'test --all, --depth, and explicit tag' ' git tag OLDTAG refs/heads/B~5 && ( cd client && From 65119878548f869270fbc8d382f042cca3a6ad05 Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Sat, 15 Sep 2012 17:18:42 +0100 Subject: [PATCH 15/15] test-string-list.c: Fix some sparse warnings In particular, sparse complains as follows: SP test-string-list.c test-string-list.c:10:6: warning: symbol 'parse_string_list' was not \ declared. Should it be static? test-string-list.c:18:6: warning: symbol 'write_list' was not \ declared. Should it be static? test-string-list.c:25:6: warning: symbol 'write_list_compact' was not \ declared. Should it be static? test-string-list.c:38:5: warning: symbol 'prefix_cb' was not \ declared. Should it be static? In order to suppress the warnings, since the above symbols do not need more than file scope, we simply include the static modifier in their declaration. Signed-off-by: Ramsay Jones Signed-off-by: Junio C Hamano --- test-string-list.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test-string-list.c b/test-string-list.c index 5e9631fe34..4693295a98 100644 --- a/test-string-list.c +++ b/test-string-list.c @@ -7,7 +7,7 @@ * list (as opposed to "", which indicates a string list containing a * single empty string). list->strdup_strings must be set. */ -void parse_string_list(struct string_list *list, const char *arg) +static void parse_string_list(struct string_list *list, const char *arg) { if (!strcmp(arg, "-")) return; @@ -15,14 +15,14 @@ void parse_string_list(struct string_list *list, const char *arg) (void)string_list_split(list, arg, ':', -1); } -void write_list(const struct string_list *list) +static void write_list(const struct string_list *list) { int i; for (i = 0; i < list->nr; i++) printf("[%d]: \"%s\"\n", i, list->items[i].string); } -void write_list_compact(const struct string_list *list) +static void write_list_compact(const struct string_list *list) { int i; if (!list->nr) @@ -35,7 +35,7 @@ void write_list_compact(const struct string_list *list) } } -int prefix_cb(struct string_list_item *item, void *cb_data) +static int prefix_cb(struct string_list_item *item, void *cb_data) { const char *prefix = (const char *)cb_data; return !prefixcmp(item->string, prefix);