threads / patch / 1703

patchChange read-tree to merge before using the index.

Subject: [PATCH] Change read-tree to merge before using the index.

## tl;dr

11 messages between Aug 31, 2005 and Sep 5, 2005. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Daniel Barkalow· Aug 31, 2005, 03:48 UTC · lore

[PATCH 0/2] Reorganize read-tree

I got mostly done with this before Linus mentioned the possibility of having multiple index entries in the same stage for a single path. I finished it anyway, but I'm not sure that we won't want to know which of the common ancestors contributed which, and, if some of them don't have a path, we wouldn't be able to tell. The other advantages I see to this approach are:

 - it uses the more common parser of tree objects, moving toward having
   only one (diff-cache still uses read_tree(), however).
 - it doesn't need to do very complicated things with the index; the
   original read-tree does a bunch of stuff with an index with a gap in
   the middle containing obsolete entries.
 - it uses a much simpler method of finding directory/file conflicts,
   which is possible because the struct trees represent directories as
   well as files.
 - it deals with each path completely before going on to the next one,
   instead of first dealing with each input tree and then dealing with
   each path.
 - it removes a lot of intimate knowledge of the index structure from the
   program.

The general idea is that it figures out what trees you want, and then iterates through the entry lists together, recursing into directories, and calls the merge function with an array of the index entries (not yet added) for the path in each tree; the merge function adds the appropriate things to the index.

Note that this set doesn't include calling merge functions with multiple ancestors or remotes; that can be done when we've decided on whether my version of read-tree is worth using.

There are various potential refinements, plus removing a bunch of memory leaks, still to do, but I think this is sufficiently close to review.

(Refinements: it ought to have two indices in memory, the old and the new, and never modify the old and only append to the new, to simplify things further; it ought to use a sentinal value for the index entry to indicate that there is something in the tree to conflict with there being a file at the given path; the --emu23 logic could be clearer)

The first patch adds a few functions to the object library. The second patch changes read-tree around; It is essentially a rewrite, except for the merge functions and main().

	-Daniel
*This .sig left intentionally blank*
Daniel Barkalow· Aug 31, 2005, 03:49 UTC · re: Daniel Barkalow · lore

[PATCH 1/2] Object model additions for read-tree

Adds object_list_append() and a function to get the struct tree from an ent.
Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 object.c |   11 +++++++++++
 object.h |    3 +++
 tree.c   |   19 +++++++++++++++++++
 tree.h   |    3 +++
 4 files changed, 36 insertions(+), 0 deletions(-)
49d33c385aa69d17c991300f73e77c6718a2b4a6
Show changes to 4 files +36 −0

object.c, object.h, tree.c, tree.h

diff --git a/object.c b/object.c
--- a/object.c
+++ b/object.c
@@ -184,6 +184,17 @@ struct object_list *object_list_insert(s
         return new_list;
 }

+void object_list_append(struct object *item,
+			struct object_list **list_p)
+{
+	while (*list_p) {
+		list_p = &((*list_p)->next);
+	}
+	*list_p = xmalloc(sizeof(struct object_list));
+	(*list_p)->next = NULL;
+	(*list_p)->item = item;
+}
+
 unsigned object_list_length(struct object_list *list)
 {
 	unsigned ret = 0;
diff --git a/object.h b/object.h
--- a/object.h
+++ b/object.h
@@ -41,6 +41,9 @@ void mark_reachable(struct object *obj,
 struct object_list *object_list_insert(struct object *item,
 				       struct object_list **list_p);

+void object_list_append(struct object *item,
+			struct object_list **list_p);
+
 unsigned object_list_length(struct object_list *list);

 int object_list_contains(struct object_list *list, struct object *obj);
diff --git a/tree.c b/tree.c
--- a/tree.c
+++ b/tree.c
@@ -1,5 +1,7 @@
 #include "tree.h"
 #include "blob.h"
+#include "commit.h"
+#include "tag.h"
 #include "cache.h"
 #include <stdlib.h>

@@ -212,3 +214,20 @@ int parse_tree(struct tree *item)
 	free(buffer);
 	return ret;
 }
+
+struct tree *parse_tree_indirect(const unsigned char *sha1)
+{
+	struct object *obj = parse_object(sha1);
+	do {
+		if (!obj)
+			return NULL;
+		if (obj->type == tree_type)
+			return (struct tree *) obj;
+		else if (obj->type == commit_type)
+			return ((struct commit *) obj)->tree;
+		else if (obj->type == tag_type)
+			obj = ((struct tag *) obj)->tagged;
+		else
+			return NULL;
+	} while (1);
+}
diff --git a/tree.h b/tree.h
--- a/tree.h
+++ b/tree.h
@@ -32,4 +32,7 @@ int parse_tree_buffer(struct tree *item,

 int parse_tree(struct tree *tree);

+/* Parses and returns the tree in the given ent, chasing tags and commits. */
+struct tree *parse_tree_indirect(const unsigned char *sha1);
+
 #endif /* TREE_H */
Daniel Barkalow· Aug 31, 2005, 03:49 UTC · re: Daniel Barkalow · lore
Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 read-tree.c |  522 ++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 297 insertions(+), 225 deletions(-)
d0f45ad81db2e133c49c23bd09c5615da344bb5c
Show changes to read-tree.c +297 −225
diff --git a/read-tree.c b/read-tree.c
--- a/read-tree.c
+++ b/read-tree.c
@@ -5,28 +5,280 @@
  */
 #include "cache.h"

-static int stage = 0;
+#include "object.h"
+#include "tree.h"
+
+static int merge = 0;
+static int emu23 = 0;
 static int update = 0;

-static int unpack_tree(unsigned char *sha1)
+static struct object_list *trees = NULL;
+
+typedef int (*merge_fn_t)(struct cache_entry **src,
+			  struct cache_entry **dest,
+			  int df_conflicts_2,
+			  int df_conflicts_3);
+
+static int unpack_trees_rec(struct tree_entry_list **posns, int len,
+			    const char *base, merge_fn_t fn,
+			    int file2, int file3, int *indpos)
+{
+	int baselen = strlen(base);
+	int src_size = len + 1;
+	if (emu23)
+		src_size++;
+	if (src_size > 4)
+		src_size = 4;
+	do {
+		int i;
+		char *first = NULL;
+		int pathlen;
+		unsigned ce_size;
+		int dir2 = 0;
+		int dir3 = 0;
+		int subfile2 = file2;
+		int subfile3 = file3;
+		struct tree_entry_list **subposns = NULL;
+		struct cache_entry **src = NULL;
+		char *cache_name = NULL;
+
+		/* Find the first name in the input. */
+
+		/* Check the cache */
+		if (merge && *indpos < active_nr) {
+			/* This is a bit tricky: */
+			/* If the index has a subdirectory (with
+			 * contents) as the first name, it'll get a
+			 * filename like "foo/bar". But that's after
+			 * "foo", so the entry in trees will get
+			 * handled first, at which point we'll go into
+			 * "foo", and deal with "bar" from the index,
+			 * because the base will be "foo/". The only
+			 * way we can actually have "foo/bar" first of
+			 * all the things is if the trees don't
+			 * contain "foo" at all, in which case we'll
+			 * handle "foo/bar" without going into the
+			 * directory, but that's fine (and will return
+			 * an error anyway, with the added unknown
+			 * file case.
+			 */
+
+			cache_name = active_cache[*indpos]->name;
+			if (strlen(cache_name) > baselen &&
+			    !memcmp(cache_name, base, baselen)) {
+				cache_name += baselen;
+				first = cache_name;
+			} else {
+				cache_name = NULL;
+			}
+		}
+
+		for (i = 0; i < len; i++) {
+			if (!posns[i])
+				continue;
+			if (!first || strcmp(first, posns[i]->name) > 0)
+				first = posns[i]->name;
+		}
+		/* No name means we're done */
+		if (!first)
+			return 0;
+
+		pathlen = strlen(first);
+		ce_size = cache_entry_size(baselen + pathlen);
+
+		if (cache_name && !strcmp(cache_name, first)) {
+			src = xmalloc(sizeof(struct cache_entry *) *
+				      src_size);
+			memset(src, 0,
+			       sizeof(struct cache_entry *) *
+			       src_size);
+			src[0] = active_cache[*indpos];
+			remove_cache_entry_at(*indpos);
+			if (emu23) {
+				// we need this in stage 2 as well as stage 0
+				struct cache_entry *copy =
+					xmalloc(ce_size);
+				memcpy(copy, src[0], ce_size);
+				copy->ce_flags =
+					create_ce_flags(baselen + pathlen, 2);
+				if (dir2 || file2) {
+					die("cannot merge index and our head tree");
+				}
+				src[2] = copy;
+				subfile2 = 1;
+			}
+		}
+
+		for (i = 0; i < len; i++) {
+			struct cache_entry *ce;
+			int ce_stage;
+			if (!posns[i] ||
+			    strcmp(first, posns[i]->name)) {
+				continue;
+			}
+
+			if (len > 3) {
+				if (i - len + 3 >= 0)
+					ce_stage = i - len + 3 + merge;
+				else
+					ce_stage = merge;
+			} else {
+				ce_stage = i + merge;
+			}
+			if (emu23 && ce_stage == 2)
+				ce_stage = 3;
+
+			if (posns[i]->directory) {
+				if (!subposns) {
+					int posnlen = sizeof(struct tree_list_entry *) * len;
+					subposns = xmalloc(posnlen);
+					memset(subposns, 0, posnlen);
+				}
+				parse_tree(posns[i]->item.tree);
+				subposns[i] = posns[i]->item.tree->entries;
+				posns[i] = posns[i]->next;
+				if (emu23 && ce_stage == 1)
+					dir2 = 1;
+				if (ce_stage == 2)
+					dir2 = 1;
+				if (ce_stage == 3)
+					dir3 = 1;
+				continue;
+			}
+
+			if (ce_stage == 2)
+				subfile2 = 1;
+			if (ce_stage == 3)
+				subfile3 = 1;
+
+			ce = xmalloc(ce_size);
+			memset(ce, 0, ce_size);
+			ce->ce_mode = create_ce_mode(posns[i]->mode);
+			ce->ce_flags = create_ce_flags(baselen + pathlen,
+						       ce_stage);
+			memcpy(ce->name, base, baselen);
+			memcpy(ce->name + baselen, first, pathlen + 1);
+
+			if (!src) {
+				src = xmalloc(sizeof(struct cache_entry *) *
+					      src_size);
+				memset(src, 0,
+				       sizeof(struct cache_entry *) *
+				       src_size);
+			}
+
+			memcpy(ce->sha1, posns[i]->item.any->sha1, 20);
+			if (ce_stage == 1 && emu23 && !src[2]) {
+				// we need this in stage 2 as well as stage 1
+				struct cache_entry *copy =
+					xmalloc(ce_size);
+				memcpy(copy, ce, ce_size);
+				copy->ce_flags =
+					create_ce_flags(baselen + pathlen, 2);
+				if (dir2 || file2) {
+					die("cannot merge index and our head tree");
+				}
+				src[2] = copy;
+				subfile2 = 1;
+			}
+			src[ce_stage] = ce;
+			posns[i] = posns[i]->next;
+		}
+		if (src) {
+			if (fn) {
+				int ret;
+				/*
+				printf("%s:\n", first);
+				for (i = 0; i < src_size; i++) {
+					printf(" %d ", i);
+					if (src[i])
+						printf("%s\n", sha1_to_hex(src[i]->sha1));
+					else
+						printf("\n");
+				}
+				*/
+				ret = fn(src, NULL,
+					 (file2 || dir2),
+					 (file3 || dir3));
+				/*printf("Added %d entries\n", ret);*/
+				*indpos += ret;
+			} else {
+				for (i = 0; i < src_size; i++) {
+					if (src[i]) {
+						add_cache_entry(src[i], ADD_CACHE_OK_TO_ADD|ADD_CACHE_SKIP_DFCHECK);
+					}
+				}
+			}
+		}
+		if (subposns) {
+			char *newbase = xmalloc(baselen + 2 + pathlen);
+			memcpy(newbase, base, baselen);
+			memcpy(newbase + baselen, first, pathlen);
+			newbase[baselen + pathlen] = '/';
+			newbase[baselen + pathlen + 1] = '\0';
+			if (unpack_trees_rec(subposns, len, newbase, fn,
+					     subfile2, subfile3, indpos))
+				return -1;
+		}
+	} while (1);
+}
+
+static void reject_merge(struct cache_entry *ce)
+{
+	die("Entry '%s' would be overwritten by merge. Cannot merge.",
+	    ce->name);
+}
+
+static void check_updates(struct cache_entry **src, int nr)
 {
-	void *buffer;
-	unsigned long size;
-	int ret;
+	static struct checkout state = {
+		.base_dir = "",
+		.force = 1,
+		.quiet = 1,
+		.refresh_cache = 1,
+	};
+	unsigned short mask = htons(CE_UPDATE);
+	while (nr--) {
+		struct cache_entry *ce = *src++;
+		if (!ce->ce_mode) {
+			if (update)
+				unlink(ce->name);
+			continue;
+		}
+		if (ce->ce_flags & mask) {
+			ce->ce_flags &= ~mask;
+			if (update)
+				checkout_entry(ce, &state);
+		}
+	}
+}

-	buffer = read_object_with_reference(sha1, "tree", &size, NULL);
-	if (!buffer)
+static int unpack_trees(merge_fn_t fn)
+{
+	int indpos = 0;
+	unsigned len = object_list_length(trees);
+	struct tree_entry_list **posns =
+		xmalloc(len * sizeof(struct tree_entry_list *));
+	int i;
+	struct object_list *posn = trees;
+	for (i = 0; i < len; i++) {
+		posns[i] = ((struct tree *) posn->item)->entries;
+		posn = posn->next;
+	}
+	if (unpack_trees_rec(posns, len, "", fn, 0, 0, &indpos))
 		return -1;
-	ret = read_tree(buffer, size, stage, NULL);
-	free(buffer);
-	return ret;
+
+	check_updates(active_cache, active_nr);
+	return 0;
 }

-static int path_matches(struct cache_entry *a, struct cache_entry *b)
+static int list_tree(unsigned char *sha1)
 {
-	int len = ce_namelen(a);
-	return ce_namelen(b) == len &&
-		!memcmp(a->name, b->name, len);
+	struct tree *tree = parse_tree_indirect(sha1);
+	if (!tree)
+		return -1;
+	object_list_append(&tree->object, &trees);
+	return 0;
 }

 static int same(struct cache_entry *a, struct cache_entry *b)
@@ -91,17 +343,6 @@ static void verify_uptodate(struct cache
 	die("Entry '%s' not uptodate. Cannot merge.", ce->name);
 }

-/*
- * If the old tree contained a CE that isn't even in the
- * result, that's always a problem, regardless of whether
- * it's up-to-date or not (ie it can be a file that we
- * have updated but not committed yet).
- */
-static void reject_merge(struct cache_entry *ce)
-{
-	die("Entry '%s' would be overwritten by merge. Cannot merge.", ce->name);
-}
-
 static int merged_entry_internal(struct cache_entry *merge, struct cache_entry *old, struct cache_entry **dst, int allow_dirty)
 {
 	merge->ce_flags |= htons(CE_UPDATE);
@@ -120,7 +361,7 @@ static int merged_entry_internal(struct
 		}
 	}
 	merge->ce_flags &= ~htons(CE_STAGEMASK);
-	*dst++ = merge;
+	add_cache_entry(merge, ADD_CACHE_OK_TO_ADD);
 	return 1;
 }

@@ -139,76 +380,19 @@ static int deleted_entry(struct cache_en
 	if (old)
 		verify_uptodate(old);
 	ce->ce_mode = 0;
-	*dst++ = ce;
+	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD);
 	return 1;
 }

-static int causes_df_conflict(struct cache_entry *ce, int stage,
-			      struct cache_entry **dst_,
-			      struct cache_entry **next_,
-			      int tail)
-{
-	/* This is called during the merge operation and walking
-	 * the active_cache[] array is messy, because it is in the
-	 * middle of overlapping copy operation.  The invariants
-	 * are:
-	 * (1) active_cache points at the first (zeroth) entry.
-	 * (2) up to dst pointer are resolved entries.
-	 * (3) from the next pointer (head-inclusive) to the tail
-	 *     of the active_cache array have the remaining paths
-	 *     to be processed.  There can be a gap between dst
-	 *     and next.  Note that next is called "src" in the
-	 *     merge_cache() function, and tail is the original
-	 *     end of active_cache array when merge_cache() started.
-	 * (4) the path corresponding to *ce is not found in (2)
-	 *     or (3).  It is in the gap.
-	 *
-	 *  active_cache -----......+++++++++++++.
-	 *                    ^dst  ^next        ^tail
-	 */
-	int i, next, dst;
-	const char *path = ce->name;
-	int namelen = ce_namelen(ce);
-
-	next = next_ - active_cache;
-	dst = dst_ - active_cache;
-
-	for (i = 0; i < tail; i++) {
-		int entlen, len;
-		const char *one, *two;
-		if (dst <= i && i < next)
-			continue;
-		ce = active_cache[i];
-		if (ce_stage(ce) != stage)
-			continue;
-		/* If ce->name is a prefix of path, then path is a file
-		 * that hangs underneath ce->name, which is bad.
-		 * If path is a prefix of ce->name, then it is the
-		 * other way around which also is bad.
-		 */
-		entlen = ce_namelen(ce);
-		if (namelen == entlen)
-			continue;
-		if (namelen < entlen) {
-			len = namelen;
-			one = path;
-			two = ce->name;
-		} else {
-			len = entlen;
-			one = ce->name;
-			two = path;
-		}
-		if (memcmp(one, two, len))
-			continue;
-		if (two[len] == '/')
-			return 1;
-	}
-	return 0;
+static int keep_entry(struct cache_entry *ce, struct cache_entry **dst)
+{
+	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD);
+	return 1;
 }

 static int threeway_merge(struct cache_entry *stages[4],
 			  struct cache_entry **dst,
-			  struct cache_entry **next, int tail)
+			  int df_conflict_2, int df_conflict_3)
 {
 	struct cache_entry *old = stages[0];
 	struct cache_entry *a = stages[1], *b = stages[2], *c = stages[3];
@@ -218,7 +402,7 @@ static int threeway_merge(struct cache_e
 	/* #5ALT */
 	if (!a && b && c && same(b, c)) {
 		if (old && !same(b, old))
-			return -1;
+			reject_merge(old);
 		return merged_entry_allow_dirty(b, old, dst);
 	}
 	/* #2ALT and #3ALT */
@@ -269,12 +453,12 @@ static int threeway_merge(struct cache_e
 		 *     and vice versa.
 		 */
 		if (c) { /* #2ALT */
-			if (!causes_df_conflict(c, 2, dst, next, tail) &&
+			if (!df_conflict_2 &&
 			    (!old || same(c, old)))
 				return merged_entry_allow_dirty(c, old, dst);
 		}
 		else { /* #3ALT */
-			if (!causes_df_conflict(b, 3, dst, next, tail) &&
+			if (!df_conflict_3 &&
 			    (!old || same(b, old)))
 				return merged_entry_allow_dirty(b, old, dst);
 		}
@@ -293,7 +477,7 @@ static int threeway_merge(struct cache_e
 	 */
 	if (old) {
 		if (!b || !same(old, b))
-			return -1;
+			reject_merge(old);
 	}
 	merge = merge_entries(a, b, c);
 	if (merge)
@@ -301,9 +485,9 @@ static int threeway_merge(struct cache_e
 	if (old)
 		verify_uptodate(old);
 	count = 0;
-	if (a) { *dst++ = a; count++; }
-	if (b) { *dst++ = b; count++; }
-	if (c) { *dst++ = c; count++; }
+	if (a) { count += keep_entry(a, dst + count); }
+	if (b) { count += keep_entry(b, dst + count); }
+	if (c) { count += keep_entry(c, dst + count); }
 	return count;
 }

@@ -317,14 +501,11 @@ static int threeway_merge(struct cache_e
  *
  */
 static int twoway_merge(struct cache_entry **src, struct cache_entry **dst,
-			struct cache_entry **next, int tail)
+			int df23, int df32)
 {
 	struct cache_entry *current = src[0];
 	struct cache_entry *oldtree = src[1], *newtree = src[2];

-	if (src[3])
-		return -1;
-
 	if (current) {
 		if ((!oldtree && !newtree) || /* 4 and 5 */
 		    (!oldtree && newtree &&
@@ -334,8 +515,7 @@ static int twoway_merge(struct cache_ent
 		    (oldtree && newtree &&
 		     !same(oldtree, newtree) && /* 18 and 19*/
 		     same(current, newtree))) {
-			*dst++ = current;
-			return 1;
+			return keep_entry(current, dst);
 		}
 		else if (oldtree && !newtree && same(current, oldtree)) {
 			/* 10 or 11 */
@@ -346,9 +526,16 @@ static int twoway_merge(struct cache_ent
 			/* 20 or 21 */
 			return merged_entry(newtree, current, dst);
 		}
-		else
+		else {
 			/* all other failures */
+			if (oldtree)
+				reject_merge(oldtree);
+			if (current)
+				reject_merge(current);
+			if (newtree)
+				reject_merge(newtree);
 			return -1;
+		}
 	}
 	else if (newtree)
 		return merged_entry(newtree, current, dst);
@@ -357,137 +544,25 @@ static int twoway_merge(struct cache_ent
 }

 /*
- * Two-way merge emulated with three-way merge.
- *
- * This treats "read-tree -m H M" by transforming it internally
- * into "read-tree -m H I+H M", where I+H is a tree that would
- * contain the contents of the current index file, overlayed on
- * top of H.  Unlike the traditional two-way merge, this leaves
- * the stages in the resulting index file and lets the user resolve
- * the merge conflicts using standard tools for three-way merge.
- *
- * This function is just to set-up such an arrangement, and the
- * actual merge uses threeway_merge() function.
- */
-static void setup_emu23(void)
-{
-	/* stage0 contains I, stage1 H, stage2 M.
-	 * move stage2 to stage3, and create stage2 entries
-	 * by scanning stage0 and stage1 entries.
-	 */
-	int i, namelen, size;
-	struct cache_entry *ce, *stage2;
-
-	for (i = 0; i < active_nr; i++) {
-		ce = active_cache[i];
-		if (ce_stage(ce) != 2)
-			continue;
-		/* hoist them up to stage 3 */
-		namelen = ce_namelen(ce);
-		ce->ce_flags = create_ce_flags(namelen, 3);
-	}
-
-	for (i = 0; i < active_nr; i++) {
-		ce = active_cache[i];
-		if (ce_stage(ce) > 1)
-			continue;
-		namelen = ce_namelen(ce);
-		size = cache_entry_size(namelen);
-		stage2 = xmalloc(size);
-		memcpy(stage2, ce, size);
-		stage2->ce_flags = create_ce_flags(namelen, 2);
-		if (add_cache_entry(stage2, ADD_CACHE_OK_TO_ADD) < 0)
-			die("cannot merge index and our head tree");
-
-		/* We are done with this name, so skip to next name */
-		while (i < active_nr &&
-		       ce_namelen(active_cache[i]) == namelen &&
-		       !memcmp(active_cache[i]->name, ce->name, namelen))
-			i++;
-		i--; /* compensate for the loop control */
-	}
-}
-
-/*
  * One-way merge.
  *
  * The rule is:
  * - take the stat information from stage0, take the data from stage1
  */
 static int oneway_merge(struct cache_entry **src, struct cache_entry **dst,
-			struct cache_entry **next, int tail)
+			int df23, int df32)
 {
 	struct cache_entry *old = src[0];
 	struct cache_entry *a = src[1];

-	if (src[2] || src[3])
-		return -1;
-
 	if (!a)
 		return 0;
 	if (old && same(old, a)) {
-		*dst++ = old;
-		return 1;
+		return keep_entry(old, dst);
 	}
 	return merged_entry(a, NULL, dst);
 }

-static void check_updates(struct cache_entry **src, int nr)
-{
-	static struct checkout state = {
-		.base_dir = "",
-		.force = 1,
-		.quiet = 1,
-		.refresh_cache = 1,
-	};
-	unsigned short mask = htons(CE_UPDATE);
-	while (nr--) {
-		struct cache_entry *ce = *src++;
-		if (!ce->ce_mode) {
-			if (update)
-				unlink(ce->name);
-			continue;
-		}
-		if (ce->ce_flags & mask) {
-			ce->ce_flags &= ~mask;
-			if (update)
-				checkout_entry(ce, &state);
-		}
-	}
-}
-
-typedef int (*merge_fn_t)(struct cache_entry **, struct cache_entry **, struct cache_entry **, int);
-
-static void merge_cache(struct cache_entry **src, int nr, merge_fn_t fn)
-{
-	struct cache_entry **dst = src;
-	int tail = nr;
-
-	while (nr) {
-		int entries;
-		struct cache_entry *name, *ce, *stages[4] = { NULL, };
-
-		name = ce = *src;
-		for (;;) {
-			int stage = ce_stage(ce);
-			stages[stage] = ce;
-			ce = *++src;
-			active_nr--;
-			if (!--nr)
-				break;
-			if (!path_matches(ce, name))
-				break;
-		}
-
-		entries = fn(stages, dst, src, tail);
-		if (entries < 0)
-			reject_merge(name);
-		dst += entries;
-		active_nr += entries;
-	}
-	check_updates(active_cache, active_nr);
-}
-
 static int read_cache_unmerged(void)
 {
 	int i, deleted;
@@ -516,8 +591,9 @@ static struct cache_file cache_file;

 int main(int argc, char **argv)
 {
-	int i, newfd, merge, reset, emu23;
+	int i, newfd, reset, stage = 0;
 	unsigned char sha1[20];
+	merge_fn_t fn = NULL;

 	newfd = hold_index_file_for_update(&cache_file, get_index_file());
 	if (newfd < 0)
@@ -569,9 +645,7 @@ int main(int argc, char **argv)

 		if (get_sha1(arg, sha1) < 0)
 			usage(read_tree_usage);
-		if (stage > 3)
-			usage(read_tree_usage);
-		if (unpack_tree(sha1) < 0)
+		if (list_tree(sha1) < 0)
 			die("failed to unpack tree object %s", arg);
 		stage++;
 	}
@@ -583,19 +657,17 @@ int main(int argc, char **argv)
 			[2] = twoway_merge,
 			[3] = threeway_merge,
 		};
-		merge_fn_t fn;

-		if (stage < 2 || stage > 4)
+		if (stage < 2)
 			die("just how do you expect me to merge %d trees?", stage-1);
 		if (emu23 && stage != 3)
 			die("--emu23 takes only two trees");
 		fn = merge_function[stage-1];
 		if (stage == 3 && emu23) {
-			setup_emu23();
 			fn = merge_function[3];
 		}
-		merge_cache(active_cache, active_nr, fn);
 	}
+	unpack_trees(fn);
 	if (write_cache(newfd, active_cache, active_nr) ||
 	    commit_index_file(&cache_file))
 		die("unable to write new index file");
Junio C Hamano· Aug 31, 2005, 06:28 UTC · re: Daniel Barkalow · lore

Re: [PATCH 0/2] Reorganize read-tree

Dan, I really really *REALLY* wanted to try this out in "pu" branch and even was about to rig some torture chamber for testing before applying the patch, but you got the shiny blue bat X-<.

I've checked with both marc and gmane [*], and believe the problem is not on my end. The MUA seems to have stripped all the trailing whitespaces, so you do not have a single SP on empty context lines, and lack excess trailing whitespace on line 41 of object.h (object_list_insert declaration), for example.

A patch to SubmittingPatches, MUA specific help section for users of Pine 4.63 would be very much appreciated.

[References]

http://marc.theaimsgroup.com/?l=git&m=112545993929060&q=raw http://www.mail-archive.com/git@vger.kernel.org/msg03590.html

Daniel Barkalow· Aug 31, 2005, 16:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Reorganize read-tree

On Tue, 30 Aug 2005, Junio C Hamano wrote:
> Dan, I really really *REALLY* wanted to try this out in "pu"
> branch and even was about to rig some torture chamber for
> testing before applying the patch, but you got the shiny blue
> bat X-<.
I'll send a replacement with the settings correct.
> A patch to SubmittingPatches, MUA specific help section for
> users of Pine 4.63 would be very much appreciated.

Ah, it looks like a recent version changed the default behavior to do the right thing, and inverted the sense of the configuration option. (Either that or Gentoo did it.) So you need to set the "no-strip-whitespace-before-send" option, unless the option you have is "strip-whitespace-before-send", in which case you should avoid checking it.

I don't actually have things set up for preparing patches from work, although I can resend the patches I prepared earlier.

	-Daniel
*This .sig left intentionally blank*
Catalin Marinas· Aug 31, 2005, 14:15 UTC · re: Daniel Barkalow · lore

Re: [PATCH 0/2] Reorganize read-tree

Daniel Barkalow <barkalow@iabervon.org> wrote:
Show 5 quoted lines
> I got mostly done with this before Linus mentioned the possibility of
> having multiple index entries in the same stage for a single path. I
> finished it anyway, but I'm not sure that we won't want to know which of
> the common ancestors contributed which, and, if some of them don't have a
> path, we wouldn't be able to tell.

I don't have time to look at the patch and I don't have a good knowledge of the GIT internals, so I will just ask. Does this patch changes the call convention for git-merge-one-file-script? I have my own script for StGIT and I would need to know whether it is affected or not.

Thanks.
-- 
Catalin
Daniel Barkalow· Aug 31, 2005, 17:04 UTC · re: Catalin Marinas · lore

Re: [PATCH 0/2] Reorganize read-tree

On Wed, 31 Aug 2005, Catalin Marinas wrote:
Show 12 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> wrote:
> > I got mostly done with this before Linus mentioned the possibility of
> > having multiple index entries in the same stage for a single path. I
> > finished it anyway, but I'm not sure that we won't want to know which of
> > the common ancestors contributed which, and, if some of them don't have a
> > path, we wouldn't be able to tell.
> 
> I don't have time to look at the patch and I don't have a good
> knowledge of the GIT internals, so I will just ask. Does this patch
> changes the call convention for git-merge-one-file-script? I have my
> own script for StGIT and I would need to know whether it is affected
> or not.

Nope, it only changes the trivial merge calling convention within read-tree.c; I think it's plausible that we might like to add information at some point, but the short-term goal is just to prevent a few bad cases in trivial merges.

Daniel Barkalow· Aug 31, 2005, 16:57 UTC · re: Daniel Barkalow · lore

[PATCH 1/2 (resend)] Object model additions for read-tree

Adds object_list_append() and a function to get the struct tree from an ent.
Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 object.c |   11 +++++++++++
 object.h |    3 +++
 tree.c   |   19 +++++++++++++++++++
 tree.h   |    3 +++
 4 files changed, 36 insertions(+), 0 deletions(-)
49d33c385aa69d17c991300f73e77c6718a2b4a6
Show changes to 4 files +36 −0

object.c, object.h, tree.c, tree.h

diff --git a/object.c b/object.c
--- a/object.c
+++ b/object.c
@@ -184,6 +184,17 @@ struct object_list *object_list_insert(s
         return new_list;
 }
 
+void object_list_append(struct object *item,
+			struct object_list **list_p)
+{
+	while (*list_p) {
+		list_p = &((*list_p)->next);
+	}
+	*list_p = xmalloc(sizeof(struct object_list));
+	(*list_p)->next = NULL;
+	(*list_p)->item = item;
+}
+
 unsigned object_list_length(struct object_list *list)
 {
 	unsigned ret = 0;
diff --git a/object.h b/object.h
--- a/object.h
+++ b/object.h
@@ -41,6 +41,9 @@ void mark_reachable(struct object *obj, 
 struct object_list *object_list_insert(struct object *item, 
 				       struct object_list **list_p);
 
+void object_list_append(struct object *item,
+			struct object_list **list_p);
+
 unsigned object_list_length(struct object_list *list);
 
 int object_list_contains(struct object_list *list, struct object *obj);
diff --git a/tree.c b/tree.c
--- a/tree.c
+++ b/tree.c
@@ -1,5 +1,7 @@
 #include "tree.h"
 #include "blob.h"
+#include "commit.h"
+#include "tag.h"
 #include "cache.h"
 #include <stdlib.h>
 
@@ -212,3 +214,20 @@ int parse_tree(struct tree *item)
 	free(buffer);
 	return ret;
 }
+
+struct tree *parse_tree_indirect(const unsigned char *sha1)
+{
+	struct object *obj = parse_object(sha1);
+	do {
+		if (!obj)
+			return NULL;
+		if (obj->type == tree_type)
+			return (struct tree *) obj;
+		else if (obj->type == commit_type)
+			return ((struct commit *) obj)->tree;
+		else if (obj->type == tag_type)
+			obj = ((struct tag *) obj)->tagged;
+		else
+			return NULL;
+	} while (1);
+}
diff --git a/tree.h b/tree.h
--- a/tree.h
+++ b/tree.h
@@ -32,4 +32,7 @@ int parse_tree_buffer(struct tree *item,
 
 int parse_tree(struct tree *tree);
 
+/* Parses and returns the tree in the given ent, chasing tags and commits. */
+struct tree *parse_tree_indirect(const unsigned char *sha1);
+
 #endif /* TREE_H */
Daniel Barkalow· Aug 31, 2005, 16:57 UTC · re: Daniel Barkalow · lore

[PATCH 2/2 (resend)] Change read-tree to merge before using the index.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 read-tree.c |  522 ++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 297 insertions(+), 225 deletions(-)
d0f45ad81db2e133c49c23bd09c5615da344bb5c
Show changes to read-tree.c +297 −225
diff --git a/read-tree.c b/read-tree.c
--- a/read-tree.c
+++ b/read-tree.c
@@ -5,28 +5,280 @@
  */
 #include "cache.h"
 
-static int stage = 0;
+#include "object.h"
+#include "tree.h"
+
+static int merge = 0;
+static int emu23 = 0;
 static int update = 0;
 
-static int unpack_tree(unsigned char *sha1)
+static struct object_list *trees = NULL;
+
+typedef int (*merge_fn_t)(struct cache_entry **src, 
+			  struct cache_entry **dest, 
+			  int df_conflicts_2,
+			  int df_conflicts_3);
+
+static int unpack_trees_rec(struct tree_entry_list **posns, int len,
+			    const char *base, merge_fn_t fn, 
+			    int file2, int file3, int *indpos)
+{
+	int baselen = strlen(base);
+	int src_size = len + 1;
+	if (emu23)
+		src_size++;
+	if (src_size > 4)
+		src_size = 4;
+	do {
+		int i;
+		char *first = NULL;
+		int pathlen;
+		unsigned ce_size;
+		int dir2 = 0;
+		int dir3 = 0;
+		int subfile2 = file2;
+		int subfile3 = file3;
+		struct tree_entry_list **subposns = NULL;
+		struct cache_entry **src = NULL;
+		char *cache_name = NULL;
+
+		/* Find the first name in the input. */
+
+		/* Check the cache */
+		if (merge && *indpos < active_nr) {
+			/* This is a bit tricky: */
+			/* If the index has a subdirectory (with
+			 * contents) as the first name, it'll get a
+			 * filename like "foo/bar". But that's after
+			 * "foo", so the entry in trees will get
+			 * handled first, at which point we'll go into
+			 * "foo", and deal with "bar" from the index,
+			 * because the base will be "foo/". The only
+			 * way we can actually have "foo/bar" first of
+			 * all the things is if the trees don't
+			 * contain "foo" at all, in which case we'll
+			 * handle "foo/bar" without going into the
+			 * directory, but that's fine (and will return
+			 * an error anyway, with the added unknown
+			 * file case.
+			 */
+
+			cache_name = active_cache[*indpos]->name;
+			if (strlen(cache_name) > baselen &&
+			    !memcmp(cache_name, base, baselen)) {
+				cache_name += baselen;
+				first = cache_name;
+			} else {
+				cache_name = NULL;
+			}
+		}
+
+		for (i = 0; i < len; i++) {
+			if (!posns[i])
+				continue;
+			if (!first || strcmp(first, posns[i]->name) > 0)
+				first = posns[i]->name;
+		}
+		/* No name means we're done */
+		if (!first)
+			return 0;
+
+		pathlen = strlen(first);
+		ce_size = cache_entry_size(baselen + pathlen);
+
+		if (cache_name && !strcmp(cache_name, first)) {
+			src = xmalloc(sizeof(struct cache_entry *) * 
+				      src_size);
+			memset(src, 0,
+			       sizeof(struct cache_entry *) * 
+			       src_size);
+			src[0] = active_cache[*indpos];
+			remove_cache_entry_at(*indpos);
+			if (emu23) {
+				// we need this in stage 2 as well as stage 0
+				struct cache_entry *copy =
+					xmalloc(ce_size);
+				memcpy(copy, src[0], ce_size);
+				copy->ce_flags = 
+					create_ce_flags(baselen + pathlen, 2);
+				if (dir2 || file2) {
+					die("cannot merge index and our head tree");
+				}
+				src[2] = copy;
+				subfile2 = 1;
+			}
+		}
+
+		for (i = 0; i < len; i++) {
+			struct cache_entry *ce;
+			int ce_stage;
+			if (!posns[i] ||
+			    strcmp(first, posns[i]->name)) {
+				continue;
+			}
+
+			if (len > 3) {
+				if (i - len + 3 >= 0)
+					ce_stage = i - len + 3 + merge;
+				else
+					ce_stage = merge;
+			} else {
+				ce_stage = i + merge;
+			}
+			if (emu23 && ce_stage == 2)
+				ce_stage = 3;
+
+			if (posns[i]->directory) {
+				if (!subposns) {
+					int posnlen = sizeof(struct tree_list_entry *) * len;
+					subposns = xmalloc(posnlen);
+					memset(subposns, 0, posnlen);
+				}
+				parse_tree(posns[i]->item.tree);
+				subposns[i] = posns[i]->item.tree->entries;
+				posns[i] = posns[i]->next;
+				if (emu23 && ce_stage == 1)
+					dir2 = 1;
+				if (ce_stage == 2)
+					dir2 = 1;
+				if (ce_stage == 3)
+					dir3 = 1;
+				continue;
+			}
+
+			if (ce_stage == 2)
+				subfile2 = 1;
+			if (ce_stage == 3)
+				subfile3 = 1;
+
+			ce = xmalloc(ce_size);
+			memset(ce, 0, ce_size);
+			ce->ce_mode = create_ce_mode(posns[i]->mode);
+			ce->ce_flags = create_ce_flags(baselen + pathlen,
+						       ce_stage);
+			memcpy(ce->name, base, baselen);
+			memcpy(ce->name + baselen, first, pathlen + 1);
+
+			if (!src) {
+				src = xmalloc(sizeof(struct cache_entry *) * 
+					      src_size);
+				memset(src, 0,
+				       sizeof(struct cache_entry *) * 
+				       src_size);
+			}
+
+			memcpy(ce->sha1, posns[i]->item.any->sha1, 20);
+			if (ce_stage == 1 && emu23 && !src[2]) {
+				// we need this in stage 2 as well as stage 1
+				struct cache_entry *copy =
+					xmalloc(ce_size);
+				memcpy(copy, ce, ce_size);
+				copy->ce_flags = 
+					create_ce_flags(baselen + pathlen, 2);
+				if (dir2 || file2) {
+					die("cannot merge index and our head tree");
+				}
+				src[2] = copy;
+				subfile2 = 1;
+			}
+			src[ce_stage] = ce;
+			posns[i] = posns[i]->next;
+		}
+		if (src) {
+			if (fn) {
+				int ret;
+				/*
+				printf("%s:\n", first);
+				for (i = 0; i < src_size; i++) {
+					printf(" %d ", i);
+					if (src[i])
+						printf("%s\n", sha1_to_hex(src[i]->sha1));
+					else
+						printf("\n");
+				}
+				*/
+				ret = fn(src, NULL,
+					 (file2 || dir2),
+					 (file3 || dir3));
+				/*printf("Added %d entries\n", ret);*/
+				*indpos += ret;
+			} else {
+				for (i = 0; i < src_size; i++) {
+					if (src[i]) {
+						add_cache_entry(src[i], ADD_CACHE_OK_TO_ADD|ADD_CACHE_SKIP_DFCHECK);
+					}
+				}
+			}
+		}
+		if (subposns) {
+			char *newbase = xmalloc(baselen + 2 + pathlen);
+			memcpy(newbase, base, baselen);
+			memcpy(newbase + baselen, first, pathlen);
+			newbase[baselen + pathlen] = '/';
+			newbase[baselen + pathlen + 1] = '\0';
+			if (unpack_trees_rec(subposns, len, newbase, fn,
+					     subfile2, subfile3, indpos))
+				return -1;
+		}
+	} while (1);
+}
+
+static void reject_merge(struct cache_entry *ce)
+{
+	die("Entry '%s' would be overwritten by merge. Cannot merge.", 
+	    ce->name);
+}
+
+static void check_updates(struct cache_entry **src, int nr)
 {
-	void *buffer;
-	unsigned long size;
-	int ret;
+	static struct checkout state = {
+		.base_dir = "",
+		.force = 1,
+		.quiet = 1,
+		.refresh_cache = 1,
+	};
+	unsigned short mask = htons(CE_UPDATE);
+	while (nr--) {
+		struct cache_entry *ce = *src++;
+		if (!ce->ce_mode) {
+			if (update)
+				unlink(ce->name);
+			continue;
+		}
+		if (ce->ce_flags & mask) {
+			ce->ce_flags &= ~mask;
+			if (update)
+				checkout_entry(ce, &state);
+		}
+	}
+}
 
-	buffer = read_object_with_reference(sha1, "tree", &size, NULL);
-	if (!buffer)
+static int unpack_trees(merge_fn_t fn)
+{
+	int indpos = 0;
+	unsigned len = object_list_length(trees);
+	struct tree_entry_list **posns = 
+		xmalloc(len * sizeof(struct tree_entry_list *));
+	int i;
+	struct object_list *posn = trees;
+	for (i = 0; i < len; i++) {
+		posns[i] = ((struct tree *) posn->item)->entries;
+		posn = posn->next;
+	}
+	if (unpack_trees_rec(posns, len, "", fn, 0, 0, &indpos))
 		return -1;
-	ret = read_tree(buffer, size, stage, NULL);
-	free(buffer);
-	return ret;
+
+	check_updates(active_cache, active_nr);
+	return 0;
 }
 
-static int path_matches(struct cache_entry *a, struct cache_entry *b)
+static int list_tree(unsigned char *sha1)
 {
-	int len = ce_namelen(a);
-	return ce_namelen(b) == len &&
-		!memcmp(a->name, b->name, len);
+	struct tree *tree = parse_tree_indirect(sha1);
+	if (!tree)
+		return -1;
+	object_list_append(&tree->object, &trees);
+	return 0;
 }
 
 static int same(struct cache_entry *a, struct cache_entry *b)
@@ -91,17 +343,6 @@ static void verify_uptodate(struct cache
 	die("Entry '%s' not uptodate. Cannot merge.", ce->name);
 }
 
-/*
- * If the old tree contained a CE that isn't even in the
- * result, that's always a problem, regardless of whether
- * it's up-to-date or not (ie it can be a file that we
- * have updated but not committed yet).
- */
-static void reject_merge(struct cache_entry *ce)
-{
-	die("Entry '%s' would be overwritten by merge. Cannot merge.", ce->name);
-}
-
 static int merged_entry_internal(struct cache_entry *merge, struct cache_entry *old, struct cache_entry **dst, int allow_dirty)
 {
 	merge->ce_flags |= htons(CE_UPDATE);
@@ -120,7 +361,7 @@ static int merged_entry_internal(struct 
 		}
 	}
 	merge->ce_flags &= ~htons(CE_STAGEMASK);
-	*dst++ = merge;
+	add_cache_entry(merge, ADD_CACHE_OK_TO_ADD);
 	return 1;
 }
 
@@ -139,76 +380,19 @@ static int deleted_entry(struct cache_en
 	if (old)
 		verify_uptodate(old);
 	ce->ce_mode = 0;
-	*dst++ = ce;
+	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD);
 	return 1;
 }
 
-static int causes_df_conflict(struct cache_entry *ce, int stage,
-			      struct cache_entry **dst_,
-			      struct cache_entry **next_,
-			      int tail)
-{
-	/* This is called during the merge operation and walking
-	 * the active_cache[] array is messy, because it is in the
-	 * middle of overlapping copy operation.  The invariants
-	 * are:
-	 * (1) active_cache points at the first (zeroth) entry.
-	 * (2) up to dst pointer are resolved entries.
-	 * (3) from the next pointer (head-inclusive) to the tail
-	 *     of the active_cache array have the remaining paths
-	 *     to be processed.  There can be a gap between dst
-	 *     and next.  Note that next is called "src" in the
-	 *     merge_cache() function, and tail is the original
-	 *     end of active_cache array when merge_cache() started.
-	 * (4) the path corresponding to *ce is not found in (2)
-	 *     or (3).  It is in the gap.
-	 *
-	 *  active_cache -----......+++++++++++++.
-	 *                    ^dst  ^next        ^tail
-	 */
-	int i, next, dst;
-	const char *path = ce->name;
-	int namelen = ce_namelen(ce);
-
-	next = next_ - active_cache;
-	dst = dst_ - active_cache;
-
-	for (i = 0; i < tail; i++) {
-		int entlen, len;
-		const char *one, *two;
-		if (dst <= i && i < next)
-			continue;
-		ce = active_cache[i];
-		if (ce_stage(ce) != stage)
-			continue;
-		/* If ce->name is a prefix of path, then path is a file
-		 * that hangs underneath ce->name, which is bad.
-		 * If path is a prefix of ce->name, then it is the
-		 * other way around which also is bad.
-		 */
-		entlen = ce_namelen(ce);
-		if (namelen == entlen)
-			continue;
-		if (namelen < entlen) {
-			len = namelen;
-			one = path;
-			two = ce->name;
-		} else {
-			len = entlen;
-			one = ce->name;
-			two = path;
-		}
-		if (memcmp(one, two, len))
-			continue;
-		if (two[len] == '/')
-			return 1;
-	}
-	return 0;
+static int keep_entry(struct cache_entry *ce, struct cache_entry **dst)
+{
+	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD);
+	return 1;
 }
 
 static int threeway_merge(struct cache_entry *stages[4],
 			  struct cache_entry **dst,
-			  struct cache_entry **next, int tail)
+			  int df_conflict_2, int df_conflict_3)
 {
 	struct cache_entry *old = stages[0];
 	struct cache_entry *a = stages[1], *b = stages[2], *c = stages[3];
@@ -218,7 +402,7 @@ static int threeway_merge(struct cache_e
 	/* #5ALT */
 	if (!a && b && c && same(b, c)) {
 		if (old && !same(b, old))
-			return -1;
+			reject_merge(old);
 		return merged_entry_allow_dirty(b, old, dst);
 	}
 	/* #2ALT and #3ALT */
@@ -269,12 +453,12 @@ static int threeway_merge(struct cache_e
 		 *     and vice versa.
 		 */
 		if (c) { /* #2ALT */
-			if (!causes_df_conflict(c, 2, dst, next, tail) &&
+			if (!df_conflict_2 &&
 			    (!old || same(c, old)))
 				return merged_entry_allow_dirty(c, old, dst);
 		}
 		else { /* #3ALT */
-			if (!causes_df_conflict(b, 3, dst, next, tail) &&
+			if (!df_conflict_3 &&
 			    (!old || same(b, old)))
 				return merged_entry_allow_dirty(b, old, dst);
 		}
@@ -293,7 +477,7 @@ static int threeway_merge(struct cache_e
 	 */
 	if (old) {
 		if (!b || !same(old, b))
-			return -1;
+			reject_merge(old);
 	}
 	merge = merge_entries(a, b, c);
 	if (merge)
@@ -301,9 +485,9 @@ static int threeway_merge(struct cache_e
 	if (old)
 		verify_uptodate(old);
 	count = 0;
-	if (a) { *dst++ = a; count++; }
-	if (b) { *dst++ = b; count++; }
-	if (c) { *dst++ = c; count++; }
+	if (a) { count += keep_entry(a, dst + count); }
+	if (b) { count += keep_entry(b, dst + count); }
+	if (c) { count += keep_entry(c, dst + count); }
 	return count;
 }
 
@@ -317,14 +501,11 @@ static int threeway_merge(struct cache_e
  *
  */
 static int twoway_merge(struct cache_entry **src, struct cache_entry **dst,
-			struct cache_entry **next, int tail)
+			int df23, int df32)
 {
 	struct cache_entry *current = src[0];
 	struct cache_entry *oldtree = src[1], *newtree = src[2];
 
-	if (src[3])
-		return -1;
-
 	if (current) {
 		if ((!oldtree && !newtree) || /* 4 and 5 */
 		    (!oldtree && newtree &&
@@ -334,8 +515,7 @@ static int twoway_merge(struct cache_ent
 		    (oldtree && newtree &&
 		     !same(oldtree, newtree) && /* 18 and 19*/
 		     same(current, newtree))) {
-			*dst++ = current;
-			return 1;
+			return keep_entry(current, dst);
 		}
 		else if (oldtree && !newtree && same(current, oldtree)) {
 			/* 10 or 11 */
@@ -346,9 +526,16 @@ static int twoway_merge(struct cache_ent
 			/* 20 or 21 */
 			return merged_entry(newtree, current, dst);
 		}
-		else
+		else {
 			/* all other failures */
+			if (oldtree)
+				reject_merge(oldtree);
+			if (current)
+				reject_merge(current);
+			if (newtree)
+				reject_merge(newtree);
 			return -1;
+		}
 	}
 	else if (newtree)
 		return merged_entry(newtree, current, dst);
@@ -357,137 +544,25 @@ static int twoway_merge(struct cache_ent
 }
 
 /*
- * Two-way merge emulated with three-way merge.
- *
- * This treats "read-tree -m H M" by transforming it internally
- * into "read-tree -m H I+H M", where I+H is a tree that would
- * contain the contents of the current index file, overlayed on
- * top of H.  Unlike the traditional two-way merge, this leaves
- * the stages in the resulting index file and lets the user resolve
- * the merge conflicts using standard tools for three-way merge.
- *
- * This function is just to set-up such an arrangement, and the
- * actual merge uses threeway_merge() function.
- */
-static void setup_emu23(void)
-{
-	/* stage0 contains I, stage1 H, stage2 M.
-	 * move stage2 to stage3, and create stage2 entries
-	 * by scanning stage0 and stage1 entries.
-	 */
-	int i, namelen, size;
-	struct cache_entry *ce, *stage2;
-
-	for (i = 0; i < active_nr; i++) {
-		ce = active_cache[i];
-		if (ce_stage(ce) != 2)
-			continue;
-		/* hoist them up to stage 3 */
-		namelen = ce_namelen(ce);
-		ce->ce_flags = create_ce_flags(namelen, 3);
-	}
-
-	for (i = 0; i < active_nr; i++) {
-		ce = active_cache[i];
-		if (ce_stage(ce) > 1)
-			continue;
-		namelen = ce_namelen(ce);
-		size = cache_entry_size(namelen);
-		stage2 = xmalloc(size);
-		memcpy(stage2, ce, size);
-		stage2->ce_flags = create_ce_flags(namelen, 2);
-		if (add_cache_entry(stage2, ADD_CACHE_OK_TO_ADD) < 0)
-			die("cannot merge index and our head tree");
-
-		/* We are done with this name, so skip to next name */
-		while (i < active_nr &&
-		       ce_namelen(active_cache[i]) == namelen &&
-		       !memcmp(active_cache[i]->name, ce->name, namelen))
-			i++;
-		i--; /* compensate for the loop control */
-	}
-}
-
-/*
  * One-way merge.
  *
  * The rule is:
  * - take the stat information from stage0, take the data from stage1
  */
 static int oneway_merge(struct cache_entry **src, struct cache_entry **dst,
-			struct cache_entry **next, int tail)
+			int df23, int df32)
 {
 	struct cache_entry *old = src[0];
 	struct cache_entry *a = src[1];
 
-	if (src[2] || src[3])
-		return -1;
-
 	if (!a)
 		return 0;
 	if (old && same(old, a)) {
-		*dst++ = old;
-		return 1;
+		return keep_entry(old, dst);
 	}
 	return merged_entry(a, NULL, dst);
 }
 
-static void check_updates(struct cache_entry **src, int nr)
-{
-	static struct checkout state = {
-		.base_dir = "",
-		.force = 1,
-		.quiet = 1,
-		.refresh_cache = 1,
-	};
-	unsigned short mask = htons(CE_UPDATE);
-	while (nr--) {
-		struct cache_entry *ce = *src++;
-		if (!ce->ce_mode) {
-			if (update)
-				unlink(ce->name);
-			continue;
-		}
-		if (ce->ce_flags & mask) {
-			ce->ce_flags &= ~mask;
-			if (update)
-				checkout_entry(ce, &state);
-		}
-	}
-}
-
-typedef int (*merge_fn_t)(struct cache_entry **, struct cache_entry **, struct cache_entry **, int);
-
-static void merge_cache(struct cache_entry **src, int nr, merge_fn_t fn)
-{
-	struct cache_entry **dst = src;
-	int tail = nr;
-
-	while (nr) {
-		int entries;
-		struct cache_entry *name, *ce, *stages[4] = { NULL, };
-
-		name = ce = *src;
-		for (;;) {
-			int stage = ce_stage(ce);
-			stages[stage] = ce;
-			ce = *++src;
-			active_nr--;
-			if (!--nr)
-				break;
-			if (!path_matches(ce, name))
-				break;
-		}
-
-		entries = fn(stages, dst, src, tail);
-		if (entries < 0)
-			reject_merge(name);
-		dst += entries;
-		active_nr += entries;
-	}
-	check_updates(active_cache, active_nr);
-}
-
 static int read_cache_unmerged(void)
 {
 	int i, deleted;
@@ -516,8 +591,9 @@ static struct cache_file cache_file;
 
 int main(int argc, char **argv)
 {
-	int i, newfd, merge, reset, emu23;
+	int i, newfd, reset, stage = 0;
 	unsigned char sha1[20];
+	merge_fn_t fn = NULL;
 
 	newfd = hold_index_file_for_update(&cache_file, get_index_file());
 	if (newfd < 0)
@@ -569,9 +645,7 @@ int main(int argc, char **argv)
 
 		if (get_sha1(arg, sha1) < 0)
 			usage(read_tree_usage);
-		if (stage > 3)
-			usage(read_tree_usage);
-		if (unpack_tree(sha1) < 0)
+		if (list_tree(sha1) < 0)
 			die("failed to unpack tree object %s", arg);
 		stage++;
 	}
@@ -583,19 +657,17 @@ int main(int argc, char **argv)
 			[2] = twoway_merge,
 			[3] = threeway_merge,
 		};
-		merge_fn_t fn;
 
-		if (stage < 2 || stage > 4)
+		if (stage < 2)
 			die("just how do you expect me to merge %d trees?", stage-1);
 		if (emu23 && stage != 3)
 			die("--emu23 takes only two trees");
 		fn = merge_function[stage-1];
 		if (stage == 3 && emu23) { 
-			setup_emu23();
 			fn = merge_function[3];
 		}
-		merge_cache(active_cache, active_nr, fn);
 	}
+	unpack_trees(fn);
 	if (write_cache(newfd, active_cache, active_nr) ||
 	    commit_index_file(&cache_file))
 		die("unable to write new index file");
Junio C Hamano· Sep 5, 2005, 02:04 UTC · re: Daniel Barkalow · lore

Re: [PATCH 0/2] Reorganize read-tree

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 6 quoted lines
> I got mostly done with this before Linus mentioned the possibility of
> having multiple index entries in the same stage for a single path. I
> finished it anyway, but I'm not sure that we won't want to know which of
> the common ancestors contributed which, and, if some of them don't have a
> path, we wouldn't be able to tell. The other advantages I see to this
> approach are:

I've finished reading your patch, after beating it reasonably heavily by feeding combinations of nonsense trees to make sure it produces the same result as the original implementation. I have not found any regression from the read-tree in "master" branch, after you fixed the path ordering issues.

> There are various potential refinements, plus removing a bunch of memory
> leaks, still to do, but I think this is sufficiently close to review.

I am not so worried about the leaks right now; they are something that could be fixed before it hits the "master" branch.

I like your approach of reading the input trees, along with the existing index contents, and re-populating the index one path at a time. It probably is more readable.

I further think that you can get the best of both worlds, by inventing a convention that mode=0 entry means 'this path does not exist in this tree'. This would allow you to have multiple entries at the same stage and still tell which one came from which tree. Instead of calling fn in unpack_trees(), you could make it only unpack the tree into the index, and then after unpacking is done, call fn() repeatedly to resolve the resulting index. Of course the semantics of merge_fn_t needs to change but if you feed N trees, the caller of a merge_fn_t function needs to pick up the first N entries (because you use mode=0 entry to mean 'missing from this tree', each path will always have N entries) and feed them to the merge function in one call, then pick up the next N entries and feed them in the next call, and so on. I think that would simplify that part of the code even further.

So if you are making an octopus of 4 trees (one being our current branch) using 2 merge bases, your intermediate index would look like:

    mode   SHA1   stage path
    100644 XXXXX  0     foo	from original index
    000000 0{40}  1     foo	merge base #1 did not have foo
    100644 ZZZZZ  1     foo	merge base #2 has it
    100644 XXXXX  2     foo	our current head
    100644 ZZZZZ  3     foo	other head #1 being merged into us
    100644 YYYYY  3     foo	other head #2 being merged into us
    100644 ZZZZZ  3     foo	other head #3 being merged into us

We cannot write something like this out without breaking backward compatibility, but I personally think this breakage is OK, because what is being broken is the index file format, not tree object format, and the index file is by definition local to a repository. IOW, it is not too much to ask people not to use old tools to read new index file they created using new tools.

Daniel Barkalow· Sep 5, 2005, 02:55 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] Reorganize read-tree

On Sun, 4 Sep 2005, Junio C Hamano wrote:
Show 14 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > I got mostly done with this before Linus mentioned the possibility of
> > having multiple index entries in the same stage for a single path. I
> > finished it anyway, but I'm not sure that we won't want to know which of
> > the common ancestors contributed which, and, if some of them don't have a
> > path, we wouldn't be able to tell. The other advantages I see to this
> > approach are:
> 
> I've finished reading your patch, after beating it reasonably
> heavily by feeding combinations of nonsense trees to make sure
> it produces the same result as the original implementation.  I
> have not found any regression from the read-tree in "master"
> branch, after you fixed the path ordering issues.
Good.
Show 6 quoted lines
> > There are various potential refinements, plus removing a bunch of memory
> > leaks, still to do, but I think this is sufficiently close to review.
> 
> I am not so worried about the leaks right now; they are
> something that could be fixed before it hits the "master"
> branch.
Right.
Show 12 quoted lines
> I like your approach of reading the input trees, along with the
> existing index contents, and re-populating the index one path at
> a time.  It probably is more readable.
> 
> I further think that you can get the best of both worlds, by
> inventing a convention that mode=0 entry means 'this path does
> not exist in this tree'. This would allow you to have multiple
> entries at the same stage and still tell which one came from
> which tree.  Instead of calling fn in unpack_trees(), you could
> make it only unpack the tree into the index, and then after
> unpacking is done, call fn() repeatedly to resolve the resulting
> index. 

I think that almost all of the benefit actually comes from calling fn() in unpack_trees() and not putting anything in the index before merging. Without that, you need the complex index management and the complicated search for DF conflicts. The main point of not reading everything into the index before calling fn() on stuff is that the index is actually really difficult to deal with in this situation (because you are simultaneously moving through it, removing and modifying entries, and searching it for conflicts). The improvement in readability comes from not doing this.

	-Daniel
*This .sig left intentionally blank*

← back to recent threads