From 389526043a277afec9dd8b8dcd9aa97c4b3f2d6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 14 Jul 2014 20:29:53 +0200 Subject: [PATCH 1/9] remote: restrict default branch to branches namespace --- src/remote.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/remote.c b/src/remote.c index 433015f60..8d35fd69a 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1971,6 +1971,9 @@ int git_remote_default_branch(git_buf *out, git_remote *remote) if (git_oid_cmp(head_id, &heads[i]->oid)) continue; + if (git__prefixcmp(heads[i]->name, GIT_REFS_HEADS_DIR)) + continue; + if (!guess) { guess = heads[i]; continue; From 26bf3a53467cd4e7adbf70c3ec8f2d83ed82a605 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sun, 10 Aug 2014 17:13:00 +0200 Subject: [PATCH 2/9] travis: no need to clean out the test repository This was added to avoid the remote's default branch to be considered to the be notes one which the first network test leaves behind. --- script/cibuild.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/script/cibuild.sh b/script/cibuild.sh index 981f95b7e..e58c0849c 100755 --- a/script/cibuild.sh +++ b/script/cibuild.sh @@ -41,7 +41,5 @@ export GITTEST_REMOTE_SSH_PASSPHRASE="" if [ -e ./libgit2_clar ]; then ./libgit2_clar -sonline::push -sonline::clone::cred_callback && - rm -rf $HOME/_temp/test.git && - git init --bare $HOME/_temp/test.git && # create an empty one ./libgit2_clar -sonline::clone::ssh_with_paths fi From 94412b009e81f0fdd474b0a0848f117cac3bd3b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sun, 10 Aug 2014 17:48:12 +0200 Subject: [PATCH 3/9] remote: assert what we want to happen when ther is no default branch Assert what we already do, so as to notice changes. --- tests/network/remote/defaultbranch.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/network/remote/defaultbranch.c b/tests/network/remote/defaultbranch.c index fa3a329db..85735a378 100644 --- a/tests/network/remote/defaultbranch.c +++ b/tests/network/remote/defaultbranch.c @@ -48,3 +48,17 @@ void test_network_remote_defaultbranch__master_on_detached(void) cl_git_pass(git_repository_detach_head(g_repo_a, NULL, NULL)); assert_default_branch("refs/heads/master"); } + +void test_network_remote_defaultbranch__no_default_branch(void) +{ + git_remote *remote_b; + const git_remote_head **heads; + size_t len; + + cl_git_pass(git_remote_create(&remote_b, g_repo_b, "self", git_repository_path(g_repo_b))); + cl_git_pass(git_remote_connect(remote_b, GIT_DIRECTION_FETCH)); + cl_git_pass(git_remote_ls(&heads, &len, remote_b)); + cl_assert_equal_i(0, len); + + git_remote_free(remote_b); +} From 0cdaa3766a772e65dcbc491a2d671b6169e97c60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sun, 10 Aug 2014 17:50:46 +0200 Subject: [PATCH 4/9] remote: short-circuit the default branch check if there is none If we do not have a HEAD ref in the heads, we already know there is no default branch. Return immedately. --- src/remote.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/remote.c b/src/remote.c index 8d35fd69a..fa5ec8b4c 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1955,6 +1955,9 @@ int git_remote_default_branch(git_buf *out, git_remote *remote) if (heads_len == 0) return GIT_ENOTFOUND; + if (strcmp(heads[0]->name, GIT_HEAD_FILE)) + return GIT_ENOTFOUND; + git_buf_sanitize(out); /* the first one must be HEAD so if that has the symref info, we're done */ if (heads[0]->symref_target) From 46254467cf1a092be899fb522f0560005b6111b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 29 Aug 2014 15:10:15 +0200 Subject: [PATCH 5/9] clone: support remotes with references but no branches A repository can have any number of references which we're not interested in such as notes or tags. For the default branch calculation we only care about branches. Make the decision about the number of branches rather than the number of refs in general. --- src/clone.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/clone.c b/src/clone.c index 7835be1b1..3dce67de1 100644 --- a/src/clone.c +++ b/src/clone.c @@ -138,6 +138,23 @@ static int update_head_to_new_branch( return error; } +/** + * Check whether there are any branches among the listed + * references. It's possible for a repository to have a long list of + * references without us downloading any of them. + */ +static bool remote_has_branches(const git_remote_head **refs, size_t len) +{ + size_t i; + + for (i = 0; i < len; i++) { + if (!git__prefixcmp(refs[i]->name, GIT_REFS_HEADS_DIR)) + return true; + } + + return false; +} + static int update_head_to_remote( git_repository *repo, git_remote *remote, @@ -156,7 +173,7 @@ static int update_head_to_remote( return error; /* Did we just clone an empty repository? */ - if (refs_len == 0) + if (!remote_has_branches(refs, refs_len)) return setup_tracking_config( repo, "master", GIT_REMOTE_ORIGIN, GIT_REFS_HEADS_MASTER_FILE); @@ -167,7 +184,7 @@ static int update_head_to_remote( found_branch = 1; } - /* Get the remote's HEAD. This is always the first ref in the list. */ + /* Get the remote's HEAD. This is always the first ref in the list if it exists */ remote_head = refs[0]; assert(remote_head); From f2ffab618ad536bbd0da15d191bd9c488916761a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 1 Sep 2014 15:59:36 +0200 Subject: [PATCH 6/9] remote: add tests for remote-branch edge cases Add tests for the case when there are no branches on the remote and when HEAD is detached but has the id of a non-branch. In both of these cases, we should return ENOTFOUND. --- tests/network/remote/defaultbranch.c | 29 ++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/network/remote/defaultbranch.c b/tests/network/remote/defaultbranch.c index 85735a378..4a49bd89d 100644 --- a/tests/network/remote/defaultbranch.c +++ b/tests/network/remote/defaultbranch.c @@ -54,11 +54,40 @@ void test_network_remote_defaultbranch__no_default_branch(void) git_remote *remote_b; const git_remote_head **heads; size_t len; + git_buf buf = GIT_BUF_INIT; cl_git_pass(git_remote_create(&remote_b, g_repo_b, "self", git_repository_path(g_repo_b))); cl_git_pass(git_remote_connect(remote_b, GIT_DIRECTION_FETCH)); cl_git_pass(git_remote_ls(&heads, &len, remote_b)); cl_assert_equal_i(0, len); + cl_git_fail_with(GIT_ENOTFOUND, git_remote_default_branch(&buf, remote_b)); + git_remote_free(remote_b); } + +void test_network_remote_defaultbranch__detached_sharing_nonbranch_id(void) +{ + git_oid id, id_cloned; + git_reference *ref; + git_buf buf = GIT_BUF_INIT; + git_repository *cloned_repo; + + cl_git_pass(git_reference_name_to_id(&id, g_repo_a, "HEAD")); + cl_git_pass(git_repository_detach_head(g_repo_a, NULL, NULL)); + cl_git_pass(git_reference_remove(g_repo_a, "refs/heads/master")); + cl_git_pass(git_reference_remove(g_repo_a, "refs/heads/not-good")); + cl_git_pass(git_reference_create(&ref, g_repo_a, "refs/foo/bar", &id, 1, NULL, NULL)); + git_reference_free(ref); + + cl_git_pass(git_remote_connect(g_remote, GIT_DIRECTION_FETCH)); + cl_git_fail_with(GIT_ENOTFOUND, git_remote_default_branch(&buf, g_remote)); + + cl_git_pass(git_clone(&cloned_repo, git_repository_path(g_repo_a), "./local-detached", NULL)); + + cl_assert(git_repository_head_detached(cloned_repo)); + cl_git_pass(git_reference_name_to_id(&id_cloned, g_repo_a, "HEAD")); + cl_assert(git_oid_equal(&id, &id_cloned)); + + git_repository_free(cloned_repo); +} From 538f908175a0912e0119f0ea8935e2d2b2dc416c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 1 Sep 2014 16:35:10 +0200 Subject: [PATCH 7/9] remote: add test for single-branch clone When cloning, we may be asking for a particular branch or subset of branches. Make sure we test for that. --- tests/network/remote/remotes.c | 61 ++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/network/remote/remotes.c b/tests/network/remote/remotes.c index 21c57119a..4e274d889 100644 --- a/tests/network/remote/remotes.c +++ b/tests/network/remote/remotes.c @@ -511,3 +511,64 @@ void test_network_remote_remotes__query_refspecs(void) git_remote_free(remote); } + +static int remote_single_branch(git_remote **out, git_repository *repo, const char *name, const char *url, void *payload) +{ + char *fetch_refspecs[] = { + "refs/heads/first-merge:refs/remotes/origin/first-merge", + }; + git_strarray fetch_refspecs_strarray = { + fetch_refspecs, + 1, + }; + + GIT_UNUSED(payload); + + cl_git_pass(git_remote_create(out, repo, name, url)); + cl_git_pass(git_remote_set_fetch_refspecs(*out, &fetch_refspecs_strarray)); + + return 0; +} + +void test_network_remote_remotes__single_branch(void) +{ + git_clone_options opts = GIT_CLONE_OPTIONS_INIT; + git_repository *repo; + git_strarray refs; + size_t i, count = 0; + + opts.remote_cb = remote_single_branch; + opts.checkout_branch = "first-merge"; + + cl_git_pass(git_clone(&repo, "git://github.com/libgit2/TestGitRepository", "./single-branch", &opts)); + cl_git_pass(git_reference_list(&refs, repo)); + + for (i = 0; i < refs.count; i++) { + if (!git__prefixcmp(refs.strings[i], "refs/heads/")) + count++; + } + cl_assert_equal_i(1, count); + + git_repository_free(repo); +} + +void test_network_remote_remotes__restricted_refspecs(void) +{ + git_clone_options opts = GIT_CLONE_OPTIONS_INIT; + git_repository *repo; + git_strarray refs; + size_t i, count = 0; + + opts.remote_cb = remote_single_branch; + + cl_git_pass(git_clone(&repo, "git://github.com/libgit2/TestGitRepository", "./restrict-refspec", &opts)); + cl_git_pass(git_reference_list(&refs, repo)); + + for (i = 0; i < refs.count; i++) { + if (!git__prefixcmp(refs.strings[i], "refs/heads/")) + count++; + } + cl_assert_equal_i(1, count); + + git_repository_free(repo); +} From e128a1af6ec552f880051b67d72ab0a4c4c7d074 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 2 Sep 2014 13:10:19 +0200 Subject: [PATCH 8/9] clone: correct handling of an unborn HEAD If the remote does not advertise HEAD, then it is unborn and we cannot checkout that branch. Handle it the same way as an empty repo. --- src/clone.c | 21 ++------------------- tests/network/remote/defaultbranch.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/clone.c b/src/clone.c index 3dce67de1..4472651da 100644 --- a/src/clone.c +++ b/src/clone.c @@ -138,23 +138,6 @@ static int update_head_to_new_branch( return error; } -/** - * Check whether there are any branches among the listed - * references. It's possible for a repository to have a long list of - * references without us downloading any of them. - */ -static bool remote_has_branches(const git_remote_head **refs, size_t len) -{ - size_t i; - - for (i = 0; i < len; i++) { - if (!git__prefixcmp(refs[i]->name, GIT_REFS_HEADS_DIR)) - return true; - } - - return false; -} - static int update_head_to_remote( git_repository *repo, git_remote *remote, @@ -172,8 +155,8 @@ static int update_head_to_remote( if ((error = git_remote_ls(&refs, &refs_len, remote)) < 0) return error; - /* Did we just clone an empty repository? */ - if (!remote_has_branches(refs, refs_len)) + /* We cloned an empty repository or one with an unborn HEAD */ + if (refs_len == 0 || strcmp(refs[0]->name, GIT_HEAD_FILE)) return setup_tracking_config( repo, "master", GIT_REMOTE_ORIGIN, GIT_REFS_HEADS_MASTER_FILE); diff --git a/tests/network/remote/defaultbranch.c b/tests/network/remote/defaultbranch.c index 4a49bd89d..243369fa2 100644 --- a/tests/network/remote/defaultbranch.c +++ b/tests/network/remote/defaultbranch.c @@ -91,3 +91,18 @@ void test_network_remote_defaultbranch__detached_sharing_nonbranch_id(void) git_repository_free(cloned_repo); } + +void test_network_remote_defaultbranch__unborn_HEAD_with_branches(void) +{ + git_reference *ref; + git_repository *cloned_repo; + + cl_git_pass(git_reference_symbolic_create(&ref, g_repo_a, "HEAD", "refs/heads/i-dont-exist", 1, NULL, NULL)); + git_reference_free(ref); + + cl_git_pass(git_clone(&cloned_repo, git_repository_path(g_repo_a), "./semi-empty", NULL)); + + cl_assert(git_repository_head_unborn(cloned_repo)); + + git_repository_free(cloned_repo); +} From 15c30b72e16528bdf71c0343e4d238600c0df7a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 2 Sep 2014 13:23:54 +0200 Subject: [PATCH 9/9] clone: handle overly restrictive refspecs When the fetch refspec does not include the remote's default branch, it indicates an error in user expectations or programmer error. Error out in that case. This lets us get rid of the dummy refspec which can never work as its zeroed out. In the cases where we did not find a default branch, we set HEAD detached immediately, which lets us refactor the "normal" path, removing `found_branch`. --- src/clone.c | 45 +++++++++++++++++----------------- tests/network/remote/remotes.c | 13 +--------- 2 files changed, 23 insertions(+), 35 deletions(-) diff --git a/src/clone.c b/src/clone.c index 4472651da..43b839003 100644 --- a/src/clone.c +++ b/src/clone.c @@ -144,9 +144,9 @@ static int update_head_to_remote( const git_signature *signature, const char *reflog_message) { - int error = 0, found_branch = 0; + int error = 0; size_t refs_len; - git_refspec dummy_spec, *refspec; + git_refspec *refspec; const git_remote_head *remote_head, **refs; const git_oid *remote_head_id; git_buf remote_master_name = GIT_BUF_INIT; @@ -160,23 +160,25 @@ static int update_head_to_remote( return setup_tracking_config( repo, "master", GIT_REMOTE_ORIGIN, GIT_REFS_HEADS_MASTER_FILE); - error = git_remote_default_branch(&branch, remote); - if (error == GIT_ENOTFOUND) { - git_buf_puts(&branch, GIT_REFS_HEADS_MASTER_FILE); - } else { - found_branch = 1; - } - - /* Get the remote's HEAD. This is always the first ref in the list if it exists */ + /* We know we have HEAD, let's see where it points */ remote_head = refs[0]; assert(remote_head); remote_head_id = &remote_head->oid; + + error = git_remote_default_branch(&branch, remote); + if (error == GIT_ENOTFOUND) { + error = git_repository_set_head_detached( + repo, remote_head_id, signature, reflog_message); + goto cleanup; + } + refspec = git_remote__matching_refspec(remote, git_buf_cstr(&branch)); if (refspec == NULL) { - memset(&dummy_spec, 0, sizeof(git_refspec)); - refspec = &dummy_spec; + giterr_set(GITERR_NET, "the remote's default branch does not fit the refspec configuration"); + error = GIT_EINVALIDSPEC; + goto cleanup; } /* Determine the remote tracking reference name from the local master */ @@ -184,21 +186,18 @@ static int update_head_to_remote( &remote_master_name, refspec, git_buf_cstr(&branch))) < 0) - return error; + goto cleanup; - if (found_branch) { - error = update_head_to_new_branch( - repo, - remote_head_id, - git_buf_cstr(&branch), - signature, reflog_message); - } else { - error = git_repository_set_head_detached( - repo, remote_head_id, signature, reflog_message); - } + error = update_head_to_new_branch( + repo, + remote_head_id, + git_buf_cstr(&branch), + signature, reflog_message); +cleanup: git_buf_free(&remote_master_name); git_buf_free(&branch); + return error; } diff --git a/tests/network/remote/remotes.c b/tests/network/remote/remotes.c index 4e274d889..5fe5143f5 100644 --- a/tests/network/remote/remotes.c +++ b/tests/network/remote/remotes.c @@ -556,19 +556,8 @@ void test_network_remote_remotes__restricted_refspecs(void) { git_clone_options opts = GIT_CLONE_OPTIONS_INIT; git_repository *repo; - git_strarray refs; - size_t i, count = 0; opts.remote_cb = remote_single_branch; - cl_git_pass(git_clone(&repo, "git://github.com/libgit2/TestGitRepository", "./restrict-refspec", &opts)); - cl_git_pass(git_reference_list(&refs, repo)); - - for (i = 0; i < refs.count; i++) { - if (!git__prefixcmp(refs.strings[i], "refs/heads/")) - count++; - } - cl_assert_equal_i(1, count); - - git_repository_free(repo); + cl_git_fail_with(GIT_EINVALIDSPEC, git_clone(&repo, "git://github.com/libgit2/TestGitRepository", "./restrict-refspec", &opts)); }