git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Christian Couder <christian.couder@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 11:06:07 -0800	[thread overview]
Message-ID: <CABPp-BHpuLjCYycBwweMW_NdQrS_r3ECmvLb8XjNW-_Rau5NTw@mail.gmail.com> (raw)
In-Reply-To: <CAP8UFD0wKnAg5oyMWchXysPTg3K9Vb4M1tRcPzPE81QM903pYg@mail.gmail.com>

On Fri, Jan 7, 2022 at 9:58 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> 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/

I can see where you're coming from, but I think the particular
comparison you use isn't quite apples to apples.  `git checkout` has a
"change branches" mode and a "revert specific paths" mode.  While
those have significant implementation overlap, they seem like
different concepts to users.  For merge-tree, the implementation is
completely orthogonal between the two modes, but the concept is
basically the same from the user viewpoint.  Yes, the output differs
in the two merge-tree modes, but command line arguments are often used
to tweak the output (much like diff has different output styles based
on various flags).  In fact, in [1] where you suggest a new command
you suggest that it should have "roughly the same features as git
merge-tree and a similar interface".  To me, that suggests that the
two may be good candidates for being similar commands.

That said, I'm not against a new command.  Personally, my main reason
for using merge-tree wasn't because that's where I thought it was best
to put it, but just that (IIRC) each of Dscho, Peff, and Junio
suggested that location.

My biggest gripe was just the command name...

> > > 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.

User-facing names tend to take a while to remove; I'd rather start
with user-facing command and option names that are meaningful to users
in terms of what they want to accomplish.  I don't think `ort`
qualifies as such; I'd rather `ort` only be used by expert users (e.g.
folks wanting to test out a new merge algorithm they heard about
before it became the default).

> > >   - 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.

Are you presuming that you can output a diff of conflicting parts?  A
diff on a modify/delete or can't-merge-binary-files won't show
anything, and won't provide any notification that there even was a
conflict.

> 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

This code is pretty hacky; it presumes that content conflicts are the
only type of conflict.  Inability to merge binary files,
file/directory conflicts, mode conflicts, modify/delete conflicts,
various rename conflicts, etc. are all other types.  I'd rather not
encourage such hacks.

Returning whether there were conflicts in the exit status is far
cleaner.  Providing some mechanism for getting a list of files which
had conflicts may also be useful, but both of these are things where a
diff is the wrong tool.

> 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/

Hmm, I had a totally different opinion.  I felt the diffs in the
current merge-tree was a hack to deal with the fact that they didn't
have good tools to make use of the results, and ended up providing a
Rube-Goldberg scheme to do anything useful with it.

Providing a tree is a concrete and easily usable object for end users.
They can feed that tree to other git commands to do additional things,
and obviates the need for the Rube-Goldberg contraption.

> > 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.

Ah, gotcha.  I was just curious if there was something you felt was
wrong and you wanted to propose an alternative.

> 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.

Sorry,  I saw Ævar's email and noticed various assumptions I felt were
problematic, but noticed he said he was just spitballing (which is
totally fair).  I figured that rather than attempt to explain each
detail, it'd be better to update my code and provide it...and I then
ignored the rest of the thread and missed your question.  Sorry.  I
certainly meant to update this series and send it out earlier, but
there were multiple other series of mine that were also backed up and
several series I needed to review, so it took me a while to clean out
the queue a bit.

  reply	other threads:[~2022-01-07 19:06 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
2022-01-07 19:06     ` Elijah Newren [this message]
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

  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-BHpuLjCYycBwweMW_NdQrS_r3ECmvLb8XjNW-_Rau5NTw@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.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).