[v2,0/4] Attr fixes

Message

Jean-Noël Avila via GitGitGadget Aug. 3, 2020, 6:41 p.m. UTC

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 <[e-mail]>
      
       ## 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 <[e-mail]>
      
       ## 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>