Re: [PATCH v2] Make git log --follow find copies among unmodified files.
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:41
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH v2] Make git log --follow find copies among unmodified files. · Bo Yang <hidden>
Bo Yang [off-list ref] writes:
quoted hunk
@@ -56,7 +56,10 @@ include::diff-options.txt[] commits, and doesn't limit diff for those commits. --follow:: - Continue listing the history of a file beyond renames. + Continue listing the history of a file beyond renames/copies. + With this, all files in a commit will be searched for + renames/copies, and it is equal to specify '--follow' with + '--follow -M -C -C'.
Hmm. Addition of "/copies" is fine, but I do not think the last three lines are good. The -M/-C options are about how the changes introduced by the found commits are shown, and because the pathspec limitation applies _before_ all the diffcore transformations (see "Gaah. Why?" message from Linus in the thread I pointed at you in another message), and because the --follow option is defined to work _only_ when you give one single path (it is not even a "pathspec" in the usual sense; see 750f7b6 (Finally implement "git log --follow", 2007-06-19)), they are inherently mutually incompatible. Cf. http://thread.gmane.org/gmane.comp.version-control.git/50512/focus=50513
quoted hunk
diff --git a/diff.c b/diff.c index d0ecbc3..6982f79 100644 --- a/diff.c +++ b/diff.c@@ -2594,6 +2594,9 @@ int diff_setup_done(struct diff_options *options) else DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS); + if (DIFF_OPT_TST(options, FOLLOW_RENAMES)) + DIFF_OPT_SET(options, FIND_COPIES_HARDER);
I don't think this is correct. You are touching the diff options on the "how do we show the found change" side here. Because the user is fixated on one single path F, it is enough to say "F is in the child but did not exist in the parent---it appears to have come from E in the parent" without saying if it is copy or rename. I think it is Ok if the change is to pay the cost of full tree scanning for only commits that try_to_follow_renames() deals with, but this instead makes it in effect for _all_ the commits, not just the ones that created the path that has been followed so far, which is unacceptable. Why isn't it just a one-liner from your previous patch but done without the conditional? I think that would indeed be a fix. With "--follow", the user expressee that it is Ok to pay extra cost in order to _follow_ that single path, and we haven't been doing as thorough a job as we could. With that single liner, you are fixing it to pay more attention to the paths in the parent. But that is _not_ about how the found commits are _shown_ (it is about how the commits are _found_).
diff --git a/tree-diff.c b/tree-diff.c
index fe9f52c..0dea53e 100644
--- a/tree-diff.c
+++ b/tree-diff.c@@ -346,6 +346,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co diff_setup(&diff_opts); DIFF_OPT_SET(&diff_opts, RECURSIVE); + DIFF_OPT_SET(&diff_opts, FIND_COPIES_HARDER); diff_opts.detect_rename = DIFF_DETECT_RENAME; diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT; diff_opts.single_follow = opt->paths[0];
--
1.7.0.2.273.gc2413.dirty