Re: [PATCH] Also use unpack_trees() in do_diff_cache()
- From
Linus Torvalds <torvalds@linux-foundation.org>
- Date
- Jan 20, 2008, 21:53 UTC
- Message-ID
- <alpine.LFD.1.00.0801201338140.2957@woody.linux-foundation.org>
- In-Reply-To
- <alpine.LFD.1.00.0801201144300.2957@woody.linux-foundation.org>
On Sun, 20 Jan 2008, Linus Torvalds wrote:
Show 7 quoted lines
> > 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)