From fe48efb5fd161babea245415ee4feef60fee4964 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 3 Aug 2020 18:41:17 +0000 Subject: [PATCH 1/4] t6038: make tests fail for the right reason t6038 had a pair of tests that were expected to fail, but weren't failing for the expected reason. Both were meant to do a merge that could be done cleanly after renormalization, but were supposed to fail for lack of renormalization. Unfortunately, both tests had staged changes, and checkout -m would abort due to the presence of those staged changes before even attempting a merge. Fix this first issue by utilizing git-restore instead of git-checkout, so that the index is left alone and just the working directory gets the changes we want. However, there is a second issue with these tests. Technically, they just wanted to verify that after renormalization, no conflicts would be present. This could have been checked for by grepping for a lack of conflict markers, but the test instead tried to compare the working directory files to an expected result. Unfortunately, the setting of "text=auto" without setting core.eol to any value meant that the content of the file (in particular, the line endings) would be platform-dependent and the tests could only pass on some platforms. Replace the existing comparison with a call to 'git diff --no-index --ignore-cr-at-eol' to verify that the contents, other than possible carriage returns in the file, match the expected results and in particular that the file has no conflicts from the checkout -m operation. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6038-merge-text-auto.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh index 5e8d5fa50c..27cea15533 100755 --- a/t/t6038-merge-text-auto.sh +++ b/t/t6038-merge-text-auto.sh @@ -168,9 +168,9 @@ test_expect_failure 'checkout -m after setting text=auto' ' git rm -fr . && rm -f .gitattributes && git reset --hard initial && - git checkout a -- . && + git restore --source=a -- . && git checkout -m b && - compare_files expected file + git diff --no-index --ignore-cr-at-eol expected file ' test_expect_failure 'checkout -m addition of text=auto' ' @@ -183,9 +183,9 @@ test_expect_failure 'checkout -m addition of text=auto' ' git rm -fr . && rm -f .gitattributes file && git reset --hard initial && - git checkout b -- . && + git restore --source=b -- . && git checkout -m a && - compare_files expected file + git diff --no-index --ignore-cr-at-eol expected file ' test_expect_failure 'cherry-pick patch from after text=auto was added' ' From 6f6e7cfb52a07578c275656a8fd735265eca1a07 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 3 Aug 2020 18:41:18 +0000 Subject: [PATCH 2/4] t6038: remove problematic test t6038.11, 'cherry-pick patch from after text=auto' was a test of undefined behavior. To make matters worse, while there are a couple possible correct answers, this test was coded to only check for an obviously incorrect answer. And the final cherry on top is that the test is marked test_expect_failure, meaning it can't provide much value, other than possibly confusing future folks who come along and try to work on attributes and look at existing tests. Because of all these problems, just remove the test. But for any future code spelunkers, here's my understanding of the two possible correct answers: This test was set up so that on a branch with no .gitattributes file, you cherry-picked a patch from a branch that had a .gitattributes file (containing '* text=auto'). Further, the two branches had a file which differed only in line endings. In this situation, correct behavior is not well defined: should the .gitattributes file affect the merge or not? If the .gitattributes file on the other branch should not affect the merge, then we would have a content conflict with all three stages different (the merge base didn't match either side). If the .gitattributes file from the other branch should affect the merge, then we would expect the line endings to be normalized to LF for the version to be recorded in the repository. This would mean that when doing a three-way content merge on the file that differed in line endings, that the three-way content merge would see that the versions on both sides matched and so the cherry-pick has no conflicts and can succeed. The line endings in the file as recorded in the repository will change from CRLF to LF. The version checked out in the working copy will depend on the platform (since there's no eol attribute defined for the file). Also, as a final side note, this test expected an error message that was built assuming cherry-pick was the old scripted version, because cherry-pick no longer uses the error message that was encoded in this test. So it was wrong for yet another reason. Given that the handling of .gitattributes is not well defined and this test was obviously broken and could do nothing but confuse future readers, just remove it. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t6038-merge-text-auto.sh | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh index 27cea15533..9337745793 100755 --- a/t/t6038-merge-text-auto.sh +++ b/t/t6038-merge-text-auto.sh @@ -188,20 +188,6 @@ test_expect_failure 'checkout -m addition of text=auto' ' git diff --no-index --ignore-cr-at-eol expected file ' -test_expect_failure 'cherry-pick patch from after text=auto was added' ' - append_cr <<-\EOF >expected && - first line - same line - EOF - - git config merge.renormalize true && - git rm -fr . && - git reset --hard b && - test_must_fail git cherry-pick a >err 2>&1 && - grep "[Nn]othing added" err && - compare_files expected file -' - test_expect_success 'Test delete/normalize conflict' ' git checkout -f side && git rm -fr . && From 8d552258f45ba3886e8d508ece8585b509a04677 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 3 Aug 2020 18:41:19 +0000 Subject: [PATCH 3/4] merge: make merge.renormalize work for all uses of merge machinery The 'merge' command is not the only one that does merges; other commands like checkout -m or rebase do as well. Unfortunately, the only area of the code that checked for the "merge.renormalize" config setting was in builtin/merge.c, meaning it could only affect merges performed by the "merge" command. Move the handling of this config setting to merge_recursive_config() so that other commands can benefit from it as well. Fixes a few tests in t6038. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/checkout.c | 7 ------- builtin/merge.c | 4 ---- merge-recursive.c | 3 +++ t/t6038-merge-text-auto.sh | 4 ++-- 4 files changed, 5 insertions(+), 13 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index af849c644f..18c49034c4 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -771,13 +771,6 @@ static int merge_working_tree(const struct checkout_opts *opts, */ add_files_to_cache(NULL, NULL, 0); - /* - * NEEDSWORK: carrying over local changes - * when branches have different end-of-line - * normalization (or clean+smudge rules) is - * a pain; plumb in an option to set - * o.renormalize? - */ init_merge_options(&o, the_repository); o.verbosity = 0; work = write_in_core_index_as_tree(the_repository); diff --git a/builtin/merge.c b/builtin/merge.c index 7da707bf55..74829a838e 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -72,7 +72,6 @@ static const char **xopts; static size_t xopts_nr, xopts_alloc; static const char *branch; static char *branch_mergeoptions; -static int option_renormalize; static int verbosity; static int allow_rerere_auto; static int abort_current_merge; @@ -621,8 +620,6 @@ static int git_merge_config(const char *k, const char *v, void *cb) return git_config_string(&pull_octopus, k, v); else if (!strcmp(k, "commit.cleanup")) return git_config_string(&cleanup_arg, k, v); - else if (!strcmp(k, "merge.renormalize")) - option_renormalize = git_config_bool(k, v); else if (!strcmp(k, "merge.ff")) { int boolval = git_parse_maybe_bool(v); if (0 <= boolval) { @@ -721,7 +718,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, if (!strcmp(strategy, "subtree")) o.subtree_shift = ""; - o.renormalize = option_renormalize; o.show_rename_progress = show_progress == -1 ? isatty(2) : show_progress; diff --git a/merge-recursive.c b/merge-recursive.c index 36948eafb7..a1c8b36ddb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3791,9 +3791,12 @@ int merge_recursive_generic(struct merge_options *opt, static void merge_recursive_config(struct merge_options *opt) { char *value = NULL; + int renormalize = 0; git_config_get_int("merge.verbosity", &opt->verbosity); git_config_get_int("diff.renamelimit", &opt->rename_limit); git_config_get_int("merge.renamelimit", &opt->rename_limit); + git_config_get_bool("merge.renormalize", &renormalize); + opt->renormalize = renormalize; if (!git_config_get_string("diff.renames", &value)) { opt->detect_renames = git_config_rename("diff.renames", value); free(value); diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh index 9337745793..89c86d4e56 100755 --- a/t/t6038-merge-text-auto.sh +++ b/t/t6038-merge-text-auto.sh @@ -158,7 +158,7 @@ test_expect_success 'Detect LF/CRLF conflict from addition of text=auto' ' compare_files expected file.fuzzy ' -test_expect_failure 'checkout -m after setting text=auto' ' +test_expect_success 'checkout -m after setting text=auto' ' cat <<-\EOF >expected && first line same line @@ -173,7 +173,7 @@ test_expect_failure 'checkout -m after setting text=auto' ' git diff --no-index --ignore-cr-at-eol expected file ' -test_expect_failure 'checkout -m addition of text=auto' ' +test_expect_success 'checkout -m addition of text=auto' ' cat <<-\EOF >expected && first line same line From 00906d6f227b259ac7c8cebd07bbea0ba4792185 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 3 Aug 2020 18:41:20 +0000 Subject: [PATCH 4/4] checkout: support renormalization with checkout -m Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/checkout.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 18c49034c4..2837195491 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -239,6 +239,8 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko mmbuffer_t result_buf; struct object_id threeway[3]; unsigned mode = 0; + struct ll_merge_options ll_opts; + int renormalize = 0; memset(threeway, 0, sizeof(threeway)); while (pos < active_nr) { @@ -259,13 +261,12 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko read_mmblob(&ours, &threeway[1]); read_mmblob(&theirs, &threeway[2]); - /* - * NEEDSWORK: re-create conflicts from merges with - * merge.renormalize set, too - */ + memset(&ll_opts, 0, sizeof(ll_opts)); + git_config_get_bool("merge.renormalize", &renormalize); + ll_opts.renormalize = renormalize; status = ll_merge(&result_buf, path, &ancestor, "base", &ours, "ours", &theirs, "theirs", - state->istate, NULL); + state->istate, &ll_opts); free(ancestor.ptr); free(ours.ptr); free(theirs.ptr);