On Wed, Jan 5, 2022 at 5:54 PM Elijah Newren <newren@xxxxxxxxx> wrote: > > On Wed, Jan 5, 2022 at 8:33 AM Christian Couder > <christian.couder@xxxxxxxxx> 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@xxxxxxxxxxxxxx/ > > 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@xxxxxxxxxxxxxxxxxxx/ > > 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@xxxxxxxxx/? 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`.
