On Sun, 20 Jan 2008, Linus Torvalds wrote:
quoted text
> 
> The following patch gets us closer, but really only for the "--cached" 
> case (for the non-cached case, it should use the working tree entry ratehr 
> than generate a unmerged entry), and I also suspect there's a case it gets 
> wrong for the case of the unmerged path not existign in the tree at all 
> (the "if (tree)" entry basically ends up being a stand-in for the "is this 
> the first index entry for this path we see" case).

This cleanup patch really looks big and fairly ugly, but it splits up the 
magic rules for "unpack_trees()" and adds a lot of comments about what is 
going on, and in the presense lays the groundwork for doing a much better 
job on unmerged entries - it now keeps track of how many index entries we 
have that match the tree entry we found, so it should be pretty trivial 
to now add the logic to do a combined diff..

It replaces the previous patch, and should fix both of the issues I 
mention above. It adds a lot more lines than it removes, but much of that 
is due to some comments and a lot cleaner abstractions, so that the 
resulting code is, I think, quite a bit more obvious.

The unpack_trees() interface really isn't trivial (it can't be: the things 
we need to do with the index for a merge - at the same time as traversing 
it! - are quite involved). So clarifying all that is definitely worth it.

With this patch in place, I think Dscho's two other patches are now good 
to go, and we now generate the same output we used to. In addition, we now 
have the infrastructure to generate *better* output, so if we want to make 
"git diff HEAD" generate a nice combined diff, this sets the stage for 
that.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 diff-lib.c |  112 ++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 90 insertions(+), 22 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 23d0fa6..2a3a9ff 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -668,45 +668,113 @@ static void mark_merge_entries(void)
 	}
 }
 
-static int oneway_diff(struct cache_entry **src,
-	struct unpack_trees_options *o,
-	int remove)
+/*
+ * This gets a mix of an existing index and a tree, one pathname entry
+ * at a time. The index entry may be a single stage-0 one, but it could
+ * also be multiple unmerged entries (in which case you idx_pos/idx_nr
+ * will give you the position and number of entries in the index).
+ */
+static void do_oneway_diff(struct unpack_trees_options *o,
+	struct cache_entry *idx,
+	struct cache_entry *tree,
+	int idx_pos, int idx_nr)
 {
-	struct cache_entry *idx = src[0];
-	struct cache_entry *tree = src[1];
 	struct rev_info *revs = o->unpack_data;
 
-	/*
-	 * Unpack-trees generates a DF/conflict entry if
-	 * there was a directory in the index and a tree
-	 * in the tree. From a diff standpoint, that's a
-	 * delete of the tree and a create of the file.
-	 */
-	if (tree == o->df_conflict_entry)
-		tree = NULL;
+	if (o->index_only && idx && ce_stage(idx)) {
+		if (tree)
+			diff_unmerge(&revs->diffopt, idx->name, idx->ce_mode, idx->sha1);
+		return;
+	}
 
 	/*
 	 * Something added to the tree?
 	 */
 	if (!tree) {
-		if (ce_path_match(idx, revs->prune_data))
-			show_new_file(revs, idx, o->index_only, 0);
-		return 1;
+		show_new_file(revs, idx, o->index_only, 0);
+		return;
 	}
 
 	/*
 	 * Something removed from the tree?
 	 */
 	if (!idx) {
-		if (ce_path_match(tree, revs->prune_data))
-			diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);
-		return 0;
+		diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);
+		return;
 	}
 
 	/* Show difference between old and new */
-	if (ce_path_match(idx, revs->prune_data))
-		show_modified(revs, tree, idx, 1, o->index_only, 0);
-	return 1;
+	show_modified(revs, tree, idx, 1, o->index_only, 0);
+}
+
+/*
+ * Count how many index entries go with the first one
+ */
+static inline int count_skip(const struct cache_entry *src, int pos)
+{
+	int skip = 1;
+
+	/* We can only have multiple entries if the first one is not stage-0 */
+	if (ce_stage(src)) {
+		struct cache_entry **p = active_cache + pos;
+		int namelen = ce_namelen(src);
+
+		for (;;) {
+			const struct cache_entry *ce;
+			pos++;
+			if (pos >= active_nr)
+				break;
+			ce = *++p;
+			if (ce_namelen(ce) != namelen)
+				break;
+			if (memcmp(ce->name, src->name, namelen))
+				break;
+			skip++;
+		}
+	}
+	return skip;
+}
+
+/*
+ * The unpack_trees() interface is designed for merging, so
+ * the different source entries are designed primarily for
+ * the source trees, with the old index being really mainly
+ * used for being replaced by the result.
+ *
+ * For diffing, the index is more important, and we only have a
+ * single tree.  
+ *
+ * We're supposed to return how many index entries we want to skip.
+ *
+ * This wrapper makes it all more readable, and takes care of all
+ * the fairly complex unpack_trees() semantic requirements, including
+ * the skipping, the path matching, the type conflict cases etc.
+ */
+static int oneway_diff(struct cache_entry **src,
+	struct unpack_trees_options *o,
+	int index_pos)
+{
+	int skip = 0;
+	struct cache_entry *idx = src[0];
+	struct cache_entry *tree = src[1];
+	struct rev_info *revs = o->unpack_data;
+
+	if (index_pos >= 0)
+		skip = count_skip(idx, index_pos);
+
+	/*
+	 * Unpack-trees generates a DF/conflict entry if
+	 * there was a directory in the index and a tree
+	 * in the tree. From a diff standpoint, that's a
+	 * delete of the tree and a create of the file.
+	 */
+	if (tree == o->df_conflict_entry)
+		tree = NULL;
+
+	if (ce_path_match(idx ? idx : tree, revs->prune_data))
+		do_oneway_diff(o, idx, tree, index_pos, skip);
+
+	return skip;
 }
 
 int run_diff_index(struct rev_info *revs, int cached)
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html