From: Christian Couder <christian.couder@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
"Junio C Hamano" <gitster@pobox.com>,
"Christian Couder" <chriscool@tuxfamily.org>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Taylor Blau" <me@ttaylorr.com>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Subject: Re: [RFC PATCH 0/2] Introduce new merge-tree-ort command
Date: Fri, 7 Jan 2022 18:58:00 +0100 [thread overview]
Message-ID: <CAP8UFD0wKnAg5oyMWchXysPTg3K9Vb4M1tRcPzPE81QM903pYg@mail.gmail.com> (raw)
In-Reply-To: <CABPp-BFh7UnQtPM=tO8rfp5bPK4-7esouv5KCx1sUSESwEA=Rw@mail.gmail.com>
On Wed, Jan 5, 2022 at 5:54 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, Jan 5, 2022 at 8:33 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> > The current `git merge-tree` command though seems to have a number of
> > issues, especially:
> >
> > - it's too much related to the old merge recursive strategy which is
> > not the default anymore since v2.34.0 and is likely to be
> > deprecated over time,
> >
> > - it seems to output things in its own special format, which is not
> > easy to customize, and which needs special code and logic to parse
>
> I agree we don't want those...but why would new merge-tree options
> have to use the old merge strategy or the old output format?
Yeah, it's not necessary if there are 2 separate modes, a "real" mode
(like what you implemented with --real in your recent patch series)
and a "trivial" mode (which is the name you give to the old code).
Adding modes like this to a command is likely to make the command and
its documentation more difficult to understand though. For example I
think that we created `git switch` because the different modes of `git
checkout` made the command hard to learn.
I gave other reasons in [1] why I prefer a separate command.
[1] https://lore.kernel.org/git/CAP8UFD1LgfZ0MT9=cMvxCcox++_MBBhWG9Twf42cMiXL42AdpQ@mail.gmail.com/
> > To move forward on this, this small RFC patch series introduces a new
> > `git merge-tree-ort` command with the following design:
>
> Slightly dislike the command name.
I am ok with changing the command name.
> `ort` was meant as a temporary,
> internal name. I don't think it's very meaningful to users, so I was
> hoping to just make `recursive` mean `ort` after we had enough
> testing, and to delete merge-recursive.[ch] at that time. Then `ort`
> merely becomes a historical footnote (...and perhaps part of the name
> of the file where the `recursive` algorithm is implemented).
I think something similar could still be done with `git
merge-tree-ort` or whatever name we give to this command. For example
we could first add --ort to `git merge-tree` and make it call `git
merge-tree-ort`, then after some time make --ort the default, then
after some more time remove `git merge-tree-ort`. And whenever we make
those changes we could also rename the builtin/merge-tree*.{h,c}
accordingly.
> > - it uses merge-ort's API as is to perform the merge
> >
> > - it gets back a tree oid and a cleanliness status from merge-ort's
> > API and prints them out first
>
> Good so far.
>
> > - it uses diff's API as is to output changed paths and code
> >
> > - the diff API, actually diff_tree_oid() is called 3 times: once for
> > the diff versus branch1 ("ours"), once for the diff versus branch2
> > ("theirs"), and once for the diff versus the base.
>
> Why? That seems to be a performance penalty for anyone that doesn't
> want/need the diffs, and since we return a tree, a caller can go and
> get whatever diffs they like.
I say somewhere else that I am planning to add options to disable this
or parts of this diff output.
I think it's still interesting for the command to be able to output
diffs, especially diffs of conflicting parts. In [2] Ævar said:
=> I.e. I'm not the first or last to have (not for anything serious)
=> implement a dry-run bare-repo merge with something like:
=>
=> git merge-tree origin/master git-for-windows/main origin/seen >diff
=> # Better regex needed, but basically this
=> grep "^\+<<<<<<< \.our$" diff && conflict=t
Also `git merge-tree` currently outputs diffs, so I thought it would
be sad if the new command couldn't do it.
[2] https://lore.kernel.org/git/211109.861r3qdpt8.gmgdl@evledraar.gmail.com/
> > Therefore:
> >
> > - its code is very simple and very easy to extend and customize, for
> > example by passing diff or merge-ort options that the code would
> > just pass on to the merge-ort and diff APIs respectively
> >
> > - its output can easily be parsed using simple code
>
> These points are good.
>
> > and existing diff parsers
> >
> > This of course means that merge-tree-ort's output is not backward
> > compatible with merge-tree's output, but it doesn't seem that there is
> > much value in keeping the same output anyway. On the contrary
> > merge-tree's output is likely to hold us back already.
> >
> > The first patch in the series adds the new command without any test
> > and documentation.
> >
> > The second patch in the series adds a few tests that let us see how
> > the command's output looks like in different very simple cases.
> >
> > Of course if this approach is considered valuable, I plan to add some
> > documentation, more tests and very likely a number of options before
> > submitting the next iteration.
>
> Was there something you didn't like about
> https://lore.kernel.org/git/pull.1114.git.git.1640927044.gitgitgadget@gmail.com/?
I was having a vacation at the time and even though I skimmed the
mailing list, I missed it. Sorry.
Also I thought that you might not be interested in this anymore as you
didn't reply to [1] and [2] where Ævar and I both said that we were
interested in your opinion on a git merge-tree on steroids.
> > I am not sure that it's worth showing the 3 diffs (versus branch1,
> > branch2 and base) by default. Maybe by default no diff at all should
> > be shown and the command should have --branch1 (or --ours), --branch2
> > (or --theirs) and --base options to ask for such output, but for an
> > RFC patch I thought it would be better to output the 3 diffs so that
> > people get a better idea of the approach this patch series is taking.
>
> I think not showing, neither by default
I am ok with not showing them by default.
> or at all would be better.
> All three of these are things users could easily generate for
> themselves with the tree we return. I'm curious, though, what's the
> usecase for wanting these specific diffs?
I think I replied to this above.
> Two things you didn't return that users cannot get any other way: (1)
> conflict and warning messages, (2) list of conflicted paths.
Yeah, I wasn't sure how they could be returned with the merge-ort (or
maybe diff) API, and I thought that the current `git merge-tree`
doesn't report them, so I was aiming for something roughly just as
powerful as the current `git merge-tree`.
next prev parent reply other threads:[~2022-01-07 17:58 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-05 16:33 [RFC PATCH 0/2] Introduce new merge-tree-ort command Christian Couder
2022-01-05 16:33 ` [RFC PATCH 1/2] merge-ort: add " Christian Couder
2022-01-05 17:08 ` Elijah Newren
2022-01-05 16:33 ` [RFC PATCH 2/2] merge-ort: add t/t4310-merge-tree-ort.sh Christian Couder
2022-01-05 17:29 ` Elijah Newren
2022-01-05 16:53 ` [RFC PATCH 0/2] Introduce new merge-tree-ort command Elijah Newren
2022-01-05 17:32 ` Elijah Newren
2022-01-07 17:58 ` Christian Couder [this message]
2022-01-07 19:06 ` Elijah Newren
2022-01-10 13:49 ` Johannes Schindelin
2022-01-10 17:56 ` Junio C Hamano
2022-01-11 13:47 ` Johannes Schindelin
2022-01-11 17:00 ` Ævar Arnfjörð Bjarmason
2022-01-11 22:25 ` Elijah Newren
2022-01-12 18:06 ` Junio C Hamano
2022-01-12 20:06 ` Elijah Newren
2022-01-13 6:08 ` Junio C Hamano
2022-01-13 8:01 ` Elijah Newren
2022-01-13 9:26 ` Ævar Arnfjörð Bjarmason
2022-01-12 17:54 ` Junio C Hamano
2022-01-13 9:22 ` Ævar Arnfjörð Bjarmason
2022-01-10 17:59 ` Elijah Newren
2022-01-11 21:15 ` Elijah Newren
2022-02-22 13:08 ` Johannes Schindelin
2022-01-11 22:30 ` Johannes Schindelin
2022-01-12 0:41 ` Elijah Newren
2022-02-22 12:44 ` Johannes Schindelin
2022-01-07 19:54 ` Johannes Schindelin
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAP8UFD0wKnAg5oyMWchXysPTg3K9Vb4M1tRcPzPE81QM903pYg@mail.gmail.com \
--to=christian.couder@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=chriscool@tuxfamily.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=newren@gmail.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.
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 NNTP newsgroup(s).