git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH 1/2] merge-ort: add new merge-tree-ort command

From
Elijah Newren <newren@gmail.com>
Date
Jan 5, 2022, 17:08 UTC
Message-ID
<CABPp-BFQv+mrWj8iH0Vo5Pr5L922v=ZsVthFjofy5pm1Sx8x5Q@mail.gmail.com>
In-Reply-To
<20220105163324.73369-2-chriscool@tuxfamily.org>

On Wed, Jan 5, 2022 at 8:33 AM Christian Couder <christian.couder@gmail.com> wrote:

Show 5 quoted lines
>
> This new command is similar as merge-tree, but uses
> merge-ort code and features, especially
> merge_incore_nonrecursive(), instead of those from
> resursive merge, to perform the merge.
s/resursive/recursive/
Show 9 quoted lines
> The output from this new command is very different from
> merge-tree's custom output, as we are only using code and
> features from diff.{h,c}, especially diff_tree_oid(). This
> should make it easy to customize and standardize the output
> using regular diff options in the future.
>
> This command will be extended to support new features
> with the long-term goal of enabling merges and rebases
> without a worktree.
And cherry-picks?  And reverts?

I think the cherry-pick/rebase replacement actually deserves a separate command from what merges should use. Replaying a sequence of commits just has a number of UI differences and abilities that I think pull it in a different direction. And I don't want replaying of a sequence of commits via a script like the old days (even if one that calls something that doesn't update the working tree and index would be better than the old rebases built on top of `git merge-recursive` and `git cherry-pick`). I'm working on such a thing, though it's still kind of early.

Show 58 quoted lines
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  .gitignore               |  1 +
>  Makefile                 |  1 +
>  builtin.h                |  1 +
>  builtin/merge-tree-ort.c | 93 ++++++++++++++++++++++++++++++++++++++++
>  git.c                    |  1 +
>  5 files changed, 97 insertions(+)
>  create mode 100644 builtin/merge-tree-ort.c
>
> diff --git a/.gitignore b/.gitignore
> index 054249b20a..2dfcb1a589 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -98,6 +98,7 @@
>  /git-merge-index
>  /git-merge-file
>  /git-merge-tree
> +/git-merge-tree-ort
>  /git-merge-octopus
>  /git-merge-one-file
>  /git-merge-ours
> diff --git a/Makefile b/Makefile
> index 75ed168adb..915e500b06 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1124,6 +1124,7 @@ BUILTIN_OBJS += builtin/merge-index.o
>  BUILTIN_OBJS += builtin/merge-ours.o
>  BUILTIN_OBJS += builtin/merge-recursive.o
>  BUILTIN_OBJS += builtin/merge-tree.o
> +BUILTIN_OBJS += builtin/merge-tree-ort.o
>  BUILTIN_OBJS += builtin/merge.o
>  BUILTIN_OBJS += builtin/mktag.o
>  BUILTIN_OBJS += builtin/mktree.o
> diff --git a/builtin.h b/builtin.h
> index 8a58743ed6..c68f46b118 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -182,6 +182,7 @@ int cmd_merge_ours(int argc, const char **argv, const char *prefix);
>  int cmd_merge_file(int argc, const char **argv, const char *prefix);
>  int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
>  int cmd_merge_tree(int argc, const char **argv, const char *prefix);
> +int cmd_merge_tree_ort(int argc, const char **argv, const char *prefix);
>  int cmd_mktag(int argc, const char **argv, const char *prefix);
>  int cmd_mktree(int argc, const char **argv, const char *prefix);
>  int cmd_multi_pack_index(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/merge-tree-ort.c b/builtin/merge-tree-ort.c
> new file mode 100644
> index 0000000000..1c8ecd16ec
> --- /dev/null
> +++ b/builtin/merge-tree-ort.c
> @@ -0,0 +1,93 @@
> +#include "builtin.h"
> +#include "merge-ort.h"
> +#include "diff.h"
> +
> +static const char merge_tree_ort_usage[] = "git merge-tree-ort <base-tree> <branch1> <branch2>";

I think this is somewhat limiting to use for merges in general; it'll generate trees that do not match what users get from `git merge ...` at the command line, because specifying a single base-tree is different than having the algorithm determine the merge base, particularly when it needs to construct a virtual merge base.

Show 35 quoted lines
> +
> +static void show_result(struct tree *base_tree,
> +                       struct tree *head_tree,
> +                       struct tree *merge_tree,
> +                       struct merge_result *result)
> +{
> +       const struct object_id *base_oid = &(base_tree->object.oid);
> +       const struct object_id *head_oid = &(head_tree->object.oid);
> +       const struct object_id *merge_oid = &(merge_tree->object.oid);
> +       const struct object_id *result_oid = &(result->tree->object.oid);
> +       struct diff_options opts;
> +
> +       repo_diff_setup(the_repository, &opts);
> +       opts.stat_width = -1; /* use full terminal width */
> +       opts.output_format |= DIFF_FORMAT_RAW | DIFF_FORMAT_PATCH;
> +       opts.detect_rename = DIFF_DETECT_RENAME;
> +       diff_setup_done(&opts);
> +
> +       printf("result tree: %s\n", oid_to_hex(result_oid));
> +       printf("clean: %d\n", result->clean);
> +
> +       printf("diff with branch1:\n");
> +       diff_tree_oid(head_oid, result_oid, "", &opts);
> +       diffcore_std(&opts);
> +       diff_flush(&opts);
> +
> +       printf("diff with branch2:\n");
> +       diff_tree_oid(merge_oid, result_oid, "", &opts);
> +       diffcore_std(&opts);
> +       diff_flush(&opts);
> +
> +       printf("diff with base:\n");
> +       diff_tree_oid(base_oid, result_oid, "", &opts);
> +       diffcore_std(&opts);
> +       diff_flush(&opts);
I commented on the diffs in my response on the cover letter.
Show 40 quoted lines
> +}
> +
> +static struct commit *get_commit_by_name_or_die(const char *name)
> +{
> +       struct commit *c = lookup_commit_reference_by_name(name);
> +       if (!c)
> +               die(_("not a valid commit name '%s'"), name);
> +       return c;
> +}
> +
> +static void merge_trees_ort(const char *base_name,
> +                           const char *branch1,
> +                           const char *branch2)
> +{
> +       struct merge_result result;
> +       struct merge_options merge_opt;
> +
> +       struct commit *base = get_commit_by_name_or_die(base_name);
> +       struct commit *head = get_commit_by_name_or_die(branch1);
> +       struct commit *merge = get_commit_by_name_or_die(branch2);
> +
> +       struct tree *base_tree = get_commit_tree(base);
> +       struct tree *head_tree = get_commit_tree(head);
> +       struct tree *merge_tree = get_commit_tree(merge);
> +
> +       memset(&result, 0, sizeof(result));
> +       init_merge_options(&merge_opt, the_repository);
> +
> +       merge_opt.show_rename_progress = 1;
> +       merge_opt.branch1 = branch1;
> +       merge_opt.branch2 = branch2;
> +       merge_opt.ancestor = base_name;
> +
> +       result.tree = head_tree;
> +
> +       merge_incore_nonrecursive(&merge_opt,
> +                                 base_tree,
> +                                 result.tree,
> +                                 merge_tree,
> +                                 &result);

I think for server side merges, merge_incore_recursive() should be used, so that they match what a `git merge ...` would provide. I think merge_incore_nonrecursive() is better used for replaying a sequence of commits.

So, I view this as more suitable for usage with the server side rebase, except that it seems to be serving as a building block and expecting the rebase to be some script on top. I think server-side rebase/cherry-pick should be a builtin of some form.

> +
> +       show_result(base_tree, head_tree, merge_tree, &result);

As noted in the cover letter, this current form doesn't provide 2 things that users can't get some other way: (1) a list of conflicted paths, and (2) the various conflict and warning messages generated during the merge. I agree that many usecases might not want those, but since they can't be obtained another way I think it'd be prudent to provide them.

And this show_result provides 3 things that users could generate on their own with the information already provided -- namely the three diffs that you do. I'm curious what the usecase is for those diffs.

Show 25 quoted lines
> +}
> +
> +int cmd_merge_tree_ort(int argc, const char **argv, const char *prefix)
> +{
> +       if (argc != 4)
> +               usage(merge_tree_ort_usage);
> +
> +       merge_trees_ort(argv[1], argv[2], argv[3]);
> +
> +       return 0;
> +}
> diff --git a/git.c b/git.c
> index 7edafd8ecf..90b8a4984c 100644
> --- a/git.c
> +++ b/git.c
> @@ -562,6 +562,7 @@ static struct cmd_struct commands[] = {
>         { "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
>         { "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
>         { "merge-tree", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },
> +       { "merge-tree-ort", cmd_merge_tree_ort, RUN_SETUP | NO_PARSEOPT },
>         { "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
>         { "mktree", cmd_mktree, RUN_SETUP },
>         { "multi-pack-index", cmd_multi_pack_index, RUN_SETUP },
> --
> 2.34.1.433.g7bc349372a.dirty
Previous: Christian CouderNext: Christian Couder
Message 3 of 28 in “Introduce new merge-tree-ort command”
  1. 0/2 Introduce new merge-tree-ort commandChristian Couder, Jan 5, 2022
  2. 1/2 merge-ort: add new merge-tree-ort commandChristian Couder, Jan 5, 2022
  3. Elijah NewrenJan 5, 2022
  4. 2/2 merge-ort: add t/t4310-merge-tree-ort.shChristian Couder, Jan 5, 2022
  5. Elijah NewrenJan 5, 2022
  6. Elijah NewrenJan 5, 2022
  7. Elijah NewrenJan 5, 2022
  8. Christian CouderJan 7, 2022
  9. Elijah NewrenJan 7, 2022
  10. Johannes SchindelinJan 10, 2022
  11. Junio C HamanoJan 10, 2022
  12. Johannes SchindelinJan 11, 2022
  13. Ævar Arnfjörð BjarmasonJan 11, 2022
  14. Elijah NewrenJan 11, 2022
  15. Junio C HamanoJan 12, 2022
  16. Elijah NewrenJan 12, 2022
  17. Junio C HamanoJan 13, 2022
  18. Elijah NewrenJan 13, 2022
  19. Ævar Arnfjörð BjarmasonJan 13, 2022
  20. Junio C HamanoJan 12, 2022
  21. Ævar Arnfjörð BjarmasonJan 13, 2022
  22. Elijah NewrenJan 10, 2022
  23. Elijah NewrenJan 11, 2022
  24. Johannes SchindelinFeb 22, 2022
  25. Johannes SchindelinJan 11, 2022
  26. Elijah NewrenJan 12, 2022
  27. Johannes SchindelinFeb 22, 2022
  28. Johannes SchindelinJan 7, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.