From 39f75d26e235798681394e46625716c187a4ee3e Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 13 Aug 2010 10:36:01 -0700 Subject: [PATCH 1/3] diff --follow: do not waste cycles while recursing The "--follow" logic is called from diff_tree_sha1() function, but the input trees to diff_tree_sha1() are not necessarily the top-level trees (compare_tree_entry() calls it while it recursively descends into subtrees). When a newly created path lives in somewhere deep in the source hierarchy, e.g. "platform/", but the rename source is in a totally different place in the destination hierarchy, e.g. "lang-api/src/com/...", running "try_to_find_renames()" while base is set to "platform/" is a wasted call. We only need to run the rename following at the very top level. Signed-off-by: Junio C Hamano Acked-by: Linus Torvalds --- tree-diff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree-diff.c b/tree-diff.c index 1fb3e94614..5b68c0864c 100644 --- a/tree-diff.c +++ b/tree-diff.c @@ -412,7 +412,7 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha init_tree_desc(&t1, tree1, size1); init_tree_desc(&t2, tree2, size2); retval = diff_tree(&t1, &t2, base, opt); - if (DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) { + if (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) { init_tree_desc(&t1, tree1, size1); init_tree_desc(&t2, tree2, size2); try_to_follow_renames(&t1, &t2, base, opt); From 44c48a909ae3d49afcaedb2b2cd042d1e329ee93 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 13 Aug 2010 12:17:45 -0700 Subject: [PATCH 2/3] diff --follow: do call diffcore_std() as necessary Usually, diff frontends populate the output queue with filepairs without any rename information and call diffcore_std() to sort the renames out. When --follow is in effect, however, diff-tree family of frontend has a hack that looks like this: diff-tree frontend -> diff_tree_sha1() . populate diff_queued_diff . if --follow is in effect and there is only one change that creates the target path, then -> try_to_follow_renames() -> diff_tree_sha1() with no pathspec but with -C -> diffcore_std() to find renames . if rename is found, tweak diff_queued_diff and put a single filepair that records the found rename there -> diffcore_std() . tweak elements on diff_queued_diff by - rename detection - path ordering - pickaxe filtering We need to skip parts of the second call to diffcore_std() that is related to rename detection, and do so only when try_to_follow_renames() did find a rename. Earlier 1da6175 (Make diffcore_std only can run once before a diff_flush, 2010-05-06) tried to deal with this issue incorrectly; it unconditionally disabled any second call to diffcore_std(). This hopefully fixes the breakage. Signed-off-by: Junio C Hamano --- diff.c | 27 +++++++++++++-------------- diff.h | 3 +++ diffcore.h | 2 -- tree-diff.c | 11 +++++++++++ 4 files changed, 27 insertions(+), 16 deletions(-) diff --git a/diff.c b/diff.c index bf65892f78..93004922de 100644 --- a/diff.c +++ b/diff.c @@ -4064,25 +4064,24 @@ void diffcore_fix_diff_index(struct diff_options *options) void diffcore_std(struct diff_options *options) { - /* We never run this function more than one time, because the - * rename/copy detection logic can only run once. - */ - if (diff_queued_diff.run) - return; - if (options->skip_stat_unmatch) diffcore_skip_stat_unmatch(options); - if (options->break_opt != -1) - diffcore_break(options->break_opt); - if (options->detect_rename) - diffcore_rename(options); - if (options->break_opt != -1) - diffcore_merge_broken(); + if (!options->found_follow) { + /* See try_to_follow_renames() in tree-diff.c */ + if (options->break_opt != -1) + diffcore_break(options->break_opt); + if (options->detect_rename) + diffcore_rename(options); + if (options->break_opt != -1) + diffcore_merge_broken(); + } if (options->pickaxe) diffcore_pickaxe(options->pickaxe, options->pickaxe_opts); if (options->orderfile) diffcore_order(options->orderfile); - diff_resolve_rename_copy(); + if (!options->found_follow) + /* See try_to_follow_renames() in tree-diff.c */ + diff_resolve_rename_copy(); diffcore_apply_filter(options->filter); if (diff_queued_diff.nr && !DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) @@ -4090,7 +4089,7 @@ void diffcore_std(struct diff_options *options) else DIFF_OPT_CLR(options, HAS_CHANGES); - diff_queued_diff.run = 1; + options->found_follow = 0; } int diff_result_code(struct diff_options *opt, int status) diff --git a/diff.h b/diff.h index 063d10ac22..6fff024e3a 100644 --- a/diff.h +++ b/diff.h @@ -126,6 +126,9 @@ struct diff_options { /* this is set by diffcore for DIFF_FORMAT_PATCH */ int found_changes; + /* to support internal diff recursion by --follow hack*/ + int found_follow; + FILE *file; int close_file; diff --git a/diffcore.h b/diffcore.h index 05ebc115a1..8b3241ad13 100644 --- a/diffcore.h +++ b/diffcore.h @@ -91,13 +91,11 @@ struct diff_queue_struct { struct diff_filepair **queue; int alloc; int nr; - int run; }; #define DIFF_QUEUE_CLEAR(q) \ do { \ (q)->queue = NULL; \ (q)->nr = (q)->alloc = 0; \ - (q)->run = 0; \ } while (0) extern struct diff_queue_struct diff_queued_diff; diff --git a/tree-diff.c b/tree-diff.c index 5b68c0864c..cd659c6fe4 100644 --- a/tree-diff.c +++ b/tree-diff.c @@ -359,6 +359,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co diff_tree_release_paths(&diff_opts); /* Go through the new set of filepairing, and see if we find a more interesting one */ + opt->found_follow = 0; for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; @@ -376,6 +377,16 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co diff_tree_release_paths(opt); opt->paths[0] = xstrdup(p->one->path); diff_tree_setup_paths(opt->paths, opt); + + /* + * The caller expects us to return a set of vanilla + * filepairs to let a later call to diffcore_std() + * it makes to sort the renames out (among other + * things), but we already have found renames + * ourselves; signal diffcore_std() not to muck with + * rename information. + */ + opt->found_follow = 1; break; } } From 65113121a50cd765033d51204213c817832c59cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Sun, 15 Aug 2010 10:16:25 +0000 Subject: [PATCH 3/3] log: test for regression introduced in v1.7.2-rc0~103^2~2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a regression test for the git log -M --follow $diff_option bug introduced in v1.7.2-rc0~103^2~2, $diff_option being diff related options like -p, --stat, --name-only etc. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/t4202-log.sh | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 2230e606ed..ead204e5cb 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -436,5 +436,17 @@ test_expect_success 'log.decorate configuration' ' ' -test_done +test_expect_success 'show added path under "--follow -M"' ' + # This tests for a regression introduced in v1.7.2-rc0~103^2~2 + test_create_repo regression && + ( + cd regression && + test_commit needs-another-commit && + test_commit foo.bar && + git log -M --follow -p foo.bar.t && + git log -M --follow --stat foo.bar.t && + git log -M --follow --name-only foo.bar.t + ) +' +test_done