From a6ebc2bdb314543a1fdf25f523f8793f4f52b8b3 Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Wed, 4 Dec 2013 15:17:39 -0800 Subject: [PATCH 1/7] Introduce GIT_DIFF_FIND_BY_CONFIG --- include/git2/diff.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/git2/diff.h b/include/git2/diff.h index db6bce2eb..312702bc2 100644 --- a/include/git2/diff.h +++ b/include/git2/diff.h @@ -468,6 +468,9 @@ typedef int (*git_diff_line_cb)( * Flags to control the behavior of diff rename/copy detection. */ typedef enum { + /** Obey `diff.renames`. This is overridden by any other GIT_DIFF_FIND_ALL flag. */ + GIT_DIFF_FIND_BY_CONFIG = 0, + /** Look for renames? (`--find-renames`) */ GIT_DIFF_FIND_RENAMES = (1u << 0), From c56c6d694563fdd28cd00ed246f35349522b836e Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Thu, 5 Dec 2013 14:13:46 -0800 Subject: [PATCH 2/7] Implement GIT_DIFF_FIND_BY_CONFIG --- src/diff_tform.c | 28 +++++---- tests/diff/rename.c | 138 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 11 deletions(-) diff --git a/src/diff_tform.c b/src/diff_tform.c index 0a28e58c7..6f29f25da 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -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")) + return GIT_PASSTHROUGH; + 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; + } } - - 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 */ @@ -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; diff --git a/tests/diff/rename.c b/tests/diff/rename.c index 919f5139a..b304ec253 100644 --- a/tests/diff/rename.c +++ b/tests/diff/rename.c @@ -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); +} From 628e92cdb31cb3561549671ea5346a347f2addcd Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Thu, 5 Dec 2013 14:47:04 -0800 Subject: [PATCH 3/7] Don't use weird return codes --- src/diff_tform.c | 11 ++++++++--- tests/diff/rename.c | 3 +++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/diff_tform.c b/src/diff_tform.c index 6f29f25da..0c9f961f2 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -293,8 +293,9 @@ static int normalize_find_opts( if (git_config_get_string(&val, cfg, "diff.renames") < 0) giterr_clear(); else if (val) { - if (!strcasecmp(val, "false")) - return GIT_PASSTHROUGH; + int boolval; + if (!git__parse_bool(&boolval, val) && !boolval) + opts->flags = 0; else if (!strcasecmp(val, "copies") || !strcasecmp(val, "copy")) opts->flags = GIT_DIFF_FIND_RENAMES | GIT_DIFF_FIND_COPIES; else @@ -834,7 +835,11 @@ int git_diff_find_similar( git_diff_file swap; if ((error = normalize_find_opts(diff, &opts, given_opts)) < 0) - return (error == GIT_PASSTHROUGH) ? 0 : error; + return error; + + /* No flags set; nothing to do */ + if ((opts.flags & GIT_DIFF_FIND_ALL) == 0) + return 0; num_deltas = diff->deltas.length; diff --git a/tests/diff/rename.c b/tests/diff/rename.c index b304ec253..c08c1a8b4 100644 --- a/tests/diff/rename.c +++ b/tests/diff/rename.c @@ -919,6 +919,7 @@ void test_diff_rename__rejected_match_can_match_others(void) char *ptr; opts.checkout_strategy = GIT_CHECKOUT_FORCE; + findopts.flags = GIT_DIFF_FIND_RENAMES; cl_git_pass(git_reference_lookup(&head, g_repo, "HEAD")); cl_git_pass(git_reference_symbolic_set_target( @@ -1003,6 +1004,7 @@ void test_diff_rename__rejected_match_can_match_others_two(void) struct rename_expected expect = { 2, status, sources, targets }; opts.checkout_strategy = GIT_CHECKOUT_FORCE; + findopts.flags = GIT_DIFF_FIND_RENAMES; cl_git_pass(git_reference_lookup(&head, g_repo, "HEAD")); cl_git_pass(git_reference_symbolic_set_target( @@ -1060,6 +1062,7 @@ void test_diff_rename__rejected_match_can_match_others_three(void) struct rename_expected expect = { 2, status, sources, targets }; opts.checkout_strategy = GIT_CHECKOUT_FORCE; + findopts.flags = GIT_DIFF_FIND_RENAMES; cl_git_pass(git_reference_lookup(&head, g_repo, "HEAD")); cl_git_pass(git_reference_symbolic_set_target( From 710f383868cef86ff837340c698b1672c7902486 Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Fri, 6 Dec 2013 09:32:09 -0800 Subject: [PATCH 4/7] Clarify default value and behavior --- include/git2/diff.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/git2/diff.h b/include/git2/diff.h index 312702bc2..1654523c8 100644 --- a/include/git2/diff.h +++ b/include/git2/diff.h @@ -576,7 +576,11 @@ typedef struct { typedef struct { unsigned int version; - /** Combination of git_diff_find_t values (default FIND_RENAMES) */ + /** + * 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. + */ uint32_t flags; /** Similarity to consider a file renamed (default 50) */ From 7fb4147f1f7e64d7544e574998d5cec04077dfc4 Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Fri, 6 Dec 2013 13:38:59 -0800 Subject: [PATCH 5/7] Don't clobber whitespace settings --- src/diff_tform.c | 10 +++++----- tests/diff/rename.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/diff_tform.c b/src/diff_tform.c index 0c9f961f2..3ff6108c8 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -294,12 +294,12 @@ static int normalize_find_opts( giterr_clear(); else if (val) { int boolval; - if (!git__parse_bool(&boolval, val) && !boolval) - opts->flags = 0; - else if (!strcasecmp(val, "copies") || !strcasecmp(val, "copy")) - opts->flags = GIT_DIFF_FIND_RENAMES | GIT_DIFF_FIND_COPIES; + if (!git__parse_bool(&boolval, val) && !boolval) { + /* do nothing */ + } 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; + opts->flags |= GIT_DIFF_FIND_RENAMES; } } GITERR_CHECK_VERSION(given, GIT_DIFF_FIND_OPTIONS_VERSION, "git_diff_find_options"); diff --git a/tests/diff/rename.c b/tests/diff/rename.c index c08c1a8b4..ca6d076d6 100644 --- a/tests/diff/rename.c +++ b/tests/diff/rename.c @@ -1522,3 +1522,45 @@ void test_diff_rename__can_override_thresholds_when_obeying_config(void) git_tree_free(tree1); git_tree_free(tree2); } + +void test_diff_rename__by_config_doesnt_mess_with_whitespace_settings(void) +{ + const char *sha1 = "1c068dee5790ef1580cfc4cd670915b48d790084"; + const char *sha2 = "19dd32dfb1520a64e5bbaae8dce6ef423dfa2f13"; + + 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); + + /* Don't ignore whitespace; this should find a change in sixserving.txt */ + opts.flags |= 0 | GIT_DIFF_FIND_DONT_IGNORE_WHITESPACE; + 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(5, exp.files); + cl_assert_equal_i(2, exp.file_status[GIT_DELTA_MODIFIED]); + cl_assert_equal_i(1, exp.file_status[GIT_DELTA_RENAMED]); + cl_assert_equal_i(1, exp.file_status[GIT_DELTA_DELETED]); + cl_assert_equal_i(1, exp.file_status[GIT_DELTA_ADDED]); + git_diff_free(diff); + + /* Cleanup */ + git_tree_free(tree1); + git_tree_free(tree2); +} From a7c83aec2149a4885bfac032859142e8a62bceab Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Fri, 6 Dec 2013 13:39:08 -0800 Subject: [PATCH 6/7] Clarify docs --- include/git2/diff.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/git2/diff.h b/include/git2/diff.h index 1654523c8..315cc1215 100644 --- a/include/git2/diff.h +++ b/include/git2/diff.h @@ -577,9 +577,9 @@ typedef struct { unsigned int version; /** - * 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. + * 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. */ uint32_t flags; From 5a52d6be4c3a2635a3121b529e0ab2ab674f6be6 Mon Sep 17 00:00:00 2001 From: Ben Straub Date: Wed, 11 Dec 2013 06:43:17 -0800 Subject: [PATCH 7/7] Check version earlier --- src/diff_tform.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/diff_tform.c b/src/diff_tform.c index 3ff6108c8..702e43bd3 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -275,6 +275,8 @@ static int normalize_find_opts( { git_config *cfg = NULL; + GITERR_CHECK_VERSION(given, GIT_DIFF_FIND_OPTIONS_VERSION, "git_diff_find_options"); + if (diff->repo != NULL && git_repository_config__weakptr(&cfg, diff->repo) < 0) return -1; @@ -302,7 +304,6 @@ static int normalize_find_opts( opts->flags |= GIT_DIFF_FIND_RENAMES; } } - GITERR_CHECK_VERSION(given, GIT_DIFF_FIND_OPTIONS_VERSION, "git_diff_find_options"); /* some flags imply others */