git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: "Sergey Organov" <sorganov@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Johannes Sixt" <j6t@kdbg.org>, "Jeff King" <peff@peff.net>,
	"David Aguilar" <davvid@gmail.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Denton Liu" <liu.denton@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>
Subject: Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle
Date: Fri, 11 Jun 2021 14:40:32 -0700	[thread overview]
Message-ID: <CABPp-BGhaSJmqXFKpCVcUKexNXaa8sLEms-6q-qno7uETzGQ=A@mail.gmail.com> (raw)
In-Reply-To: <60c3d0296f869_8d0f208ec@natae.notmuch>

On Fri, Jun 11, 2021 at 2:05 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Elijah Newren wrote:
> > On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
> > > Or just use the base of the virtual merge:
> > >
> > >   <<<<<<< HEAD
> > >   D
> > >   ||||||| merged common ancestors
> > >   1
> > >   =======
> > >   C
> > >   >>>>>>> C
> >
> > I think that implementing this choice would look like this (again, not
> > compiled or tested and I'm not familiar with xdiff so take it with a
> > big grain of salt):
> >
> >
> > diff --git a/ll-merge.c b/ll-merge.c
> > index 095a4d820e..dbc7f76951 100644
> > --- a/ll-merge.c
> > +++ b/ll-merge.c
> > @@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
> >       memset(&xmp, 0, sizeof(xmp));
> >       xmp.level = XDL_MERGE_ZEALOUS;
> >       xmp.favor = opts->variant;
> > +     if (git_xmerge_style >= 0 && opts->virtual_ancestor)
> > +             xmp.favor = XDL_MERGE_FAVOR_BASE;
>
> The only time git_xmerge_style isn't >= 0 is when no merge style has
> been configured by the user.

Yep, probably should have just been

+     if (opts->virtual_ancestor)
+             xmp.favor = XDL_MERGE_FAVOR_BASE;

Though the difference doesn't matter a lot.  Since
merge.conflictStyle=merge (which is the current default) doesn't
display the contents from the merge base in a three-way content merge,
setting xmp.favor to XDL_MERGE_FAVOR_BASE vs. leaving it as 0 for the
recursive/intermediate merges won't generally end up affecting the end
result.  It'd only matter for diff3 and zdiff3 users.


Going on a slight tangent, I think there's actually a related bug
here.  We probably should not honor XDL_MERGE_FAVOR_{OURS,THEIRS} when
opts->virtual_ancestor is true; that's just asking for trouble.  I
think it'd paradoxically result in reversing the desired behavior
(e.g. users would see what they'd consider XDL_MERGE_FAVOR_THEIRS
behavior when they asked for XDL_MERGE_FAVOR_OURS) in some cases as a
result.

> In fact, I don't see why any style should change that desired behavior.
> If you said there's issues with the "merge" style too, perhaps the above
> will help for those cases too.
>
> > > We don't have to use diff3 all the way.
> >
> > Right, thus my mention in the other email to consider adding a
> > XDL_MERGE_FAVOR_BASE -- which you then also mention here in your third
> > option, and which I've now given at least a partial patch for.  Not
> > sure if it's a crazy idea or a great idea, since I don't do very many
> > criss-cross merges myself.
>
> I thought you meant as a separate configurable flag, not something done
> by default.
>
> Now that I understand what you meant I think it could be a great idea.

If someone that does lots of criss-cross merges can comment on the
idea, and agree that it's worth a shot, I can try to turn it into real
patches.

(I might even try to investigate the zdiff3 stuff too, which sounds
like something I've wanted many times...but I'd really rather
concentrate on merge-ort until its upstreaming is finished.)

  reply	other threads:[~2021-06-11 21:42 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-09 19:28 [PATCH 0/7] Make diff3 the default conflict style Felipe Contreras
2021-06-09 19:28 ` [PATCH 1/7] test: add merge style config test Felipe Contreras
2021-06-09 19:42   ` Eric Sunshine
2021-06-09 20:29     ` Felipe Contreras
2021-06-10  9:18   ` Phillip Wood
2021-06-10 13:26     ` Felipe Contreras
2021-06-10 14:54       ` Phillip Wood
2021-06-10 16:34         ` Felipe Contreras
2021-06-10 14:58       ` Phillip Wood
2021-06-10 16:47         ` Felipe Contreras
2021-06-11  9:19           ` Phillip Wood
2021-06-11 14:39             ` Felipe Contreras
2021-06-09 19:28 ` [PATCH 2/7] merge-tree: fix merge.conflictstyle handling Felipe Contreras
2021-06-09 19:28 ` [PATCH 3/7] notes: " Felipe Contreras
2021-06-09 19:28 ` [PATCH 4/7] checkout: " Felipe Contreras
2021-06-10  9:32   ` Phillip Wood
2021-06-10 14:11     ` Felipe Contreras
2021-06-10 14:50       ` Phillip Wood
2021-06-10 16:32         ` Felipe Contreras
2021-06-11  9:18           ` Phillip Wood
2021-06-11 14:34             ` Felipe Contreras
2021-06-11  9:18   ` Phillip Wood
2021-06-09 19:28 ` [PATCH 5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3 Felipe Contreras
2021-06-10  9:21   ` Phillip Wood
2021-06-10 13:33     ` Felipe Contreras
2021-06-11  3:17     ` Junio C Hamano
2021-06-11 13:42       ` Felipe Contreras
2021-06-09 19:28 ` [PATCH 6/7] xdiff: simplify style assignments Felipe Contreras
2021-06-10  9:26   ` Phillip Wood
2021-06-10 13:50     ` Felipe Contreras
2021-06-09 19:28 ` [PATCH 7/7] xdiff: make diff3 the default conflictStyle Felipe Contreras
2021-06-10  6:41   ` Johannes Sixt
2021-06-10  7:53     ` Đoàn Trần Công Danh
2021-06-10 13:18       ` Felipe Contreras
2021-06-10 13:18     ` Felipe Contreras
2021-06-10 13:49     ` Jeff King
2021-06-10 16:00       ` Felipe Contreras
2021-06-10 16:31         ` Jeff King
2021-06-11  1:20       ` Junio C Hamano
2021-06-11  6:23         ` Johannes Sixt
2021-06-11  6:43           ` Junio C Hamano
2021-06-11  7:02             ` Johannes Sixt
2021-06-11  7:14               ` Junio C Hamano
2021-06-11 11:51                 ` Sergey Organov
2021-06-11 15:32                   ` Felipe Contreras
2021-06-11 15:52                     ` Sergey Organov
2021-06-11 16:36                       ` Felipe Contreras
     [not found]                     ` <CABPp-BHRQSF2_aYTBfpfnW4Bh3Hz7vLFj_QNGj8R4WeCS6_utw@mail.gmail.com>
2021-06-11 17:57                       ` Felipe Contreras
2021-06-11 19:02                         ` Elijah Newren
2021-06-11 21:05                           ` Felipe Contreras
2021-06-11 21:40                             ` Elijah Newren [this message]
2021-06-13 14:34                               ` Felipe Contreras
2021-06-11 16:41                   ` Johannes Sixt
2021-06-11 17:21                     ` Felipe Contreras
2021-06-11 17:40                       ` Sergey Organov
2021-06-11 18:10                         ` Felipe Contreras
2021-06-11 18:22                           ` Sergey Organov
2021-06-11 14:28                 ` Felipe Contreras
2021-06-11 14:25               ` Felipe Contreras
2021-06-11 16:53                 ` Johannes Sixt
     [not found]                 ` <CABPp-BH0aRiSUw03nSK6jHRNQ+zcpUzr6WjeJ5GpdUCqCKxbag@mail.gmail.com>
2021-06-11 17:32                   ` Felipe Contreras
2021-06-11 17:57                     ` Elijah Newren
2021-06-11 18:28                       ` Felipe Contreras
2021-06-11 14:20           ` Felipe Contreras
2021-06-11 14:09         ` Felipe Contreras
2021-06-10  9:40   ` Phillip Wood
2021-06-10 14:19     ` Felipe Contreras
2021-06-17 17:40 ` [PATCH 0/7] Make diff3 the default conflict style Phillip Wood
2021-06-17 18:24   ` Felipe Contreras

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-BGhaSJmqXFKpCVcUKexNXaa8sLEms-6q-qno7uETzGQ=A@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=avarab@gmail.com \
    --cc=bagasdotme@gmail.com \
    --cc=davvid@gmail.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=liu.denton@gmail.com \
    --cc=peff@peff.net \
    --cc=sorganov@gmail.com \
    --subject='Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle' \
    /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

Code repositories for project(s) associated with this 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).