This fixes a few issues surrounding .gitattributes files and usage of the merge machinery outside of "git merge". All were issues I found and fixed while working on merge-ort. Changes since v1: * Made the fixes suggested by Eric and Junio * Just ripped out the test in patch 2 that was testing undefined behavior (especially since it was a test_expect_failure, and clearly was testing multiple things wrong), as suggested by Junio. Elijah Newren (4): t6038: make tests fail for the right reason t6038: remove problematic test merge: make merge.renormalize work for all uses of merge machinery checkout: support renormalization with checkout -m <paths> builtin/checkout.c | 18 ++++++------------ builtin/merge.c | 4 ---- merge-recursive.c | 3 +++ t/t6038-merge-text-auto.sh | 26 ++++++-------------------- 4 files changed, 15 insertions(+), 36 deletions(-) base-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-825%2Fnewren%2Fattr-fixes-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-825/newren/attr-fixes-v2 Pull-Request: https://github.com/git/git/pull/825 Range-diff vs v1: 1: 3619118175 ! 1: 21033c4c14 t6038: make tests fail for the right reason @@ Commit message 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 has staged + 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. 2: 83a50f7e0b ! 2: 305fe534c5 t6038: fix test with obviously incorrect expectations @@ Metadata Author: Elijah Newren <newren@xxxxxxxxx> ## Commit message ## - t6038: fix test with obviously incorrect expectations + t6038: remove problematic test - t6038.11, 'cherry-pick patch from after text=auto' 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? + 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 so - that the versions from both sides match, and then the cherry-pick has no - conflicts and can succeed. After the cherry-pick, the line endings in - the file will change from CRLF to LF. - - This test had an expectation that the line endings would remain CRLF. - Further, it 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, although I - don't know what correct behavior for this test is, I know that this test - was not testing for it. - - Since I have no idea which of the two cases above provides correct - behavior, let's just assume for now it's the case where we assume that - .gitattributes affects the merge and update it accordingly. + 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 <newren@xxxxxxxxx> ## t/t6038-merge-text-auto.sh ## @@ t/t6038-merge-text-auto.sh: 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' ' +-test_expect_failure 'cherry-pick patch from after text=auto was added' ' - append_cr <<-\EOF >expected && -+ cat <<-\EOF >expected && - first line - same line - EOF -@@ t/t6038-merge-text-auto.sh: test_expect_failure 'cherry-pick patch from after text=auto was added' ' - git config merge.renormalize true && - git rm -fr . && - git reset --hard b && +- 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 -+ git cherry-pick a && -+ git cat-file -p HEAD:file >actual && -+ compare_files expected actual - ' - +-' +- test_expect_success 'Test delete/normalize conflict' ' + git checkout -f side && + git rm -fr . && 3: 08c8080b31 ! 3: 379a87ea82 merge: make merge.renormalize work for all uses of merge machinery @@ builtin/merge.c: static const char **xopts; static const char *branch; static char *branch_mergeoptions; -static int option_renormalize; -+static int option_renormalize = -1; static int verbosity; static int allow_rerere_auto; static int abort_current_merge; @@ builtin/merge.c: static int try_merge_strategy(const char *strategy, struct comm o.subtree_shift = ""; - o.renormalize = option_renormalize; -+ if (option_renormalize != -1) -+ o.renormalize = option_renormalize; o.show_rename_progress = show_progress == -1 ? isatty(2) : show_progress; 4: fcc7ea3add = 4: 36e08a75a3 checkout: support renormalization with checkout -m <paths> -- gitgitgadget
