Commit db0e7878d386e40080d4004e483e4845b15f8bd7

Russell Belfer 2014-03-28T16:50:49

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.

diff --git a/src/config_file.h b/src/config_file.h
index d4a1a40..fcccbd5 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 ea0815e..6cc8ea1 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 259a314..17f04f0 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 94bed8a..5588a4f 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 4b9828a..a6182be 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))