10000 Rename detection using diff.renames by ben · Pull Request #1985 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content

Rename detection using diff.renames #1985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 11, 2013
Merged

Rename detection using diff.renames #1985

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a6ebc2b
Introduce GIT_DIFF_FIND_BY_CONFIG
ben Dec 4, 2013
c56c6d6
Implement GIT_DIFF_FIND_BY_CONFIG
ben Dec 5, 2013
628e92c
Don't use weird return codes
ben Dec 5, 2013
710f383
Clarify default value and behavior
ben Dec 6, 2013
7fb4147
Don't clobber whitespace settings
ben Dec 6, 2013
a7c83ae
Clarify docs
ben Dec 6, 2013
5a52d6b
Check version earlier
ben Dec 11, 2013
File filter

Filter by extension

Filter by extension 10000
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Implement GIT_DIFF_FIND_BY_CONFIG
  • Loading branch information
@ben
ben committed Dec 5, 2013
commit c56c6d694563fdd28cd00ed246f35349522b836e
28 changes: 17 additions & 11 deletions src/diff_tform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,23 +279,29 @@ static int normalize_find_opts(
git_repository_config__weakptr(&cfg, diff->repo) < 0)
return -1;

if (given != NULL)
if (given) {
memcpy(opts, given, sizeof(*opts));
else {
const char *val = NULL;

} else {
GIT_INIT_STRUCTURE(opts, GIT_DIFF_FIND_OPTIONS_VERSION);
}

opts->flags = GIT_DIFF_FIND_RENAMES;
if (!given ||
(given->flags & GIT_DIFF_FIND_ALL) == GIT_DIFF_FIND_BY_CONFIG)
{
const char *val = NULL;

if (git_config_get_string(&val, cfg, "diff.renames") < 0)
giterr_clear();
else if (val &&
(!strcasecmp(val, "copies") || !strcasecmp(val, "copy")))
opts->flags = GIT_DIFF_FIND_RENAMES | GIT_DIFF_FIND_COPIES;
else if (val) {
if (!strcasecmp(val, "false"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably just be using git__parse_bool

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooooh, pretty. ✨

return GIT_PASSTHROUGH;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I like that better.

else if (!strcasecmp(val, "copies") || !strcasecmp(val, "copy"))
opts->flags = GIT_DIFF_FIND_RENAMES | GIT_DIFF_FIND_COPIES;
else
opts->flags = GIT_DIFF_FIND_RENAMES;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the above manipulation of flags should be updated to preserve the high-order bits that control whitespace, etc.

The opts->flags = 0 can just be removed, since you know that opts->flags & GIT_DIFF_FIND_ALL is zero from the entry condition to this section of code.

The other two can just get opts->flags |= ... to mix in the new flag settings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I added a test that checks for clobberation.

}

GITERR_CHECK_VERSION(opts, GIT_DIFF_FIND_OPTIONS_VERSION, "git_diff_find_options");
GITERR_CHECK_VERSION(given, GIT_DIFF_FIND_OPTIONS_VERSION, "git_diff_find_options");

/* some flags imply others */

Expand Down Expand Up @@ -828,7 +834,7 @@ int git_diff_find_similar(
git_diff_file swap;

if ((error = normalize_find_opts(diff, &opts, given_opts)) < 0)
return error;
return (error == GIT_PASSTHROUGH) ? 0 : error;

num_deltas = diff->deltas.length;

Expand Down
138 changes: 138 additions & 0 deletions tests/diff/rename.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1381,3 +1381,141 @@ void test_diff_rename__can_delete_unmodified_deltas(void)

git_buf_free(&c1);
}

void test_diff_rename__matches_config_behavior(void)
{
const char *sha0 = "31e47d8c1fa36d7f8d537b96158e3f024de0a9f2";
const char *sha1 = "2bc7f351d20b53f1c72c16c4b036e491c478c49a";
const char *sha2 = "1c068dee5790ef1580cfc4cd670915b48d790084";

git_tree *tree0, *tree1, *tree2;
git_config *cfg;
git_diff *diff;
git_diff_options diffopts = GIT_DIFF_OPTIONS_INIT;
git_diff_find_options opts = GIT_DIFF_FIND_OPTIONS_INIT;
diff_expects exp;

opts.flags = GIT_DIFF_FIND_BY_CONFIG;
tree0 = resolve_commit_oid_to_tree(g_repo, sha0);
tree1 = resolve_commit_oid_to_tree(g_repo, sha1);
tree2 = resolve_commit_oid_to_tree(g_repo, sha2);

diffopts.flags |= GIT_DIFF_INCLUDE_UNMODIFIED;
cl_git_pass(git_repository_config(&cfg, g_repo));

/* diff.renames = false; no rename detection should happen */
cl_git_pass(git_config_set_bool(cfg, "diff.renames", false));
cl_git_pass(git_diff_tree_to_tree(
&diff, g_repo, tree0, tree1, &diffopts));
memset(&exp, 0, sizeof(exp));
cl_git_pass(git_diff_find_similar(diff, &opts));
cl_git_pass(git_diff_foreach(diff,
diff_file_cb, diff_hunk_cb, diff_line_cb, &exp));
cl_assert_equal_i(4, exp.files);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNMODIFIED]);
cl_assert_equal_i(2, exp.file_status[GIT_DELTA_ADDED]);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_DELETED]);
git_diff_free(diff);

/* diff.renames = true; should act like -M */
cl_git_pass(git_config_set_bool(cfg, "diff.renames", true));
cl_git_pass(git_diff_tree_to_tree(
&diff, g_repo, tree0, tree1, &diffopts));
memset(&exp, 0, sizeof(exp));
cl_git_pass(git_diff_find_similar(diff, &opts));
cl_git_pass(git_diff_foreach(diff,
diff_file_cb, diff_hunk_cb, diff_line_cb, &exp));
cl_assert_equal_i(3, exp.files);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNMODIFIED]);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_ADDED]);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_RENAMED]);
git_diff_free(diff);

/* diff.renames = copies; should act like -M -C */
cl_git_pass(git_config_set_string(cfg, "diff.renames", "copies"));
cl_git_pass(git_diff_tree_to_tree(
&diff, g_repo, tree1, tree2, &diffopts));
memset(&exp, 0, sizeof(exp));
cl_git_pass(git_diff_find_similar(diff, &opts));
cl_git_pass(git_diff_foreach(diff,
diff_file_cb, diff_hunk_cb, diff_line_cb, &exp));
cl_assert_equal_i(4, exp.files);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNMODIFIED]);
cl_assert_equal_i(2, exp.file_status[GIT_DELTA_MODIFIED]);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_COPIED]);
git_diff_free(diff);

/* NULL find options is the same as GIT_DIFF_FIND_BY_CONFIG */
cl_git_pass(git_diff_tree_to_tree(
&diff, g_repo, tree1, tree2, &diffopts));
memset(&exp, 0, sizeof(exp));
cl_git_pass(git_diff_find_similar(diff, NULL));
cl_git_pass(git_diff_foreach(diff,
diff_file_cb, diff_hunk_cb, diff_line_cb, &exp));
cl_assert_equal_i(4, exp.files);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNMODIFIED]);
cl_assert_equal_i(2, exp.file_status[GIT_DELTA_MODIFIED]);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_COPIED]);
git_diff_free(diff);

/* Cleanup */
git_tree_free(tree0);
git_tree_free(tree1);
git_tree_free(tree2);
git_config_free(cfg);
}

void test_diff_rename__can_override_thresholds_when_obeying_config(void)
{
const char *sha1 = "2bc7f351d20b53f1c72c16c4b036e491c478c49a";
const char *sha2 = "1c068dee5790ef1580cfc4cd670915b48d790084";

git_tree *tree1, *tree2;
git_config *cfg;
git_diff *diff;
git_diff_options diffopts = GIT_DIFF_OPTIONS_INIT;
git_diff_find_options opts = GIT_DIFF_FIND_OPTIONS_INIT;
diff_expects exp;

tree1 = resolve_commit_oid_to_tree(g_repo, sha1);
tree2 = resolve_commit_oid_to_tree(g_repo, sha2);

diffopts.flags |= GIT_DIFF_INCLUDE_UNMODIFIED;
opts.flags = GIT_DIFF_FIND_BY_CONFIG;

cl_git_pass(git_repository_config(&cfg, g_repo));
cl_git_pass(git_config_set_string(cfg, "diff.renames", "copies"));
git_config_free(cfg);

/* copy threshold = 96%, should see creation of ikeepsix.txt */
opts.copy_threshold = 96;
cl_git_pass(git_diff_tree_to_tree(
&diff, g_repo, tree1, tree2, &diffopts));
memset(&exp, 0, sizeof(exp));
cl_git_pass(git_diff_find_similar(diff, &opts));
cl_git_pass(git_diff_foreach(diff,
diff_file_cb, diff_hunk_cb, diff_line_cb, &exp));
cl_assert_equal_i(4, exp.files);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNMODIFIED]);
cl_assert_equal_i(2, exp.file_status[GIT_DELTA_MODIFIED]);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_ADDED]);
git_diff_free(diff);

/* copy threshold = 20%, should see sixserving.txt => ikeepsix.txt */
opts.copy_threshold = 20;
cl_git_pass(git_diff_tree_to_tree(
&diff, g_repo, tree1, tree2, &diffopts));
memset(&exp, 0, sizeof(exp));
cl_git_pass(git_diff_find_similar(diff, &opts));
cl_git_pass(git_diff_foreach(diff,
diff_file_cb, diff_hunk_cb, diff_line_cb, &exp));
cl_assert_equal_i(4, exp.files);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNMODIFIED]);
cl_assert_equal_i(2, exp.file_status[GIT_DELTA_MODIFIED]);
cl_assert_equal_i(1, exp.file_status[GIT_DELTA_COPIED]);
git_diff_free(diff);

/* Cleanup */
git_tree_free(tree1);
git_tree_free(tree2);
}
0