git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 2/4] t6038: fix test with obviously incorrect expectations
Date: Mon, 3 Aug 2020 08:36:28 -0700	[thread overview]
Message-ID: <CABPp-BHNebxZK1-tXx0HMmPDuRoj=_XWG=pVJ2HCvTkttvA4oQ@mail.gmail.com> (raw)
In-Reply-To: <xmqq4kpkstwk.fsf@gitster.c.googlers.com>

On Sun, Aug 2, 2020 at 12:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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...

  reply	other threads:[~2020-08-03 15:36 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-02  6:33 [PATCH 0/4] Attr fixes Elijah Newren via GitGitGadget
2020-08-02  6:33 ` [PATCH 1/4] t6038: make tests fail for the right reason Elijah Newren via GitGitGadget
2020-08-02 18:17   ` Junio C Hamano
2020-08-02 19:10   ` Eric Sunshine
2020-08-03 15:06     ` Elijah Newren
2020-08-02  6:33 ` [PATCH 2/4] t6038: fix test with obviously incorrect expectations Elijah Newren via GitGitGadget
2020-08-02 19:57   ` Junio C Hamano
2020-08-03 15:36     ` Elijah Newren [this message]
2020-08-03 15:50       ` Junio C Hamano
2020-08-02  6:33 ` [PATCH 3/4] merge: make merge.renormalize work for all uses of merge machinery Elijah Newren via GitGitGadget
2020-08-02 19:23   ` Junio C Hamano
2020-08-03 15:07     ` Elijah Newren
2020-08-02  6:33 ` [PATCH 4/4] checkout: support renormalization with checkout -m <paths> Elijah Newren via GitGitGadget
2020-08-03 18:41 ` [PATCH v2 0/4] Attr fixes Elijah Newren via GitGitGadget
2020-08-03 18:41   ` [PATCH v2 1/4] t6038: make tests fail for the right reason Elijah Newren via GitGitGadget
2020-08-03 18:41   ` [PATCH v2 2/4] t6038: remove problematic test Elijah Newren via GitGitGadget
2020-08-03 18:41   ` [PATCH v2 3/4] merge: make merge.renormalize work for all uses of merge machinery Elijah Newren via GitGitGadget
2020-08-03 18:41   ` [PATCH v2 4/4] checkout: support renormalization with checkout -m <paths> Elijah Newren via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BHNebxZK1-tXx0HMmPDuRoj=_XWG=pVJ2HCvTkttvA4oQ@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).