Rename detection using diff.renames #1985
Conversation
| @@ -468,6 +468,9 @@ typedef int (*git_diff_line_cb)( | |||
| * Flags to control the behavior of diff rename/copy detection. | |||
| */ | |||
| typedef enum { | |||
| /** Try to obey `find.renames` */ | |||
There was a problem hiding this comment.
Do, or do not. There is no try.
Also: diff.renames!
| @@ -468,6 +468,9 @@ typedef int (*git_diff_line_cb)( | |||
| * Flags to control 8000 the behavior of diff rename/copy detection. | |||
| */ | |||
| typedef enum { | |||
| /** Obey `diff.renames`. This is overridden by any other GIT_DIFF_FIND_ALL flag. */ | |||
|
There, now it works. Next question: should
What do y'all think? |
| opts->flags = GIT_DIFF_FIND_RENAMES | GIT_DIFF_FIND_COPIES; | ||
| else if (val) { | ||
| if (!strcasecmp(val, "false")) | ||
| return GIT_PASSTHROUGH; |
There was a problem hiding this comment.
Instead of using an error here, I think it is enough to just return 0 and then after the normalize_find_opts returns, if (opts & GIT_DIFF_FIND_ALL) == 0 then just return 0 because no find options were turned on.
To reiterate my response: this seems like the most reasonable default to me, and the concern about |
| * Combination of git_diff_find_t values (default FIND_BY_CONFIG). Note | ||
| * that if the configuration value is falsy, this will result in | ||
| * `git_diff_find_similar` doing nothing. | ||
| */ |
There was a problem hiding this comment.
@dahlbyk, is this a strong-enough warning?
There was a problem hiding this comment.
I would slightly prefer a more explicit if-you-do-this-you-may-get-that wording, like "If you don't pass in any explicit find actions and rely on the default behavior from the user configuration, diff.renames could be false which will result in git_diff_find_similar doing nothing"
| * Combination of git_diff_find_t values (default FIND_BY_CONFIG). | ||
| * Note that if you don't explicitly set this, `diff.renames` could be set | ||
| * to false, resulting in `git_diff_find_similar` doing nothing. | ||
| */ |
|
Any more comments? I think this is ready to go. |
|
Yes, I think it's fine. I hesitated to merge it because I know that it's going to conflict with #1986 in a somewhat painful way and was thinking it would probably be easier to rebase this on top of that than resolve these changes there, but since I'm now on a mission to make sure that #1986 has strong test coverage, we should probably just merge this and I'll deal. |
Rename detection using diff.renames
Rename detection using diff.renames
As noticed in libgit2/libgit2sharp#573 (comment), there's an unsupported use-case with
git_diff_find_similar, where the caller wants to obey thediff.renamesconfig value for how hard to try, but possibly override some or all of the threshold values (i.e.git diff -B70%).