From afe7c5ff1f7f6741d35f9852a766d151da23d7d8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Dec 2011 19:41:37 -0500 Subject: [PATCH 1/4] drop "match" parameter from get_remote_heads The get_remote_heads function reads the list of remote refs during git protocol session. It dates all the way back to def88e9 (Commit first cut at "git-fetch-pack", 2005-07-04). At that time, the idea was to come up with a list of refs we were interested in, and then filter the list as we got it from the remote side. Later, 1baaae5 (Make maximal use of the remote refs, 2005-10-28) stopped filtering at the get_remote_heads layer, letting us use the non-matching refs to find common history. As a result, all callers now simply pass an empty match list (and any future callers will want to do the same). So let's drop these now-useless parameters. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 2 +- builtin/send-pack.c | 3 +-- cache.h | 2 +- connect.c | 3 --- remote-curl.c | 2 +- transport.c | 7 +++---- 6 files changed, 7 insertions(+), 12 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index c6bc8eb0aa..46688dc14b 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -976,7 +976,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) args.verbose ? CONNECT_VERBOSE : 0); } - get_remote_heads(fd[0], &ref, 0, NULL, 0, NULL); + get_remote_heads(fd[0], &ref, 0, NULL); ref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, pack_lockfile_ptr); diff --git a/builtin/send-pack.c b/builtin/send-pack.c index e0b8030f2b..cd1115ffc6 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -494,8 +494,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix) memset(&extra_have, 0, sizeof(extra_have)); - get_remote_heads(fd[0], &remote_refs, 0, NULL, REF_NORMAL, - &extra_have); + get_remote_heads(fd[0], &remote_refs, REF_NORMAL, &extra_have); transport_verify_remote_names(nr_refspecs, refspecs); diff --git a/cache.h b/cache.h index 2e6ad3604e..451960175c 100644 --- a/cache.h +++ b/cache.h @@ -1033,7 +1033,7 @@ struct extra_have_objects { int nr, alloc; unsigned char (*array)[20]; }; -extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *); +extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *); extern int server_supports(const char *feature); extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path); diff --git a/connect.c b/connect.c index 51990fa0cb..48df90bfe5 100644 --- a/connect.c +++ b/connect.c @@ -53,7 +53,6 @@ static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1 * Read all the refs from the other end */ struct ref **get_remote_heads(int in, struct ref **list, - int nr_match, char **match, unsigned int flags, struct extra_have_objects *extra_have) { @@ -92,8 +91,6 @@ struct ref **get_remote_heads(int in, struct ref **list, if (!check_ref(name, name_len, flags)) continue; - if (nr_match && !path_match(name, nr_match, match)) - continue; ref = alloc_ref(buffer + 41); hashcpy(ref->old_sha1, old_sha1); *list = ref; diff --git a/remote-curl.c b/remote-curl.c index 0e720ee8bb..94dc4886d0 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -200,7 +200,7 @@ static struct ref *parse_git_refs(struct discovery *heads) if (start_async(&async)) die("cannot start thread to parse advertised refs"); - get_remote_heads(async.out, &list, 0, NULL, 0, NULL); + get_remote_heads(async.out, &list, 0, NULL); close(async.out); if (finish_async(&async)) die("ref parsing thread failed"); diff --git a/transport.c b/transport.c index 51814b5da3..c2245d4f0d 100644 --- a/transport.c +++ b/transport.c @@ -502,7 +502,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus struct ref *refs; connect_setup(transport, for_push, 0); - get_remote_heads(data->fd[0], &refs, 0, NULL, + get_remote_heads(data->fd[0], &refs, for_push ? REF_NORMAL : 0, &data->extra_have); data->got_remote_heads = 1; @@ -537,7 +537,7 @@ static int fetch_refs_via_pack(struct transport *transport, if (!data->got_remote_heads) { connect_setup(transport, 0, 0); - get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL); + get_remote_heads(data->fd[0], &refs_tmp, 0, NULL); data->got_remote_heads = 1; } @@ -772,8 +772,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re struct ref *tmp_refs; connect_setup(transport, 1, 0); - get_remote_heads(data->fd[0], &tmp_refs, 0, NULL, REF_NORMAL, - NULL); + get_remote_heads(data->fd[0], &tmp_refs, REF_NORMAL, NULL); data->got_remote_heads = 1; } From e9d866e32c0c7411a005cb6d6549d3a89f8102ee Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Dec 2011 19:44:40 -0500 Subject: [PATCH 2/4] t5500: give fully-qualified refs to fetch-pack The fetch-pack documentation is very clear that refs given on the command line are to be full refs: ...:: The remote heads to update from. This is relative to $GIT_DIR (e.g. "HEAD", "refs/heads/master"). When unspecified, update from all heads the remote side has. and this has been the case since fetch-pack was originally documented in 8b3d9dc ([PATCH] Documentation: clone/fetch/upload., 2005-07-14). Let's follow our own documentation to set a good example, and to avoid breaking when this restriction is enforced in the next patch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5500-fetch-pack.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index bafcca765e..9bf69e9a0f 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -97,7 +97,7 @@ test_expect_success 'setup' ' git symbolic-ref HEAD refs/heads/B ' -pull_to_client 1st "B A" $((11*3)) +pull_to_client 1st "refs/heads/B refs/heads/A" $((11*3)) test_expect_success 'post 1st pull setup' ' add A11 $A10 && @@ -110,9 +110,9 @@ test_expect_success 'post 1st pull setup' ' done ' -pull_to_client 2nd "B" $((64*3)) +pull_to_client 2nd "refs/heads/B" $((64*3)) -pull_to_client 3rd "A" $((1*3)) +pull_to_client 3rd "refs/heads/A" $((1*3)) test_expect_success 'clone shallow' ' git clone --depth 2 "file://$(pwd)/." shallow From 1e7ba0f9caf1993491aa4c5cbd796cd31cb0f4af Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Dec 2011 19:48:08 -0500 Subject: [PATCH 3/4] fetch-pack: match refs exactly When we are determining the list of refs to fetch via fetch-pack, we have two sets of refs to compare: those on the remote side, and a "match" list of things we want to fetch. We iterate through the remote refs alphabetically, seeing if each one is wanted by the "match" list. Since def88e9 (Commit first cut at "git-fetch-pack", 2005-07-04), we have used the "path_match" function to do a suffix match, where a remote ref is considered wanted if any of the "match" elements is a suffix of the remote refname. This enables callers of fetch-pack to specify unqualified refs and have them matched up with remote refs (e.g., ask for "A" and get remote's "refs/heads/A"). However, if you provide a fully qualified ref, then there are corner cases where we provide the wrong answer. For example, given a remote with two refs: refs/foo/refs/heads/master refs/heads/master asking for "refs/heads/master" will first match "refs/foo/refs/heads/master" by the suffix rule, and we will erroneously fetch it instead of refs/heads/master. As it turns out, all callers of fetch_pack do provide fully-qualified refs for the match list. There are two ways fetch_pack can get match lists: 1. Through the transport code (i.e., via git-fetch) 2. On the command-line of git-fetch-pack In the first case, we will always be providing the names of fully-qualified refs from "struct ref" objects. We will have pre-matched those ref objects already (since we have to handle more advanced matching, like wildcard refspecs), and are just providing a list of the refs whose objects we need. In the second case, users could in theory be providing non-qualified refs on the command-line. However, the fetch-pack documentation claims that refs should be fully qualified (and has always done so since it was written in 2005). Let's change this path_match call to simply check for string equality, matching what the callers of fetch_pack are expecting. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 13 +++++++++---- t/t5527-fetch-odd-refs.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) create mode 100755 t/t5527-fetch-odd-refs.sh diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 46688dc14b..6207ecd298 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -556,11 +556,16 @@ static void filter_refs(struct ref **refs, int nr_match, char **match) continue; } else { - int order = path_match(ref->name, nr_match, match); - if (order) { - return_refs[order-1] = ref; - continue; /* we will link it later */ + int i; + for (i = 0; i < nr_match; i++) { + if (!strcmp(ref->name, match[i])) { + match[i][0] = '\0'; + return_refs[i] = ref; + break; + } } + if (i < nr_match) + continue; /* we will link it later */ } free(ref); } diff --git a/t/t5527-fetch-odd-refs.sh b/t/t5527-fetch-odd-refs.sh new file mode 100755 index 0000000000..edea9f957e --- /dev/null +++ b/t/t5527-fetch-odd-refs.sh @@ -0,0 +1,29 @@ +#!/bin/sh + +test_description='test fetching of oddly-named refs' +. ./test-lib.sh + +# afterwards we will have: +# HEAD - two +# refs/for/refs/heads/master - one +# refs/heads/master - three +test_expect_success 'setup repo with odd suffix ref' ' + echo content >file && + git add . && + git commit -m one && + git update-ref refs/for/refs/heads/master HEAD && + echo content >>file && + git commit -a -m two && + echo content >>file && + git commit -a -m three && + git checkout HEAD^ +' + +test_expect_success 'suffix ref is ignored during fetch' ' + git clone --bare file://"$PWD" suffix && + echo three >expect && + git --git-dir=suffix log -1 --format=%s refs/heads/master >actual && + test_cmp expect actual +' + +test_done From bab8d28e774c255a326ad5592af6351e4925efcb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Dec 2011 19:49:59 -0500 Subject: [PATCH 4/4] connect.c: drop path_match function This function was used for comparing local and remote ref names during fetch (which makes it a candidate for "most confusingly named function of the year"). It no longer has any callers, so let's get rid of it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 1 - connect.c | 21 --------------------- 2 files changed, 22 deletions(-) diff --git a/cache.h b/cache.h index 451960175c..79c612fc2f 100644 --- a/cache.h +++ b/cache.h @@ -1028,7 +1028,6 @@ extern char *git_getpass(const char *prompt); extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags); extern int finish_connect(struct child_process *conn); extern int git_connection_is_socket(struct child_process *conn); -extern int path_match(const char *path, int nr, char **match); struct extra_have_objects { int nr, alloc; unsigned char (*array)[20]; diff --git a/connect.c b/connect.c index 48df90bfe5..2a0a0401af 100644 --- a/connect.c +++ b/connect.c @@ -105,27 +105,6 @@ int server_supports(const char *feature) strstr(server_capabilities, feature) != NULL; } -int path_match(const char *path, int nr, char **match) -{ - int i; - int pathlen = strlen(path); - - for (i = 0; i < nr; i++) { - char *s = match[i]; - int len = strlen(s); - - if (!len || len > pathlen) - continue; - if (memcmp(path + pathlen - len, s, len)) - continue; - if (pathlen > len && path[pathlen - len - 1] != '/') - continue; - *s = 0; - return (i + 1); - } - return 0; -} - enum protocol { PROTO_LOCAL = 1, PROTO_SSH,