[2/4] t6038: fix test with obviously incorrect expectations

Commit Message

Jean-Noël Avila via GitGitGadget Aug. 2, 2020, 6:33 a.m. UTC

From: Elijah Newren <[e-mail]>

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?

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.

Signed-off-by: Elijah Newren <[e-mail]>
---
 t/t6038-merge-text-auto.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Aug. 2, 2020, 7:57 p.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> From: Elijah Newren <[e-mail]>
>
> 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?

I'd say that it is probably more intuitive to expect whatever
attributes set on the currently checked out and receiving the
cherry-picked change would take effect, but I do agree with you that
this is not well defined.  I think "changing attributes in the
middle may produce unexpected/undefined results---validate it when
you cross such a boundary" is a prudent advice we should give to the
users.

Would it make more sense not to test ill defined behaviour at all
instead, I wonder?

Thanks.

Elijah Newren Aug. 3, 2020, 3:36 p.m. UTC | #2

On Sun, Aug 2, 2020 at 12:57 PM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> > From: Elijah Newren <[e-mail]>
> >
> > 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?
>
> I'd say that it is probably more intuitive to expect whatever
> attributes set on the currently checked out and receiving the
> cherry-picked change would take effect, but I do agree with you that
> this is not well defined.

Turns out this question is kind of important since merge-ort does not
naturally get to take advantage of the existing infrastructure for
attribute handling; I have to implement some stuff to trick it into
using it, and the stuff I implement makes it glaringly obvious what
choice is being made.  Your answer here doesn't really help me since
it's not even applicable in some cases.  Anyway, I'll list a bunch of
questions I'm facing with it, but if you want you can skip to the next
quoted block of text and ignore this topic until I post the relevant
patches for merge-ort, if you want.

My questions about attribute handling in merges and which version(s)
of .gitattributes should be used for three-way content merges:

What if you don't have any version checked out, and are doing a merge
in a bare-repo or are just redoing a merge (on some other branch)
without touching the working directory or index just so you can view
that other merge?  In that case, your answer doesn't even apply and I
need to implement something else.

Also, what if you were doing a merge in a dirty working tree, where
your .gitattributes was locally modified?  Should the local
.gitattributes file override the .gitattributes file recorded in
history for how those versions are merged (which seems somewhat
suggested by your answer)?  Even if it makes the merge not
reproducible by others?

Are we okay with merging one way resulting in no conflicts while
merges the other way (with the order of parents reversed) yielding
conflicts due to use of different .gitattributes files?  Also, there
can be differences in what the user sees in a single merge while
resolving, due to 'git checkout -m $CONFLICTED_PATH'.  This happens in
a few cases...explored in the next two paragraphs:

What if the first parent had a .gitattributes file and the merge base
did too (with same contents), but the second parent didn't?  Do we use
the .gitattributes from before the merge, despite the fact that the
merge is going to delete the .gitattributes?  If there are any
conflicts and the users messes up a single file and needs to redo it,
then a 'git checkout -m $CONFLICTED_PATH' later might give them a
different result.

Assuming there is no .gitattributes file in the first parent and none
locally before merging, but the second parent did have a
.gitattributes file, if we only pay attention to the .gitattributes
file from the local working directory or the first parent, then we
again run into a case where the merge may produce a different result
than a subsequent 'git checkout -m $CONFLICTED_PATH'.

What if the .gitattributes file itself needed to be merged?  If it
merges cleanly, should we use that clean merged version of
.gitattributes to define the settings for all three-way content merges
in the remainder of the merge?  If it doesn't merge cleanly...should
we just pick the one from the first parent?  From the second?  Throw a
merge conflict stating that .gitattributes can't be merged and thus we
don't know how to do content merging on any other conflicted path?
(And what if the .gitattributes file only cleanly merges depending on
if it's merged with one of the two .gitattributes settings from one of
its parents?)

> I think "changing attributes in the
> middle may produce unexpected/undefined results---validate it when
> you cross such a boundary" is a prudent advice we should give to the
> users.

Makes sense; especially given all the cases above.

> Would it make more sense not to test ill defined behaviour at all
> instead, I wonder?

I'd be happy to toss the test and punt this conversation until I post
the relevant patches for merge-ort, but we might not be kicking the
can all that far down the road...

Junio C Hamano Aug. 3, 2020, 3:50 p.m. UTC | #3

Elijah Newren <[e-mail]> writes:

>> I'd say that it is probably more intuitive to expect whatever
>> attributes set on the currently checked out and receiving the
>> cherry-picked change would take effect, but I do agree with you that
>> this is not well defined.
> ...
> What if you don't have any version checked out, and are doing a merge
> in a bare-repo or are just redoing a merge (on some other branch)
> without touching the working directory or index just so you can view
> that other merge?

The "receiving" is the keyword in my "I'd say".  Whey you view the
result of merge, the merge may be symmetric, but when initiating a
merge, you have a clear distinction between the target and other: I
am merging these other side branches into this one.

But as I said, I think it is not well defined whose attribute should
be used.  Some might even dream that .gitattributes from the tips
being merged are somehow magically merged first and then the other
paths' merges should use that resulting merged .gitattributes.

> Also, what if you were doing a merge in a dirty working tree, where
> your .gitattributes was locally modified?  Should the local
> .gitattributes file override the .gitattributes file recorded in
> history for how those versions are merged (which seems somewhat
> suggested by your answer)?

Yes, that is quite deliberate outcome from "when doing a merge, the
person who is merging is fully aware into which branch others are
merged into".

Patch

diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 27cea15533..39413d5b48 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh
@@ -189,7 +189,7 @@  test_expect_failure 'checkout -m addition of text=auto' '
 '
 
 test_expect_failure 'cherry-pick patch from after text=auto was added' '
-	append_cr <<-\EOF >expected &&
+	cat <<-\EOF >expected &&
 	first line
 	same line
 	EOF
@@ -197,9 +197,9 @@  test_expect_failure 'cherry-pick patch from after text=auto was added' '
 	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' '