From 18234b14ad55157581ca26ec763afc1af3ec6e76 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 21 Feb 2014 09:14:16 -0800 Subject: [PATCH 01/16] Add efficient git_buf join3 API There are a few places where we need to join three strings to assemble a path. This adds a simple join3 function to avoid the comparatively expensive join_n (which calls strlen on each string twice). --- src/buffer.c | 53 +++++++++++++++++++++++++++++++++++++++++++++ src/buffer.h | 4 ++++ src/refdb_fs.c | 2 +- src/submodule.c | 9 ++++---- tests/core/buffer.c | 32 +++++++++++++++++++++++++++ 5 files changed, 95 insertions(+), 5 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index a83ca8792..83960e912 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -467,6 +467,59 @@ int git_buf_join( return 0; } +int git_buf_join3( + git_buf *buf, + char separator, + const char *str_a, + const char *str_b, + const char *str_c) +{ + size_t len_a = strlen(str_a), len_b = strlen(str_b), len_c = strlen(str_c); + int sep_a = 0, sep_b = 0; + char *tgt; + + /* for this function, disallow pointers into the existing buffer */ + assert(str_a < buf->ptr || str_a >= buf->ptr + buf->size); + assert(str_b < buf->ptr || str_b >= buf->ptr + buf->size); + assert(str_c < buf->ptr || str_c >= buf->ptr + buf->size); + + if (separator) { + if (len_a > 0) { + while (*str_b == separator) { str_b++; len_b--; } + sep_a = (str_a[len_a - 1] != separator); + } + if (len_a > 0 || len_b > 0) + while (*str_c == separator) { str_c++; len_c--; } + if (len_b > 0) + sep_b = (str_b[len_b - 1] != separator); + } + + if (git_buf_grow(buf, len_a + sep_a + len_b + sep_b + len_c + 1) < 0) + return -1; + + tgt = buf->ptr; + + if (len_a) { + memcpy(tgt, str_a, len_a); + tgt += len_a; + } + if (sep_a) + *tgt++ = separator; + if (len_b) { + memcpy(tgt, str_b, len_b); + tgt += len_b; + } + if (sep_b) + *tgt++ = separator; + if (len_c) + memcpy(tgt, str_c, len_c); + + buf->size = len_a + sep_a + len_b + sep_b + len_c; + buf->ptr[buf->size] = '\0'; + + return 0; +} + void git_buf_rtrim(git_buf *buf) { while (buf->size > 0) { diff --git a/src/buffer.h b/src/buffer.h index 4c852b3cb..dba594d97 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -98,8 +98,12 @@ void git_buf_truncate(git_buf *buf, size_t len); void git_buf_shorten(git_buf *buf, size_t amount); void git_buf_rtruncate_at_char(git_buf *path, char separator); +/** General join with separator */ int git_buf_join_n(git_buf *buf, char separator, int nbuf, ...); +/** Fast join of two strings - first may legally point into `buf` data */ int git_buf_join(git_buf *buf, char separator, const char *str_a, const char *str_b); +/** Fast join of three strings - cannot reference `buf` data */ +int git_buf_join3(git_buf *buf, char separator, const char *str_a, const char *str_b, const char *str_c); /** * Join two strings as paths, inserting a slash between as needed. diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 9120a3e87..2550b7e26 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -1432,7 +1432,7 @@ static int create_new_reflog_file(const char *filepath) GIT_INLINE(int) retrieve_reflog_path(git_buf *path, git_repository *repo, const char *name) { - return git_buf_join_n(path, '/', 3, repo->path_repository, GIT_REFLOG_DIR, name); + return git_buf_join3(path, '/', repo->path_repository, GIT_REFLOG_DIR, name); } static int refdb_reflog_fs__ensure_log(git_refdb_backend *_backend, const char *name) diff --git a/src/submodule.c b/src/submodule.c index fdcc2251a..a0ce5317f 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -168,10 +168,11 @@ int git_submodule_lookup( if (git_repository_workdir(repo)) { git_buf path = GIT_BUF_INIT; - if (git_buf_joinpath(&path, git_repository_workdir(repo), name) < 0) + if (git_buf_join3(&path, + '/', git_repository_workdir(repo), name, DOT_GIT) < 0) return -1; - if (git_path_contains(&path, DOT_GIT)) + if (git_path_exists(path.ptr)) error = GIT_EEXISTS; git_buf_free(&path); @@ -328,8 +329,8 @@ int git_submodule_add_setup( else if (use_gitlink) { git_buf repodir = GIT_BUF_INIT; - error = git_buf_join_n( - &repodir, '/', 3, git_repository_path(repo), "modules", path); + error = git_buf_join3( + &repodir, '/', git_repository_path(repo), "modules", path); if (error < 0) goto cleanup; diff --git a/tests/core/buffer.c b/tests/core/buffer.c index 0e7dd3d70..eb1d95a95 100644 --- a/tests/core/buffer.c +++ b/tests/core/buffer.c @@ -599,6 +599,38 @@ void test_core_buffer__10(void) git_buf_free(&a); } +void test_core_buffer__join3(void) +{ + git_buf a = GIT_BUF_INIT; + + cl_git_pass(git_buf_join3(&a, '/', "test", "string", "join")); + cl_assert_equal_s("test/string/join", a.ptr); + cl_git_pass(git_buf_join3(&a, '/', "test/", "string", "join")); + cl_assert_equal_s("test/string/join", a.ptr); + cl_git_pass(git_buf_join3(&a, '/', "test/", "/string", "join")); + cl_assert_equal_s("test/string/join", a.ptr); + cl_git_pass(git_buf_join3(&a, '/', "test/", "/string/", "join")); + cl_assert_equal_s("test/string/join", a.ptr); + cl_git_pass(git_buf_join3(&a, '/', "test/", "/string/", "/join")); + cl_assert_equal_s("test/string/join", a.ptr); + + cl_git_pass(git_buf_join3(&a, '/', "", "string", "join")); + cl_assert_equal_s("string/join", a.ptr); + cl_git_pass(git_buf_join3(&a, '/', "", "string/", "join")); + cl_assert_equal_s("string/join", a.ptr); + cl_git_pass(git_buf_join3(&a, '/', "", "string/", "/join")); + cl_assert_equal_s("string/join", a.ptr); + + cl_git_pass(git_buf_join3(&a, '/', "string", "", "join")); + cl_assert_equal_s("string/join", a.ptr); + cl_git_pass(git_buf_join3(&a, '/', "string/", "", "join")); + cl_assert_equal_s("string/join", a.ptr); + cl_git_pass(git_buf_join3(&a, '/', "string/", "", "/join")); + cl_assert_equal_s("string/join", a.ptr); + + git_buf_free(&a); +} + void test_core_buffer__11(void) { git_buf a = GIT_BUF_INIT; From 8286300a1e2d24dfe184316c0f9798f2c69d0ef4 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 18 Dec 2013 11:48:57 -0800 Subject: [PATCH 02/16] Fix git_submodule_sync and add new config helper This fixes `git_submodule_sync` to correctly update the remote URL of the default branch of the submodule along with the URL in the parent repository config (i.e. match core Git's behavior). Also move some useful helper logic from the submodule code into a shared config API `git_config__update_entry` that can either set or delete an entry with constraints like not overwriting or not creating a new entry. I used that helper to update a couple other places in the code. --- src/config.c | 30 +++++++++++ src/config.h | 8 +++ src/remote.c | 88 +++++++++++++----------------- src/submodule.c | 138 ++++++++++++++++++++++++++---------------------- 4 files changed, 150 insertions(+), 114 deletions(-) diff --git a/src/config.c b/src/config.c index 1a205fe13..b3168f735 100644 --- a/src/config.c +++ b/src/config.c @@ -615,6 +615,36 @@ int git_config_set_string(git_config *cfg, const char *name, const char *value) return error; } +int git_config__update_entry( + git_config *config, + const char *key, + const char *value, + bool overwrite_existing, + bool only_if_existing) +{ + int error = 0; + const git_config_entry *ce = NULL; + + if ((error = git_config__lookup_entry(&ce, config, key, false)) < 0) + return error; + + if (!ce && only_if_existing) /* entry doesn't exist */ + return 0; + if (ce && !overwrite_existing) /* entry would be overwritten */ + return 0; + if (value && ce && ce->value && !strcmp(ce->value, value)) /* no change */ + return 0; + if (!value && (!ce || !ce->value)) /* asked to delete absent entry */ + return 0; + + if (!value) + error = git_config_delete_entry(config, key); + else + error = git_config_set_string(config, key, value); + + return error; +} + /*********** * Getters ***********/ diff --git a/src/config.h b/src/config.h index 03d910616..00b6063e7 100644 --- a/src/config.h +++ b/src/config.h @@ -53,6 +53,14 @@ extern int git_config__lookup_entry( const char *key, bool no_errors); +/* internal only: update and/or delete entry string with constraints */ +extern int git_config__update_entry( + git_config *cfg, + const char *key, + const char *value, + bool overwrite_existing, + bool only_if_existing); + /* * Lookup functions that cannot fail. These functions look up a config * value and return a fallback value if the value is missing or if any diff --git a/src/remote.c b/src/remote.c index 62ee90375..53ff79707 100644 --- a/src/remote.c +++ b/src/remote.c @@ -495,9 +495,10 @@ cleanup: int git_remote_save(const git_remote *remote) { int error; - git_config *config; + git_config *cfg; const char *tagopt = NULL; git_buf buf = GIT_BUF_INIT; + const git_config_entry *existing; assert(remote); @@ -509,43 +510,31 @@ int git_remote_save(const git_remote *remote) if ((error = ensure_remote_name_is_valid(remote->name)) < 0) return error; - if (git_repository_config__weakptr(&config, remote->repo) < 0) - return -1; + if ((error = git_repository_config__weakptr(&cfg, remote->repo)) < 0) + return error; - if (git_buf_printf(&buf, "remote.%s.url", remote->name) < 0) - return -1; + if ((error = git_buf_printf(&buf, "remote.%s.url", remote->name)) < 0) + return error; - if (git_config_set_string(config, git_buf_cstr(&buf), remote->url) < 0) { - git_buf_free(&buf); - return -1; - } + /* after this point, buffer is allocated so end with cleanup */ + + if ((error = git_config_set_string( + cfg, git_buf_cstr(&buf), remote->url)) < 0) + goto cleanup; git_buf_clear(&buf); - if (git_buf_printf(&buf, "remote.%s.pushurl", remote->name) < 0) - return -1; + if ((error = git_buf_printf(&buf, "remote.%s.pushurl", remote->name)) < 0) + goto cleanup; - if (remote->pushurl) { - if (git_config_set_string(config, git_buf_cstr(&buf), remote->pushurl) < 0) { - git_buf_free(&buf); - return -1; - } - } else { - int error = git_config_delete_entry(config, git_buf_cstr(&buf)); - if (error == GIT_ENOTFOUND) { - error = 0; - giterr_clear(); - } - if (error < 0) { - git_buf_free(&buf); - return error; - } - } + if ((error = git_config__update_entry( + cfg, git_buf_cstr(&buf), remote->pushurl, true, false)) < 0) + goto cleanup; - if (update_config_refspec(remote, config, GIT_DIRECTION_FETCH) < 0) - goto on_error; + if ((error = update_config_refspec(remote, cfg, GIT_DIRECTION_FETCH)) < 0) + goto cleanup; - if (update_config_refspec(remote, config, GIT_DIRECTION_PUSH) < 0) - goto on_error; + if ((error = update_config_refspec(remote, cfg, GIT_DIRECTION_PUSH)) < 0) + goto cleanup; /* * What action to take depends on the old and new values. This @@ -561,31 +550,26 @@ int git_remote_save(const git_remote *remote) */ git_buf_clear(&buf); - if (git_buf_printf(&buf, "remote.%s.tagopt", remote->name) < 0) - goto on_error; + if ((error = git_buf_printf(&buf, "remote.%s.tagopt", remote->name)) < 0) + goto cleanup; - error = git_config_get_string(&tagopt, config, git_buf_cstr(&buf)); - if (error < 0 && error != GIT_ENOTFOUND) - goto on_error; + if ((error = git_config__lookup_entry( + &existing, cfg, git_buf_cstr(&buf), false)) < 0) + goto cleanup; - if (remote->download_tags == GIT_REMOTE_DOWNLOAD_TAGS_ALL) { - if (git_config_set_string(config, git_buf_cstr(&buf), "--tags") < 0) - goto on_error; - } else if (remote->download_tags == GIT_REMOTE_DOWNLOAD_TAGS_NONE) { - if (git_config_set_string(config, git_buf_cstr(&buf), "--no-tags") < 0) - goto on_error; - } else if (tagopt) { - if (git_config_delete_entry(config, git_buf_cstr(&buf)) < 0) - goto on_error; - } + if (remote->download_tags == GIT_REMOTE_DOWNLOAD_TAGS_ALL) + tagopt = "--tags"; + else if (remote->download_tags == GIT_REMOTE_DOWNLOAD_TAGS_NONE) + tagopt = "--no-tags"; + else if (existing != NULL) + tagopt = NULL; + error = git_config__update_entry( + cfg, git_buf_cstr(&buf), tagopt, true, false); + +cleanup: git_buf_free(&buf); - - return 0; - -on_error: - git_buf_free(&buf); - return -1; + return error; } const char *git_remote_name(const git_remote *remote) diff --git a/src/submodule.c b/src/submodule.c index a0ce5317f..dc0ea435e 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -83,7 +83,6 @@ static int lookup_head_remote(git_buf *url, git_repository *repo); static int submodule_get(git_submodule **, git_repository *, const char *, const char *); static int submodule_load_from_config(const git_config_entry *, void *); static int submodule_load_from_wd_lite(git_submodule *); -static int submodule_update_config(git_submodule *, const char *, const char *, bool, bool); static void submodule_get_index_status(unsigned int *, git_submodule *); static void submodule_get_wd_status(unsigned int *, git_submodule *, git_repository *, git_submodule_ignore_t); @@ -716,46 +715,105 @@ git_submodule_recurse_t git_submodule_set_fetch_recurse_submodules( return old; } -int git_submodule_init(git_submodule *submodule, int overwrite) +int git_submodule_init(git_submodule *sm, int overwrite) { int error; const char *val; + git_buf key = GIT_BUF_INIT; + git_config *cfg = NULL; - /* write "submodule.NAME.url" */ - - if (!submodule->url) { + if (!sm->url) { giterr_set(GITERR_SUBMODULE, - "No URL configured for submodule '%s'", submodule->name); + "No URL configured for submodule '%s'", sm->name); return -1; } - error = submodule_update_config( - submodule, "url", submodule->url, overwrite != 0, false); - if (error < 0) + if ((error = git_repository_config(&cfg, sm->repo)) < 0) return error; + /* write "submodule.NAME.url" */ + + if ((error = git_buf_printf(&key, "submodule.%s.url", sm->name)) < 0 || + (error = git_config__update_entry( + cfg, key.ptr, sm->url, overwrite != 0, false)) < 0) + goto cleanup; + /* write "submodule.NAME.update" if not default */ - val = (submodule->update == GIT_SUBMODULE_UPDATE_CHECKOUT) ? - NULL : git_submodule_update_to_str(submodule->update); - error = submodule_update_config( - submodule, "update", val, (overwrite != 0), false); + val = (sm->update == GIT_SUBMODULE_UPDATE_CHECKOUT) ? + NULL : git_submodule_update_to_str(sm->update); + + if ((error = git_buf_printf(&key, "submodule.%s.update", sm->name)) < 0 || + (error = git_config__update_entry( + cfg, key.ptr, val, overwrite != 0, false)) < 0) + goto cleanup; + + /* success */ + +cleanup: + git_config_free(cfg); + git_buf_free(&key); return error; } -int git_submodule_sync(git_submodule *submodule) +int git_submodule_sync(git_submodule *sm) { - if (!submodule->url) { + int error = 0; + git_config *cfg = NULL; + git_buf key = GIT_BUF_INIT; + + if (!sm->url) { giterr_set(GITERR_SUBMODULE, - "No URL configured for submodule '%s'", submodule->name); + "No URL configured for submodule '%s'", sm->name); return -1; } /* copy URL over to config only if it already exists */ - return submodule_update_config( - submodule, "url", submodule->url, true, true); + if (!(error = git_repository_config__weakptr(&cfg, sm->repo)) && + !(error = git_buf_printf(&key, "submodule.%s.url", sm->name))) + error = git_config__update_entry(cfg, key.ptr, sm->url, true, true); + + /* if submodule exists in the working directory, update remote url */ + + if (!error && (sm->flags & GIT_SUBMODULE_STATUS_IN_WD) != 0) { + git_repository *smrepo = NULL; + git_reference *smhead = NULL; + const char *remote = "origin"; + + if ((error = git_submodule_open(&smrepo, sm)) < 0 || + (error = git_repository_head(&smhead, smrepo)) < 0 || + (error = git_repository_config__weakptr(&cfg, smrepo)) < 0) + goto smcleanup; + + /* get remote for default branch and set remote..url */ + + if (git_reference_type(smhead) == GIT_REF_SYMBOLIC) { + const char *bname = git_reference_shorthand(smhead); + const git_config_entry *ce; + + git_buf_clear(&key); + if ((error = git_buf_printf(&key, "branch.%s.remote", bname)) < 0 || + (error = git_config__lookup_entry(&ce, cfg, key.ptr, 0)) < 0) + goto smcleanup; + + if (ce && ce->value) + remote = ce->value; + } + + git_buf_clear(&key); + if (!(error = git_buf_printf(&key, "remote.%s.url", remote))) + error = git_config__update_entry(cfg, key.ptr, sm->url, true, true); + +smcleanup: + git_reference_free(smhead); + git_repository_free(smrepo); + } + + git_buf_free(&key); + + return error; } static int git_submodule__open( @@ -1640,50 +1698,6 @@ cleanup: return error; } -static int submodule_update_config( - git_submodule *submodule, - const char *attr, - const char *value, - bool overwrite, - bool only_existing) -{ - int error; - git_config *config; - git_buf key = GIT_BUF_INIT; - const git_config_entry *ce = NULL; - - assert(submodule); - - error = git_repository_config__weakptr(&config, submodule->repo); - if (error < 0) - return error; - - error = git_buf_printf(&key, "submodule.%s.%s", submodule->name, attr); - if (error < 0) - goto cleanup; - - if ((error = git_config__lookup_entry(&ce, config, key.ptr, false)) < 0) - goto cleanup; - - if (!ce && only_existing) - goto cleanup; - if (ce && !overwrite) - goto cleanup; - if (value && ce && ce->value && !strcmp(ce->value, value)) - goto cleanup; - if (!value && (!ce || !ce->value)) - goto cleanup; - - if (!value) - error = git_config_delete_entry(config, key.ptr); - else - error = git_config_set_string(config, key.ptr, value); - -cleanup: - git_buf_free(&key); - return error; -} - static void submodule_get_index_status(unsigned int *status, git_submodule *sm) { const git_oid *head_oid = git_submodule_head_id(sm); From e402d2f134ff6ac76726bedd37ac4f1c0aa5e9ab Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 24 Mar 2014 11:25:59 -0700 Subject: [PATCH 03/16] Submodule sync refactoring Turns out there was already a helper to do what I wanted to do, so I just made it so that I could use it for sync and switched to that instead. --- src/path.h | 8 +++++ src/submodule.c | 88 +++++++++++++++++++++++-------------------------- 2 files changed, 49 insertions(+), 47 deletions(-) diff --git a/src/path.h b/src/path.h index f26175d15..2367d707b 100644 --- a/src/path.h +++ b/src/path.h @@ -119,6 +119,14 @@ GIT_INLINE(void) git_path_mkposix(char *path) # define git_path_mkposix(p) /* blank */ #endif +/** + * Check if string is a relative path (i.e. starts with "./" or "../") + */ +GIT_INLINE(int) git_path_is_relative(const char *p) +{ + return (p[0] == '.' && (p[1] == '/' || (p[1] == '.' && p[2] == '/'))); +} + extern int git__percent_decode(git_buf *decoded_out, const char *input); /** diff --git a/src/submodule.c b/src/submodule.c index dc0ea435e..69791ef58 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -79,6 +79,7 @@ __KHASH_IMPL( static int load_submodule_config(git_repository *repo, bool reload); static git_config_backend *open_gitmodules(git_repository *, bool, const git_oid *); +static int lookup_head_remote_key(git_buf *key, git_repository *repo); static int lookup_head_remote(git_buf *url, git_repository *repo); static int submodule_get(git_submodule **, git_repository *, const char *, const char *); static int submodule_load_from_config(const git_config_entry *, void *); @@ -271,8 +272,7 @@ int git_submodule_add_setup( } /* resolve parameters */ - error = git_submodule_resolve_url(&real_url, repo, url); - if (error) + if ((error = git_submodule_resolve_url(&real_url, repo, url)) < 0) goto cleanup; /* validate and normalize path */ @@ -576,7 +576,7 @@ int git_submodule_resolve_url(git_buf *out, git_repository *repo, const char *ur assert(url); - if (url[0] == '.' && (url[1] == '/' || (url[1] == '.' && url[2] == '/'))) { + if (git_path_is_relative(url)) { if (!(error = lookup_head_remote(out, repo))) error = git_path_apply_relative(out, url); } else if (strchr(url, ':') != NULL || url[0] == '/') { @@ -762,6 +762,7 @@ int git_submodule_sync(git_submodule *sm) int error = 0; git_config *cfg = NULL; git_buf key = GIT_BUF_INIT; + git_repository *smrepo = NULL; if (!sm->url) { giterr_set(GITERR_SUBMODULE, @@ -777,37 +778,17 @@ int git_submodule_sync(git_submodule *sm) /* if submodule exists in the working directory, update remote url */ - if (!error && (sm->flags & GIT_SUBMODULE_STATUS_IN_WD) != 0) { - git_repository *smrepo = NULL; - git_reference *smhead = NULL; - const char *remote = "origin"; - - if ((error = git_submodule_open(&smrepo, sm)) < 0 || - (error = git_repository_head(&smhead, smrepo)) < 0 || - (error = git_repository_config__weakptr(&cfg, smrepo)) < 0) - goto smcleanup; - - /* get remote for default branch and set remote..url */ - - if (git_reference_type(smhead) == GIT_REF_SYMBOLIC) { - const char *bname = git_reference_shorthand(smhead); - const git_config_entry *ce; - - git_buf_clear(&key); - if ((error = git_buf_printf(&key, "branch.%s.remote", bname)) < 0 || - (error = git_config__lookup_entry(&ce, cfg, key.ptr, 0)) < 0) - goto smcleanup; - - if (ce && ce->value) - remote = ce->value; + if (!error && + (sm->flags & GIT_SUBMODULE_STATUS_IN_WD) != 0 && + !(error = git_submodule_open(&smrepo, sm))) + { + if ((error = lookup_head_remote_key(&key, smrepo)) < 0) { + giterr_clear(); + git_buf_sets(&key, "branch.origin.remote"); } - git_buf_clear(&key); - if (!(error = git_buf_printf(&key, "remote.%s.url", remote))) - error = git_config__update_entry(cfg, key.ptr, sm->url, true, true); + error = git_config__update_entry(cfg, key.ptr, sm->url, true, true); -smcleanup: - git_reference_free(smhead); git_repository_free(smrepo); } @@ -1633,27 +1614,27 @@ cleanup: return error; } -static int lookup_head_remote(git_buf *url, git_repository *repo) +static int lookup_head_remote_key(git_buf *key, git_repository *repo) { int error; git_config *cfg; git_reference *head = NULL, *remote = NULL; const char *tgt, *scan; - git_buf key = GIT_BUF_INIT; /* 1. resolve HEAD -> refs/heads/BRANCH * 2. lookup config branch.BRANCH.remote -> ORIGIN - * 3. lookup remote.ORIGIN.url + * 3. return remote.ORIGIN.url */ + git_buf_clear(key); + if ((error = git_repository_config__weakptr(&cfg, repo)) < 0) return error; if (git_reference_lookup(&head, repo, GIT_HEAD_FILE) < 0) { giterr_set(GITERR_SUBMODULE, "Cannot resolve relative URL when HEAD cannot be resolved"); - error = GIT_ENOTFOUND; - goto cleanup; + return GIT_ENOTFOUND; } if (git_reference_type(head) != GIT_REF_SYMBOLIC) { @@ -1669,7 +1650,8 @@ static int lookup_head_remote(git_buf *url, git_repository *repo) /* remote should refer to something like refs/remotes/ORIGIN/BRANCH */ if (git_reference_type(remote) != GIT_REF_SYMBOLIC || - git__prefixcmp(git_reference_symbolic_target(remote), GIT_REFS_REMOTES_DIR) != 0) + git__prefixcmp( + git_reference_symbolic_target(remote), GIT_REFS_REMOTES_DIR) != 0) { giterr_set(GITERR_SUBMODULE, "Cannot resolve relative URL when HEAD is not symbolic"); @@ -1677,27 +1659,39 @@ static int lookup_head_remote(git_buf *url, git_repository *repo) goto cleanup; } - scan = tgt = git_reference_symbolic_target(remote) + strlen(GIT_REFS_REMOTES_DIR); + scan = tgt = git_reference_symbolic_target(remote) + + strlen(GIT_REFS_REMOTES_DIR); while (*scan && (*scan != '/' || (scan > tgt && scan[-1] != '\\'))) scan++; /* find non-escaped slash to end ORIGIN name */ - error = git_buf_printf(&key, "remote.%.*s.url", (int)(scan - tgt), tgt); - if (error < 0) - goto cleanup; - - if ((error = git_config_get_string(&tgt, cfg, key.ptr)) < 0) - goto cleanup; - - error = git_buf_sets(url, tgt); + error = git_buf_printf(key, "remote.%.*s.url", (int)(scan - tgt), tgt); cleanup: - git_buf_free(&key); + if (error < 0) + git_buf_clear(key); + git_reference_free(head); git_reference_free(remote); return error; } +static int lookup_head_remote(git_buf *url, git_repository *repo) +{ + int error; + git_config *cfg; + const char *tgt; + git_buf key = GIT_BUF_INIT; + + if (!(error = lookup_head_remote_key(&key, repo)) && + !(error = git_repository_config__weakptr(&cfg, repo)) && + !(error = git_config_get_string(&tgt, cfg, key.ptr))) + error = git_buf_sets(url, tgt); + + git_buf_free(&key); + return error; +} + static void submodule_get_index_status(unsigned int *status, git_submodule *sm) { const git_oid *head_oid = git_submodule_head_id(sm); From d543d59cf0d90264aa84ac49b7f21c6a603d5d3a Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 28 Mar 2014 10:42:38 -0700 Subject: [PATCH 04/16] Add some funny options for debugging status This allows you to use a --repeat option to run status over and over and see how the output changes as you make local directory changes without reopening the git_repository object each time. Also, adds a flag to explicitly list the submodules before status. --- examples/status.c | 59 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/examples/status.c b/examples/status.c index feba77f84..5f619a055 100644 --- a/examples/status.c +++ b/examples/status.c @@ -13,6 +13,7 @@ */ #include "common.h" +#include /** * This example demonstrates the use of the libgit2 status APIs, @@ -44,19 +45,22 @@ enum { #define MAX_PATHSPEC 8 struct opts { - git_status_options statusopt; - char *repodir; - char *pathspec[MAX_PATHSPEC]; - int npaths; - int format; - int zterm; - int showbranch; + git_status_options statusopt; + char *repodir; + char *pathspec[MAX_PATHSPEC]; + int npaths; + int format; + int zterm; + int showbranch; + int showsubmod; + int repeat; }; static void parse_opts(struct opts *o, int argc, char *argv[]); static void show_branch(git_repository *repo, int format); static void print_long(git_status_list *status); static void print_short(git_repository *repo, git_status_list *status); +static int print_submod(git_submodule *sm, const char *name, void *payload); int main(int argc, char *argv[]) { @@ -84,6 +88,10 @@ int main(int argc, char *argv[]) fatal("Cannot report status on bare repository", git_repository_path(repo)); +show_status: + if (o.repeat) + printf("\033[H\033[2J"); + /** * Run status on the repository * @@ -98,17 +106,29 @@ int main(int argc, char *argv[]) * about what results are presented. */ check_lg2(git_status_list_new(&status, repo, &o.statusopt), - "Could not get status", NULL); + "Could not get status", NULL); if (o.showbranch) show_branch(repo, o.format); + if (o.showsubmod) { + int submod_count = 0; + check_lg2(git_submodule_foreach(repo, print_submod, &submod_count), + "Cannot iterate submodules", o.repodir); + } + if (o.format == FORMAT_LONG) print_long(status); else print_short(repo, status); git_status_list_free(status); + + if (o.repeat) { + sleep(o.repeat); + goto show_status; + } + git_repository_free(repo); git_threads_shutdown(); @@ -381,7 +401,7 @@ static void print_short(git_repository *repo, git_status_list *status) } /** - * Now that we have all the information, it's time to format the output. + * Now that we have all the information, format the output. */ if (s->head_to_index) { @@ -417,6 +437,21 @@ static void print_short(git_repository *repo, git_status_list *status) } } +static int print_submod(git_submodule *sm, const char *name, void *payload) +{ + int *count = payload; + (void)name; + + if (*count == 0) + printf("# Submodules\n"); + (*count)++; + + printf("# - submodule '%s' at %s\n", + git_submodule_name(sm), git_submodule_path(sm)); + + return 0; +} + /** * Parse options that git's status command supports. */ @@ -462,6 +497,12 @@ static void parse_opts(struct opts *o, int argc, char *argv[]) o->statusopt.flags |= GIT_STATUS_OPT_EXCLUDE_SUBMODULES; else if (!strncmp(a, "--git-dir=", strlen("--git-dir="))) o->repodir = a + strlen("--git-dir="); + else if (!strcmp(a, "--repeat")) + o->repeat = 10; + else if (match_int_arg(&o->repeat, &args, "--repeat", 0)) + /* okay */; + else if (!strcmp(a, "--list-submodules")) + o->showsubmod = 1; else check_lg2(-1, "Unsupported option", a); } From 69b6ffc4c53d578800274993b5222fa5a7699f21 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 28 Mar 2014 14:02:21 -0700 Subject: [PATCH 05/16] Make a real submodule cache object This takes the old submodule cache which was just a git_strmap and makes a real git_submodule_cache object that can contain other things like a lock and timestamp-ish data to control refreshing of submodule info. --- src/repository.c | 2 +- src/repository.h | 9 +- src/submodule.c | 347 +++++++++++++++++++++++++++++------------------ src/submodule.h | 29 ++++ 4 files changed, 244 insertions(+), 143 deletions(-) diff --git a/src/repository.c b/src/repository.c index fccc16faa..49d1bc63e 100644 --- a/src/repository.c +++ b/src/repository.c @@ -93,6 +93,7 @@ void git_repository__cleanup(git_repository *repo) git_cache_clear(&repo->objects); git_attr_cache_flush(repo); + git_submodule_cache_free(repo); set_config(repo, NULL); set_index(repo, NULL); @@ -108,7 +109,6 @@ void git_repository_free(git_repository *repo) git_repository__cleanup(repo); git_cache_free(&repo->objects); - git_submodule_config_free(repo); git_diff_driver_registry_free(repo->diff_drivers); repo->diff_drivers = NULL; diff --git a/src/repository.h b/src/repository.h index 4e79714f1..99923b63b 100644 --- a/src/repository.h +++ b/src/repository.h @@ -19,7 +19,7 @@ #include "buffer.h" #include "object.h" #include "attrcache.h" -#include "strmap.h" +#include "submodule.h" #include "diff_driver.h" #define DOT_GIT ".git" @@ -105,10 +105,10 @@ struct git_repository { git_refdb *_refdb; git_config *_config; git_index *_index; + git_submodule_cache *_submodules; git_cache objects; git_attr_cache attrcache; - git_strmap *submodules; git_diff_driver_registry *diff_drivers; char *path_repository; @@ -149,11 +149,6 @@ int git_repository_index__weakptr(git_index **out, git_repository *repo); int git_repository__cvar(int *out, git_repository *repo, git_cvar_cached cvar); void git_repository__cvar_cache_clear(git_repository *repo); -/* - * Submodule cache - */ -extern void git_submodule_config_free(git_repository *repo); - GIT_INLINE(int) git_repository__ensure_not_bare( git_repository *repo, const char *operation_name) diff --git a/src/submodule.c b/src/submodule.c index 69791ef58..94bed8a6f 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -77,11 +77,13 @@ __KHASH_IMPL( str, static kh_inline, const char *, void *, 1, str_hash_no_trailing_slash, str_equal_no_trailing_slash); -static int load_submodule_config(git_repository *repo, bool reload); -static git_config_backend *open_gitmodules(git_repository *, bool, const git_oid *); +static int submodule_cache_init(git_repository *repo, bool refresh); +static void submodule_cache_free(git_submodule_cache *cache); + +static git_config_backend *open_gitmodules(git_submodule_cache *, bool); static int lookup_head_remote_key(git_buf *key, git_repository *repo); static int lookup_head_remote(git_buf *url, git_repository *repo); -static int submodule_get(git_submodule **, git_repository *, const char *, const char *); +static int submodule_get(git_submodule **, git_submodule_cache *, const char *, const char *); static int submodule_load_from_config(const git_config_entry *, void *); static int submodule_load_from_wd_lite(git_submodule *); static void submodule_get_index_status(unsigned int *, git_submodule *); @@ -102,7 +104,7 @@ static int submodule_config_key_trunc_puts(git_buf *key, const char *suffix) /* lookup submodule or return ENOTFOUND if it doesn't exist */ static int submodule_lookup( git_submodule **out, - git_strmap *cache, + git_submodule_cache *cache, const char *name, const char *alternate) { @@ -110,18 +112,18 @@ static int submodule_lookup( /* lock cache */ - pos = git_strmap_lookup_index(cache, name); + pos = git_strmap_lookup_index(cache->submodules, name); - if (!git_strmap_valid_index(cache, pos) && alternate) - pos = git_strmap_lookup_index(cache, alternate); + if (!git_strmap_valid_index(cache->submodules, pos) && alternate) + pos = git_strmap_lookup_index(cache->submodules, alternate); - if (!git_strmap_valid_index(cache, pos)) { + if (!git_strmap_valid_index(cache->submodules, pos)) { /* unlock cache */ return GIT_ENOTFOUND; /* don't set error - caller will cope */ } if (out != NULL) { - git_submodule *sm = git_strmap_value_at(cache, pos); + git_submodule *sm = git_strmap_value_at(cache->submodules, pos); GIT_REFCOUNT_INC(sm); *out = sm; } @@ -139,17 +141,45 @@ bool git_submodule__is_submodule(git_repository *repo, const char *name) { git_strmap *map; - if (load_submodule_config(repo, false) < 0) { + if (submodule_cache_init(repo, false) < 0) { giterr_clear(); return false; } - if (!(map = repo->submodules)) + if (!repo->_submodules || !(map = repo->_submodules->submodules)) return false; return git_strmap_valid_index(map, git_strmap_lookup_index(map, name)); } +static void submodule_set_lookup_error(int error, const char *name) +{ + if (!error) + return; + + giterr_set(GITERR_SUBMODULE, (error == GIT_ENOTFOUND) ? + "No submodule named '%s'" : + "Submodule '%s' has not been added yet", name); +} + +int git_submodule__lookup( + git_submodule **out, /* NULL if user only wants to test existence */ + git_repository *repo, + const char *name) /* trailing slash is allowed */ +{ + int error; + + assert(repo && name); + + if ((error = submodule_cache_init(repo, false)) < 0) + return error; + + if ((error = submodule_lookup(out, repo->_submodules, name, NULL)) < 0) + submodule_set_lookup_error(error, name); + + return error; +} + int git_submodule_lookup( git_submodule **out, /* NULL if user only wants to test existence */ git_repository *repo, @@ -159,10 +189,10 @@ int git_submodule_lookup( assert(repo && name); - if ((error = load_submodule_config(repo, false)) < 0) + if ((error = submodule_cache_init(repo, true)) < 0) return error; - if ((error = submodule_lookup(out, repo->submodules, name, NULL)) < 0) { + if ((error = submodule_lookup(out, repo->_submodules, name, NULL)) < 0) { /* check if a plausible submodule exists at path */ if (git_repository_workdir(repo)) { @@ -178,9 +208,7 @@ int git_submodule_lookup( git_buf_free(&path); } - giterr_set(GITERR_SUBMODULE, (error == GIT_ENOTFOUND) ? - "No submodule named '%s'" : - "Submodule '%s' has not been added yet", name); + submodule_set_lookup_error(error, name); } return error; @@ -198,10 +226,10 @@ int git_submodule_foreach( assert(repo && callback); - if ((error = load_submodule_config(repo, true)) < 0) + if ((error = submodule_cache_init(repo, true)) < 0) return error; - git_strmap_foreach_value(repo->submodules, sm, { + git_strmap_foreach_value(repo->_submodules->submodules, sm, { /* Usually the following will not come into play - it just prevents * us from issuing a callback twice for a submodule where the name * and path are not the same. @@ -224,24 +252,14 @@ int git_submodule_foreach( return error; } -void git_submodule_config_free(git_repository *repo) +void git_submodule_cache_free(git_repository *repo) { - git_strmap *smcfg; - git_submodule *sm; + git_submodule_cache *cache; assert(repo); - smcfg = repo->submodules; - repo->submodules = NULL; - - if (smcfg == NULL) - return; - - git_strmap_foreach_value(smcfg, sm, { - sm->repo = NULL; /* disconnect from repo */; - git_submodule_free(sm); - }); - git_strmap_free(smcfg); + if ((cache = git__swap(repo->_submodules, NULL)) != NULL) + submodule_cache_free(cache); } int git_submodule_add_setup( @@ -262,12 +280,11 @@ int git_submodule_add_setup( /* see if there is already an entry for this submodule */ - if (git_submodule_lookup(&sm, repo, path) < 0) + if (git_submodule_lookup(NULL, repo, path) < 0) giterr_clear(); else { - git_submodule_free(sm); giterr_set(GITERR_SUBMODULE, - "Attempt to add a submodule that already exists"); + "Attempt to add submodule '%s' that already exists", path); return GIT_EEXISTS; } @@ -288,7 +305,7 @@ int git_submodule_add_setup( /* update .gitmodules */ - if ((mods = open_gitmodules(repo, true, NULL)) == NULL) { + if ((mods = open_gitmodules(repo->_submodules, true)) == NULL) { giterr_set(GITERR_SUBMODULE, "Adding submodules to a bare repository is not supported (for now)"); return -1; @@ -348,7 +365,7 @@ int git_submodule_add_setup( /* add submodule to hash and "reload" it */ - if (!(error = submodule_get(&sm, repo, path, NULL)) && + if (!(error = submodule_get(&sm, repo->_submodules, path, NULL)) && !(error = git_submodule_reload(sm, false))) error = git_submodule_init(sm, false); @@ -489,7 +506,7 @@ int git_submodule_save(git_submodule *submodule) assert(submodule); - mods = open_gitmodules(submodule->repo, true, NULL); + mods = open_gitmodules(submodule->repo->_submodules, true); if (!mods) { giterr_set(GITERR_SUBMODULE, "Adding submodules to a bare repository is not supported (for now)"); @@ -862,7 +879,7 @@ int git_submodule_open(git_repository **subrepo, git_submodule *sm) } static void submodule_cache_remove_item( - git_strmap *cache, + git_strmap *map, const char *name, git_submodule *expected, bool free_after_remove) @@ -870,21 +887,21 @@ static void submodule_cache_remove_item( khiter_t pos; git_submodule *found; - if (!cache) + if (!map) return; - pos = git_strmap_lookup_index(cache, name); + pos = git_strmap_lookup_index(map, name); - if (!git_strmap_valid_index(cache, pos)) + if (!git_strmap_valid_index(map, pos)) return; - found = git_strmap_value_at(cache, pos); + found = git_strmap_value_at(map, pos); if (expected && found != expected) return; - git_strmap_set_value_at(cache, pos, NULL); - git_strmap_delete_at(cache, pos); + git_strmap_set_value_at(map, pos, NULL); + git_strmap_delete_at(map, pos); if (free_after_remove) git_submodule_free(found); @@ -894,27 +911,31 @@ int git_submodule_reload_all(git_repository *repo, int force) { int error = 0; git_submodule *sm; + git_strmap *map; GIT_UNUSED(force); assert(repo); - if (repo->submodules) - git_strmap_foreach_value(repo->submodules, sm, { sm->flags = 0; }); + if (repo->_submodules) { + map = repo->_submodules->submodules; + git_strmap_foreach_value(map, sm, { sm->flags = 0; }); + } - if ((error = load_submodule_config(repo, true)) < 0) + if ((error = submodule_cache_init(repo, true)) < 0) return error; - git_strmap_foreach_value(repo->submodules, sm, { - git_strmap *cache = repo->submodules; + if (!repo->_submodules || !(map = repo->_submodules->submodules)) + return error; + git_strmap_foreach_value(map, sm, { if (sm && (sm->flags & GIT_SUBMODULE_STATUS__IN_FLAGS) == 0) { /* we must check path != name before first remove, in case * that call frees the submodule */ bool free_as_path = (sm->path != sm->name); - submodule_cache_remove_item(cache, sm->name, sm, true); + submodule_cache_remove_item(map, sm->name, sm, true); if (free_as_path) - submodule_cache_remove_item(cache, sm->path, sm, true); + submodule_cache_remove_item(map, sm->path, sm, true); } }); @@ -995,41 +1016,44 @@ static int submodule_update_head(git_submodule *submodule) } -int git_submodule_reload(git_submodule *submodule, int force) +int git_submodule_reload(git_submodule *sm, int force) { int error = 0; git_config_backend *mods; + git_submodule_cache *cache; GIT_UNUSED(force); - assert(submodule); + assert(sm); + + cache = sm->repo->_submodules; /* refresh index data */ - if ((error = submodule_update_index(submodule)) < 0) + if ((error = submodule_update_index(sm)) < 0) return error; /* refresh HEAD tree data */ - if ((error = submodule_update_head(submodule)) < 0) + if ((error = submodule_update_head(sm)) < 0) return error; /* done if bare */ - if (git_repository_is_bare(submodule->repo)) + if (git_repository_is_bare(sm->repo)) return error; /* refresh config data */ - mods = open_gitmodules(submodule->repo, false, NULL); + mods = open_gitmodules(cache, false); if (mods != NULL) { git_buf path = GIT_BUF_INIT; git_buf_sets(&path, "submodule\\."); - git_buf_text_puts_escape_regex(&path, submodule->name); + git_buf_text_puts_escape_regex(&path, sm->name); git_buf_puts(&path, ".*"); if (git_buf_oom(&path)) error = -1; else error = git_config_file_foreach_match( - mods, path.ptr, submodule_load_from_config, submodule->repo); + mods, path.ptr, submodule_load_from_config, cache); git_buf_free(&path); git_config_file_free(mods); @@ -1039,9 +1063,9 @@ int git_submodule_reload(git_submodule *submodule, int force) } /* refresh wd data */ - submodule->flags &= ~GIT_SUBMODULE_STATUS__ALL_WD_FLAGS; + sm->flags &= ~GIT_SUBMODULE_STATUS__ALL_WD_FLAGS; - return submodule_load_from_wd_lite(submodule); + return submodule_load_from_wd_lite(sm); } static void submodule_copy_oid_maybe( @@ -1135,34 +1159,35 @@ int git_submodule_location(unsigned int *location, git_submodule *sm) * INTERNAL FUNCTIONS */ -static git_submodule *submodule_alloc(git_repository *repo, const char *name) +static int submodule_alloc( + git_submodule **out, git_submodule_cache *cache, const char *name) { size_t namelen; git_submodule *sm; if (!name || !(namelen = strlen(name))) { giterr_set(GITERR_SUBMODULE, "Invalid submodule name"); - return NULL; + return -1; } sm = git__calloc(1, sizeof(git_submodule)); - if (sm == NULL) - return NULL; + GITERR_CHECK_ALLOC(sm); sm->name = sm->path = git__strdup(name); if (!sm->name) { git__free(sm); - return NULL; + return -1; } GIT_REFCOUNT_INC(sm); sm->ignore = sm->ignore_default = GIT_SUBMODULE_IGNORE_NONE; sm->update = sm->update_default = GIT_SUBMODULE_UPDATE_CHECKOUT; sm->fetch_recurse = sm->fetch_recurse_default = GIT_SUBMODULE_RECURSE_NO; - sm->repo = repo; + sm->repo = cache->repo; sm->branch = NULL; - return sm; + *out = sm; + return 0; } static void submodule_release(git_submodule *sm) @@ -1170,11 +1195,15 @@ static void submodule_release(git_submodule *sm) if (!sm) return; - if (sm->repo) { - git_strmap *cache = sm->repo->submodules; - submodule_cache_remove_item(cache, sm->name, sm, false); - if (sm->path != sm->name) - submodule_cache_remove_item(cache, sm->path, sm, false); + if (sm->repo && sm->repo->_submodules) { + git_strmap *map = sm->repo->_submodules->submodules; + bool free_as_path = (sm->path != sm->name); + + sm->repo = NULL; + + submodule_cache_remove_item(map, sm->name, sm, false); + if (free_as_path) + submodule_cache_remove_item(map, sm->path, sm, false); } if (sm->path != sm->name) @@ -1195,40 +1224,39 @@ void git_submodule_free(git_submodule *sm) static int submodule_get( git_submodule **out, - git_repository *repo, + git_submodule_cache *cache, const char *name, const char *alternate) { int error = 0; - git_strmap *smcfg = repo->submodules; khiter_t pos; git_submodule *sm; - pos = git_strmap_lookup_index(smcfg, name); + pos = git_strmap_lookup_index(cache->submodules, name); - if (!git_strmap_valid_index(smcfg, pos) && alternate) - pos = git_strmap_lookup_index(smcfg, alternate); + if (!git_strmap_valid_index(cache->submodules, pos) && alternate) + pos = git_strmap_lookup_index(cache->submodules, alternate); - if (!git_strmap_valid_index(smcfg, pos)) { - sm = submodule_alloc(repo, name); - GITERR_CHECK_ALLOC(sm); + if (!git_strmap_valid_index(cache->submodules, pos)) { + if ((error = submodule_alloc(&sm, cache, name)) < 0) + return error; /* insert value at name - if another thread beats us to it, then use * their record and release our own. */ - pos = kh_put(str, smcfg, sm->name, &error); + pos = kh_put(str, cache->submodules, sm->name, &error); if (error < 0) goto done; else if (error == 0) { git_submodule_free(sm); - sm = git_strmap_value_at(smcfg, pos); + sm = git_strmap_value_at(cache->submodules, pos); } else { error = 0; - git_strmap_set_value_at(smcfg, pos, sm); + git_strmap_set_value_at(cache->submodules, pos, sm); } } else { - sm = git_strmap_value_at(smcfg, pos); + sm = git_strmap_value_at(cache->submodules, pos); } done: @@ -1294,8 +1322,7 @@ int git_submodule_parse_recurse(git_submodule_recurse_t *out, const char *value) static int submodule_load_from_config( const git_config_entry *entry, void *payload) { - git_repository *repo = payload; - git_strmap *smcfg = repo->submodules; + git_submodule_cache *cache = payload; const char *namestart, *property, *alternate = NULL; const char *key = entry->name, *value = entry->value, *path; git_buf name = GIT_BUF_INIT; @@ -1315,7 +1342,7 @@ static int submodule_load_from_config( path = !strcasecmp(property, "path") ? value : NULL; if ((error = git_buf_set(&name, namestart, property - namestart - 1)) < 0 || - (error = submodule_get(&sm, repo, name.ptr, path)) < 0) + (error = submodule_get(&sm, cache, name.ptr, path)) < 0) goto done; sm->flags |= GIT_SUBMODULE_STATUS_IN_CONFIG; @@ -1341,7 +1368,7 @@ static int submodule_load_from_config( /* Found a alternate key for the submodule */ if (alternate) { void *old_sm = NULL; - git_strmap_insert2(smcfg, alternate, sm, old_sm, error); + git_strmap_insert2(cache->submodules, alternate, sm, old_sm, error); if (error < 0) goto done; @@ -1423,36 +1450,35 @@ static int submodule_load_from_wd_lite(git_submodule *sm) return 0; } -static int load_submodule_config_from_index( - git_repository *repo, git_oid *gitmodules_oid) +static int submodule_cache_refresh_from_index(git_submodule_cache *cache) { int error; git_index *index; git_iterator *i; const git_index_entry *entry; - if ((error = git_repository_index__weakptr(&index, repo)) < 0 || + if ((error = git_repository_index__weakptr(&index, cache->repo)) < 0 || (error = git_iterator_for_index(&i, index, 0, NULL, NULL)) < 0) return error; while (!(error = git_iterator_advance(&entry, i))) { - khiter_t pos = git_strmap_lookup_index(repo->submodules, entry->path); + khiter_t pos = git_strmap_lookup_index(cache->submodules, entry->path); git_submodule *sm; - if (git_strmap_valid_index(repo->submodules, pos)) { - sm = git_strmap_value_at(repo->submodules, pos); + if (git_strmap_valid_index(cache->submodules, pos)) { + sm = git_strmap_value_at(cache->submodules, pos); if (S_ISGITLINK(entry->mode)) submodule_update_from_index_entry(sm, entry); else sm->flags |= GIT_SUBMODULE_STATUS__INDEX_NOT_SUBMODULE; } else if (S_ISGITLINK(entry->mode)) { - if (!submodule_get(&sm, repo, entry->path, NULL)) { + if (!submodule_get(&sm, cache, entry->path, NULL)) { submodule_update_from_index_entry(sm, entry); git_submodule_free(sm); } } else if (strcmp(entry->path, GIT_MODULES_FILE) == 0) - git_oid_cpy(gitmodules_oid, &entry->id); + git_oid_cpy(&cache->gitmodules_id, &entry->id); } if (error == GIT_ITEROVER) @@ -1463,8 +1489,7 @@ static int load_submodule_config_from_index( return error; } -static int load_submodule_config_from_head( - git_repository *repo, git_oid *gitmodules_oid) +static int submodule_cache_refresh_from_head(git_submodule_cache *cache) { int error; git_tree *head; @@ -1472,7 +1497,7 @@ static int load_submodule_config_from_head( const git_index_entry *entry; /* if we can't look up current head, then there's no submodule in it */ - if (git_repository_head_tree(&head, repo) < 0) { + if (git_repository_head_tree(&head, cache->repo) < 0) { giterr_clear(); return 0; } @@ -1483,11 +1508,11 @@ static int load_submodule_config_from_head( } while (!(error = git_iterator_advance(&entry, i))) { - khiter_t pos = git_strmap_lookup_index(repo->submodules, entry->path); + khiter_t pos = git_strmap_lookup_index(cache->submodules, entry->path); git_submodule *sm; - if (git_strmap_valid_index(repo->submodules, pos)) { - sm = git_strmap_value_at(repo->submodules, pos); + if (git_strmap_valid_index(cache->submodules, pos)) { + sm = git_strmap_value_at(cache->submodules, pos); if (S_ISGITLINK(entry->mode)) submodule_update_from_head_data( @@ -1495,14 +1520,14 @@ static int load_submodule_config_from_head( else sm->flags |= GIT_SUBMODULE_STATUS__HEAD_NOT_SUBMODULE; } else if (S_ISGITLINK(entry->mode)) { - if (!submodule_get(&sm, repo, entry->path, NULL)) { + if (!submodule_get(&sm, cache, entry->path, NULL)) { submodule_update_from_head_data( sm, entry->mode, &entry->id); git_submodule_free(sm); } } else if (strcmp(entry->path, GIT_MODULES_FILE) == 0 && - git_oid_iszero(gitmodules_oid)) { - git_oid_cpy(gitmodules_oid, &entry->id); + git_oid_iszero(&cache->gitmodules_id)) { + git_oid_cpy(&cache->gitmodules_id, &entry->id); } } @@ -1516,11 +1541,10 @@ static int load_submodule_config_from_head( } static git_config_backend *open_gitmodules( - git_repository *repo, - bool okay_to_create, - const git_oid *gitmodules_oid) + git_submodule_cache *cache, + bool okay_to_create) { - const char *workdir = git_repository_workdir(repo); + const char *workdir = git_repository_workdir(cache->repo); git_buf path = GIT_BUF_INIT; git_config_backend *mods = NULL; @@ -1540,7 +1564,7 @@ static git_config_backend *open_gitmodules( } } - if (!mods && gitmodules_oid && !git_oid_iszero(gitmodules_oid)) { + if (!mods && !git_oid_iszero(&cache->gitmodules_id)) { /* TODO: Retrieve .gitmodules content from ODB */ /* Should we actually do this? Core git does not, but it means you @@ -1553,53 +1577,86 @@ static git_config_backend *open_gitmodules( return mods; } -static int load_submodule_config(git_repository *repo, bool reload) +static void submodule_cache_free(git_submodule_cache *cache) +{ + git_submodule *sm; + + if (!cache) + return; + + git_strmap_foreach_value(cache->submodules, sm, { + sm->repo = NULL; /* disconnect from repo */ + git_submodule_free(sm); + }); + git_strmap_free(cache->submodules); + + git_buf_free(&cache->gitmodules_path); + git_mutex_free(&cache->lock); + git__free(cache); +} + +static int submodule_cache_alloc( + git_submodule_cache **out, git_repository *repo) +{ + git_submodule_cache *cache = git__calloc(1, sizeof(git_submodule_cache)); + GITERR_CHECK_ALLOC(cache); + + if (git_mutex_init(&cache->lock) < 0) { + giterr_set(GITERR_OS, "Unable to initialize submodule cache lock"); + git__free(cache); + return -1; + } + + cache->submodules = git_strmap_alloc(); + if (!cache->submodules) { + submodule_cache_free(cache); + return -1; + } + + cache->repo = repo; + git_buf_init(&cache->gitmodules_path, 0); + + *out = cache; + return 0; +} + +static int submodule_cache_refresh(git_submodule_cache *cache, bool force) { int error; - git_oid gitmodules_oid; git_config_backend *mods = NULL; - if (!reload && repo->submodules) - return 0; - - memset(&gitmodules_oid, 0, sizeof(gitmodules_oid)); - - /* Submodule data is kept in a hashtable keyed by both name and path. - * These are usually the same, but that is not guaranteed. - */ - if (!repo->submodules) { - repo->submodules = git_strmap_alloc(); - GITERR_CHECK_ALLOC(repo->submodules); - } + GIT_UNUSED(force); /* TODO: only do the following if the sources appear modified */ + memset(&cache->gitmodules_id, 0, sizeof(cache->gitmodules_id)); + /* add submodule information from index */ - if ((error = load_submodule_config_from_index(repo, &gitmodules_oid)) < 0) + if ((error = submodule_cache_refresh_from_index(cache)) < 0) goto cleanup; /* add submodule information from HEAD */ - if ((error = load_submodule_config_from_head(repo, &gitmodules_oid)) < 0) + if ((error = submodule_cache_refresh_from_head(cache)) < 0) goto cleanup; /* add submodule information from .gitmodules */ - if ((mods = open_gitmodules(repo, false, &gitmodules_oid)) != NULL && + if ((mods = open_gitmodules(cache, false)) != NULL && (error = git_config_file_foreach( - mods, submodule_load_from_config, repo)) < 0) + mods, submodule_load_from_config, cache)) < 0) goto cleanup; /* shallow scan submodules in work tree */ - if (!git_repository_is_bare(repo)) { + if (!git_repository_is_bare(cache->repo)) { git_submodule *sm; - git_strmap_foreach_value(repo->submodules, sm, { + git_strmap_foreach_value(cache->submodules, sm, { sm->flags &= ~GIT_SUBMODULE_STATUS__ALL_WD_FLAGS; }); - git_strmap_foreach_value(repo->submodules, sm, { + git_strmap_foreach_value(cache->submodules, sm, { submodule_load_from_wd_lite(sm); }); } @@ -1608,8 +1665,28 @@ cleanup: if (mods != NULL) git_config_file_free(mods); - if (error) - git_submodule_config_free(repo); + /* TODO: if we got an error, mark submodule config as invalid? */ + + return error; +} + +static int submodule_cache_init(git_repository *repo, bool refresh) +{ + int error = 0; + git_submodule_cache *cache = NULL; + + /* if submodules already exist, just refresh as requested */ + if (repo->_submodules) + return refresh ? submodule_cache_refresh(repo->_submodules, false) : 0; + + /* otherwise create a new cache, load it, and atomically swap it in */ + if (!(error = submodule_cache_alloc(&cache, repo)) && + !(error = submodule_cache_refresh(cache, true))) + cache = git__compare_and_swap(&repo->_submodules, NULL, cache); + + /* might have raced with another thread to set cache, so free if needed */ + if (cache) + submodule_cache_free(cache); return error; } diff --git a/src/submodule.h b/src/submodule.h index 1c41897e3..4b9828a9c 100644 --- a/src/submodule.h +++ b/src/submodule.h @@ -99,6 +99,30 @@ struct git_submodule { git_oid wd_oid; }; +/** + * The git_submodule_cache stores known submodules along with timestamps, + * etc. about when they were loaded + */ +typedef struct { + git_repository *repo; + git_strmap *submodules; + git_mutex lock; + + /* cache invalidation data */ + git_oid head_id; + git_futils_filestamp index_stamp; + git_buf gitmodules_path; + git_futils_filestamp gitmodules_stamp; + git_oid gitmodules_id; + git_futils_filestamp config_stamp; +} git_submodule_cache; + +/* Force revalidation of submodule data cache (alloc as needed) */ +extern int git_submodule_cache_refresh(git_repository *repo); + +/* Release all submodules */ +extern void git_submodule_cache_free(git_repository *repo); + /* Additional flags on top of public GIT_SUBMODULE_STATUS values */ enum { GIT_SUBMODULE_STATUS__WD_SCANNED = (1u << 20), @@ -122,6 +146,10 @@ enum { /* Internal submodule check does not attempt to refresh cached data */ extern bool git_submodule__is_submodule(git_repository *repo, const char *name); +/* Internal lookup does not attempt to refresh cached data */ +extern int git_submodule__lookup( + git_submodule **out, git_repository *repo, const char *path); + /* Internal status fn returns status and optionally the various OIDs */ extern int git_submodule__status( unsigned int *out_status, @@ -143,5 +171,6 @@ extern int git_submodule_parse_update( extern const char *git_submodule_ignore_to_str(git_submodule_ignore_t); extern const char *git_submodule_update_to_str(git_submodule_update_t); +extern const char *git_submodule_recurse_to_str(git_submodule_recurse_t); #endif From db0e7878d386e40080d4004e483e4845b15f8bd7 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Fri, 28 Mar 2014 16:50:49 -0700 Subject: [PATCH 06/16] Make submodule refresh a bit smarter This makes submodule cache refresh actually look at the timestamps from the data sources for submodules and reload as needed if they have changed since the last refresh. --- src/config_file.h | 3 +- src/index.c | 10 +++ src/index.h | 7 +++ src/submodule.c | 153 +++++++++++++++++++++++++++++----------------- src/submodule.h | 6 -- 5 files changed, 115 insertions(+), 64 deletions(-) diff --git a/src/config_file.h b/src/config_file.h index d4a1a4061..fcccbd5cc 100644 --- a/src/config_file.h +++ b/src/config_file.h @@ -16,7 +16,8 @@ GIT_INLINE(int) git_config_file_open(git_config_backend *cfg, unsigned int level GIT_INLINE(void) git_config_file_free(git_config_backend *cfg) { - cfg->free(cfg); + if (cfg) + cfg->free(cfg); } GIT_INLINE(int) git_config_file_get_string( diff --git a/src/index.c b/src/index.c index ea0815e4c..6cc8ea1a3 100644 --- a/src/index.c +++ b/src/index.c @@ -517,6 +517,16 @@ int git_index_read(git_index *index, int force) return error; } +int git_index__changed_relative_to( + git_index *index, const git_futils_filestamp *fs) +{ + /* attempt to update index (ignoring errors) */ + if (git_index_read(index, false) < 0) + giterr_clear(); + + return (memcmp(&index->stamp, fs, sizeof(index->stamp)) == 0); +} + int git_index_write(git_index *index) { git_filebuf file = GIT_FILEBUF_INIT; diff --git a/src/index.h b/src/index.h index 259a3149f..17f04f0ad 100644 --- a/src/index.h +++ b/src/index.h @@ -62,4 +62,11 @@ extern void git_index__set_ignore_case(git_index *index, bool ignore_case); extern unsigned int git_index__create_mode(unsigned int mode); +GIT_INLINE(const git_futils_filestamp *) git_index__filestamp(git_index *index) +{ + return &index->stamp; +} + +extern int git_index__changed_relative_to(git_index *index, const git_futils_filestamp *fs); + #endif diff --git a/src/submodule.c b/src/submodule.c index 94bed8a6f..5588a4f69 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -133,6 +133,18 @@ static int submodule_lookup( return 0; } +/* clear a set of flags on all submodules */ +static void submodule_cache_clear_flags( + git_submodule_cache *cache, uint32_t mask) +{ + git_submodule *sm; + uint32_t inverted_mask = ~mask; + + git_strmap_foreach_value(cache->submodules, sm, { + sm->flags &= inverted_mask; + }); +} + /* * PUBLIC APIS */ @@ -377,8 +389,7 @@ cleanup: if (out != NULL) *out = sm; - if (mods != NULL) - git_config_file_free(mods); + git_config_file_free(mods); git_repository_free(subrepo); git_buf_free(&real_url); git_buf_free(&name); @@ -556,8 +567,7 @@ int git_submodule_save(git_submodule *submodule) submodule->flags |= GIT_SUBMODULE_STATUS_IN_CONFIG; cleanup: - if (mods != NULL) - git_config_file_free(mods); + git_config_file_free(mods); git_buf_free(&key); return error; @@ -916,10 +926,8 @@ int git_submodule_reload_all(git_repository *repo, int force) GIT_UNUSED(force); assert(repo); - if (repo->_submodules) { - map = repo->_submodules->submodules; - git_strmap_foreach_value(map, sm, { sm->flags = 0; }); - } + if (repo->_submodules) + submodule_cache_clear_flags(repo->_submodules, 0xFFFFFFFFu); if ((error = submodule_cache_init(repo, true)) < 0) return error; @@ -1063,7 +1071,9 @@ int git_submodule_reload(git_submodule *sm, int force) } /* refresh wd data */ - sm->flags &= ~GIT_SUBMODULE_STATUS__ALL_WD_FLAGS; + sm->flags &= + ~(GIT_SUBMODULE_STATUS_IN_WD | GIT_SUBMODULE_STATUS__WD_OID_VALID | + GIT_SUBMODULE_STATUS__WD_FLAGS); return submodule_load_from_wd_lite(sm); } @@ -1450,15 +1460,14 @@ static int submodule_load_from_wd_lite(git_submodule *sm) return 0; } -static int submodule_cache_refresh_from_index(git_submodule_cache *cache) +static int submodule_cache_refresh_from_index( + git_submodule_cache *cache, git_index *idx) { int error; - git_index *index; git_iterator *i; const git_index_entry *entry; - if ((error = git_repository_index__weakptr(&index, cache->repo)) < 0 || - (error = git_iterator_for_index(&i, index, 0, NULL, NULL)) < 0) + if ((error = git_iterator_for_index(&i, idx, 0, NULL, NULL)) < 0) return error; while (!(error = git_iterator_advance(&entry, i))) { @@ -1477,8 +1486,7 @@ static int submodule_cache_refresh_from_index(git_submodule_cache *cache) submodule_update_from_index_entry(sm, entry); git_submodule_free(sm); } - } else if (strcmp(entry->path, GIT_MODULES_FILE) == 0) - git_oid_cpy(&cache->gitmodules_id, &entry->id); + } } if (error == GIT_ITEROVER) @@ -1489,23 +1497,15 @@ static int submodule_cache_refresh_from_index(git_submodule_cache *cache) return error; } -static int submodule_cache_refresh_from_head(git_submodule_cache *cache) +static int submodule_cache_refresh_from_head( + git_submodule_cache *cache, git_tree *head) { int error; - git_tree *head; git_iterator *i; const git_index_entry *entry; - /* if we can't look up current head, then there's no submodule in it */ - if (git_repository_head_tree(&head, cache->repo) < 0) { - giterr_clear(); - return 0; - } - - if ((error = git_iterator_for_tree(&i, head, 0, NULL, NULL)) < 0) { - git_tree_free(head); + if ((error = git_iterator_for_tree(&i, head, 0, NULL, NULL)) < 0) return error; - } while (!(error = git_iterator_advance(&entry, i))) { khiter_t pos = git_strmap_lookup_index(cache->submodules, entry->path); @@ -1515,8 +1515,7 @@ static int submodule_cache_refresh_from_head(git_submodule_cache *cache) sm = git_strmap_value_at(cache->submodules, pos); if (S_ISGITLINK(entry->mode)) - submodule_update_from_head_data( - sm, entry->mode, &entry->id); + submodule_update_from_head_data(sm, entry->mode, &entry->id); else sm->flags |= GIT_SUBMODULE_STATUS__HEAD_NOT_SUBMODULE; } else if (S_ISGITLINK(entry->mode)) { @@ -1525,9 +1524,6 @@ static int submodule_cache_refresh_from_head(git_submodule_cache *cache) sm, entry->mode, &entry->id); git_submodule_free(sm); } - } else if (strcmp(entry->path, GIT_MODULES_FILE) == 0 && - git_oid_iszero(&cache->gitmodules_id)) { - git_oid_cpy(&cache->gitmodules_id, &entry->id); } } @@ -1535,7 +1531,6 @@ static int submodule_cache_refresh_from_head(git_submodule_cache *cache) error = 0; git_iterator_free(i); - git_tree_free(head); return error; } @@ -1564,14 +1559,6 @@ static git_config_backend *open_gitmodules( } } - if (!mods && !git_oid_iszero(&cache->gitmodules_id)) { - /* TODO: Retrieve .gitmodules content from ODB */ - - /* Should we actually do this? Core git does not, but it means you - * can't really get much information about submodules on bare repos. - */ - } - git_buf_free(&path); return mods; @@ -1622,51 +1609,103 @@ static int submodule_cache_alloc( static int submodule_cache_refresh(git_submodule_cache *cache, bool force) { - int error; + int error = 0, updates = 0, changed; git_config_backend *mods = NULL; + const char *wd; + git_index *idx = NULL; + git_tree *head = NULL; + git_buf path = GIT_BUF_INIT; - GIT_UNUSED(force); + if (git_mutex_lock(&cache->lock) < 0) { + giterr_set(GITERR_OS, "Unable to acquire lock on submodule cache"); + return -1; + } /* TODO: only do the following if the sources appear modified */ - memset(&cache->gitmodules_id, 0, sizeof(cache->gitmodules_id)); - /* add submodule information from index */ - if ((error = submodule_cache_refresh_from_index(cache)) < 0) - goto cleanup; + if (!git_repository_index(&idx, cache->repo)) { + if (force || git_index__changed_relative_to(idx, &cache->index_stamp)) { + if ((error = submodule_cache_refresh_from_index(cache, idx)) < 0) + goto cleanup; + + updates += 1; + git_futils_filestamp_set( + &cache->index_stamp, git_index__filestamp(idx)); + } + } else { + giterr_clear(); + + submodule_cache_clear_flags( + cache, GIT_SUBMODULE_STATUS_IN_INDEX | + GIT_SUBMODULE_STATUS__INDEX_FLAGS | + GIT_SUBMODULE_STATUS__INDEX_OID_VALID); + } /* add submodule information from HEAD */ - if ((error = submodule_cache_refresh_from_head(cache)) < 0) - goto cleanup; + if (!git_repository_head_tree(&head, cache->repo)) { + if (force || !git_oid_equal(&cache->head_id, git_tree_id(head))) { + if ((error = submodule_cache_refresh_from_head(cache, head)) < 0) + goto cleanup; + + updates += 1; + git_oid_cpy(&cache->head_id, git_tree_id(head)); + } + } else { + giterr_clear(); + + submodule_cache_clear_flags( + cache, GIT_SUBMODULE_STATUS_IN_HEAD | + GIT_SUBMODULE_STATUS__HEAD_OID_VALID); + } /* add submodule information from .gitmodules */ - if ((mods = open_gitmodules(cache, false)) != NULL && - (error = git_config_file_foreach( - mods, submodule_load_from_config, cache)) < 0) + wd = git_repository_workdir(cache->repo); + + if (wd && (error = git_buf_joinpath(&path, wd, GIT_MODULES_FILE)) < 0) goto cleanup; + changed = git_futils_filestamp_check(&cache->gitmodules_stamp, path.ptr); + if (changed < 0) { + giterr_clear(); + submodule_cache_clear_flags(cache, GIT_SUBMODULE_STATUS_IN_CONFIG); + } else if (changed > 0 && (mods = open_gitmodules(cache, false)) != NULL) { + if ((error = git_config_file_foreach( + mods, submodule_load_from_config, cache)) < 0) + goto cleanup; + updates += 1; + } + /* shallow scan submodules in work tree */ - if (!git_repository_is_bare(cache->repo)) { + if (wd && updates > 0) { git_submodule *sm; - git_strmap_foreach_value(cache->submodules, sm, { - sm->flags &= ~GIT_SUBMODULE_STATUS__ALL_WD_FLAGS; - }); + submodule_cache_clear_flags( + cache, GIT_SUBMODULE_STATUS_IN_WD | + GIT_SUBMODULE_STATUS__WD_SCANNED | + GIT_SUBMODULE_STATUS__WD_FLAGS | + GIT_SUBMODULE_STATUS__WD_OID_VALID); + git_strmap_foreach_value(cache->submodules, sm, { submodule_load_from_wd_lite(sm); }); } cleanup: - if (mods != NULL) - git_config_file_free(mods); + git_config_file_free(mods); /* TODO: if we got an error, mark submodule config as invalid? */ + git_mutex_unlock(&cache->lock); + + git_index_free(idx); + git_tree_free(head); + git_buf_free(&path); + return error; } diff --git a/src/submodule.h b/src/submodule.h index 4b9828a9c..a6182beca 100644 --- a/src/submodule.h +++ b/src/submodule.h @@ -113,7 +113,6 @@ typedef struct { git_futils_filestamp index_stamp; git_buf gitmodules_path; git_futils_filestamp gitmodules_stamp; - git_oid gitmodules_id; git_futils_filestamp config_stamp; } git_submodule_cache; @@ -135,11 +134,6 @@ enum { GIT_SUBMODULE_STATUS__INDEX_MULTIPLE_ENTRIES = (1u << 27), }; -#define GIT_SUBMODULE_STATUS__ALL_WD_FLAGS \ - (GIT_SUBMODULE_STATUS_IN_WD | \ - GIT_SUBMODULE_STATUS__WD_OID_VALID | \ - GIT_SUBMODULE_STATUS__WD_FLAGS) - #define GIT_SUBMODULE_STATUS__CLEAR_INTERNAL(S) \ ((S) & ~(0xFFFFFFFFu << 20)) From a4ccd2b00199306889585b23845368455ff348f4 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Sat, 29 Mar 2014 15:23:01 -0700 Subject: [PATCH 07/16] Use enums instead of bools for submodule options When forcing cache flushes or reload, etc., it is easier to keep track of intent using enums instead of plain bools. Also, this fixes a bug where the cache was not being properly refreshes by a git_submodule_reload_all. --- src/submodule.c | 57 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 20 deletions(-) diff --git a/src/submodule.c b/src/submodule.c index 5588a4f69..7b06c25c2 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -49,6 +49,16 @@ static git_cvar_map _sm_recurse_map[] = { {GIT_CVAR_TRUE, NULL, GIT_SUBMODULE_RECURSE_YES}, }; +enum { + CACHE_OK = 0, + CACHE_REFRESH = 1, + CACHE_FLUSH = 2 +}; +enum { + GITMODULES_EXISTING = 0, + GITMODULES_CREATE = 1, +}; + static kh_inline khint_t str_hash_no_trailing_slash(const char *s) { khint_t h; @@ -77,10 +87,10 @@ __KHASH_IMPL( str, static kh_inline, const char *, void *, 1, str_hash_no_trailing_slash, str_equal_no_trailing_slash); -static int submodule_cache_init(git_repository *repo, bool refresh); +static int submodule_cache_init(git_repository *repo, int refresh); static void submodule_cache_free(git_submodule_cache *cache); -static git_config_backend *open_gitmodules(git_submodule_cache *, bool); +static git_config_backend *open_gitmodules(git_submodule_cache *, int gitmod); static int lookup_head_remote_key(git_buf *key, git_repository *repo); static int lookup_head_remote(git_buf *url, git_repository *repo); static int submodule_get(git_submodule **, git_submodule_cache *, const char *, const char *); @@ -153,7 +163,7 @@ bool git_submodule__is_submodule(git_repository *repo, const char *name) { git_strmap *map; - if (submodule_cache_init(repo, false) < 0) { + if (submodule_cache_init(repo, CACHE_OK) < 0) { giterr_clear(); return false; } @@ -183,7 +193,7 @@ int git_submodule__lookup( assert(repo && name); - if ((error = submodule_cache_init(repo, false)) < 0) + if ((error = submodule_cache_init(repo, CACHE_OK)) < 0) return error; if ((error = submodule_lookup(out, repo->_submodules, name, NULL)) < 0) @@ -201,7 +211,7 @@ int git_submodule_lookup( assert(repo && name); - if ((error = submodule_cache_init(repo, true)) < 0) + if ((error = submodule_cache_init(repo, CACHE_REFRESH)) < 0) return error; if ((error = submodule_lookup(out, repo->_submodules, name, NULL)) < 0) { @@ -238,7 +248,7 @@ int git_submodule_foreach( assert(repo && callback); - if ((error = submodule_cache_init(repo, true)) < 0) + if ((error = submodule_cache_init(repo, CACHE_REFRESH)) < 0) return error; git_strmap_foreach_value(repo->_submodules->submodules, sm, { @@ -317,9 +327,9 @@ int git_submodule_add_setup( /* update .gitmodules */ - if ((mods = open_gitmodules(repo->_submodules, true)) == NULL) { + if (!(mods = open_gitmodules(repo->_submodules, GITMODULES_CREATE))) { giterr_set(GITERR_SUBMODULE, - "Adding submodules to a bare repository is not supported (for now)"); + "Adding submodules to a bare repository is not supported"); return -1; } @@ -517,10 +527,10 @@ int git_submodule_save(git_submodule *submodule) assert(submodule); - mods = open_gitmodules(submodule->repo->_submodules, true); + mods = open_gitmodules(submodule->repo->_submodules, GITMODULES_CREATE); if (!mods) { giterr_set(GITERR_SUBMODULE, - "Adding submodules to a bare repository is not supported (for now)"); + "Adding submodules to a bare repository is not supported"); return -1; } @@ -929,7 +939,7 @@ int git_submodule_reload_all(git_repository *repo, int force) if (repo->_submodules) submodule_cache_clear_flags(repo->_submodules, 0xFFFFFFFFu); - if ((error = submodule_cache_init(repo, true)) < 0) + if ((error = submodule_cache_init(repo, CACHE_FLUSH)) < 0) return error; if (!repo->_submodules || !(map = repo->_submodules->submodules)) @@ -1049,7 +1059,7 @@ int git_submodule_reload(git_submodule *sm, int force) return error; /* refresh config data */ - mods = open_gitmodules(cache, false); + mods = open_gitmodules(cache, GITMODULES_EXISTING); if (mods != NULL) { git_buf path = GIT_BUF_INIT; @@ -1537,7 +1547,7 @@ static int submodule_cache_refresh_from_head( static git_config_backend *open_gitmodules( git_submodule_cache *cache, - bool okay_to_create) + int okay_to_create) { const char *workdir = git_repository_workdir(cache->repo); git_buf path = GIT_BUF_INIT; @@ -1607,15 +1617,19 @@ static int submodule_cache_alloc( return 0; } -static int submodule_cache_refresh(git_submodule_cache *cache, bool force) +static int submodule_cache_refresh(git_submodule_cache *cache, int refresh) { - int error = 0, updates = 0, changed; + int error = 0, updates = 0, flush, changed; git_config_backend *mods = NULL; const char *wd; git_index *idx = NULL; git_tree *head = NULL; git_buf path = GIT_BUF_INIT; + if (!refresh) + return 0; + flush = (refresh == CACHE_FLUSH); + if (git_mutex_lock(&cache->lock) < 0) { giterr_set(GITERR_OS, "Unable to acquire lock on submodule cache"); return -1; @@ -1626,7 +1640,7 @@ static int submodule_cache_refresh(git_submodule_cache *cache, bool force) /* add submodule information from index */ if (!git_repository_index(&idx, cache->repo)) { - if (force || git_index__changed_relative_to(idx, &cache->index_stamp)) { + if (flush || git_index__changed_relative_to(idx, &cache->index_stamp)) { if ((error = submodule_cache_refresh_from_index(cache, idx)) < 0) goto cleanup; @@ -1646,7 +1660,7 @@ static int submodule_cache_refresh(git_submodule_cache *cache, bool force) /* add submodule information from HEAD */ if (!git_repository_head_tree(&head, cache->repo)) { - if (force || !git_oid_equal(&cache->head_id, git_tree_id(head))) { + if (flush || !git_oid_equal(&cache->head_id, git_tree_id(head))) { if ((error = submodule_cache_refresh_from_head(cache, head)) < 0) goto cleanup; @@ -1669,6 +1683,9 @@ static int submodule_cache_refresh(git_submodule_cache *cache, bool force) goto cleanup; changed = git_futils_filestamp_check(&cache->gitmodules_stamp, path.ptr); + if (flush && !changed) + changed = 1; + if (changed < 0) { giterr_clear(); submodule_cache_clear_flags(cache, GIT_SUBMODULE_STATUS_IN_CONFIG); @@ -1709,18 +1726,18 @@ cleanup: return error; } -static int submodule_cache_init(git_repository *repo, bool refresh) +static int submodule_cache_init(git_repository *repo, int cache_refresh) { int error = 0; git_submodule_cache *cache = NULL; /* if submodules already exist, just refresh as requested */ if (repo->_submodules) - return refresh ? submodule_cache_refresh(repo->_submodules, false) : 0; + return submodule_cache_refresh(repo->_submodules, cache_refresh); /* otherwise create a new cache, load it, and atomically swap it in */ if (!(error = submodule_cache_alloc(&cache, repo)) && - !(error = submodule_cache_refresh(cache, true))) + !(error = submodule_cache_refresh(cache, CACHE_FLUSH))) cache = git__compare_and_swap(&repo->_submodules, NULL, cache); /* might have raced with another thread to set cache, so free if needed */ From eeeb9654f064e1047ef8f3cb5a38636133426cdd Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Sun, 30 Mar 2014 15:35:56 -0700 Subject: [PATCH 08/16] Reinstate efficient submodule reloading This makes it so that git_submodule_reload_all will actually only reload changed items unless the `force` flag is used. --- src/submodule.c | 238 +++++++++++++++++++++++------------------------- 1 file changed, 113 insertions(+), 125 deletions(-) diff --git a/src/submodule.c b/src/submodule.c index 7b06c25c2..96b445ace 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -898,66 +898,9 @@ int git_submodule_open(git_repository **subrepo, git_submodule *sm) return git_submodule__open(subrepo, sm, false); } -static void submodule_cache_remove_item( - git_strmap *map, - const char *name, - git_submodule *expected, - bool free_after_remove) -{ - khiter_t pos; - git_submodule *found; - - if (!map) - return; - - pos = git_strmap_lookup_index(map, name); - - if (!git_strmap_valid_index(map, pos)) - return; - - found = git_strmap_value_at(map, pos); - - if (expected && found != expected) - return; - - git_strmap_set_value_at(map, pos, NULL); - git_strmap_delete_at(map, pos); - - if (free_after_remove) - git_submodule_free(found); -} - int git_submodule_reload_all(git_repository *repo, int force) { - int error = 0; - git_submodule *sm; - git_strmap *map; - - GIT_UNUSED(force); - assert(repo); - - if (repo->_submodules) - submodule_cache_clear_flags(repo->_submodules, 0xFFFFFFFFu); - - if ((error = submodule_cache_init(repo, CACHE_FLUSH)) < 0) - return error; - - if (!repo->_submodules || !(map = repo->_submodules->submodules)) - return error; - - git_strmap_foreach_value(map, sm, { - if (sm && (sm->flags & GIT_SUBMODULE_STATUS__IN_FLAGS) == 0) { - /* we must check path != name before first remove, in case - * that call frees the submodule */ - bool free_as_path = (sm->path != sm->name); - - submodule_cache_remove_item(map, sm->name, sm, true); - if (free_as_path) - submodule_cache_remove_item(map, sm->path, sm, true); - } - }); - - return error; + return submodule_cache_init(repo, force ? CACHE_FLUSH : CACHE_REFRESH); } static void submodule_update_from_index_entry( @@ -1210,20 +1153,49 @@ static int submodule_alloc( return 0; } +static void submodule_cache_remove_item( + git_submodule_cache *cache, + git_submodule *item, + bool free_after_remove) +{ + git_strmap *map; + const char *name, *alt; + + if (!cache || !(map = cache->submodules) || !item) + return; + + name = item->name; + alt = (item->path != item->name) ? item->path : NULL; + + for (; name; name = alt, alt = NULL) { + khiter_t pos = git_strmap_lookup_index(map, name); + git_submodule *found; + + if (!git_strmap_valid_index(map, pos)) + continue; + + found = git_strmap_value_at(map, pos); + + if (found != item) + continue; + + git_strmap_set_value_at(map, pos, NULL); + git_strmap_delete_at(map, pos); + + if (free_after_remove) + git_submodule_free(found); + } +} + static void submodule_release(git_submodule *sm) { if (!sm) return; - if (sm->repo && sm->repo->_submodules) { - git_strmap *map = sm->repo->_submodules->submodules; - bool free_as_path = (sm->path != sm->name); - + if (sm->repo) { + git_submodule_cache *cache = sm->repo->_submodules; sm->repo = NULL; - - submodule_cache_remove_item(map, sm->name, sm, false); - if (free_as_path) - submodule_cache_remove_item(map, sm->path, sm, false); + submodule_cache_remove_item(cache, sm, false); } if (sm->path != sm->name) @@ -1619,99 +1591,115 @@ static int submodule_cache_alloc( static int submodule_cache_refresh(git_submodule_cache *cache, int refresh) { - int error = 0, updates = 0, flush, changed; - git_config_backend *mods = NULL; - const char *wd; + int error = 0, update_index, update_head, update_gitmod; git_index *idx = NULL; git_tree *head = NULL; + const char *wd = NULL; git_buf path = GIT_BUF_INIT; + git_submodule *sm; + git_config_backend *mods = NULL; + uint32_t mask; - if (!refresh) + if (!cache || !cache->repo || !refresh) return 0; - flush = (refresh == CACHE_FLUSH); if (git_mutex_lock(&cache->lock) < 0) { giterr_set(GITERR_OS, "Unable to acquire lock on submodule cache"); return -1; } - /* TODO: only do the following if the sources appear modified */ + /* get sources that we will need to check */ - /* add submodule information from index */ - - if (!git_repository_index(&idx, cache->repo)) { - if (flush || git_index__changed_relative_to(idx, &cache->index_stamp)) { - if ((error = submodule_cache_refresh_from_index(cache, idx)) < 0) - goto cleanup; - - updates += 1; - git_futils_filestamp_set( - &cache->index_stamp, git_index__filestamp(idx)); - } - } else { + if (git_repository_index(&idx, cache->repo) < 0) + giterr_clear(); + if (git_repository_head_tree(&head, cache->repo) < 0) giterr_clear(); - submodule_cache_clear_flags( - cache, GIT_SUBMODULE_STATUS_IN_INDEX | + wd = git_repository_workdir(cache->repo); + if (wd && (error = git_buf_joinpath(&path, wd, GIT_MODULES_FILE)) < 0) + goto cleanup; + + /* check for invalidation */ + + if (refresh == CACHE_FLUSH) + update_index = update_head = update_gitmod = true; + else { + update_index = + !idx || git_index__changed_relative_to(idx, &cache->index_stamp); + update_head = + !head || !git_oid_equal(&cache->head_id, git_tree_id(head)); + + update_gitmod = (wd != NULL) ? + git_futils_filestamp_check(&cache->gitmodules_stamp, path.ptr) : + (cache->gitmodules_stamp.mtime != 0); + if (update_gitmod < 0) + giterr_clear(); + } + + /* clear submodule flags that are to be refreshed */ + + mask = 0; + if (!idx || update_index) + mask |= GIT_SUBMODULE_STATUS_IN_INDEX | GIT_SUBMODULE_STATUS__INDEX_FLAGS | - GIT_SUBMODULE_STATUS__INDEX_OID_VALID); + GIT_SUBMODULE_STATUS__INDEX_OID_VALID | + GIT_SUBMODULE_STATUS__INDEX_MULTIPLE_ENTRIES; + if (!head || update_head) + mask |= GIT_SUBMODULE_STATUS_IN_HEAD | + GIT_SUBMODULE_STATUS__HEAD_OID_VALID; + if (update_gitmod) + mask |= GIT_SUBMODULE_STATUS_IN_CONFIG; + if (mask != 0) + mask |= GIT_SUBMODULE_STATUS_IN_WD | + GIT_SUBMODULE_STATUS__WD_SCANNED | + GIT_SUBMODULE_STATUS__WD_FLAGS | + GIT_SUBMODULE_STATUS__WD_OID_VALID; + + submodule_cache_clear_flags(cache, mask); + + /* add back submodule information from index */ + + if (idx && update_index) { + if ((error = submodule_cache_refresh_from_index(cache, idx)) < 0) + goto cleanup; + + git_futils_filestamp_set( + &cache->index_stamp, git_index__filestamp(idx)); } /* add submodule information from HEAD */ - if (!git_repository_head_tree(&head, cache->repo)) { - if (flush || !git_oid_equal(&cache->head_id, git_tree_id(head))) { - if ((error = submodule_cache_refresh_from_head(cache, head)) < 0) - goto cleanup; + if (head && update_head) { + if ((error = submodule_cache_refresh_from_head(cache, head)) < 0) + goto cleanup; - updates += 1; - git_oid_cpy(&cache->head_id, git_tree_id(head)); - } - } else { - giterr_clear(); - - submodule_cache_clear_flags( - cache, GIT_SUBMODULE_STATUS_IN_HEAD | - GIT_SUBMODULE_STATUS__HEAD_OID_VALID); + git_oid_cpy(&cache->head_id, git_tree_id(head)); } /* add submodule information from .gitmodules */ - wd = git_repository_workdir(cache->repo); - - if (wd && (error = git_buf_joinpath(&path, wd, GIT_MODULES_FILE)) < 0) - goto cleanup; - - changed = git_futils_filestamp_check(&cache->gitmodules_stamp, path.ptr); - if (flush && !changed) - changed = 1; - - if (changed < 0) { - giterr_clear(); - submodule_cache_clear_flags(cache, GIT_SUBMODULE_STATUS_IN_CONFIG); - } else if (changed > 0 && (mods = open_gitmodules(cache, false)) != NULL) { - if ((error = git_config_file_foreach( + if (wd && update_gitmod > 0) { + if ((mods = open_gitmodules(cache, false)) != NULL && + (error = git_config_file_foreach( mods, submodule_load_from_config, cache)) < 0) goto cleanup; - updates += 1; } - /* shallow scan submodules in work tree */ - - if (wd && updates > 0) { - git_submodule *sm; - - submodule_cache_clear_flags( - cache, GIT_SUBMODULE_STATUS_IN_WD | - GIT_SUBMODULE_STATUS__WD_SCANNED | - GIT_SUBMODULE_STATUS__WD_FLAGS | - GIT_SUBMODULE_STATUS__WD_OID_VALID); + /* shallow scan submodules in work tree as needed */ + if (wd && mask != 0) { git_strmap_foreach_value(cache->submodules, sm, { submodule_load_from_wd_lite(sm); }); } + /* remove submodules that no longer exist */ + + git_strmap_foreach_value(cache->submodules, sm, { + if (sm && (sm->flags & GIT_SUBMODULE_STATUS__IN_FLAGS) == 0) + submodule_cache_remove_item(cache, sm, true); + }); + cleanup: git_config_file_free(mods); From aa78c9ba778d8c7c2c62f875b974ae53ba90d12b Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 1 Apr 2014 10:22:51 -0700 Subject: [PATCH 09/16] Minor submodule cache locking improvements This improvement the management of the lock around submodule cache updates slightly, using the lock to make sure that foreach can safely make a snapshot of all existing submodules and making sure that git_submodule_add_setup also grabs a lock before inserting the new submodule. Cache initialization / refresh should already have been holding the lock correctly as it adds submodules. --- src/submodule.c | 60 ++++++++++++++++++++++++++++++++++++------------- src/vector.c | 2 +- 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/src/submodule.c b/src/submodule.c index 96b445ace..5061cadc6 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -236,40 +236,62 @@ int git_submodule_lookup( return error; } +static void submodule_free_dup(void *sm) +{ + git_submodule_free(sm); +} + int git_submodule_foreach( git_repository *repo, int (*callback)(git_submodule *sm, const char *name, void *payload), void *payload) { int error; + size_t i; git_submodule *sm; - git_vector seen = GIT_VECTOR_INIT; - git_vector_set_cmp(&seen, submodule_cmp); + git_submodule_cache *cache; + git_vector snapshot = GIT_VECTOR_INIT; assert(repo && callback); if ((error = submodule_cache_init(repo, CACHE_REFRESH)) < 0) return error; - git_strmap_foreach_value(repo->_submodules->submodules, sm, { - /* Usually the following will not come into play - it just prevents - * us from issuing a callback twice for a submodule where the name - * and path are not the same. - */ - if (GIT_REFCOUNT_VAL(sm) > 1) { - if (git_vector_bsearch(NULL, &seen, sm) != GIT_ENOTFOUND) - continue; - if ((error = git_vector_insert(&seen, sm)) < 0) - break; - } + cache = repo->_submodules; + if (git_mutex_lock(&cache->lock) < 0) { + giterr_set(GITERR_OS, "Unable to acquire lock on submodule cache"); + return -1; + } + + if (!(error = git_vector_init( + &snapshot, kh_size(cache->submodules), submodule_cmp))) { + + git_strmap_foreach_value(cache->submodules, sm, { + if ((error = git_vector_insert(&snapshot, sm)) < 0) + break; + GIT_REFCOUNT_INC(sm); + }); + } + + git_mutex_unlock(&cache->lock); + + if (error < 0) + goto done; + + git_vector_uniq(&snapshot, submodule_free_dup); + + git_vector_foreach(&snapshot, i, sm) { if ((error = callback(sm, sm->name, payload)) != 0) { giterr_set_after_callback(error); break; } - }); + } - git_vector_free(&seen); +done: + git_vector_foreach(&snapshot, i, sm) + git_submodule_free(sm); + git_vector_free(&snapshot); return error; } @@ -387,10 +409,18 @@ int git_submodule_add_setup( /* add submodule to hash and "reload" it */ + if (git_mutex_lock(&repo->_submodules->lock) < 0) { + giterr_set(GITERR_OS, "Unable to acquire lock on submodule cache"); + error = -1; + goto cleanup; + } + if (!(error = submodule_get(&sm, repo->_submodules, path, NULL)) && !(error = git_submodule_reload(sm, false))) error = git_submodule_init(sm, false); + git_mutex_unlock(&repo->_submodules->lock); + cleanup: if (error && sm) { git_submodule_free(sm); diff --git a/src/vector.c b/src/vector.c index e5d8919d3..c2c67e6b8 100644 --- a/src/vector.c +++ b/src/vector.c @@ -54,7 +54,7 @@ int git_vector_dup(git_vector *v, const git_vector *src, git_vector_cmp cmp) bytes = src->length * sizeof(void *); v->_alloc_size = src->length; - v->_cmp = cmp; + v->_cmp = cmp ? cmp : src->_cmp; v->length = src->length; v->flags = src->flags; if (cmp != src->_cmp) From 4ece3e225b566816598238902667552dee4c7deb Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 1 Apr 2014 12:19:11 -0700 Subject: [PATCH 10/16] Fix submodule accounting for name and path changes Wrote tests that try adding, removing, and updating the name of submodules which showed a number of problems with how we account for changes when incrementally updating the submodule info. Most of these issues didn't exist before because reloading would always blow away the old submodule data. --- src/submodule.c | 50 ++++++++++++++++++++++++------ tests/submodule/nosubs.c | 67 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 10 deletions(-) diff --git a/src/submodule.c b/src/submodule.c index 5061cadc6..913c97a87 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -1345,8 +1345,9 @@ static int submodule_load_from_config( const git_config_entry *entry, void *payload) { git_submodule_cache *cache = payload; - const char *namestart, *property, *alternate = NULL; + const char *namestart, *property; const char *key = entry->name, *value = entry->value, *path; + char *alternate = NULL, *replaced = NULL; git_buf name = GIT_BUF_INIT; git_submodule *sm = NULL; int error = 0; @@ -1377,17 +1378,39 @@ static int submodule_load_from_config( * should be strcasecmp */ - if (strcmp(sm->name, name.ptr) != 0) { - alternate = sm->name = git_buf_detach(&name); - } else if (path && strcmp(path, sm->path) != 0) { - alternate = sm->path = git__strdup(value); - if (!sm->path) { - error = -1; - goto done; + if (strcmp(sm->name, name.ptr) != 0) { /* name changed */ + if (!strcmp(sm->path, name.ptr)) { /* already set as path */ + replaced = sm->name; + sm->name = sm->path; + } else { + if (sm->name != sm->path) + replaced = sm->name; + alternate = sm->name = git_buf_detach(&name); + } + } + else if (path && strcmp(path, sm->path) != 0) { /* path changed */ + if (!strcmp(sm->name, value)) { /* already set as name */ + replaced = sm->path; + sm->path = sm->name; + } else { + if (sm->path != sm->name) + replaced = sm->path; + if ((alternate = git__strdup(value)) == NULL) { + error = -1; + goto done; + } + sm->path = alternate; } } - /* Found a alternate key for the submodule */ + /* Deregister under name being replaced */ + if (replaced) { + git_strmap_delete(cache->submodules, replaced); + git_submodule_free(sm); + git__free(replaced); + } + + /* Insert under alternate key */ if (alternate) { void *old_sm = NULL; git_strmap_insert2(cache->submodules, alternate, sm, old_sm, error); @@ -1684,6 +1707,8 @@ static int submodule_cache_refresh(git_submodule_cache *cache, int refresh) GIT_SUBMODULE_STATUS__WD_SCANNED | GIT_SUBMODULE_STATUS__WD_FLAGS | GIT_SUBMODULE_STATUS__WD_OID_VALID; + else + goto cleanup; /* nothing to do */ submodule_cache_clear_flags(cache, mask); @@ -1726,7 +1751,12 @@ static int submodule_cache_refresh(git_submodule_cache *cache, int refresh) /* remove submodules that no longer exist */ git_strmap_foreach_value(cache->submodules, sm, { - if (sm && (sm->flags & GIT_SUBMODULE_STATUS__IN_FLAGS) == 0) + /* purge unless in HEAD, index, or .gitmodules; no sm for wd only */ + if (sm != NULL && + !(sm->flags & + (GIT_SUBMODULE_STATUS_IN_HEAD | + GIT_SUBMODULE_STATUS_IN_INDEX | + GIT_SUBMODULE_STATUS_IN_CONFIG))) submodule_cache_remove_item(cache, sm, true); }); diff --git a/tests/submodule/nosubs.c b/tests/submodule/nosubs.c index 5ef4f42ab..cabb53ead 100644 --- a/tests/submodule/nosubs.c +++ b/tests/submodule/nosubs.c @@ -2,6 +2,7 @@ #include "clar_libgit2.h" #include "posix.h" +#include "fileops.h" void test_submodule_nosubs__cleanup(void) { @@ -93,3 +94,69 @@ void test_submodule_nosubs__bad_gitmodules(void) cl_git_pass(git_submodule_lookup(NULL, repo, "foobar")); cl_assert_equal_i(GIT_ENOTFOUND, git_submodule_lookup(NULL, repo, "subdir")); } + +void test_submodule_nosubs__add_and_delete(void) +{ + git_repository *repo = cl_git_sandbox_init("status"); + git_submodule *sm; + git_buf buf = GIT_BUF_INIT; + + /* note the lack of calls to git_submodule_reload - this *should* work */ + + cl_git_fail(git_submodule_lookup(NULL, repo, "libgit2")); + cl_git_fail(git_submodule_lookup(NULL, repo, "submodules/libgit2")); + + /* create */ + + cl_git_pass(git_submodule_add_setup( + &sm, repo, "https://github.com/libgit2/libgit2.git", "submodules/libgit2", 1)); + cl_assert_equal_s("submodules/libgit2", git_submodule_name(sm)); + cl_assert_equal_s("submodules/libgit2", git_submodule_path(sm)); + git_submodule_free(sm); + + cl_git_pass(git_futils_readbuffer(&buf, "status/.gitmodules")); + cl_assert(strstr(buf.ptr, "[submodule \"submodules/libgit2\"]") != NULL); + cl_assert(strstr(buf.ptr, "path = submodules/libgit2") != NULL); + git_buf_free(&buf); + + /* lookup */ + + cl_git_fail(git_submodule_lookup(&sm, repo, "libgit2")); + cl_git_pass(git_submodule_lookup(&sm, repo, "submodules/libgit2")); + cl_assert_equal_s("submodules/libgit2", git_submodule_name(sm)); + cl_assert_equal_s("submodules/libgit2", git_submodule_path(sm)); + git_submodule_free(sm); + + /* update name */ + + cl_git_rewritefile( + "status/.gitmodules", + "[submodule \"libgit2\"]\n" + " path = submodules/libgit2\n" + " url = https://github.com/libgit2/libgit2.git\n"); + + cl_git_pass(git_submodule_lookup(&sm, repo, "libgit2")); + cl_assert_equal_s("libgit2", git_submodule_name(sm)); + cl_assert_equal_s("submodules/libgit2", git_submodule_path(sm)); + git_submodule_free(sm); + cl_git_pass(git_submodule_lookup(&sm, repo, "submodules/libgit2")); + git_submodule_free(sm); + + /* revert name update */ + + cl_git_rewritefile( + "status/.gitmodules", + "[submodule \"submodules/libgit2\"]\n" + " path = submodules/libgit2\n" + " url = https://github.com/libgit2/libgit2.git\n"); + + cl_git_fail(git_submodule_lookup(&sm, repo, "libgit2")); + cl_git_pass(git_submodule_lookup(&sm, repo, "submodules/libgit2")); + git_submodule_free(sm); + + /* remove completely */ + + cl_must_pass(p_unlink("status/.gitmodules")); + cl_git_fail(git_submodule_lookup(&sm, repo, "libgit2")); + cl_git_fail(git_submodule_lookup(&sm, repo, "submodules/libgit2")); +} From 8061d519b33c95a8858752e7d70d40fe8bae90f9 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 1 Apr 2014 13:24:06 -0700 Subject: [PATCH 11/16] Remove most submodule reloads from tests With the new submodule cache validity checks, we generally don't need to call git_submodule_reload_all to have up-to-date submodule data. Some tests are still calling it where I want to actually test that it can be called safely and doesn't break anything, but mostly it is not needed. This also expands some of the existing submodule tests to cover some variants on the behavior that was already being tested. --- tests/diff/submodules.c | 5 --- tests/submodule/lookup.c | 68 +++++++++++++++++++++++----------------- tests/submodule/nosubs.c | 18 +++++++++-- 3 files changed, 56 insertions(+), 35 deletions(-) diff --git a/tests/diff/submodules.c b/tests/diff/submodules.c index 2881f74be..02870ac86 100644 --- a/tests/diff/submodules.c +++ b/tests/diff/submodules.c @@ -131,8 +131,6 @@ void test_diff_submodules__dirty_submodule_2(void) g_repo = setup_fixture_submodules(); - cl_git_pass(git_submodule_reload_all(g_repo, 1)); - opts.flags = GIT_DIFF_INCLUDE_UNTRACKED | GIT_DIFF_SHOW_UNTRACKED_CONTENT | GIT_DIFF_RECURSE_UNTRACKED_DIRS | @@ -165,8 +163,6 @@ void test_diff_submodules__dirty_submodule_2(void) git_diff_free(diff); - cl_git_pass(git_submodule_reload_all(g_repo, 1)); - cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts)); check_diff_patches(diff, expected_dirty); git_diff_free(diff); @@ -299,7 +295,6 @@ void test_diff_submodules__invalid_cache(void) git_submodule_free(sm); - cl_git_pass(git_submodule_reload_all(g_repo, 1)); cl_git_pass(git_submodule_lookup(&sm, g_repo, smpath)); cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts)); diff --git a/tests/submodule/lookup.c b/tests/submodule/lookup.c index 36bde4f6e..86ba25c3a 100644 --- a/tests/submodule/lookup.c +++ b/tests/submodule/lookup.c @@ -1,7 +1,7 @@ #include "clar_libgit2.h" #include "submodule_helpers.h" -#include "posix.h" #include "git2/sys/repository.h" +#include "fileops.h" static git_repository *g_repo = NULL; @@ -115,13 +115,7 @@ void test_submodule_lookup__lookup_even_with_unborn_head(void) &head, g_repo, "HEAD", "refs/heads/garbage", 1, NULL, NULL)); git_reference_free(head); - assert_submodule_exists(g_repo, "sm_unchanged"); - assert_submodule_exists(g_repo, "sm_added_and_uncommited"); - assert_submodule_exists(g_repo, "sm_gitmodules_only"); - refute_submodule_exists(g_repo, "not-submodule", GIT_EEXISTS); - refute_submodule_exists(g_repo, "just_a_dir", GIT_ENOTFOUND); - refute_submodule_exists(g_repo, "just_a_file", GIT_ENOTFOUND); - refute_submodule_exists(g_repo, "no_such_file", GIT_ENOTFOUND); + test_submodule_lookup__simple_lookup(); /* baseline should still pass */ } void test_submodule_lookup__lookup_even_with_missing_index(void) @@ -133,44 +127,62 @@ void test_submodule_lookup__lookup_even_with_missing_index(void) git_repository_set_index(g_repo, idx); git_index_free(idx); - assert_submodule_exists(g_repo, "sm_unchanged"); - assert_submodule_exists(g_repo, "sm_added_and_uncommited"); - assert_submodule_exists(g_repo, "sm_gitmodules_only"); - refute_submodule_exists(g_repo, "not-submodule", GIT_EEXISTS); - refute_submodule_exists(g_repo, "just_a_dir", GIT_ENOTFOUND); - refute_submodule_exists(g_repo, "just_a_file", GIT_ENOTFOUND); - refute_submodule_exists(g_repo, "no_such_file", GIT_ENOTFOUND); + test_submodule_lookup__simple_lookup(); /* baseline should still pass */ } void test_submodule_lookup__just_added(void) { git_submodule *sm; + git_buf snap1 = GIT_BUF_INIT, snap2 = GIT_BUF_INIT; - cl_git_pass(git_submodule_add_setup(&sm, g_repo, "https://github.com/libgit2/libgit2.git", "sm_just_added", 1)); + refute_submodule_exists(g_repo, "sm_just_added", GIT_ENOTFOUND); + refute_submodule_exists(g_repo, "sm_just_added_2", GIT_ENOTFOUND); + refute_submodule_exists(g_repo, "mismatch_name", GIT_ENOTFOUND); + refute_submodule_exists(g_repo, "mismatch_path", GIT_ENOTFOUND); + test_submodule_lookup__simple_lookup(); /* baseline */ + + cl_git_pass(git_futils_readbuffer(&snap1, "submod2/.gitmodules")); + + cl_git_pass(git_submodule_add_setup(&sm, g_repo, + "https://github.com/libgit2/libgit2.git", "sm_just_added", 1)); git_submodule_free(sm); assert_submodule_exists(g_repo, "sm_just_added"); - cl_git_pass(git_submodule_add_setup(&sm, g_repo, "https://github.com/libgit2/libgit2.git", "sm_just_added_2", 1)); + cl_git_pass(git_submodule_add_setup(&sm, g_repo, + "https://github.com/libgit2/libgit2.git", "sm_just_added_2", 1)); assert_submodule_exists(g_repo, "sm_just_added_2"); git_submodule_free(sm); - cl_git_append2file("submod2/.gitmodules", "\n[submodule \"mismatch_name\"]\n\tpath = mismatch_path\n\turl = https://example.com/example.git\n\n"); + cl_git_pass(git_futils_readbuffer(&snap2, "submod2/.gitmodules")); - cl_git_pass(git_submodule_reload_all(g_repo, 1)); + cl_git_append2file( + "submod2/.gitmodules", + "\n[submodule \"mismatch_name\"]\n" + "\tpath = mismatch_path\n" + "\turl = https://example.com/example.git\n\n"); assert_submodule_exists(g_repo, "mismatch_name"); assert_submodule_exists(g_repo, "mismatch_path"); - assert_submodule_exists(g_repo, "sm_just_added"); assert_submodule_exists(g_repo, "sm_just_added_2"); + test_submodule_lookup__simple_lookup(); - /* all the regular ones should still be working right, too */ + cl_git_rewritefile("submod2/.gitmodules", snap2.ptr); + git_buf_free(&snap2); - assert_submodule_exists(g_repo, "sm_unchanged"); - assert_submodule_exists(g_repo, "sm_added_and_uncommited"); - assert_submodule_exists(g_repo, "sm_gitmodules_only"); - refute_submodule_exists(g_repo, "not-submodule", GIT_EEXISTS); - refute_submodule_exists(g_repo, "just_a_dir", GIT_ENOTFOUND); - refute_submodule_exists(g_repo, "just_a_file", GIT_ENOTFOUND); - refute_submodule_exists(g_repo, "no_such_file", GIT_ENOTFOUND); + refute_submodule_exists(g_repo, "mismatch_name", GIT_ENOTFOUND); + refute_submodule_exists(g_repo, "mismatch_path", GIT_ENOTFOUND); + assert_submodule_exists(g_repo, "sm_just_added"); + assert_submodule_exists(g_repo, "sm_just_added_2"); + test_submodule_lookup__simple_lookup(); + + cl_git_rewritefile("submod2/.gitmodules", snap1.ptr); + git_buf_free(&snap1); + + refute_submodule_exists(g_repo, "mismatch_name", GIT_ENOTFOUND); + refute_submodule_exists(g_repo, "mismatch_path", GIT_ENOTFOUND); + /* note error code change, because add_setup made a repo in the workdir */ + refute_submodule_exists(g_repo, "sm_just_added", GIT_EEXISTS); + refute_submodule_exists(g_repo, "sm_just_added_2", GIT_EEXISTS); + test_submodule_lookup__simple_lookup(); } diff --git a/tests/submodule/nosubs.c b/tests/submodule/nosubs.c index cabb53ead..e343c1620 100644 --- a/tests/submodule/nosubs.c +++ b/tests/submodule/nosubs.c @@ -69,7 +69,10 @@ void test_submodule_nosubs__reload_add_reload(void) cl_git_pass(git_submodule_reload_all(repo, 0)); - cl_git_pass(git_submodule_add_setup(&sm, repo, "https://github.com/libgit2/libgit2.git", "submodules/libgit2", 1)); + /* try one add with a reload (to make sure no errors happen) */ + + cl_git_pass(git_submodule_add_setup(&sm, repo, + "https://github.com/libgit2/libgit2.git", "submodules/libgit2", 1)); cl_git_pass(git_submodule_reload_all(repo, 0)); @@ -79,6 +82,17 @@ void test_submodule_nosubs__reload_add_reload(void) cl_git_pass(git_submodule_lookup(&sm, repo, "submodules/libgit2")); cl_assert_equal_s("submodules/libgit2", git_submodule_name(sm)); git_submodule_free(sm); + + /* try one add without a reload (to make sure cache inval works, too) */ + + cl_git_pass(git_submodule_add_setup(&sm, repo, + "https://github.com/libgit2/libgit2.git", "libgit2-again", 1)); + cl_assert_equal_s("libgit2-again", git_submodule_name(sm)); + git_submodule_free(sm); + + cl_git_pass(git_submodule_lookup(&sm, repo, "libgit2-again")); + cl_assert_equal_s("libgit2-again", git_submodule_name(sm)); + git_submodule_free(sm); } void test_submodule_nosubs__bad_gitmodules(void) @@ -101,7 +115,7 @@ void test_submodule_nosubs__add_and_delete(void) git_submodule *sm; git_buf buf = GIT_BUF_INIT; - /* note the lack of calls to git_submodule_reload - this *should* work */ + /* note lack of calls to git_submodule_reload_all - this *should* work */ cl_git_fail(git_submodule_lookup(NULL, repo, "libgit2")); cl_git_fail(git_submodule_lookup(NULL, repo, "submodules/libgit2")); From 8f4e5275e4f7f287b6782e70ff96c5de8fa4059e Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Tue, 1 Apr 2014 16:46:25 -0700 Subject: [PATCH 12/16] More tests and fix submodule index refresh There was a little bug where the submodule cache thought that the index date was out of date even when it wasn't that was resulting in some extra scans of index data even when not needed. Mostly this commit adds a bunch of new tests including adding and removing submodules in the index and in the HEAD and seeing if we can automatically pick them up when refreshing. --- src/index.c | 4 +- tests/submodule/lookup.c | 91 +++++++++++++++++++++++++++-- tests/submodule/submodule_helpers.c | 36 +++++++++--- tests/submodule/submodule_helpers.h | 15 ++++- 4 files changed, 132 insertions(+), 14 deletions(-) diff --git a/src/index.c b/src/index.c index 6cc8ea1a3..24e447928 100644 --- a/src/index.c +++ b/src/index.c @@ -524,7 +524,9 @@ int git_index__changed_relative_to( if (git_index_read(index, false) < 0) giterr_clear(); - return (memcmp(&index->stamp, fs, sizeof(index->stamp)) == 0); + return (index->stamp.mtime != fs->mtime || + index->stamp.size != fs->size || + index->stamp.ino != fs->ino); } int git_index_write(git_index *index) diff --git a/tests/submodule/lookup.c b/tests/submodule/lookup.c index 86ba25c3a..34de5923e 100644 --- a/tests/submodule/lookup.c +++ b/tests/submodule/lookup.c @@ -130,18 +130,63 @@ void test_submodule_lookup__lookup_even_with_missing_index(void) test_submodule_lookup__simple_lookup(); /* baseline should still pass */ } +static void baseline_tests(void) +{ + /* small baseline that should work even if we change the index or make + * commits from the index + */ + assert_submodule_exists(g_repo, "sm_unchanged"); + assert_submodule_exists(g_repo, "sm_gitmodules_only"); + refute_submodule_exists(g_repo, "not-submodule", GIT_EEXISTS); +} + +static void add_submodule_with_commit(const char *name) +{ + git_submodule *sm; + git_repository *smrepo; + git_index *idx; + git_buf p = GIT_BUF_INIT; + + cl_git_pass(git_submodule_add_setup(&sm, g_repo, + "https://github.com/libgit2/libgit2.git", name, 1)); + + assert_submodule_exists(g_repo, name); + + cl_git_pass(git_submodule_open(&smrepo, sm)); + cl_git_pass(git_repository_index(&idx, smrepo)); + + cl_git_pass(git_buf_joinpath(&p, git_repository_workdir(smrepo), "file")); + cl_git_mkfile(p.ptr, "new file"); + git_buf_free(&p); + + cl_git_pass(git_index_add_bypath(idx, "file")); + cl_git_pass(git_index_write(idx)); + git_index_free(idx); + + cl_repo_commit_from_index(NULL, smrepo, NULL, 0, "initial commit"); + git_repository_free(smrepo); + + cl_git_pass(git_submodule_add_finalize(sm)); + + git_submodule_free(sm); +} + void test_submodule_lookup__just_added(void) { git_submodule *sm; git_buf snap1 = GIT_BUF_INIT, snap2 = GIT_BUF_INIT; + git_reference *original_head = NULL; refute_submodule_exists(g_repo, "sm_just_added", GIT_ENOTFOUND); refute_submodule_exists(g_repo, "sm_just_added_2", GIT_ENOTFOUND); + refute_submodule_exists(g_repo, "sm_just_added_idx", GIT_ENOTFOUND); + refute_submodule_exists(g_repo, "sm_just_added_head", GIT_ENOTFOUND); refute_submodule_exists(g_repo, "mismatch_name", GIT_ENOTFOUND); refute_submodule_exists(g_repo, "mismatch_path", GIT_ENOTFOUND); - test_submodule_lookup__simple_lookup(); /* baseline */ + baseline_tests(); cl_git_pass(git_futils_readbuffer(&snap1, "submod2/.gitmodules")); + cl_git_pass(git_repository_head(&original_head, g_repo)); cl_git_pass(git_submodule_add_setup(&sm, g_repo, "https://github.com/libgit2/libgit2.git", "sm_just_added", 1)); @@ -151,8 +196,16 @@ void test_submodule_lookup__just_added(void) cl_git_pass(git_submodule_add_setup(&sm, g_repo, "https://github.com/libgit2/libgit2.git", "sm_just_added_2", 1)); assert_submodule_exists(g_repo, "sm_just_added_2"); + cl_git_fail(git_submodule_add_finalize(sm)); /* fails if no HEAD */ git_submodule_free(sm); + add_submodule_with_commit("sm_just_added_head"); + cl_repo_commit_from_index(NULL, g_repo, NULL, 0, "commit new sm to head"); + assert_submodule_exists(g_repo, "sm_just_added_head"); + + add_submodule_with_commit("sm_just_added_idx"); + assert_submodule_exists(g_repo, "sm_just_added_idx"); + cl_git_pass(git_futils_readbuffer(&snap2, "submod2/.gitmodules")); cl_git_append2file( @@ -165,7 +218,9 @@ void test_submodule_lookup__just_added(void) assert_submodule_exists(g_repo, "mismatch_path"); assert_submodule_exists(g_repo, "sm_just_added"); assert_submodule_exists(g_repo, "sm_just_added_2"); - test_submodule_lookup__simple_lookup(); + assert_submodule_exists(g_repo, "sm_just_added_idx"); + assert_submodule_exists(g_repo, "sm_just_added_head"); + baseline_tests(); cl_git_rewritefile("submod2/.gitmodules", snap2.ptr); git_buf_free(&snap2); @@ -174,7 +229,9 @@ void test_submodule_lookup__just_added(void) refute_submodule_exists(g_repo, "mismatch_path", GIT_ENOTFOUND); assert_submodule_exists(g_repo, "sm_just_added"); assert_submodule_exists(g_repo, "sm_just_added_2"); - test_submodule_lookup__simple_lookup(); + assert_submodule_exists(g_repo, "sm_just_added_idx"); + assert_submodule_exists(g_repo, "sm_just_added_head"); + baseline_tests(); cl_git_rewritefile("submod2/.gitmodules", snap1.ptr); git_buf_free(&snap1); @@ -184,5 +241,31 @@ void test_submodule_lookup__just_added(void) /* note error code change, because add_setup made a repo in the workdir */ refute_submodule_exists(g_repo, "sm_just_added", GIT_EEXISTS); refute_submodule_exists(g_repo, "sm_just_added_2", GIT_EEXISTS); - test_submodule_lookup__simple_lookup(); + /* these still exist in index and head respectively */ + assert_submodule_exists(g_repo, "sm_just_added_idx"); + assert_submodule_exists(g_repo, "sm_just_added_head"); + baseline_tests(); + + { + git_index *idx; + cl_git_pass(git_repository_index(&idx, g_repo)); + cl_git_pass(git_index_remove_bypath(idx, "sm_just_added_idx")); + cl_git_pass(git_index_remove_bypath(idx, "sm_just_added_head")); + cl_git_pass(git_index_write(idx)); + git_index_free(idx); + } + + refute_submodule_exists(g_repo, "sm_just_added_idx", GIT_EEXISTS); + assert_submodule_exists(g_repo, "sm_just_added_head"); + + { + git_signature *sig; + cl_git_pass(git_signature_now(&sig, "resetter", "resetter@email.com")); + cl_git_pass(git_reference_create(NULL, g_repo, "refs/heads/master", git_reference_target(original_head), 1, sig, "move head back")); + git_signature_free(sig); + git_reference_free(original_head); + } + + refute_submodule_exists(g_repo, "sm_just_added_head", GIT_EEXISTS); } + diff --git a/tests/submodule/submodule_helpers.c b/tests/submodule/submodule_helpers.c index 546f0913a..50aa97568 100644 --- a/tests/submodule/submodule_helpers.c +++ b/tests/submodule/submodule_helpers.c @@ -126,20 +126,26 @@ git_repository *setup_fixture_submod2(void) return repo; } -void assert_submodule_exists(git_repository *repo, const char *name) +void assert__submodule_exists( + git_repository *repo, const char *name, + const char *msg, const char *file, int line) { git_submodule *sm; - cl_git_pass(git_submodule_lookup(&sm, repo, name)); - cl_assert(sm); + int error = git_submodule_lookup(&sm, repo, name); + if (error) + cl_git_report_failure(error, file, line, msg); + cl_assert_at_line(sm != NULL, file, line); git_submodule_free(sm); } -void refute_submodule_exists( - git_repository *repo, const char *name, int expected_error) +void refute__submodule_exists( + git_repository *repo, const char *name, int expected_error, + const char *msg, const char *file, int line) { git_submodule *sm; - cl_assert_equal_i( - expected_error, git_submodule_lookup(&sm, repo, name)); + clar__assert_equal( + file, line, msg, 1, "%i", + expected_error, (int)(git_submodule_lookup(&sm, repo, name))); } unsigned int get_submodule_status(git_repository *repo, const char *name) @@ -154,3 +160,19 @@ unsigned int get_submodule_status(git_repository *repo, const char *name) return status; } + +static int print_submodules(git_submodule *sm, const char *name, void *p) +{ + unsigned int loc = 0; + GIT_UNUSED(p); + git_submodule_location(&loc, sm); + fprintf(stderr, "# submodule %s (at %s) flags %x\n", + name, git_submodule_path(sm), loc); + return 0; +} + +void dump_submodules(git_repository *repo) +{ + git_submodule_foreach(repo, print_submodules, NULL); +} + diff --git a/tests/submodule/submodule_helpers.h b/tests/submodule/submodule_helpers.h index ec5510e3c..4b2620bfa 100644 --- a/tests/submodule/submodule_helpers.h +++ b/tests/submodule/submodule_helpers.h @@ -6,5 +6,16 @@ extern git_repository *setup_fixture_submod2(void); extern unsigned int get_submodule_status(git_repository *, const char *); -extern void assert_submodule_exists(git_repository *, const char *); -extern void refute_submodule_exists(git_repository *, const char *, int err); +extern void assert__submodule_exists( + git_repository *, const char *, const char *, const char *, int); + +#define assert_submodule_exists(repo,name) \ + assert__submodule_exists(repo, name, "git_submodule_lookup(" #name ") failed", __FILE__, __LINE__) + +extern void refute__submodule_exists( + git_repository *, const char *, int err, const char *, const char *, int); + +#define refute_submodule_exists(repo,name,code) \ + refute__submodule_exists(repo, name, code, "expected git_submodule_lookup(" #name ") to fail with error " #code, __FILE__, __LINE__) + +extern void dump_submodules(git_repository *repo); From 12d4ed4de34e312cb491ce1b5562d84846aeb456 Mon Sep 17 00:00:00 2001 From: Jan Melcher Date: Sat, 8 Mar 2014 23:04:56 +0100 Subject: [PATCH 13/16] Test git_submodule_add_setup with relative url --- tests/submodule/modify.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/submodule/modify.c b/tests/submodule/modify.c index 1aaa56388..7cd44e23e 100644 --- a/tests/submodule/modify.c +++ b/tests/submodule/modify.c @@ -9,6 +9,10 @@ static git_repository *g_repo = NULL; #define SM_LIBGIT2 "sm_libgit2" #define SM_LIBGIT2B "sm_libgit2b" +#define SM_RELATIVE_URL "../TestGitRepository" +#define SM_RELATIVE_RESOLVED_URL "https://github.com/libgit2/TestGitRepository" +#define SM_RELATIVE "TestGitRepository" + void test_submodule_modify__initialize(void) { g_repo = setup_fixture_submod2(); @@ -62,6 +66,26 @@ void test_submodule_modify__add(void) git_config_free(cfg); } +void test_submodule_modify__add_with_relative_url(void) { + git_submodule *sm; + git_config *cfg; + const char *s; + + git_repository* repo; + /* setup_fixture_submod2 does not work here because it does not set up origin configuration */ + cl_git_pass(git_clone(&repo, SM_LIBGIT2_URL, "./sandbox/submodules_cloned", NULL)); + + cl_git_pass( + git_submodule_add_setup(&sm, repo, SM_RELATIVE_URL, SM_RELATIVE, 1) + ); + + cl_git_pass(git_repository_config(&cfg, repo)); + cl_git_pass( + git_config_get_string(&s, cfg, "submodule." SM_RELATIVE ".url")); + cl_assert_equal_s(s, SM_RELATIVE_RESOLVED_URL); + git_config_free(cfg); +} + static int delete_one_config(const git_config_entry *entry, void *payload) { git_config *cfg = payload; From f2fb4bac68e7ab38cf6082655b2da153866a012d Mon Sep 17 00:00:00 2001 From: Jan Melcher Date: Wed, 2 Apr 2014 23:55:21 +0200 Subject: [PATCH 14/16] git_submodule_resolve_url supports relative urls The base for the relative urls is determined as follows, with descending priority: - remote url of HEAD's remote tracking branch - remote "origin" - workdir This follows git.git behaviour --- src/submodule.c | 156 +++++++++++++++++++++++---------------- tests/submodule/add.c | 115 +++++++++++++++++++++++++++++ tests/submodule/modify.c | 73 ------------------ 3 files changed, 208 insertions(+), 136 deletions(-) create mode 100644 tests/submodule/add.c diff --git a/src/submodule.c b/src/submodule.c index 913c97a87..24f250aa1 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -91,8 +91,10 @@ static int submodule_cache_init(git_repository *repo, int refresh); static void submodule_cache_free(git_submodule_cache *cache); static git_config_backend *open_gitmodules(git_submodule_cache *, int gitmod); -static int lookup_head_remote_key(git_buf *key, git_repository *repo); -static int lookup_head_remote(git_buf *url, git_repository *repo); +static int get_url_base(git_buf *url, git_repository *repo); +static int lookup_default_remote(git_remote **remote, git_repository *repo); +static int lookup_head_remote_key(git_buf *remote_key, git_repository *repo); +static int lookup_head_remote(git_remote **remote, git_repository *repo); static int submodule_get(git_submodule **, git_submodule_cache *, const char *, const char *); static int submodule_load_from_config(const git_config_entry *, void *); static int submodule_load_from_wd_lite(git_submodule *); @@ -644,7 +646,7 @@ int git_submodule_resolve_url(git_buf *out, git_repository *repo, const char *ur assert(url); if (git_path_is_relative(url)) { - if (!(error = lookup_head_remote(out, repo))) + if (!(error = get_url_base(out, repo))) error = git_path_apply_relative(out, url); } else if (strchr(url, ':') != NULL || url[0] == '/') { error = git_buf_sets(out, url); @@ -1795,81 +1797,109 @@ static int submodule_cache_init(git_repository *repo, int cache_refresh) return error; } -static int lookup_head_remote_key(git_buf *key, git_repository *repo) +static int get_url_base(git_buf *url, git_repository *repo) { int error; - git_config *cfg; - git_reference *head = NULL, *remote = NULL; - const char *tgt, *scan; + git_remote *remote; + error = lookup_default_remote(&remote, repo); + const char *url_ptr; - /* 1. resolve HEAD -> refs/heads/BRANCH - * 2. lookup config branch.BRANCH.remote -> ORIGIN - * 3. return remote.ORIGIN.url - */ - - git_buf_clear(key); - - if ((error = git_repository_config__weakptr(&cfg, repo)) < 0) - return error; - - if (git_reference_lookup(&head, repo, GIT_HEAD_FILE) < 0) { - giterr_set(GITERR_SUBMODULE, - "Cannot resolve relative URL when HEAD cannot be resolved"); - return GIT_ENOTFOUND; + assert(url && repo); + + if (!error) { + url_ptr = git_remote_url(remote); + } else if (error == GIT_ENOTFOUND) { + /* if repository does not have a default remote, use workdir instead */ + giterr_clear(); + error = 0; + url_ptr = git_repository_workdir(repo); } - - if (git_reference_type(head) != GIT_REF_SYMBOLIC) { - giterr_set(GITERR_SUBMODULE, - "Cannot resolve relative URL when HEAD is not symbolic"); - error = GIT_ENOTFOUND; + + if (error < 0) goto cleanup; - } - - if ((error = git_branch_upstream(&remote, head)) < 0) - goto cleanup; - - /* remote should refer to something like refs/remotes/ORIGIN/BRANCH */ - - if (git_reference_type(remote) != GIT_REF_SYMBOLIC || - git__prefixcmp( - git_reference_symbolic_target(remote), GIT_REFS_REMOTES_DIR) != 0) - { - giterr_set(GITERR_SUBMODULE, - "Cannot resolve relative URL when HEAD is not symbolic"); - error = GIT_ENOTFOUND; - goto cleanup; - } - - scan = tgt = git_reference_symbolic_target(remote) + - strlen(GIT_REFS_REMOTES_DIR); - while (*scan && (*scan != '/' || (scan > tgt && scan[-1] != '\\'))) - scan++; /* find non-escaped slash to end ORIGIN name */ - - error = git_buf_printf(key, "remote.%.*s.url", (int)(scan - tgt), tgt); + + error = git_buf_sets(url, url_ptr); cleanup: - if (error < 0) - git_buf_clear(key); - - git_reference_free(head); - git_reference_free(remote); + git_remote_free(remote); return error; } -static int lookup_head_remote(git_buf *url, git_repository *repo) +/** + * Lookup the remote that is considered the default remote in the current state + */ +static int lookup_default_remote(git_remote **remote, git_repository *repo) { int error; - git_config *cfg; - const char *tgt; - git_buf key = GIT_BUF_INIT; + error = lookup_head_remote(remote, repo); - if (!(error = lookup_head_remote_key(&key, repo)) && - !(error = git_repository_config__weakptr(&cfg, repo)) && - !(error = git_config_get_string(&tgt, cfg, key.ptr))) - error = git_buf_sets(url, tgt); + assert(remote && repo); + + // if that failed, use 'origin' instead + if (error == GIT_ENOTFOUND) { + error = git_remote_load(remote, repo, "origin"); + } + + if (error == GIT_ENOTFOUND) { + giterr_set(GITERR_SUBMODULE, + "Neither HEAD points to a local tracking branch, nor does origin exist"); + } + + return error; +} + +/** + * Lookup name of remote of the local tracking branch HEAD points to + */ +static int lookup_head_remote_key(git_buf *remote_name, git_repository *repo) +{ + int error; + git_reference *head = NULL; + git_buf upstream_name = GIT_BUF_INIT; + + /* lookup and dereference HEAD */ + if ((error = git_repository_head(&head, repo) < 0)) + goto cleanup; + + /* lookup remote tracking branch of HEAD */ + if ((error = git_branch_upstream_name(&upstream_name, repo, git_reference_name(head))) < 0) + goto cleanup; + + /* lookup remote of remote tracking branch */ + if ((error = git_branch_remote_name(remote_name, repo, upstream_name.ptr)) < 0) + goto cleanup; + +cleanup: + git_buf_free(&upstream_name); + if (head) + git_reference_free(head); + + return error; +} + +/** + * Lookup the remote of the local tracking branch HEAD points to + */ +static int lookup_head_remote(git_remote **remote, git_repository *repo) +{ + int error; + git_buf remote_name = GIT_BUF_INIT; + + assert(remote && repo); + + /* should be NULL in case of error */ + *remote = NULL; + + /* lookup remote of remote tracking branch name */ + if ((error = lookup_head_remote_key(&remote_name, repo)) < 0) + goto cleanup; + + error = git_remote_load(remote, repo, remote_name.ptr); + +cleanup: + git_buf_free(&remote_name); - git_buf_free(&key); return error; } diff --git a/tests/submodule/add.c b/tests/submodule/add.c new file mode 100644 index 000000000..2d51b3d7e --- /dev/null +++ b/tests/submodule/add.c @@ -0,0 +1,115 @@ +#include "clar_libgit2.h" +#include "posix.h" +#include "path.h" +#include "submodule_helpers.h" + +static git_repository *g_repo = NULL; + +static void assert_submodule_url(const char* name, const char *url); + +void test_submodule_add__cleanup(void) +{ + cl_git_sandbox_cleanup(); +} + +void test_submodule_add__url_absolute(void) +{ + g_repo = setup_fixture_submod2(); + git_submodule *sm; + + /* re-add existing submodule */ + cl_assert_equal_i( + GIT_EEXISTS, + git_submodule_add_setup(NULL, g_repo, "whatever", "sm_unchanged", 1)); + + /* add a submodule using a gitlink */ + + cl_git_pass( + git_submodule_add_setup(&sm, g_repo, "https://github.com/libgit2/libgit2.git", "sm_libgit2", 1) + ); + git_submodule_free(sm); + + cl_assert(git_path_isfile("submod2/" "sm_libgit2" "/.git")); + + cl_assert(git_path_isdir("submod2/.git/modules")); + cl_assert(git_path_isdir("submod2/.git/modules/" "sm_libgit2")); + cl_assert(git_path_isfile("submod2/.git/modules/" "sm_libgit2" "/HEAD")); + assert_submodule_url("sm_libgit2", "https://github.com/libgit2/libgit2.git"); + + /* add a submodule not using a gitlink */ + + cl_git_pass( + git_submodule_add_setup(&sm, g_repo, "https://github.com/libgit2/libgit2.git", "sm_libgit2b", 0) + ); + git_submodule_free(sm); + + cl_assert(git_path_isdir("submod2/" "sm_libgit2b" "/.git")); + cl_assert(git_path_isfile("submod2/" "sm_libgit2b" "/.git/HEAD")); + cl_assert(!git_path_exists("submod2/.git/modules/" "sm_libgit2b")); + assert_submodule_url("sm_libgit2b", "https://github.com/libgit2/libgit2.git"); +} + +void test_submodule_add__url_relative(void) { + git_submodule *sm; + git_remote *remote; + + /* default remote url is https://github.com/libgit2/false.git */ + g_repo = cl_git_sandbox_init("testrepo2"); + + /* make sure we're not defaulting 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_fail(git_remote_load(&remote, g_repo, "origin")); + git_remote_free(remote); + + cl_git_pass( + git_submodule_add_setup(&sm, g_repo, "../TestGitRepository", "TestGitRepository", 1) + ); + git_submodule_free(sm); + + assert_submodule_url("TestGitRepository", "https://github.com/libgit2/TestGitRepository"); +} + +void test_submodule_add__url_relative_to_origin(void) { + git_submodule *sm; + + /* default remote url is https://github.com/libgit2/false.git */ + g_repo = cl_git_sandbox_init("testrepo2"); + + cl_git_pass( + git_submodule_add_setup(&sm, g_repo, "../TestGitRepository", "TestGitRepository", 1) + ); + git_submodule_free(sm); + + assert_submodule_url("TestGitRepository", "https://github.com/libgit2/TestGitRepository"); +} + +void test_submodule_add__url_relative_to_workdir(void) { + git_submodule *sm; + + /* In this repo, HEAD (master) has no remote tracking branc h*/ + g_repo = cl_git_sandbox_init("testrepo"); + + cl_git_pass( + git_submodule_add_setup(&sm, g_repo, "./", "TestGitRepository", 1) + ); + git_submodule_free(sm); + + assert_submodule_url("TestGitRepository", git_repository_workdir(g_repo)); +} + +static void assert_submodule_url(const char* name, const char *url) +{ + git_config *cfg; + const char *s; + git_buf key = GIT_BUF_INIT; + + cl_git_pass(git_repository_config(&cfg, g_repo)); + + cl_git_pass(git_buf_printf(&key, "submodule.%s.url", name)); + cl_git_pass(git_config_get_string(&s, cfg, git_buf_cstr(&key))); + cl_assert_equal_s(s, url); + + git_config_free(cfg); + git_buf_free(&key); +} diff --git a/tests/submodule/modify.c b/tests/submodule/modify.c index 7cd44e23e..7e76f3572 100644 --- a/tests/submodule/modify.c +++ b/tests/submodule/modify.c @@ -7,85 +7,12 @@ static git_repository *g_repo = NULL; #define SM_LIBGIT2_URL "https://github.com/libgit2/libgit2.git" #define SM_LIBGIT2 "sm_libgit2" -#define SM_LIBGIT2B "sm_libgit2b" - -#define SM_RELATIVE_URL "../TestGitRepository" -#define SM_RELATIVE_RESOLVED_URL "https://github.com/libgit2/TestGitRepository" -#define SM_RELATIVE "TestGitRepository" void test_submodule_modify__initialize(void) { g_repo = setup_fixture_submod2(); } -void test_submodule_modify__add(void) -{ - git_submodule *sm; - git_config *cfg; - const char *s; - - /* re-add existing submodule */ - cl_assert_equal_i( - GIT_EEXISTS, - git_submodule_add_setup(NULL, g_repo, "whatever", "sm_unchanged", 1)); - - /* add a submodule using a gitlink */ - - cl_git_pass( - git_submodule_add_setup(&sm, g_repo, SM_LIBGIT2_URL, SM_LIBGIT2, 1) - ); - git_submodule_free(sm); - - cl_assert(git_path_isfile("submod2/" SM_LIBGIT2 "/.git")); - - cl_assert(git_path_isdir("submod2/.git/modules")); - cl_assert(git_path_isdir("submod2/.git/modules/" SM_LIBGIT2)); - cl_assert(git_path_isfile("submod2/.git/modules/" SM_LIBGIT2 "/HEAD")); - - cl_git_pass(git_repository_config(&cfg, g_repo)); - cl_git_pass( - git_config_get_string(&s, cfg, "submodule." SM_LIBGIT2 ".url")); - cl_assert_equal_s(s, SM_LIBGIT2_URL); - git_config_free(cfg); - - /* add a submodule not using a gitlink */ - - cl_git_pass( - git_submodule_add_setup(&sm, g_repo, SM_LIBGIT2_URL, SM_LIBGIT2B, 0) - ); - git_submodule_free(sm); - - cl_assert(git_path_isdir("submod2/" SM_LIBGIT2B "/.git")); - cl_assert(git_path_isfile("submod2/" SM_LIBGIT2B "/.git/HEAD")); - cl_assert(!git_path_exists("submod2/.git/modules/" SM_LIBGIT2B)); - - cl_git_pass(git_repository_config(&cfg, g_repo)); - cl_git_pass( - git_config_get_string(&s, cfg, "submodule." SM_LIBGIT2B ".url")); - cl_assert_equal_s(s, SM_LIBGIT2_URL); - git_config_free(cfg); -} - -void test_submodule_modify__add_with_relative_url(void) { - git_submodule *sm; - git_config *cfg; - const char *s; - - git_repository* repo; - /* setup_fixture_submod2 does not work here because it does not set up origin configuration */ - cl_git_pass(git_clone(&repo, SM_LIBGIT2_URL, "./sandbox/submodules_cloned", NULL)); - - cl_git_pass( - git_submodule_add_setup(&sm, repo, SM_RELATIVE_URL, SM_RELATIVE, 1) - ); - - cl_git_pass(git_repository_config(&cfg, repo)); - cl_git_pass( - git_config_get_string(&s, cfg, "submodule." SM_RELATIVE ".url")); - cl_assert_equal_s(s, SM_RELATIVE_RESOLVED_URL); - git_config_free(cfg); -} - static int delete_one_config(const git_config_entry *entry, void *payload) { git_config *cfg = payload; From 18cc7d28c4c5ad4c9ecf7bfeab98a035500fd9d7 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 3 Apr 2014 11:29:08 -0700 Subject: [PATCH 15/16] Minor code cleanup --- src/submodule.c | 131 ++++++++++++++++-------------------------- tests/submodule/add.c | 62 ++++++++++---------- 2 files changed, 82 insertions(+), 111 deletions(-) diff --git a/src/submodule.c b/src/submodule.c index 24f250aa1..e49f09699 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -92,9 +92,7 @@ static void submodule_cache_free(git_submodule_cache *cache); static git_config_backend *open_gitmodules(git_submodule_cache *, int gitmod); static int get_url_base(git_buf *url, git_repository *repo); -static int lookup_default_remote(git_remote **remote, git_repository *repo); static int lookup_head_remote_key(git_buf *remote_key, git_repository *repo); -static int lookup_head_remote(git_remote **remote, git_repository *repo); static int submodule_get(git_submodule **, git_submodule_cache *, const char *, const char *); static int submodule_load_from_config(const git_config_entry *, void *); static int submodule_load_from_wd_lite(git_submodule *); @@ -1797,61 +1795,7 @@ static int submodule_cache_init(git_repository *repo, int cache_refresh) return error; } -static int get_url_base(git_buf *url, git_repository *repo) -{ - int error; - git_remote *remote; - error = lookup_default_remote(&remote, repo); - const char *url_ptr; - - assert(url && repo); - - if (!error) { - url_ptr = git_remote_url(remote); - } else if (error == GIT_ENOTFOUND) { - /* if repository does not have a default remote, use workdir instead */ - giterr_clear(); - error = 0; - url_ptr = git_repository_workdir(repo); - } - - if (error < 0) - goto cleanup; - - error = git_buf_sets(url, url_ptr); - -cleanup: - git_remote_free(remote); - - return error; -} - -/** - * Lookup the remote that is considered the default remote in the current state - */ -static int lookup_default_remote(git_remote **remote, git_repository *repo) -{ - int error; - error = lookup_head_remote(remote, repo); - - assert(remote && repo); - - // if that failed, use 'origin' instead - if (error == GIT_ENOTFOUND) { - error = git_remote_load(remote, repo, "origin"); - } - - if (error == GIT_ENOTFOUND) { - giterr_set(GITERR_SUBMODULE, - "Neither HEAD points to a local tracking branch, nor does origin exist"); - } - - return error; -} - -/** - * Lookup name of remote of the local tracking branch HEAD points to - */ +/* Lookup name of remote of the local tracking branch HEAD points to */ static int lookup_head_remote_key(git_buf *remote_name, git_repository *repo) { int error; @@ -1859,50 +1803,75 @@ static int lookup_head_remote_key(git_buf *remote_name, git_repository *repo) git_buf upstream_name = GIT_BUF_INIT; /* lookup and dereference HEAD */ - if ((error = git_repository_head(&head, repo) < 0)) - goto cleanup; + if ((error = git_repository_head(&head, repo)) < 0) + return error; /* lookup remote tracking branch of HEAD */ - if ((error = git_branch_upstream_name(&upstream_name, repo, git_reference_name(head))) < 0) - goto cleanup; + if (!(error = git_branch_upstream_name( + &upstream_name, repo, git_reference_name(head)))) + { + /* lookup remote of remote tracking branch */ + error = git_branch_remote_name(remote_name, repo, upstream_name.ptr); - /* lookup remote of remote tracking branch */ - if ((error = git_branch_remote_name(remote_name, repo, upstream_name.ptr)) < 0) - goto cleanup; + git_buf_free(&upstream_name); + } -cleanup: - git_buf_free(&upstream_name); - if (head) - git_reference_free(head); + git_reference_free(head); return error; } -/** - * Lookup the remote of the local tracking branch HEAD points to - */ +/* Lookup the remote of the local tracking branch HEAD points to */ static int lookup_head_remote(git_remote **remote, git_repository *repo) { int error; git_buf remote_name = GIT_BUF_INIT; - assert(remote && repo); - - /* should be NULL in case of error */ - *remote = NULL; - /* lookup remote of remote tracking branch name */ - if ((error = lookup_head_remote_key(&remote_name, repo)) < 0) - goto cleanup; + if (!(error = lookup_head_remote_key(&remote_name, repo))) + error = git_remote_load(remote, repo, remote_name.ptr); - error = git_remote_load(remote, repo, remote_name.ptr); - -cleanup: git_buf_free(&remote_name); return error; } +/* Lookup remote, either from HEAD or fall back on origin */ +static int lookup_default_remote(git_remote **remote, git_repository *repo) +{ + int error = lookup_head_remote(remote, repo); + + /* if that failed, use 'origin' instead */ + if (error == GIT_ENOTFOUND) + error = git_remote_load(remote, repo, "origin"); + + if (error == GIT_ENOTFOUND) + giterr_set( + GITERR_SUBMODULE, + "Cannot get default remote for submodule - no local tracking " + "branch for HEAD and origin does not exist"); + + return error; +} + +static int get_url_base(git_buf *url, git_repository *repo) +{ + int error; + git_remote *remote = NULL; + + if (!(error = lookup_default_remote(&remote, repo))) { + error = git_buf_sets(url, git_remote_url(remote)); + git_remote_free(remote); + } + else if (error == GIT_ENOTFOUND) { + /* if repository does not have a default remote, use workdir instead */ + giterr_clear(); + error = git_buf_sets(url, git_repository_workdir(repo)); + } + + return error; +} + static void submodule_get_index_status(unsigned int *status, git_submodule *sm) { const git_oid *head_oid = git_submodule_head_id(sm); diff --git a/tests/submodule/add.c b/tests/submodule/add.c index 2d51b3d7e..af81713f1 100644 --- a/tests/submodule/add.c +++ b/tests/submodule/add.c @@ -5,20 +5,35 @@ static git_repository *g_repo = NULL; -static void assert_submodule_url(const char* name, const char *url); - void test_submodule_add__cleanup(void) { cl_git_sandbox_cleanup(); } +static void assert_submodule_url(const char* name, const char *url) +{ + git_config *cfg; + const char *s; + git_buf key = GIT_BUF_INIT; + + cl_git_pass(git_repository_config(&cfg, g_repo)); + + cl_git_pass(git_buf_printf(&key, "submodule.%s.url", name)); + cl_git_pass(git_config_get_string(&s, cfg, git_buf_cstr(&key))); + cl_assert_equal_s(s, url); + + git_config_free(cfg); + git_buf_free(&key); +} + void test_submodule_add__url_absolute(void) { - g_repo = setup_fixture_submod2(); git_submodule *sm; + g_repo = setup_fixture_submod2(); + /* re-add existing submodule */ - cl_assert_equal_i( + cl_git_fail_with( GIT_EEXISTS, git_submodule_add_setup(NULL, g_repo, "whatever", "sm_unchanged", 1)); @@ -49,14 +64,15 @@ void test_submodule_add__url_absolute(void) assert_submodule_url("sm_libgit2b", "https://github.com/libgit2/libgit2.git"); } -void test_submodule_add__url_relative(void) { +void test_submodule_add__url_relative(void) +{ git_submodule *sm; git_remote *remote; - + /* default remote url is https://github.com/libgit2/false.git */ g_repo = cl_git_sandbox_init("testrepo2"); - - /* make sure we're not defaulting to origin - rename origin -> test_remote */ + + /* 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_fail(git_remote_load(&remote, g_repo, "origin")); @@ -66,13 +82,14 @@ void test_submodule_add__url_relative(void) { git_submodule_add_setup(&sm, g_repo, "../TestGitRepository", "TestGitRepository", 1) ); git_submodule_free(sm); - + assert_submodule_url("TestGitRepository", "https://github.com/libgit2/TestGitRepository"); } -void test_submodule_add__url_relative_to_origin(void) { +void test_submodule_add__url_relative_to_origin(void) +{ git_submodule *sm; - + /* default remote url is https://github.com/libgit2/false.git */ g_repo = cl_git_sandbox_init("testrepo2"); @@ -80,11 +97,12 @@ void test_submodule_add__url_relative_to_origin(void) { git_submodule_add_setup(&sm, g_repo, "../TestGitRepository", "TestGitRepository", 1) ); git_submodule_free(sm); - + assert_submodule_url("TestGitRepository", "https://github.com/libgit2/TestGitRepository"); } -void test_submodule_add__url_relative_to_workdir(void) { +void test_submodule_add__url_relative_to_workdir(void) +{ git_submodule *sm; /* In this repo, HEAD (master) has no remote tracking branc h*/ @@ -94,22 +112,6 @@ void test_submodule_add__url_relative_to_workdir(void) { git_submodule_add_setup(&sm, g_repo, "./", "TestGitRepository", 1) ); git_submodule_free(sm); - + assert_submodule_url("TestGitRepository", git_repository_workdir(g_repo)); } - -static void assert_submodule_url(const char* name, const char *url) -{ - git_config *cfg; - const char *s; - git_buf key = GIT_BUF_INIT; - - cl_git_pass(git_repository_config(&cfg, g_repo)); - - cl_git_pass(git_buf_printf(&key, "submodule.%s.url", name)); - cl_git_pass(git_config_get_string(&s, cfg, git_buf_cstr(&key))); - cl_assert_equal_s(s, url); - - git_config_free(cfg); - git_buf_free(&key); -} From eedeeb9e8f708e9f60568adc4a63307754a603f5 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 3 Apr 2014 11:58:51 -0700 Subject: [PATCH 16/16] Test (and fix) the git_submodule_sync changes I wrote this stuff a while ago and forgot to write tests. Wanted to do so now to wrap up the PR and immediately found problems. --- src/submodule.c | 15 ++++++++++++--- tests/submodule/modify.c | 34 ++++++++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/submodule.c b/src/submodule.c index e49f09699..bea096df5 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -849,12 +849,21 @@ int git_submodule_sync(git_submodule *sm) (sm->flags & GIT_SUBMODULE_STATUS_IN_WD) != 0 && !(error = git_submodule_open(&smrepo, sm))) { - if ((error = lookup_head_remote_key(&key, smrepo)) < 0) { + git_buf remote_name = GIT_BUF_INIT; + + if ((error = git_repository_config__weakptr(&cfg, smrepo)) < 0) + /* return error from reading submodule config */; + else if ((error = lookup_head_remote_key(&remote_name, smrepo)) < 0) { giterr_clear(); - git_buf_sets(&key, "branch.origin.remote"); + error = git_buf_sets(&key, "branch.origin.remote"); + } else { + error = git_buf_join3( + &key, '.', "branch", remote_name.ptr, "remote"); + git_buf_free(&remote_name); } - error = git_config__update_entry(cfg, key.ptr, sm->url, true, true); + if (!error) + error = git_config__update_entry(cfg, key.ptr, sm->url, true, false); git_repository_free(smrepo); } diff --git a/tests/submodule/modify.c b/tests/submodule/modify.c index 7e76f3572..582d4166b 100644 --- a/tests/submodule/modify.c +++ b/tests/submodule/modify.c @@ -69,6 +69,26 @@ static int sync_one_submodule( return git_submodule_sync(sm); } +static void assert_submodule_url_is_synced( + git_submodule *sm, const char *parent_key, const char *child_key) +{ + git_config *cfg; + const char *str; + git_repository *smrepo; + + cl_git_pass(git_repository_config(&cfg, g_repo)); + cl_git_pass(git_config_get_string(&str, cfg, parent_key)); + cl_assert_equal_s(git_submodule_url(sm), str); + git_config_free(cfg); + + cl_git_pass(git_submodule_open(&smrepo, sm)); + cl_git_pass(git_repository_config(&cfg, smrepo)); + cl_git_pass(git_config_get_string(&str, cfg, child_key)); + cl_assert_equal_s(git_submodule_url(sm), str); + git_config_free(cfg); + git_repository_free(smrepo); +} + void test_submodule_modify__sync(void) { git_submodule *sm1, *sm2, *sm3; @@ -104,14 +124,12 @@ void test_submodule_modify__sync(void) cl_git_pass(git_submodule_foreach(g_repo, sync_one_submodule, NULL)); /* check that submodule config is updated */ - cl_git_pass(git_repository_config(&cfg, g_repo)); - cl_git_pass(git_config_get_string(&str, cfg, "submodule."SM1".url")); - cl_assert_equal_s(git_submodule_url(sm1), str); - cl_git_pass(git_config_get_string(&str, cfg, "submodule."SM2".url")); - cl_assert_equal_s(git_submodule_url(sm2), str); - cl_git_pass(git_config_get_string(&str, cfg, "submodule."SM3".url")); - cl_assert_equal_s(git_submodule_url(sm3), str); - git_config_free(cfg); + assert_submodule_url_is_synced( + sm1, "submodule."SM1".url", "branch.origin.remote"); + assert_submodule_url_is_synced( + sm2, "submodule."SM2".url", "branch.origin.remote"); + assert_submodule_url_is_synced( + sm3, "submodule."SM3".url", "branch.origin.remote"); git_submodule_free(sm1); git_submodule_free(sm2);