From fe3b9d07317732830a3416187e2946979caafc92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jun 2014 00:54:11 +0200 Subject: remote: failing test for rename When there is a reference in the target namespace, we should overwrite it. Instead it gets a different name under the current code. --- tests/network/remote/rename.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/network/remote/rename.c b/tests/network/remote/rename.c index 4d8628425..b7ec44726 100644 --- a/tests/network/remote/rename.c +++ b/tests/network/remote/rename.c @@ -172,3 +172,33 @@ void test_network_remote_rename__cannot_rename_an_inmemory_remote(void) git_remote_free(remote); } + +void test_network_remote_rename__overwrite_ref_in_target(void) +{ + git_oid id; + char idstr[GIT_OID_HEXSZ + 1] = {0}; + git_remote *remote; + git_reference *ref; + git_branch_t btype; + git_branch_iterator *iter; + + cl_git_pass(git_oid_fromstr(&id, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750")); + cl_git_pass(git_reference_create(&ref, _repo, "refs/remotes/renamed/master", &id, 1, NULL, NULL)); + git_reference_free(ref); + + cl_git_pass(git_remote_load(&remote, _repo, "test")); + cl_git_pass(git_remote_rename(remote, "renamed", dont_call_me_cb, NULL)); + git_remote_free(remote); + + + /* make sure there's only one remote-tracking branch */ + cl_git_pass(git_branch_iterator_new(&iter, _repo, GIT_BRANCH_REMOTE)); + cl_git_pass(git_branch_next(&ref, &btype, iter)); + cl_assert_equal_s("refs/remotes/renamed/master", git_reference_name(ref)); + git_oid_fmt(idstr, git_reference_target(ref)); + cl_assert_equal_s("be3563ae3f795b2b4353bcce3a527ad0a4f7f644", idstr); + git_reference_free(ref); + + cl_git_fail_with(GIT_ITEROVER, git_branch_next(&ref, &btype, iter)); + git_branch_iterator_free(iter); +} -- cgit v1.2.1 From a52ab4b82a06f3a4e6065ecfa1b27917b4ba216b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jun 2014 01:09:49 +0200 Subject: remote: tighten up reference renaming Tighten up which references we consider for renaming so we don't try to rename unrelated ones and end up with unexplained references. If there is a reference on the target namespace, git overwrites it, so let's do the same. --- src/remote.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/remote.c b/src/remote.c index 0c82433d1..0d1a88ea7 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1380,7 +1380,7 @@ static int rename_one_remote_reference( goto cleanup; error = git_reference_rename( - NULL, reference, git_buf_cstr(&new_name), 0, + NULL, reference, git_buf_cstr(&new_name), 1, NULL, git_buf_cstr(&log_message)); git_reference_free(reference); @@ -1396,18 +1396,20 @@ static int rename_remote_references( const char *new_name) { int error; + git_buf buf = GIT_BUF_INIT; git_reference *ref; git_reference_iterator *iter; - if ((error = git_reference_iterator_new(&iter, repo)) < 0) + if ((error = git_buf_printf(&buf, GIT_REFS_REMOTES_DIR "%s/*", old_name)) < 0) return error; - while ((error = git_reference_next(&ref, iter)) == 0) { - if (git__prefixcmp(ref->name, GIT_REFS_REMOTES_DIR)) { - git_reference_free(ref); - continue; - } + error = git_reference_iterator_glob_new(&iter, repo, git_buf_cstr(&buf)); + git_buf_free(&buf); + if (error < 0) + return error; + + while ((error = git_reference_next(&ref, iter)) == 0) { if ((error = rename_one_remote_reference(ref, old_name, new_name)) < 0) break; } -- cgit v1.2.1 From 05554d839d8068d7da37a53655bd065569306763 Mon Sep 17 00:00:00 2001 From: Arthur Schreiber Date: Fri, 6 Jun 2014 11:01:20 +0200 Subject: Update AUTHORS Add me. :) --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index 6854ed016..2eaec0ff6 100644 --- a/AUTHORS +++ b/AUTHORS @@ -6,6 +6,7 @@ Alexei Sholik Andreas Ericsson Anton "antong" Gyllenberg Ankur Sethi +Arthur Schreiber Ben Noordhuis Ben Straub Benjamin C Meyer -- cgit v1.2.1 From 5a49ff9fa0b89c9fd479e04e0f694fa5f07fd6f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jun 2014 15:54:42 +0200 Subject: remote: remove rename code for anonymous remotes We don't allow renames of anonymous remotes, so there's no need to handle them. A remote is always associated with a repository, so there's no need to check for that. --- src/remote.c | 53 +++++++++++------------------------------------------ 1 file changed, 11 insertions(+), 42 deletions(-) diff --git a/src/remote.c b/src/remote.c index 0d1a88ea7..dd88cf91a 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1442,10 +1442,8 @@ static int rename_fetch_refspecs( if (spec->push) continue; - /* Every refspec is a problem refspec for an anonymous remote, OR */ /* Does the dst part of the refspec follow the expected format? */ - if (!remote->name || - strcmp(git_buf_cstr(&base), spec->string)) { + if (strcmp(git_buf_cstr(&base), spec->string)) { if ((error = callback(spec->string, payload)) != 0) { giterr_set_after_callback(error); @@ -1497,49 +1495,20 @@ int git_remote_rename( if ((error = ensure_remote_name_is_valid(new_name)) < 0) return error; - if (remote->repo) { - if ((error = ensure_remote_doesnot_exist(remote->repo, new_name)) < 0) - return error; + if ((error = ensure_remote_doesnot_exist(remote->repo, new_name)) < 0) + return error; - if (!remote->name) { - if ((error = rename_fetch_refspecs( - remote, - new_name, - callback, - payload)) < 0) - return error; + if ((error = rename_remote_config_section(remote->repo, remote->name, new_name)) < 0) + return error; - remote->name = git__strdup(new_name); - GITERR_CHECK_ALLOC(remote->name); + if ((error = update_branch_remote_config_entry(remote->repo, remote->name, new_name)) < 0) + return error; - return git_remote_save(remote); - } + if ((error = rename_remote_references(remote->repo, remote->name, new_name)) < 0) + return error; - if ((error = rename_remote_config_section( - remote->repo, - remote->name, - new_name)) < 0) - return error; - - if ((error = update_branch_remote_config_entry( - remote->repo, - remote->name, - new_name)) < 0) - return error; - - if ((error = rename_remote_references( - remote->repo, - remote->name, - new_name)) < 0) - return error; - - if ((error = rename_fetch_refspecs( - remote, - new_name, - callback, - payload)) < 0) - return error; - } + if ((error = rename_fetch_refspecs(remote, new_name, callback, payload)) < 0) + return error; git__free(remote->name); -- cgit v1.2.1 From 61dcfe1400a5ff1cf4dc805795cb72657c524906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jun 2014 15:57:37 +0200 Subject: remote: make sure the name stays valid on rename We must make sure that the name pointer remains valid, so make sure to allocate the new one before freeing the old one and swap them so the user never sees an invalid pointer. --- src/remote.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/remote.c b/src/remote.c index dd88cf91a..c7b97ca62 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1484,6 +1484,7 @@ int git_remote_rename( void *payload) { int error; + char *tmp, *dup; assert(remote && new_name); @@ -1510,10 +1511,12 @@ int git_remote_rename( if ((error = rename_fetch_refspecs(remote, new_name, callback, payload)) < 0) return error; - git__free(remote->name); + dup = git__strdup(new_name); + GITERR_CHECK_ALLOC(dup); - remote->name = git__strdup(new_name); - GITERR_CHECK_ALLOC(remote->name); + tmp = remote->name; + remote->name = dup; + git__free(tmp); return 0; } -- cgit v1.2.1 From 72bca13e5d0d421da7992f029e275d950c864105 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jun 2014 16:33:54 +0200 Subject: remote: return problem refspecs instead of using a callback There is no reason why we need to use a callback here. A string array fits better with the usage, as this is not an event and we don't need anything from the user. --- include/git2/remote.h | 8 ++-- src/remote.c | 40 +++++++++++-------- tests/network/remote/rename.c | 90 +++++++++++++++++++++++++------------------ tests/submodule/add.c | 5 ++- 4 files changed, 87 insertions(+), 56 deletions(-) diff --git a/include/git2/remote.h b/include/git2/remote.h index 28771ac42..8d3744265 100644 --- a/include/git2/remote.h +++ b/include/git2/remote.h @@ -572,6 +572,9 @@ GIT_EXTERN(void) git_remote_set_autotag( * * A temporary in-memory remote cannot be given a name with this method. * + * @param problems non-default refspecs cannot be renamed and will be + * stored here for further processing by the caller. Always free this + * strarray on succesful return. * @param remote the remote to rename * @param new_name the new name the remote should bear * @param callback Optional callback to notify the consumer of fetch refspecs @@ -580,10 +583,9 @@ GIT_EXTERN(void) git_remote_set_autotag( * @return 0, GIT_EINVALIDSPEC, GIT_EEXISTS or an error code */ GIT_EXTERN(int) git_remote_rename( + git_strarray *problems, git_remote *remote, - const char *new_name, - git_remote_rename_problem_cb callback, - void *payload); + const char *new_name); /** * Retrieve the update FETCH_HEAD setting. diff --git a/src/remote.c b/src/remote.c index c7b97ca62..4e6f350d5 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1419,11 +1419,7 @@ static int rename_remote_references( return (error == GIT_ITEROVER) ? 0 : error; } -static int rename_fetch_refspecs( - git_remote *remote, - const char *new_name, - int (*callback)(const char *problematic_refspec, void *payload), - void *payload) +static int rename_fetch_refspecs(git_vector *problems, git_remote *remote, const char *new_name) { git_config *config; git_buf base = GIT_BUF_INIT, var = GIT_BUF_INIT, val = GIT_BUF_INIT; @@ -1434,6 +1430,9 @@ static int rename_fetch_refspecs( if ((error = git_repository_config__weakptr(&config, remote->repo)) < 0) return error; + if ((error = git_vector_init(problems, 1, NULL)) < 0) + return error; + if ((error = git_buf_printf( &base, "+refs/heads/*:refs/remotes/%s/*", remote->name)) < 0) return error; @@ -1444,11 +1443,13 @@ static int rename_fetch_refspecs( /* Does the dst part of the refspec follow the expected format? */ if (strcmp(git_buf_cstr(&base), spec->string)) { + char *dup; - if ((error = callback(spec->string, payload)) != 0) { - giterr_set_after_callback(error); + dup = git__strdup(spec->string); + GITERR_CHECK_ALLOC(dup); + + if ((error = git_vector_insert(problems, dup)) < 0) break; - } continue; } @@ -1474,19 +1475,25 @@ static int rename_fetch_refspecs( git_buf_free(&base); git_buf_free(&var); git_buf_free(&val); + + if (error < 0) { + char *str; + git_vector_foreach(problems, i, str) + git__free(str); + + git_vector_free(problems); + } + return error; } -int git_remote_rename( - git_remote *remote, - const char *new_name, - git_remote_rename_problem_cb callback, - void *payload) +int git_remote_rename(git_strarray *out, git_remote *remote, const char *new_name) { int error; + git_vector problem_refspecs; char *tmp, *dup; - assert(remote && new_name); + assert(out && remote && new_name); if (!remote->name) { giterr_set(GITERR_INVALID, "Can't rename an anonymous remote."); @@ -1508,9 +1515,12 @@ int git_remote_rename( if ((error = rename_remote_references(remote->repo, remote->name, new_name)) < 0) return error; - if ((error = rename_fetch_refspecs(remote, new_name, callback, payload)) < 0) + if ((error = rename_fetch_refspecs(&problem_refspecs, remote, new_name)) < 0) return error; + out->count = problem_refspecs.length; + out->strings = (char **) problem_refspecs.contents; + dup = git__strdup(new_name); GITERR_CHECK_ALLOC(dup); diff --git a/tests/network/remote/rename.c b/tests/network/remote/rename.c index b7ec44726..48a33038a 100644 --- a/tests/network/remote/rename.c +++ b/tests/network/remote/rename.c @@ -33,10 +33,14 @@ static int dont_call_me_cb(const char *fetch_refspec, void *payload) void test_network_remote_rename__renaming_a_remote_moves_related_configuration_section(void) { + git_strarray problems = {0}; + assert_config_entry_existence(_repo, "remote.test.fetch", true); assert_config_entry_existence(_repo, "remote.just/renamed.fetch", false); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); assert_config_entry_existence(_repo, "remote.test.fetch", false); assert_config_entry_existence(_repo, "remote.just/renamed.fetch", true); @@ -44,16 +48,24 @@ void test_network_remote_rename__renaming_a_remote_moves_related_configuration_s void test_network_remote_rename__renaming_a_remote_updates_branch_related_configuration_entries(void) { + git_strarray problems = {0}; + assert_config_entry_value(_repo, "branch.master.remote", "test"); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); assert_config_entry_value(_repo, "branch.master.remote", "just/renamed"); } void test_network_remote_rename__renaming_a_remote_updates_default_fetchrefspec(void) { - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + git_strarray problems = {0}; + + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); assert_config_entry_value(_repo, "remote.just/renamed.fetch", "+refs/heads/*:refs/remotes/just/renamed/*"); } @@ -61,6 +73,7 @@ void test_network_remote_rename__renaming_a_remote_updates_default_fetchrefspec( void test_network_remote_rename__renaming_a_remote_without_a_fetchrefspec_doesnt_create_one(void) { git_config *config; + git_strarray problems = {0}; git_remote_free(_remote); cl_git_pass(git_repository_config__weakptr(&config, _repo)); @@ -70,70 +83,64 @@ void test_network_remote_rename__renaming_a_remote_without_a_fetchrefspec_doesnt assert_config_entry_existence(_repo, "remote.test.fetch", false); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); assert_config_entry_existence(_repo, "remote.just/renamed.fetch", false); } -static int ensure_refspecs(const char* refspec_name, void *payload) -{ - int i = 0; - bool found = false; - const char ** exp = (const char **)payload; - - while (exp[i]) { - if (strcmp(exp[i++], refspec_name)) - continue; - - found = true; - break; - } - - cl_assert(found); - - return 0; -} - void test_network_remote_rename__renaming_a_remote_notifies_of_non_default_fetchrefspec(void) { git_config *config; - char *expected_refspecs[] = { - "+refs/*:refs/*", - NULL - }; + git_strarray problems = {0}; git_remote_free(_remote); cl_git_pass(git_repository_config__weakptr(&config, _repo)); cl_git_pass(git_config_set_string(config, "remote.test.fetch", "+refs/*:refs/*")); cl_git_pass(git_remote_load(&_remote, _repo, "test")); - cl_git_pass(git_remote_rename(_remote, "just/renamed", ensure_refspecs, &expected_refspecs)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(1, problems.count); + cl_assert_equal_s("+refs/*:refs/*", problems.strings[0]); + git_strarray_free(&problems); assert_config_entry_value(_repo, "remote.just/renamed.fetch", "+refs/*:refs/*"); + + git_strarray_free(&problems); } void test_network_remote_rename__new_name_can_contain_dots(void) { - cl_git_pass(git_remote_rename(_remote, "just.renamed", dont_call_me_cb, NULL)); + git_strarray problems = {0}; + + cl_git_pass(git_remote_rename(&problems, _remote, "just.renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); cl_assert_equal_s("just.renamed", git_remote_name(_remote)); } void test_network_remote_rename__new_name_must_conform_to_reference_naming_conventions(void) { + git_strarray problems = {0}; + cl_assert_equal_i( GIT_EINVALIDSPEC, - git_remote_rename(_remote, "new@{name", dont_call_me_cb, NULL)); + git_remote_rename(&problems, _remote, "new@{name")); } void test_network_remote_rename__renamed_name_is_persisted(void) { git_remote *renamed; git_repository *another_repo; + git_strarray problems = {0}; cl_git_fail(git_remote_load(&renamed, _repo, "just/renamed")); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); cl_git_pass(git_repository_open(&another_repo, "testrepo.git")); cl_git_pass(git_remote_load(&renamed, _repo, "just/renamed")); @@ -144,19 +151,24 @@ void test_network_remote_rename__renamed_name_is_persisted(void) void test_network_remote_rename__cannot_overwrite_an_existing_remote(void) { - cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(_remote, "test", dont_call_me_cb, NULL)); - cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(_remote, "test_with_pushurl", dont_call_me_cb, NULL)); + git_strarray problems = {0}; + + cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(&problems, _remote, "test")); + cl_assert_equal_i(GIT_EEXISTS, git_remote_rename(&problems, _remote, "test_with_pushurl")); } void test_network_remote_rename__renaming_a_remote_moves_the_underlying_reference(void) { git_reference *underlying; + git_strarray problems = {0}; cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&underlying, _repo, "refs/remotes/just/renamed")); cl_git_pass(git_reference_lookup(&underlying, _repo, "refs/remotes/test/master")); git_reference_free(underlying); - cl_git_pass(git_remote_rename(_remote, "just/renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, _remote, "just/renamed")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&underlying, _repo, "refs/remotes/test/master")); cl_git_pass(git_reference_lookup(&underlying, _repo, "refs/remotes/just/renamed/master")); @@ -166,10 +178,12 @@ void test_network_remote_rename__renaming_a_remote_moves_the_underlying_referenc void test_network_remote_rename__cannot_rename_an_inmemory_remote(void) { git_remote *remote; + git_strarray problems = {0}; cl_git_pass(git_remote_create_anonymous(&remote, _repo, "file:///blah", NULL)); - cl_git_fail(git_remote_rename(remote, "newname", NULL, NULL)); + cl_git_fail(git_remote_rename(&problems, remote, "newname")); + git_strarray_free(&problems); git_remote_free(remote); } @@ -181,15 +195,17 @@ void test_network_remote_rename__overwrite_ref_in_target(void) git_reference *ref; git_branch_t btype; git_branch_iterator *iter; + git_strarray problems = {0}; cl_git_pass(git_oid_fromstr(&id, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750")); cl_git_pass(git_reference_create(&ref, _repo, "refs/remotes/renamed/master", &id, 1, NULL, NULL)); git_reference_free(ref); cl_git_pass(git_remote_load(&remote, _repo, "test")); - cl_git_pass(git_remote_rename(remote, "renamed", dont_call_me_cb, NULL)); + cl_git_pass(git_remote_rename(&problems, remote, "renamed")); git_remote_free(remote); - + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); /* make sure there's only one remote-tracking branch */ cl_git_pass(git_branch_iterator_new(&iter, _repo, GIT_BRANCH_REMOTE)); diff --git a/tests/submodule/add.c b/tests/submodule/add.c index af81713f1..9fdc7cc57 100644 --- a/tests/submodule/add.c +++ b/tests/submodule/add.c @@ -68,13 +68,16 @@ void test_submodule_add__url_relative(void) { git_submodule *sm; git_remote *remote; + git_strarray problems = {0}; /* default remote url is https://github.com/libgit2/false.git */ g_repo = cl_git_sandbox_init("testrepo2"); /* make sure we don't default to origin - rename origin -> test_remote */ cl_git_pass(git_remote_load(&remote, g_repo, "origin")); - cl_git_pass(git_remote_rename(remote, "test_remote", NULL, NULL)); + cl_git_pass(git_remote_rename(&problems, remote, "test_remote")); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); cl_git_fail(git_remote_load(&remote, g_repo, "origin")); git_remote_free(remote); -- cgit v1.2.1 From eb6aa791a7c1a311b556398acce69cde3d43e3cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jun 2014 22:01:35 +0200 Subject: remote: failing test for renaming with a symref --- tests/network/remote/rename.c | 48 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/network/remote/rename.c b/tests/network/remote/rename.c index 48a33038a..1b819a445 100644 --- a/tests/network/remote/rename.c +++ b/tests/network/remote/rename.c @@ -218,3 +218,51 @@ void test_network_remote_rename__overwrite_ref_in_target(void) cl_git_fail_with(GIT_ITEROVER, git_branch_next(&ref, &btype, iter)); git_branch_iterator_free(iter); } + +void test_network_remote_rename__symref_head(void) +{ + int error; + git_remote *remote; + git_reference *ref; + git_branch_t btype; + git_branch_iterator *iter; + git_strarray problems = {0}; + char idstr[GIT_OID_HEXSZ + 1] = {0}; + git_vector refs; + + cl_git_pass(git_reference_symbolic_create(&ref, _repo, "refs/remotes/test/HEAD", "refs/remotes/test/master", 0, NULL, NULL)); + git_reference_free(ref); + + cl_git_pass(git_remote_load(&remote, _repo, "test")); + cl_git_pass(git_remote_rename(&problems, remote, "renamed")); + git_remote_free(remote); + cl_assert_equal_i(0, problems.count); + git_strarray_free(&problems); + + cl_git_pass(git_vector_init(&refs, 2, (git_vector_cmp) git_reference_cmp)); + cl_git_pass(git_branch_iterator_new(&iter, _repo, GIT_BRANCH_REMOTE)); + + while ((error = git_branch_next(&ref, &btype, iter)) == 0) { + cl_git_pass(git_vector_insert(&refs, ref)); + } + cl_assert_equal_i(GIT_ITEROVER, error); + git_vector_sort(&refs); + + cl_assert_equal_i(2, refs.length); + + ref = git_vector_get(&refs, 0); + cl_assert_equal_s("refs/remotes/renamed/HEAD", git_reference_name(ref)); + cl_assert_equal_s("refs/remotes/renamed/master", git_reference_symbolic_target(ref)); + git_reference_free(ref); + + ref = git_vector_get(&refs, 1); + cl_assert_equal_s("refs/remotes/renamed/master", git_reference_name(ref)); + git_oid_fmt(idstr, git_reference_target(ref)); + cl_assert_equal_s("be3563ae3f795b2b4353bcce3a527ad0a4f7f644", idstr); + git_reference_free(ref); + + git_vector_free(&refs); + + cl_git_fail_with(GIT_ITEROVER, git_branch_next(&ref, &btype, iter)); + git_branch_iterator_free(iter); +} -- cgit v1.2.1 From d15445646440807a082feb54a1e92d54864137d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jun 2014 22:38:26 +0200 Subject: remote: handle symrefs when renaming A symref inside the namespace gets renamed, we should make it point to the target's new name. This is for the origin/HEAD -> origin/master type of situations. --- src/remote.c | 49 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/src/remote.c b/src/remote.c index 4e6f350d5..827c54f9d 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1359,19 +1359,24 @@ static int update_branch_remote_config_entry( } static int rename_one_remote_reference( - git_reference *reference, + git_reference *reference_in, const char *old_remote_name, const char *new_remote_name) { int error; + git_reference *ref = NULL, *dummy = NULL; + git_buf namespace = GIT_BUF_INIT, old_namespace = GIT_BUF_INIT; git_buf new_name = GIT_BUF_INIT; git_buf log_message = GIT_BUF_INIT; + size_t pfx_len; + const char *target; - if ((error = git_buf_printf( - &new_name, - GIT_REFS_REMOTES_DIR "%s%s", - new_remote_name, - reference->name + strlen(GIT_REFS_REMOTES_DIR) + strlen(old_remote_name))) < 0) + if ((error = git_buf_printf(&namespace, GIT_REFS_REMOTES_DIR "%s/", new_remote_name)) < 0) + return error; + + pfx_len = strlen(GIT_REFS_REMOTES_DIR) + strlen(old_remote_name) + 1; + git_buf_puts(&new_name, namespace.ptr); + if ((error = git_buf_puts(&new_name, git_reference_name(reference_in) + pfx_len)) < 0) goto cleanup; if ((error = git_buf_printf(&log_message, @@ -1379,12 +1384,36 @@ static int rename_one_remote_reference( old_remote_name, new_remote_name)) < 0) goto cleanup; - error = git_reference_rename( - NULL, reference, git_buf_cstr(&new_name), 1, - NULL, git_buf_cstr(&log_message)); - git_reference_free(reference); + if ((error = git_reference_rename(&ref, reference_in, git_buf_cstr(&new_name), 1, + NULL, git_buf_cstr(&log_message))) < 0) + goto cleanup; + + if (git_reference_type(ref) != GIT_REF_SYMBOLIC) + goto cleanup; + + /* Handle refs like origin/HEAD -> origin/master */ + target = git_reference_symbolic_target(ref); + if ((error = git_buf_printf(&old_namespace, GIT_REFS_REMOTES_DIR "%s/", old_remote_name)) < 0) + goto cleanup; + + if (git__prefixcmp(target, old_namespace.ptr)) + goto cleanup; + + git_buf_clear(&new_name); + git_buf_puts(&new_name, namespace.ptr); + if ((error = git_buf_puts(&new_name, target + pfx_len)) < 0) + goto cleanup; + + error = git_reference_symbolic_set_target(&dummy, ref, git_buf_cstr(&new_name), + NULL, git_buf_cstr(&log_message)); + + git_reference_free(dummy); cleanup: + git_reference_free(reference_in); + git_reference_free(ref); + git_buf_free(&namespace); + git_buf_free(&old_namespace); git_buf_free(&new_name); git_buf_free(&log_message); return error; -- cgit v1.2.1 From 231f350d91e71e3c171041a64f0d238888fad002 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 6 Jun 2014 22:55:34 +0200 Subject: remote: don't free the remote on delete This was a bad idea. Don't free except in the free function. --- include/git2/remote.h | 2 -- src/remote.c | 2 -- tests/network/remote/delete.c | 2 +- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/include/git2/remote.h b/include/git2/remote.h index 8d3744265..cba57c4f6 100644 --- a/include/git2/remote.h +++ b/include/git2/remote.h @@ -618,8 +618,6 @@ GIT_EXTERN(int) git_remote_is_valid_name(const char *remote_name); * All remote-tracking branches and configuration settings * for the remote will be removed. * -* once deleted, the passed remote object will be freed and invalidated. -* * @param remote A valid remote * @return 0 on success, or an error code. */ diff --git a/src/remote.c b/src/remote.c index 827c54f9d..47b61b1b1 100644 --- a/src/remote.c +++ b/src/remote.c @@ -1921,8 +1921,6 @@ int git_remote_delete(git_remote *remote) repo, git_remote_name(remote), NULL)) < 0) return error; - git_remote_free(remote); - return 0; } diff --git a/tests/network/remote/delete.c b/tests/network/remote/delete.c index db55b0768..664f47a43 100644 --- a/tests/network/remote/delete.c +++ b/tests/network/remote/delete.c @@ -15,6 +15,7 @@ void test_network_remote_delete__initialize(void) void test_network_remote_delete__cleanup(void) { + git_remote_free(_remote); cl_git_sandbox_cleanup(); } @@ -27,7 +28,6 @@ void test_network_remote_delete__cannot_delete_an_anonymous_remote(void) cl_git_fail(git_remote_delete(remote)); git_remote_free(remote); - git_remote_free(_remote); } void test_network_remote_delete__remove_remote_tracking_branches(void) -- cgit v1.2.1 From 6d1b04383ed0744b0346dc9834e34ef7b634a2e0 Mon Sep 17 00:00:00 2001 From: Philip Kelley Date: Sat, 7 Jun 2014 12:18:24 -0400 Subject: Win32: Fix failing clone_mirror test --- tests/online/clone.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/online/clone.c b/tests/online/clone.c index e269771c0..8a2a64f95 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -194,6 +194,9 @@ void test_online_clone__clone_mirror(void) git_remote_free(remote); git_reference_free(head); git_buf_free(&path); + git_repository_free(g_repo); + g_repo = NULL; + cl_fixture_cleanup("./foo.git"); } -- cgit v1.2.1 From daf2a648b1dcc02665dcf9c6f5317db61d84be89 Mon Sep 17 00:00:00 2001 From: Philip Kelley Date: Sat, 7 Jun 2014 12:18:56 -0400 Subject: Win32: Fix diff::workdir::submodules test #2361 --- tests/submodule/submodule_helpers.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/submodule/submodule_helpers.c b/tests/submodule/submodule_helpers.c index 50aa97568..c6d04b40a 100644 --- a/tests/submodule/submodule_helpers.c +++ b/tests/submodule/submodule_helpers.c @@ -19,8 +19,8 @@ void rewrite_gitmodules(const char *workdir) cl_git_pass(git_buf_joinpath(&in_f, workdir, "gitmodules")); cl_git_pass(git_buf_joinpath(&out_f, workdir, ".gitmodules")); - cl_assert((in = fopen(in_f.ptr, "r")) != NULL); - cl_assert((out = fopen(out_f.ptr, "w")) != NULL); + cl_assert((in = fopen(in_f.ptr, "rb")) != NULL); + cl_assert((out = fopen(out_f.ptr, "wb")) != NULL); while (fgets(line, sizeof(line), in) != NULL) { char *scan = line; -- cgit v1.2.1 From fb5917679dd1cc0ee50d1d88893c07cbcd82471f Mon Sep 17 00:00:00 2001 From: Philip Kelley Date: Sat, 7 Jun 2014 12:51:48 -0400 Subject: Win32: Fix object::cache::threadmania test on x64 --- src/global.c | 4 +- src/pack-objects.c | 2 +- src/thread-utils.h | 19 ++++++-- src/win32/pthread.c | 108 +++++++++++++++++++++++------------------ src/win32/pthread.h | 21 +++++--- tests/object/cache.c | 4 +- tests/threads/refdb.c | 4 +- tests/threads/thread_helpers.c | 2 +- 8 files changed, 96 insertions(+), 68 deletions(-) diff --git a/src/global.c b/src/global.c index 7da31853e..1c4a7a1a9 100644 --- a/src/global.c +++ b/src/global.c @@ -78,7 +78,7 @@ static void git__shutdown(void) static DWORD _tls_index; static volatile LONG _mutex = 0; -static int synchronized_threads_init() +static int synchronized_threads_init(void) { int error; @@ -112,7 +112,7 @@ int git_threads_init(void) return error; } -static void synchronized_threads_shutdown() +static void synchronized_threads_shutdown(void) { /* Shut down any subsystems that have global state */ git__shutdown(); diff --git a/src/pack-objects.c b/src/pack-objects.c index 3d3330ae8..0040a826b 100644 --- a/src/pack-objects.c +++ b/src/pack-objects.c @@ -1209,7 +1209,7 @@ static int ll_find_deltas(git_packbuilder *pb, git_pobject **list, git_mutex_unlock(&target->mutex); if (!sub_size) { - git_thread_join(target->thread, NULL); + git_thread_join(&target->thread, NULL); git_cond_free(&target->cond); git_mutex_free(&target->mutex); active_threads--; diff --git a/src/thread-utils.h b/src/thread-utils.h index 50d8610a3..0cc7b04ad 100644 --- a/src/thread-utils.h +++ b/src/thread-utils.h @@ -40,12 +40,23 @@ typedef git_atomic git_atomic_ssize; #ifdef GIT_THREADS +#if defined(GIT_WIN32) + +#define git_thread git_win32_thread +#define git_thread_create(thread, attr, start_routine, arg) \ + git_win32__thread_create(thread, attr, start_routine, arg) +#define git_thread_join(thread_ptr, status) \ + git_win32__thread_join(thread_ptr, status) + +#else + #define git_thread pthread_t #define git_thread_create(thread, attr, start_routine, arg) \ pthread_create(thread, attr, start_routine, arg) -#define git_thread_kill(thread) pthread_cancel(thread) -#define git_thread_exit(status) pthread_exit(status) -#define git_thread_join(id, status) pthread_join(id, status) +#define git_thread_join(thread_ptr, status) \ + pthread_join(*(thread_ptr), status) + +#endif #if defined(GIT_WIN32) #define git_thread_yield() Sleep(0) @@ -179,8 +190,6 @@ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend) #define git_thread unsigned int #define git_thread_create(thread, attr, start_routine, arg) 0 -#define git_thread_kill(thread) (void)0 -#define git_thread_exit(status) (void)0 #define git_thread_join(id, status) (void)0 #define git_thread_yield() (void)0 diff --git a/src/win32/pthread.c b/src/win32/pthread.c index db8927471..b14b35621 100644 --- a/src/win32/pthread.c +++ b/src/win32/pthread.c @@ -8,32 +8,63 @@ #include "pthread.h" #include "../global.h" -int pthread_create( - pthread_t *GIT_RESTRICT thread, +#define CLEAN_THREAD_EXIT 0x6F012842 + +/* The thread procedure stub used to invoke the caller's procedure + * and capture the return value for later collection. Windows will + * only hold a DWORD, but we need to be able to store an entire + * void pointer. This requires the indirection. */ +static DWORD WINAPI git_win32__threadproc(LPVOID lpParameter) +{ + git_win32_thread *thread = lpParameter; + + thread->value = thread->proc(thread->value); + + return CLEAN_THREAD_EXIT; +} + +int git_win32__thread_create( + git_win32_thread *GIT_RESTRICT thread, const pthread_attr_t *GIT_RESTRICT attr, void *(*start_routine)(void*), void *GIT_RESTRICT arg) { GIT_UNUSED(attr); - *thread = CreateThread( - NULL, 0, (LPTHREAD_START_ROUTINE)start_routine, arg, 0, NULL); - return *thread ? 0 : -1; + + thread->value = arg; + thread->proc = start_routine; + thread->thread = CreateThread( + NULL, 0, git_win32__threadproc, thread, 0, NULL); + + return thread->thread ? 0 : -1; } -int pthread_join(pthread_t thread, void **value_ptr) +int git_win32__thread_join( + git_win32_thread *thread, + void **value_ptr) { - DWORD ret = WaitForSingleObject(thread, INFINITE); + DWORD exit; + + if (WaitForSingleObject(thread->thread, INFINITE) != WAIT_OBJECT_0) + return -1; + + if (!GetExitCodeThread(thread->thread, &exit)) { + CloseHandle(thread->thread); + return -1; + } - if (ret == WAIT_OBJECT_0) { - if (value_ptr != NULL) { - *value_ptr = NULL; - GetExitCodeThread(thread, (void *)value_ptr); - } - CloseHandle(thread); - return 0; + /* Check for the thread having exited uncleanly. If exit was unclean, + * then we don't have a return value to give back to the caller. */ + if (exit != CLEAN_THREAD_EXIT) { + assert(false); + thread->value = NULL; } - return -1; + if (value_ptr) + *value_ptr = thread->value; + + CloseHandle(thread->thread); + return 0; } int pthread_mutex_init( @@ -144,9 +175,6 @@ int pthread_num_processors_np(void) return n ? n : 1; } - -static HINSTANCE win32_kernel32_dll; - typedef void (WINAPI *win32_srwlock_fn)(GIT_SRWLOCK *); static win32_srwlock_fn win32_srwlock_initialize; @@ -159,7 +187,7 @@ int pthread_rwlock_init( pthread_rwlock_t *GIT_RESTRICT lock, const pthread_rwlockattr_t *GIT_RESTRICT attr) { - (void)attr; + GIT_UNUSED(attr); if (win32_srwlock_initialize) win32_srwlock_initialize(&lock->native.srwl); @@ -217,38 +245,22 @@ int pthread_rwlock_destroy(pthread_rwlock_t *lock) return 0; } - -static void win32_pthread_shutdown(void) -{ - if (win32_kernel32_dll) { - FreeLibrary(win32_kernel32_dll); - win32_kernel32_dll = NULL; - } -} - int win32_pthread_initialize(void) { - if (win32_kernel32_dll) - return 0; - - win32_kernel32_dll = LoadLibrary("Kernel32.dll"); - if (!win32_kernel32_dll) { - giterr_set(GITERR_OS, "Could not load Kernel32.dll!"); - return -1; + HMODULE hModule = GetModuleHandleW(L"kernel32"); + + if (hModule) { + win32_srwlock_initialize = (win32_srwlock_fn) + GetProcAddress(hModule, "InitializeSRWLock"); + win32_srwlock_acquire_shared = (win32_srwlock_fn) + GetProcAddress(hModule, "AcquireSRWLockShared"); + win32_srwlock_release_shared = (win32_srwlock_fn) + GetProcAddress(hModule, "ReleaseSRWLockShared"); + win32_srwlock_acquire_exclusive = (win32_srwlock_fn) + GetProcAddress(hModule, "AcquireSRWLockExclusive"); + win32_srwlock_release_exclusive = (win32_srwlock_fn) + GetProcAddress(hModule, "ReleaseSRWLockExclusive"); } - win32_srwlock_initialize = (win32_srwlock_fn) - GetProcAddress(win32_kernel32_dll, "InitializeSRWLock"); - win32_srwlock_acquire_shared = (win32_srwlock_fn) - GetProcAddress(win32_kernel32_dll, "AcquireSRWLockShared"); - win32_srwlock_release_shared = (win32_srwlock_fn) - GetProcAddress(win32_kernel32_dll, "ReleaseSRWLockShared"); - win32_srwlock_acquire_exclusive = (win32_srwlock_fn) - GetProcAddress(win32_kernel32_dll, "AcquireSRWLockExclusive"); - win32_srwlock_release_exclusive = (win32_srwlock_fn) - GetProcAddress(win32_kernel32_dll, "ReleaseSRWLockExclusive"); - - git__on_shutdown(win32_pthread_shutdown); - return 0; } diff --git a/src/win32/pthread.h b/src/win32/pthread.h index af5b121f0..a15504ed5 100644 --- a/src/win32/pthread.h +++ b/src/win32/pthread.h @@ -16,13 +16,18 @@ # define GIT_RESTRICT __restrict__ #endif +typedef struct { + HANDLE thread; + void *(*proc)(void *); + void *value; +} git_win32_thread; + typedef int pthread_mutexattr_t; typedef int pthread_condattr_t; typedef int pthread_attr_t; typedef int pthread_rwlockattr_t; typedef CRITICAL_SECTION pthread_mutex_t; -typedef HANDLE pthread_t; typedef HANDLE pthread_cond_t; typedef struct { void *Ptr; } GIT_SRWLOCK; @@ -36,13 +41,15 @@ typedef struct { #define PTHREAD_MUTEX_INITIALIZER {(void*)-1} -int pthread_create( - pthread_t *GIT_RESTRICT thread, - const pthread_attr_t *GIT_RESTRICT attr, - void *(*start_routine)(void*), - void *GIT_RESTRICT arg); +int git_win32__thread_create( + git_win32_thread *GIT_RESTRICT, + const pthread_attr_t *GIT_RESTRICT, + void *(*) (void *), + void *GIT_RESTRICT); -int pthread_join(pthread_t, void **); +int git_win32__thread_join( + git_win32_thread *, + void **); int pthread_mutex_init( pthread_mutex_t *GIT_RESTRICT mutex, diff --git a/tests/object/cache.c b/tests/object/cache.c index b927b2514..bdf12da7a 100644 --- a/tests/object/cache.c +++ b/tests/object/cache.c @@ -229,7 +229,7 @@ void test_object_cache__threadmania(void) #ifdef GIT_THREADS for (th = 0; th < THREADCOUNT; ++th) { - cl_git_pass(git_thread_join(t[th], &data)); + cl_git_pass(git_thread_join(&t[th], &data)); cl_assert_equal_i(th, ((int *)data)[0]); git__free(data); } @@ -276,7 +276,7 @@ void test_object_cache__fast_thread_rush(void) #ifdef GIT_THREADS for (th = 0; th < THREADCOUNT*2; ++th) { void *rval; - cl_git_pass(git_thread_join(t[th], &rval)); + cl_git_pass(git_thread_join(&t[th], &rval)); cl_assert_equal_i(th, *((int *)rval)); } #endif diff --git a/tests/threads/refdb.c b/tests/threads/refdb.c index c1cd29677..94a21f259 100644 --- a/tests/threads/refdb.c +++ b/tests/threads/refdb.c @@ -84,7 +84,7 @@ void test_threads_refdb__iterator(void) #ifdef GIT_THREADS for (t = 0; t < THREADS; ++t) { - cl_git_pass(git_thread_join(th[t], NULL)); + cl_git_pass(git_thread_join(&th[t], NULL)); } #endif @@ -215,7 +215,7 @@ void test_threads_refdb__edit_while_iterate(void) } for (t = 0; t < THREADS; ++t) { - cl_git_pass(git_thread_join(th[t], NULL)); + cl_git_pass(git_thread_join(&th[t], NULL)); } #endif } diff --git a/tests/threads/thread_helpers.c b/tests/threads/thread_helpers.c index 25370dddb..760a7bd33 100644 --- a/tests/threads/thread_helpers.c +++ b/tests/threads/thread_helpers.c @@ -32,7 +32,7 @@ void run_in_parallel( #ifdef GIT_THREADS for (t = 0; t < threads; ++t) - cl_git_pass(git_thread_join(th[t], NULL)); + cl_git_pass(git_thread_join(&th[t], NULL)); memset(th, 0, threads * sizeof(git_thread)); #endif -- cgit v1.2.1 From 1b4e29b7f68ce80bb01fd878f6ddc6cf20819581 Mon Sep 17 00:00:00 2001 From: Philip Kelley Date: Sat, 7 Jun 2014 13:56:39 -0400 Subject: React to review feedback --- src/thread-utils.h | 21 ++++++++------------- src/win32/pthread.c | 9 +++++---- src/win32/pthread.h | 14 +++++++++++++- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/thread-utils.h b/src/thread-utils.h index 0cc7b04ad..daec14eeb 100644 --- a/src/thread-utils.h +++ b/src/thread-utils.h @@ -40,21 +40,16 @@ typedef git_atomic git_atomic_ssize; #ifdef GIT_THREADS -#if defined(GIT_WIN32) - -#define git_thread git_win32_thread -#define git_thread_create(thread, attr, start_routine, arg) \ - git_win32__thread_create(thread, attr, start_routine, arg) -#define git_thread_join(thread_ptr, status) \ - git_win32__thread_join(thread_ptr, status) +#if !defined(GIT_WIN32) -#else +typedef struct { + pthread_t thread; +} git_thread; -#define git_thread pthread_t -#define git_thread_create(thread, attr, start_routine, arg) \ - pthread_create(thread, attr, start_routine, arg) -#define git_thread_join(thread_ptr, status) \ - pthread_join(*(thread_ptr), status) +#define git_thread_create(git_thread_ptr, attr, start_routine, arg) \ + pthread_create(&(git_thread_ptr)->thread, attr, start_routine, arg) +#define git_thread_join(git_thread_ptr, status) \ + pthread_join((git_thread_ptr)->thread, status) #endif diff --git a/src/win32/pthread.c b/src/win32/pthread.c index b14b35621..ec45ecbe5 100644 --- a/src/win32/pthread.c +++ b/src/win32/pthread.c @@ -18,7 +18,7 @@ static DWORD WINAPI git_win32__threadproc(LPVOID lpParameter) { git_win32_thread *thread = lpParameter; - thread->value = thread->proc(thread->value); + thread->result = thread->proc(thread->param); return CLEAN_THREAD_EXIT; } @@ -31,7 +31,8 @@ int git_win32__thread_create( { GIT_UNUSED(attr); - thread->value = arg; + thread->result = NULL; + thread->param = arg; thread->proc = start_routine; thread->thread = CreateThread( NULL, 0, git_win32__threadproc, thread, 0, NULL); @@ -57,11 +58,11 @@ int git_win32__thread_join( * then we don't have a return value to give back to the caller. */ if (exit != CLEAN_THREAD_EXIT) { assert(false); - thread->value = NULL; + thread->result = NULL; } if (value_ptr) - *value_ptr = thread->value; + *value_ptr = thread->result; CloseHandle(thread->thread); return 0; diff --git a/src/win32/pthread.h b/src/win32/pthread.h index a15504ed5..e4826ca7f 100644 --- a/src/win32/pthread.h +++ b/src/win32/pthread.h @@ -19,7 +19,8 @@ typedef struct { HANDLE thread; void *(*proc)(void *); - void *value; + void *param; + void *result; } git_win32_thread; typedef int pthread_mutexattr_t; @@ -51,6 +52,17 @@ int git_win32__thread_join( git_win32_thread *, void **); +#ifdef GIT_THREADS + +typedef git_win32_thread git_thread; + +#define git_thread_create(git_thread_ptr, attr, start_routine, arg) \ + git_win32__thread_create(git_thread_ptr, attr, start_routine, arg) +#define git_thread_join(git_thread_ptr, status) \ + git_win32__thread_join(git_thread_ptr, status) + +#endif + int pthread_mutex_init( pthread_mutex_t *GIT_RESTRICT mutex, const pthread_mutexattr_t *GIT_RESTRICT mutexattr); -- cgit v1.2.1 From 99807672055b6bb86e44311a199592ceb35c72bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sun, 8 Jun 2014 19:42:54 +0200 Subject: Change SOVERSION at API breaks Since the SOVERSION doesn't need to follow the library's version and simply needs to be monotonically increasing whenever we release something that breaks the ABI, we can set some number and allow multiple versions of the library to be installed side-by-side. We start here with the minor version as that's what we release for now, and it allows to backport this change to earlier versions. --- CMakeLists.txt | 5 ++++- include/git2/version.h | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6f731d491..6f1a97edb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -127,6 +127,9 @@ STRING(REGEX REPLACE "^.*LIBGIT2_VERSION \"[0-9]+\\.([0-9]+).*$" "\\1" LIBGIT2_V STRING(REGEX REPLACE "^.*LIBGIT2_VERSION \"[0-9]+\\.[0-9]+\\.([0-9]+).*$" "\\1" LIBGIT2_VERSION_REV "${GIT2_HEADER}") SET(LIBGIT2_VERSION_STRING "${LIBGIT2_VERSION_MAJOR}.${LIBGIT2_VERSION_MINOR}.${LIBGIT2_VERSION_REV}") +FILE(STRINGS "include/git2/version.h" GIT2_HEADER_SOVERSION REGEX "^#define LIBGIT2_SOVERSION [0-9]+$") +STRING(REGEX REPLACE "^.*LIBGIT2_SOVERSION ([0-9]+)$" "\\1" LIBGIT2_SOVERSION "${GIT2_HEADER_SOVERSION}") + # Find required dependencies INCLUDE_DIRECTORIES(src include) @@ -397,7 +400,7 @@ MSVC_SPLIT_SOURCES(git2) IF (SONAME) SET_TARGET_PROPERTIES(git2 PROPERTIES VERSION ${LIBGIT2_VERSION_STRING}) - SET_TARGET_PROPERTIES(git2 PROPERTIES SOVERSION ${LIBGIT2_VERSION_MAJOR}) + SET_TARGET_PROPERTIES(git2 PROPERTIES SOVERSION ${LIBGIT2_SOVERSION}) IF (LIBGIT2_FILENAME) ADD_DEFINITIONS(-DLIBGIT2_FILENAME=\"${LIBGIT2_FILENAME}\") SET_TARGET_PROPERTIES(git2 PROPERTIES OUTPUT_NAME ${LIBGIT2_FILENAME}) diff --git a/include/git2/version.h b/include/git2/version.h index c4c5e8eb1..9d8a74edd 100644 --- a/include/git2/version.h +++ b/include/git2/version.h @@ -12,4 +12,6 @@ #define LIBGIT2_VER_MINOR 20 #define LIBGIT2_VER_REVISION 0 +#define LIBGIT2_SOVERSION 20 + #endif -- cgit v1.2.1 From 4fb32a44f928de534f79c9642e2cdb1ab9d9129b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sun, 8 Jun 2014 20:01:02 +0200 Subject: Bump version to 0.21.0 Bump library version to 0.21.0 and SONAME to 21 --- include/git2/version.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/git2/version.h b/include/git2/version.h index 9d8a74edd..5bda42735 100644 --- a/include/git2/version.h +++ b/include/git2/version.h @@ -7,11 +7,11 @@ #ifndef INCLUDE_git_version_h__ #define INCLUDE_git_version_h__ -#define LIBGIT2_VERSION "0.20.0" +#define LIBGIT2_VERSION "0.21.0" #define LIBGIT2_VER_MAJOR 0 -#define LIBGIT2_VER_MINOR 20 +#define LIBGIT2_VER_MINOR 21 #define LIBGIT2_VER_REVISION 0 -#define LIBGIT2_SOVERSION 20 +#define LIBGIT2_SOVERSION 21 #endif -- cgit v1.2.1 From 281da0043ca6fe8c5ba6bc24c24c10d80db93956 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 9 Jun 2014 19:35:41 +0200 Subject: remote: fix rename docs --- include/git2/remote.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/include/git2/remote.h b/include/git2/remote.h index cba57c4f6..c72c9c8cc 100644 --- a/include/git2/remote.h +++ b/include/git2/remote.h @@ -577,9 +577,6 @@ GIT_EXTERN(void) git_remote_set_autotag( * strarray on succesful return. * @param remote the remote to rename * @param new_name the new name the remote should bear - * @param callback Optional callback to notify the consumer of fetch refspecs - * that haven't been automatically updated and need potential manual tweaking. - * @param payload Additional data to pass to the callback * @return 0, GIT_EINVALIDSPEC, GIT_EEXISTS or an error code */ GIT_EXTERN(int) git_remote_rename( -- cgit v1.2.1 From 2c11d2eeb550ed2f795c714829b3fd3788b2ab68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 9 Jun 2014 23:23:53 +0200 Subject: treebuilder: insert sorted By inserting in the right position, we can keep the vector sorted, making entry insertion almost twice as fast. --- src/tree.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tree.c b/src/tree.c index 94f779eca..b64efe460 100644 --- a/src/tree.c +++ b/src/tree.c @@ -460,7 +460,7 @@ static int append_entry( git_oid_cpy(&entry->oid, id); entry->attr = (uint16_t)filemode; - if (git_vector_insert(&bld->entries, entry) < 0) { + if (git_vector_insert_sorted(&bld->entries, entry, NULL) < 0) { git__free(entry); return -1; } @@ -671,7 +671,7 @@ int git_treebuilder_insert( entry = alloc_entry(filename); GITERR_CHECK_ALLOC(entry); - if (git_vector_insert(&bld->entries, entry) < 0) { + if (git_vector_insert_sorted(&bld->entries, entry, NULL) < 0) { git__free(entry); return -1; } -- cgit v1.2.1 From 17fbf852a5c210cfa6a4e1e906dd9e35d2adc1c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 10 Jun 2014 03:53:26 +0200 Subject: pathspec: use C guards in header --- include/git2/pathspec.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/git2/pathspec.h b/include/git2/pathspec.h index 2fb0bb716..de6f027c5 100644 --- a/include/git2/pathspec.h +++ b/include/git2/pathspec.h @@ -12,6 +12,8 @@ #include "strarray.h" #include "diff.h" +GIT_BEGIN_DECL + /** * Compiled pathspec */ @@ -257,4 +259,5 @@ GIT_EXTERN(size_t) git_pathspec_match_list_failed_entrycount( GIT_EXTERN(const char *) git_pathspec_match_list_failed_entry( const git_pathspec_match_list *m, size_t pos); +GIT_END_DECL #endif -- cgit v1.2.1 From f9a97667945a87abfca50e153a7d2fdf5c4319a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 11 Jun 2014 00:06:44 +0200 Subject: revwalk: more sensible array handling Instead of using a sentinel empty value to detect the last commit, let's check for when we get a NULL from popping the stack, which lets us know when we're done. The current code causes us to read uninitialized data, although only on RHEL/CentOS 6 in release mode. This is a readability win overall. --- src/revwalk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/revwalk.c b/src/revwalk.c index 7aedd1f44..530c9705e 100644 --- a/src/revwalk.c +++ b/src/revwalk.c @@ -48,7 +48,7 @@ static int mark_uninteresting(git_revwalk *walk, git_commit_list_node *commit) assert(commit); - git_array_alloc(pending); + git_array_init_to_size(pending, 2); GITERR_CHECK_ARRAY(pending); do { @@ -67,7 +67,7 @@ static int mark_uninteresting(git_revwalk *walk, git_commit_list_node *commit) tmp = git_array_pop(pending); commit = tmp ? *tmp : NULL; - } while (git_array_size(pending) > 0); + } while (commit != NULL); git_array_clear(pending); -- cgit v1.2.1 From 1d3364ac9d2bcd2d194f0afa0d301456d0d4e276 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 11 Jun 2014 20:52:15 +0200 Subject: netops: init OpenSSL once under lock The OpenSSL init functions are not reentrant, which means that running multiple fetches in parallel can cause us to crash. Use a mutex to init OpenSSL, and since we're adding this extra checks, init it only once. --- src/global.c | 3 +++ src/global.h | 2 ++ src/netops.c | 34 ++++++++++++++++++++++++++++++++-- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/global.c b/src/global.c index 7da31853e..fe410587e 100644 --- a/src/global.c +++ b/src/global.c @@ -16,6 +16,9 @@ git_mutex git__mwindow_mutex; #define MAX_SHUTDOWN_CB 8 +git_mutex git__ssl_mutex; +git_atomic git__ssl_init; + static git_global_shutdown_fn git__shutdown_callbacks[MAX_SHUTDOWN_CB]; static git_atomic git__n_shutdown_callbacks; static git_atomic git__n_inits; diff --git a/src/global.h b/src/global.h index 778250376..245f811e4 100644 --- a/src/global.h +++ b/src/global.h @@ -18,6 +18,8 @@ typedef struct { git_global_st *git__global_state(void); extern git_mutex git__mwindow_mutex; +extern git_mutex git__ssl_mutex; +extern git_atomic git__ssl_init; #define GIT_GLOBAL (git__global_state()) diff --git a/src/netops.c b/src/netops.c index a0193a022..7bce159ad 100644 --- a/src/netops.c +++ b/src/netops.c @@ -33,6 +33,7 @@ #include "posix.h" #include "buffer.h" #include "http_parser.h" +#include "global.h" #ifdef GIT_WIN32 static void net_set_error(const char *str) @@ -386,12 +387,41 @@ cert_fail_name: return -1; } -static int ssl_setup(gitno_socket *socket, const char *host, int flags) +/** + * The OpenSSL init functions are not reentrant so we need to init + * them under lock. + */ +static int init_ssl(void) { - int ret; + if (git__ssl_init.val) + return 0; + + if (git_mutex_lock(&git__ssl_mutex) < 0) { + giterr_set(GITERR_OS, "failed to acquire ssl init lock"); + return -1; + } + + /* if we had to wait for the lock, someone else did it, we can return */ + if (git__ssl_init.val) + return 0; + SSL_library_init(); SSL_load_error_strings(); + + git_atomic_set(&git__ssl_init, 1); + git_mutex_unlock(&git__ssl_mutex); + + return 0; +} + +static int ssl_setup(gitno_socket *socket, const char *host, int flags) +{ + int ret; + + if (init_ssl() < 0) + return -1; + socket->ssl.ctx = SSL_CTX_new(SSLv23_method()); if (socket->ssl.ctx == NULL) return ssl_set_error(&socket->ssl, 0); -- cgit v1.2.1 From 76f76162b2af4b5dfe093654851339b957f7305a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 11 Jun 2014 21:14:45 +0200 Subject: remote: update documentation Add docs for git_clone_local_t and move the docs for the git_clone_options to each field. --- include/git2/clone.h | 78 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 60 insertions(+), 18 deletions(-) diff --git a/include/git2/clone.h b/include/git2/clone.h index b2c944a78..05b7522ce 100644 --- a/include/git2/clone.h +++ b/include/git2/clone.h @@ -23,10 +23,31 @@ */ GIT_BEGIN_DECL +/** + * Options for bypassing the git-aware transport on clone. Bypassing + * it means that instead of a fetch, libgit2 will copy the object + * database directory instead of figuring out what it needs, which is + * faster. If possible, it will hardlink the files to save space. + */ typedef enum { + /** + * Auto-detect (default), libgit2 will bypass the git-aware + * transport for local paths, but use a normal fetch for + * `file://` urls. + */ GIT_CLONE_LOCAL_AUTO, + /** + * Bypass the git-aware transport even for a `file://` url. + */ GIT_CLONE_LOCAL, + /** + * Do no bypass the git-aware transport + */ GIT_CLONE_NO_LOCAL, + /** + * Bypass the git-aware transport, but do not try to use + * hardlinks. + */ GIT_CLONE_LOCAL_NO_LINKS, } git_clone_local_t; @@ -36,37 +57,58 @@ typedef enum { * Use the GIT_CLONE_OPTIONS_INIT to get the default settings, like this: * * git_clone_options opts = GIT_CLONE_OPTIONS_INIT; - * - * - `checkout_opts` are option passed to the checkout step. To disable - * checkout, set the `checkout_strategy` to GIT_CHECKOUT_NONE. - * Generally you will want the use GIT_CHECKOUT_SAFE_CREATE to create - * all files in the working directory for the newly cloned repository. - * - `bare` should be set to zero (false) to create a standard repo, - * or non-zero for a bare repo - * - `ignore_cert_errors` should be set to 1 if errors validating the - * remote host's certificate should be ignored. - * - * ** "origin" remote options: ** - * - * - `remote_name` is the name to be given to the "origin" remote. The - * default is "origin". - * - `checkout_branch` gives the name of the branch to checkout. NULL - * means use the remote's HEAD. - * - `signature` is the identity used when updating the reflog. NULL means to - * use the default signature using the config. */ typedef struct git_clone_options { unsigned int version; + /** + * These options are passed to the checkout step. To disable + * checkout, set the `checkout_strategy` to + * `GIT_CHECKOUT_NONE`. Generally you will want the use + * GIT_CHECKOUT_SAFE_CREATE to create all files in the working + * directory for the newly cloned repository. + */ git_checkout_options checkout_opts; + + /** + * Callbacks to use for reporting fetch progress. + */ git_remote_callbacks remote_callbacks; + /** + * Set to zero (false) to create a standard repo, or non-zero + * for a bare repo + */ int bare; + + /** + * Set to 1 if errors validating the remote host's certificate + * should be ignored. + */ int ignore_cert_errors; + + /** + * Whether to use a fetch or copy the object database. + */ git_clone_local_t local; + + /** + * The name to be given to the remote that will be + * created. The default is "origin". + */ const char *remote_name; + + /** + * The name of the branch to checkout. NULL means use the + * remote's default branch. + */ const char* checkout_branch; + + /** + * The identity used when updating the reflog. NULL means to + * use the default signature using the config. + */ git_signature *signature; } git_clone_options; -- cgit v1.2.1 From 5fa0494328b78a1ce8dba983c3f8028d123a62a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 11 Jun 2014 23:19:48 +0200 Subject: ssl: use locking When using in a multithreaded context, OpenSSL needs to lock, and leaves it up to application to provide said locks. We were not doing this, and it's just luck that's kept us from crashing up to now. --- src/netops.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/netops.c b/src/netops.c index 7bce159ad..ba1a329e0 100644 --- a/src/netops.c +++ b/src/netops.c @@ -35,6 +35,11 @@ #include "http_parser.h" #include "global.h" +#if defined(GIT_SSL) && defined(GIT_THREADS) +/* OpenSSL wants us to keep an array of locks */ +static git_mutex *openssl_locks; +#endif + #ifdef GIT_WIN32 static void net_set_error(const char *str) { @@ -387,6 +392,24 @@ cert_fail_name: return -1; } +#ifdef GIT_THREADS +void openssl_locking_function(int mode, int n, const char *file, int line) +{ + int lock; + + GIT_UNUSED(file); + GIT_UNUSED(line); + + lock = mode & CRYPTO_LOCK; + + if (lock) { + git_mutex_lock(&openssl_locks[n]); + } else { + git_mutex_unlock(&openssl_locks[n]); + } +} +#endif + /** * The OpenSSL init functions are not reentrant so we need to init * them under lock. @@ -409,6 +432,25 @@ static int init_ssl(void) SSL_library_init(); SSL_load_error_strings(); +#ifdef GIT_THREADS + { + int num_locks, i; + + + CRYPTO_set_locking_callback(openssl_locking_function); + + num_locks = CRYPTO_num_locks(); + openssl_locks = git__calloc(num_locks, sizeof(git_mutex)); + for (i = 0; i < num_locks; i++) { + if (git_mutex_init(&openssl_locks[i]) < 0) { + git_mutex_unlock(&git__ssl_mutex); + giterr_set(GITERR_SSL, "failed to init lock %d", i); + return -1; + } + } + } +#endif + git_atomic_set(&git__ssl_init, 1); git_mutex_unlock(&git__ssl_mutex); -- cgit v1.2.1 From cf15ac8aa96304a36699ae65398b7adac0d2ddde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 12 Jun 2014 03:20:34 +0200 Subject: ssl: cargo-cult thread safety OpenSSL's tests init everything in the main thread, so let's do that. --- src/global.c | 20 ++++++++++++++++++++ src/global.h | 5 +++++ src/netops.c | 43 +++++++++++++++++++++---------------------- src/netops.h | 1 - 4 files changed, 46 insertions(+), 23 deletions(-) diff --git a/src/global.c b/src/global.c index fe410587e..e9c940f2c 100644 --- a/src/global.c +++ b/src/global.c @@ -16,6 +16,11 @@ git_mutex git__mwindow_mutex; #define MAX_SHUTDOWN_CB 8 +#ifdef GIT_SSL +# include +SSL_CTX *git__ssl_ctx; +#endif + git_mutex git__ssl_mutex; git_atomic git__ssl_init; @@ -160,6 +165,15 @@ static pthread_key_t _tls_key; static pthread_once_t _once_init = PTHREAD_ONCE_INIT; int init_error = 0; +static void init_ssl(void) +{ +#ifdef GIT_SSL + SSL_load_error_strings(); + OpenSSL_add_ssl_algorithms(); + git__ssl_ctx = SSL_CTX_new(SSLv23_method()); +#endif +} + static void cb__free_status(void *st) { git__free(st); @@ -169,12 +183,18 @@ static void init_once(void) { if ((init_error = git_mutex_init(&git__mwindow_mutex)) != 0) return; + if ((init_error = git_mutex_init(&git__ssl_mutex)) != 0) + return; pthread_key_create(&_tls_key, &cb__free_status); + /* Initialize any other subsystems that have global state */ if ((init_error = git_hash_global_init()) >= 0) init_error = git_sysdir_global_init(); + /* OpenSSL needs to be initialized from the main thread */ + init_ssl(); + GIT_MEMORY_BARRIER; } diff --git a/src/global.h b/src/global.h index 245f811e4..8904e2de5 100644 --- a/src/global.h +++ b/src/global.h @@ -15,6 +15,11 @@ typedef struct { git_error error_t; } git_global_st; +#ifdef GIT_SSL +# include +extern SSL_CTX *git__ssl_ctx; +#endif + git_global_st *git__global_state(void); extern git_mutex git__mwindow_mutex; diff --git a/src/netops.c b/src/netops.c index ba1a329e0..54804d418 100644 --- a/src/netops.c +++ b/src/netops.c @@ -163,7 +163,7 @@ void gitno_buffer_setup_callback( void gitno_buffer_setup(gitno_socket *socket, gitno_buffer *buf, char *data, size_t len) { #ifdef GIT_SSL - if (socket->ssl.ctx) { + if (socket->ssl.ssl) { gitno_buffer_setup_callback(socket, buf, data, len, gitno__recv_ssl, NULL); return; } @@ -208,7 +208,6 @@ static int gitno_ssl_teardown(gitno_ssl *ssl) ret = 0; SSL_free(ssl->ssl); - SSL_CTX_free(ssl->ctx); return ret; } @@ -428,30 +427,39 @@ static int init_ssl(void) if (git__ssl_init.val) return 0; - - SSL_library_init(); - SSL_load_error_strings(); + SSL_CTX_set_mode(git__ssl_ctx, SSL_MODE_AUTO_RETRY); + SSL_CTX_set_verify(git__ssl_ctx, SSL_VERIFY_NONE, NULL); + if (!SSL_CTX_set_default_verify_paths(git__ssl_ctx)) { + unsigned long err = ERR_get_error(); + giterr_set(GITERR_SSL, "failed to set verify paths: %s\n", ERR_error_string(err, NULL)); + return -1; + } #ifdef GIT_THREADS { int num_locks, i; - - CRYPTO_set_locking_callback(openssl_locking_function); - num_locks = CRYPTO_num_locks(); openssl_locks = git__calloc(num_locks, sizeof(git_mutex)); + if (openssl_locks == NULL) { + git_mutex_unlock(&git__ssl_mutex); + return -1; + } + GITERR_CHECK_ALLOC(openssl_locks); + for (i = 0; i < num_locks; i++) { - if (git_mutex_init(&openssl_locks[i]) < 0) { + if (git_mutex_init(&openssl_locks[i]) != 0) { git_mutex_unlock(&git__ssl_mutex); giterr_set(GITERR_SSL, "failed to init lock %d", i); return -1; } } } + + CRYPTO_set_locking_callback(openssl_locking_function); #endif - git_atomic_set(&git__ssl_init, 1); + git_atomic_inc(&git__ssl_init); git_mutex_unlock(&git__ssl_mutex); return 0; @@ -464,16 +472,7 @@ static int ssl_setup(gitno_socket *socket, const char *host, int flags) if (init_ssl() < 0) return -1; - socket->ssl.ctx = SSL_CTX_new(SSLv23_method()); - if (socket->ssl.ctx == NULL) - return ssl_set_error(&socket->ssl, 0); - - SSL_CTX_set_mode(socket->ssl.ctx, SSL_MODE_AUTO_RETRY); - SSL_CTX_set_verify(socket->ssl.ctx, SSL_VERIFY_NONE, NULL); - if (!SSL_CTX_set_default_verify_paths(socket->ssl.ctx)) - return ssl_set_error(&socket->ssl, 0); - - socket->ssl.ssl = SSL_new(socket->ssl.ctx); + socket->ssl.ssl = SSL_new(git__ssl_ctx); if (socket->ssl.ssl == NULL) return ssl_set_error(&socket->ssl, 0); @@ -610,7 +609,7 @@ int gitno_send(gitno_socket *socket, const char *msg, size_t len, int flags) size_t off = 0; #ifdef GIT_SSL - if (socket->ssl.ctx) + if (socket->ssl.ssl) return gitno_send_ssl(&socket->ssl, msg, len, flags); #endif @@ -631,7 +630,7 @@ int gitno_send(gitno_socket *socket, const char *msg, size_t len, int flags) int gitno_close(gitno_socket *s) { #ifdef GIT_SSL - if (s->ssl.ctx && + if (s->ssl.ssl && gitno_ssl_teardown(&s->ssl) < 0) return -1; #endif diff --git a/src/netops.h b/src/netops.h index 8e3a2524f..dfb4ab7b4 100644 --- a/src/netops.h +++ b/src/netops.h @@ -16,7 +16,6 @@ struct gitno_ssl { #ifdef GIT_SSL - SSL_CTX *ctx; SSL *ssl; #else size_t dummy; -- cgit v1.2.1 From 8f897b6f2f4742a7d9189528e082cfe7cecd6f29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 12 Jun 2014 14:50:08 +0200 Subject: ssl: init also without threads --- src/global.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/global.c b/src/global.c index e9c940f2c..41428ec42 100644 --- a/src/global.c +++ b/src/global.c @@ -47,6 +47,15 @@ static void git__shutdown(void) } +static void init_ssl(void) +{ +#ifdef GIT_SSL + SSL_load_error_strings(); + OpenSSL_add_ssl_algorithms(); + git__ssl_ctx = SSL_CTX_new(SSLv23_method()); +#endif +} + /** * Handle the global state with TLS * @@ -165,15 +174,6 @@ static pthread_key_t _tls_key; static pthread_once_t _once_init = PTHREAD_ONCE_INIT; int init_error = 0; -static void init_ssl(void) -{ -#ifdef GIT_SSL - SSL_load_error_strings(); - OpenSSL_add_ssl_algorithms(); - git__ssl_ctx = SSL_CTX_new(SSLv23_method()); -#endif -} - static void cb__free_status(void *st) { git__free(st); @@ -248,6 +248,7 @@ static git_global_st __state; int git_threads_init(void) { + init_ssl(); git_atomic_inc(&git__n_inits); return 0; } -- cgit v1.2.1 From 081e76bac26e1ed6adafb402d15b30c622866d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 12 Jun 2014 16:20:52 +0200 Subject: ssl: init everything all the time Bring together all of the OpenSSL initialization to git_threads_init() so it's together and doesn't need locks. Moving it here also gives us libssh2 thread safety (when built against openssl). --- src/global.c | 53 ++++++++++++++++++++++++++++++++++---- src/global.h | 2 -- src/netops.c | 83 +++--------------------------------------------------------- 3 files changed, 51 insertions(+), 87 deletions(-) diff --git a/src/global.c b/src/global.c index 41428ec42..b144b050a 100644 --- a/src/global.c +++ b/src/global.c @@ -19,11 +19,9 @@ git_mutex git__mwindow_mutex; #ifdef GIT_SSL # include SSL_CTX *git__ssl_ctx; +static git_mutex *openssl_locks; #endif -git_mutex git__ssl_mutex; -git_atomic git__ssl_init; - static git_global_shutdown_fn git__shutdown_callbacks[MAX_SHUTDOWN_CB]; static git_atomic git__n_shutdown_callbacks; static git_atomic git__n_inits; @@ -47,12 +45,59 @@ static void git__shutdown(void) } +#if defined(GIT_THREADS) && defined(GIT_SSL) +void openssl_locking_function(int mode, int n, const char *file, int line) +{ + int lock; + + GIT_UNUSED(file); + GIT_UNUSED(line); + + lock = mode & CRYPTO_LOCK; + + if (lock) { + git_mutex_lock(&openssl_locks[n]); + } else { + git_mutex_unlock(&openssl_locks[n]); + } +} +#endif + + static void init_ssl(void) { #ifdef GIT_SSL SSL_load_error_strings(); OpenSSL_add_ssl_algorithms(); git__ssl_ctx = SSL_CTX_new(SSLv23_method()); + SSL_CTX_set_mode(git__ssl_ctx, SSL_MODE_AUTO_RETRY); + SSL_CTX_set_verify(git__ssl_ctx, SSL_VERIFY_NONE, NULL); + if (!SSL_CTX_set_default_verify_paths(git__ssl_ctx)) { + SSL_CTX_free(git__ssl_ctx); + git__ssl_ctx = NULL; + } + +# ifdef GIT_THREADS + { + int num_locks, i; + + num_locks = CRYPTO_num_locks(); + openssl_locks = git__calloc(num_locks, sizeof(git_mutex)); + if (openssl_locks == NULL) { + SSL_CTX_free(git__ssl_ctx); + git__ssl_ctx = NULL; + } + + for (i = 0; i < num_locks; i++) { + if (git_mutex_init(&openssl_locks[i]) != 0) { + SSL_CTX_free(git__ssl_ctx); + git__ssl_ctx = NULL; + } + } + + CRYPTO_set_locking_callback(openssl_locking_function); + } +# endif #endif } @@ -183,8 +228,6 @@ static void init_once(void) { if ((init_error = git_mutex_init(&git__mwindow_mutex)) != 0) return; - if ((init_error = git_mutex_init(&git__ssl_mutex)) != 0) - return; pthread_key_create(&_tls_key, &cb__free_status); diff --git a/src/global.h b/src/global.h index 8904e2de5..745df3e4a 100644 --- a/src/global.h +++ b/src/global.h @@ -23,8 +23,6 @@ extern SSL_CTX *git__ssl_ctx; git_global_st *git__global_state(void); extern git_mutex git__mwindow_mutex; -extern git_mutex git__ssl_mutex; -extern git_atomic git__ssl_init; #define GIT_GLOBAL (git__global_state()) diff --git a/src/netops.c b/src/netops.c index 54804d418..965e4775d 100644 --- a/src/netops.c +++ b/src/netops.c @@ -35,11 +35,6 @@ #include "http_parser.h" #include "global.h" -#if defined(GIT_SSL) && defined(GIT_THREADS) -/* OpenSSL wants us to keep an array of locks */ -static git_mutex *openssl_locks; -#endif - #ifdef GIT_WIN32 static void net_set_error(const char *str) { @@ -391,86 +386,14 @@ cert_fail_name: return -1; } -#ifdef GIT_THREADS -void openssl_locking_function(int mode, int n, const char *file, int line) -{ - int lock; - - GIT_UNUSED(file); - GIT_UNUSED(line); - - lock = mode & CRYPTO_LOCK; - - if (lock) { - git_mutex_lock(&openssl_locks[n]); - } else { - git_mutex_unlock(&openssl_locks[n]); - } -} -#endif - -/** - * The OpenSSL init functions are not reentrant so we need to init - * them under lock. - */ -static int init_ssl(void) -{ - if (git__ssl_init.val) - return 0; - - if (git_mutex_lock(&git__ssl_mutex) < 0) { - giterr_set(GITERR_OS, "failed to acquire ssl init lock"); - return -1; - } - - /* if we had to wait for the lock, someone else did it, we can return */ - if (git__ssl_init.val) - return 0; - - SSL_CTX_set_mode(git__ssl_ctx, SSL_MODE_AUTO_RETRY); - SSL_CTX_set_verify(git__ssl_ctx, SSL_VERIFY_NONE, NULL); - if (!SSL_CTX_set_default_verify_paths(git__ssl_ctx)) { - unsigned long err = ERR_get_error(); - giterr_set(GITERR_SSL, "failed to set verify paths: %s\n", ERR_error_string(err, NULL)); - return -1; - } - -#ifdef GIT_THREADS - { - int num_locks, i; - - num_locks = CRYPTO_num_locks(); - openssl_locks = git__calloc(num_locks, sizeof(git_mutex)); - if (openssl_locks == NULL) { - git_mutex_unlock(&git__ssl_mutex); - return -1; - } - GITERR_CHECK_ALLOC(openssl_locks); - - for (i = 0; i < num_locks; i++) { - if (git_mutex_init(&openssl_locks[i]) != 0) { - git_mutex_unlock(&git__ssl_mutex); - giterr_set(GITERR_SSL, "failed to init lock %d", i); - return -1; - } - } - } - - CRYPTO_set_locking_callback(openssl_locking_function); -#endif - - git_atomic_inc(&git__ssl_init); - git_mutex_unlock(&git__ssl_mutex); - - return 0; -} - static int ssl_setup(gitno_socket *socket, const char *host, int flags) { int ret; - if (init_ssl() < 0) + if (git__ssl_ctx == NULL) { + giterr_set(GITERR_NET, "OpenSSL initialization failed"); return -1; + } socket->ssl.ssl = SSL_new(git__ssl_ctx); if (socket->ssl.ssl == NULL) -- cgit v1.2.1 From 9c3e4e97f6995bde7879a5907497c35f83ef6b56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 13 Jun 2014 02:35:33 +0200 Subject: http: fix typo in credentials logic We want to check whether the credentials callback is NULL, not whether the payload is. --- src/transports/http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transports/http.c b/src/transports/http.c index a7eff7365..ae608ab3d 100644 --- a/src/transports/http.c +++ b/src/transports/http.c @@ -260,7 +260,7 @@ static int on_headers_complete(http_parser *parser) if (parser->status_code == 401 && get_verb == s->verb) { - if (!t->owner->cred_acquire_payload) { + if (!t->owner->cred_acquire_cb) { no_callback = 1; } else { int allowed_types = 0; -- cgit v1.2.1 From 3382d8b14fca33f03c98295c07bb804cee62b7f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 13 Jun 2014 22:24:43 +0200 Subject: test: use read-only account Don't write in plaintext the password of an account which has full control over the repository. Instead use an account with read-only access. --- tests/online/clone.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/online/clone.c b/tests/online/clone.c index 8a2a64f95..4d4cdbf38 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -8,9 +8,9 @@ #define LIVE_REPO_URL "http://github.com/libgit2/TestGitRepository" #define LIVE_EMPTYREPO_URL "http://github.com/libgit2/TestEmptyRepository" -#define BB_REPO_URL "https://libgit2@bitbucket.org/libgit2/testgitrepository.git" -#define BB_REPO_URL_WITH_PASS "https://libgit2:libgit2@bitbucket.org/libgit2/testgitrepository.git" -#define BB_REPO_URL_WITH_WRONG_PASS "https://libgit2:wrong@bitbucket.org/libgit2/testgitrepository.git" +#define BB_REPO_URL "https://libgit3@bitbucket.org/libgit2/testgitrepository.git" +#define BB_REPO_URL_WITH_PASS "https://libgit3:libgit3@bitbucket.org/libgit2/testgitrepository.git" +#define BB_REPO_URL_WITH_WRONG_PASS "https://libgit3:wrong@bitbucket.org/libgit2/testgitrepository.git" #define ASSEMBLA_REPO_URL "https://libgit2:_Libgit2@git.assembla.com/libgit2-test-repos.git" static git_repository *g_repo; -- cgit v1.2.1 From 09561d33e45274eca4ecec7cca785737ed7a4397 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 13 Jun 2014 22:27:46 +0200 Subject: test: remove assembla clone test The assembla failure we were seeing referred to a private repository, which is not what is there at the moment. This reverts 1fd21b0342f --- tests/online/clone.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/online/clone.c b/tests/online/clone.c index 4d4cdbf38..4f4312a8c 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -11,7 +11,6 @@ #define BB_REPO_URL "https://libgit3@bitbucket.org/libgit2/testgitrepository.git" #define BB_REPO_URL_WITH_PASS "https://libgit3:libgit3@bitbucket.org/libgit2/testgitrepository.git" #define BB_REPO_URL_WITH_WRONG_PASS "https://libgit3:wrong@bitbucket.org/libgit2/testgitrepository.git" -#define ASSEMBLA_REPO_URL "https://libgit2:_Libgit2@git.assembla.com/libgit2-test-repos.git" static git_repository *g_repo; static git_clone_options g_options; @@ -290,11 +289,6 @@ void test_online_clone__bitbucket_style(void) cl_fixture_cleanup("./foo"); } -void test_online_clone__assembla_style(void) -{ - cl_git_pass(git_clone(&g_repo, ASSEMBLA_REPO_URL, "./foo", NULL)); -} - static int cancel_at_half(const git_transfer_progress *stats, void *payload) { GIT_UNUSED(payload); -- cgit v1.2.1