diff mbox series

[v3,12/24] cache-tree: share code between functions writing an index as a tree

Message ID 20190815214053.16594-13-newren@gmail.com (mailing list archive)
State New, archived
Headers show
Series Clean up merge API | expand

Commit Message

Elijah Newren Aug. 15, 2019, 9:40 p.m. UTC 
write_tree_from_memory() appeared to be a merge-recursive special that
basically duplicated write_index_as_tree().  The two have a different
signature, but the bigger difference was just that write_index_as_tree()
would always unconditionally read the index off of disk instead of
working on the current in-memory index.  So:

  * split out common code into write_index_as_tree_internal()

  * rename write_tree_from_memory() to write_inmemory_index_as_tree(),
    make it call write_index_as_tree_internal(), and move it to
    cache-tree.c

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/checkout.c |  2 +-
 cache-tree.c       | 81 +++++++++++++++++++++++++++++++++-------------
 cache-tree.h       |  3 +-
 merge-recursive.c  | 34 ++-----------------
 merge-recursive.h  |  1 -
 5 files changed, 63 insertions(+), 58 deletions(-)

Comments

Junio C Hamano Aug. 16, 2019, 10:01 p.m. UTC | #1 
Elijah Newren <newren@gmail.com> writes:

> write_tree_from_memory() appeared to be a merge-recursive special that
> basically duplicated write_index_as_tree().  The two have a different
> signature, but the bigger difference was just that write_index_as_tree()
> would always unconditionally read the index off of disk instead of
> working on the current in-memory index.  So:
>
>   * split out common code into write_index_as_tree_internal()
>
>   * rename write_tree_from_memory() to write_inmemory_index_as_tree(),

Somewhat minor, but I find "inmemory_index" hard to see while
scanning the patch.  Perhaps call it "in_core_index" instead?

I originally started the above with "Very minor, ...", but as this
is exposed to the public in a header file, the name matters a bit
more than that.
Elijah Newren Aug. 16, 2019, 10:39 p.m. UTC | #2 
On Fri, Aug 16, 2019 at 3:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > write_tree_from_memory() appeared to be a merge-recursive special that
> > basically duplicated write_index_as_tree().  The two have a different
> > signature, but the bigger difference was just that write_index_as_tree()
> > would always unconditionally read the index off of disk instead of
> > working on the current in-memory index.  So:
> >
> >   * split out common code into write_index_as_tree_internal()
> >
> >   * rename write_tree_from_memory() to write_inmemory_index_as_tree(),
>
> Somewhat minor, but I find "inmemory_index" hard to see while
> scanning the patch.  Perhaps call it "in_core_index" instead?
>
> I originally started the above with "Very minor, ...", but as this
> is exposed to the public in a header file, the name matters a bit
> more than that.

Sounds good; will change.
diff mbox series

Patch

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 90e0eaf25e..19a3a0bae5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -760,7 +760,7 @@  static int merge_working_tree(const struct checkout_opts *opts,
 			 */
 			init_merge_options(&o, the_repository);
 			o.verbosity = 0;
-			work = write_tree_from_memory(&o);
+			work = write_inmemory_index_as_tree(the_repository);
 
 			ret = reset_tree(new_tree,
 					 opts, 1,
diff --git a/cache-tree.c b/cache-tree.c
index 706ffcf188..00eda3e537 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -608,11 +608,62 @@  static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat
 	return it;
 }
 
+int write_index_as_tree_internal(struct object_id *oid, struct index_state *index_state, int cache_tree_valid, int flags, const char *prefix)
+{
+	if (flags & WRITE_TREE_IGNORE_CACHE_TREE) {
+		cache_tree_free(&index_state->cache_tree);
+		cache_tree_valid = 0;
+	}
+
+	if (!index_state->cache_tree)
+		index_state->cache_tree = cache_tree();
+
+	if (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)
+		return WRITE_TREE_UNMERGED_INDEX;
+
+	if (prefix) {
+		struct cache_tree *subtree;
+		subtree = cache_tree_find(index_state->cache_tree, prefix);
+		if (!subtree)
+			return WRITE_TREE_PREFIX_ERROR;
+		oidcpy(oid, &subtree->oid);
+	}
+	else
+		oidcpy(oid, &index_state->cache_tree->oid);
+
+	return 0;
+}
+
+struct tree* write_inmemory_index_as_tree(struct repository *repo) {
+	struct object_id o;
+	int was_valid, ret;
+
+	struct index_state *index_state	= repo->index;
+	was_valid = index_state->cache_tree &&
+		    cache_tree_fully_valid(index_state->cache_tree);
+
+	ret = write_index_as_tree_internal(&o, index_state, was_valid, 0, NULL);
+	if (ret == WRITE_TREE_UNMERGED_INDEX) {
+		int i;
+		fprintf(stderr, "BUG: There are unmerged index entries:\n");
+		for (i = 0; i < index_state->cache_nr; i++) {
+			const struct cache_entry *ce = index_state->cache[i];
+			if (ce_stage(ce))
+				fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
+					(int)ce_namelen(ce), ce->name);
+		}
+		BUG("unmerged index entries when writing inmemory index");
+	}
+
+	return lookup_tree(repo, &index_state->cache_tree->oid);
+}
+
+
 int write_index_as_tree(struct object_id *oid, struct index_state *index_state, const char *index_path, int flags, const char *prefix)
 {
 	int entries, was_valid;
 	struct lock_file lock_file = LOCK_INIT;
-	int ret = 0;
+	int ret;
 
 	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
 
@@ -621,18 +672,14 @@  int write_index_as_tree(struct object_id *oid, struct index_state *index_state,
 		ret = WRITE_TREE_UNREADABLE_INDEX;
 		goto out;
 	}
-	if (flags & WRITE_TREE_IGNORE_CACHE_TREE)
-		cache_tree_free(&index_state->cache_tree);
 
-	if (!index_state->cache_tree)
-		index_state->cache_tree = cache_tree();
+	was_valid = !(flags & WRITE_TREE_IGNORE_CACHE_TREE) &&
+		    index_state->cache_tree &&
+		    cache_tree_fully_valid(index_state->cache_tree);
 
-	was_valid = cache_tree_fully_valid(index_state->cache_tree);
-	if (!was_valid) {
-		if (cache_tree_update(index_state, flags) < 0) {
-			ret = WRITE_TREE_UNMERGED_INDEX;
-			goto out;
-		}
+	ret = write_index_as_tree_internal(oid, index_state, was_valid, flags,
+					   prefix);
+	if (!ret && !was_valid) {
 		write_locked_index(index_state, &lock_file, COMMIT_LOCK);
 		/* Not being able to write is fine -- we are only interested
 		 * in updating the cache-tree part, and if the next caller
@@ -642,18 +689,6 @@  int write_index_as_tree(struct object_id *oid, struct index_state *index_state,
 		 */
 	}
 
-	if (prefix) {
-		struct cache_tree *subtree;
-		subtree = cache_tree_find(index_state->cache_tree, prefix);
-		if (!subtree) {
-			ret = WRITE_TREE_PREFIX_ERROR;
-			goto out;
-		}
-		oidcpy(oid, &subtree->oid);
-	}
-	else
-		oidcpy(oid, &index_state->cache_tree->oid);
-
 out:
 	rollback_lock_file(&lock_file);
 	return ret;
diff --git a/cache-tree.h b/cache-tree.h
index 757bbc48bc..8ce1632bc6 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -34,7 +34,7 @@  int cache_tree_fully_valid(struct cache_tree *);
 int cache_tree_update(struct index_state *, int);
 void cache_tree_verify(struct repository *, struct index_state *);
 
-/* bitmasks to write_cache_as_tree flags */
+/* bitmasks to write_index_as_tree flags */
 #define WRITE_TREE_MISSING_OK 1
 #define WRITE_TREE_IGNORE_CACHE_TREE 2
 #define WRITE_TREE_DRY_RUN 4
@@ -46,6 +46,7 @@  void cache_tree_verify(struct repository *, struct index_state *);
 #define WRITE_TREE_UNMERGED_INDEX (-2)
 #define WRITE_TREE_PREFIX_ERROR (-3)
 
+struct tree* write_inmemory_index_as_tree(struct repository *repo);
 int write_index_as_tree(struct object_id *oid, struct index_state *index_state, const char *index_path, int flags, const char *prefix);
 void prime_cache_tree(struct repository *, struct index_state *, struct tree *);
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 6cee867d37..1fbae52402 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -412,37 +412,6 @@  static void unpack_trees_finish(struct merge_options *opt)
 	clear_unpack_trees_porcelain(&opt->unpack_opts);
 }
 
-struct tree *write_tree_from_memory(struct merge_options *opt)
-{
-	struct tree *result = NULL;
-	struct index_state *istate = opt->repo->index;
-
-	if (unmerged_index(istate)) {
-		int i;
-		fprintf(stderr, "BUG: There are unmerged index entries:\n");
-		for (i = 0; i < istate->cache_nr; i++) {
-			const struct cache_entry *ce = istate->cache[i];
-			if (ce_stage(ce))
-				fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
-					(int)ce_namelen(ce), ce->name);
-		}
-		BUG("unmerged index entries in merge-recursive.c");
-	}
-
-	if (!istate->cache_tree)
-		istate->cache_tree = cache_tree();
-
-	if (!cache_tree_fully_valid(istate->cache_tree) &&
-	    cache_tree_update(istate, 0) < 0) {
-		err(opt, _("error building trees"));
-		return NULL;
-	}
-
-	result = lookup_tree(opt->repo, &istate->cache_tree->oid);
-
-	return result;
-}
-
 static int save_files_dirs(const struct object_id *oid,
 			   struct strbuf *base, const char *path,
 			   unsigned int mode, int stage, void *context)
@@ -3472,7 +3441,8 @@  static int merge_trees_internal(struct merge_options *opt,
 
 	unpack_trees_finish(opt);
 
-	if (opt->call_depth && !(*result = write_tree_from_memory(opt)))
+	if (opt->call_depth &&
+	    !(*result = write_inmemory_index_as_tree(opt->repo)))
 		return -1;
 
 	return clean;
diff --git a/merge-recursive.h b/merge-recursive.h
index 18012fff9d..0a3033bdb0 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -113,7 +113,6 @@  int merge_recursive_generic(struct merge_options *o,
 
 void init_merge_options(struct merge_options *o,
 			struct repository *repo);
-struct tree *write_tree_from_memory(struct merge_options *o);
 
 int parse_merge_opt(struct merge_options *out, const char *s);