Re: [PATCH v3 03/15] merge-tree: add option parsing and initial shell for real merge function
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Feb 7, 2022, 22:41 UTC
- Message-ID
- <YgGgCasl1eVz679E@google.com>
- In-Reply-To
- <63f42df21aec5bda50e4414493eb59dcb64e5558.1643787281.git.gitgitgadget@gmail.com>
On Wed, Feb 02, 2022 at 07:34:29AM +0000, Elijah Newren via GitGitGadget wrote:
Show 101 quoted lines
>
>
> Let merge-tree accept a `--write-tree` parameter for choosing real
> merges instead of trivial merges, and accept an optional
> `--trivial-merge` option to get the traditional behavior. Note that
> these accept different numbers of arguments, though, so these names
> need not actually be used.
>
> Note that real merges differ from trivial merges in that they handle:
> - three way content merges
> - recursive ancestor consolidation
> - renames
> - proper directory/file conflict handling
> - etc.
> Basically all the stuff you'd expect from `git merge`, just without
> updating the index and working tree. The initial shell added here does
> nothing more than die with "real merges are not yet implemented", but
> that will be fixed in subsequent commits.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> builtin/merge-tree.c | 61 +++++++++++++++++++++++++++++++++++++-------
> git.c | 2 +-
> 2 files changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 914ec960b7e..e98ec8a9f1d 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -3,13 +3,12 @@
> #include "tree-walk.h"
> #include "xdiff-interface.h"
> #include "object-store.h"
> +#include "parse-options.h"
> #include "repository.h"
> #include "blob.h"
> #include "exec-cmd.h"
> #include "merge-blobs.h"
>
> -static const char merge_tree_usage[] = "git merge-tree <base-tree> <branch1> <branch2>";
> -
> struct merge_list {
> struct merge_list *next;
> struct merge_list *link; /* other stages for this object */
> @@ -366,15 +365,17 @@ static void *get_tree_descriptor(struct repository *r,
> return buf;
> }
>
> -static int trivial_merge(int argc, const char **argv)
> +static int trivial_merge(const char *base,
> + const char *branch1,
> + const char *branch2)
> {
> struct repository *r = the_repository;
> struct tree_desc t[3];
> void *buf1, *buf2, *buf3;
>
> - buf1 = get_tree_descriptor(r, t+0, argv[1]);
> - buf2 = get_tree_descriptor(r, t+1, argv[2]);
> - buf3 = get_tree_descriptor(r, t+2, argv[3]);
> + buf1 = get_tree_descriptor(r, t+0, base);
> + buf2 = get_tree_descriptor(r, t+1, branch1);
> + buf3 = get_tree_descriptor(r, t+2, branch2);
> trivial_merge_trees(t, "");
> free(buf1);
> free(buf2);
> @@ -384,9 +385,51 @@ static int trivial_merge(int argc, const char **argv)
> return 0;
> }
>
> +struct merge_tree_options {
> + int mode;
> +};
> +
> +static int real_merge(struct merge_tree_options *o,
> + const char *branch1, const char *branch2)
> +{
> + die(_("real merges are not yet implemented"));
> +}
> +
> int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> {
> - if (argc != 4)
> - usage(merge_tree_usage);
> - return trivial_merge(argc, argv);
> + struct merge_tree_options o = { 0 };
> + int expected_remaining_argc;
> +
> + const char * const merge_tree_usage[] = {
> + N_("git merge-tree [--write-tree] <branch1> <branch2>"),
> + N_("git merge-tree [--trivial-merge] <base-tree> <branch1> <branch2>"),
> + NULL
> + };
> + struct option mt_options[] = {
> + OPT_CMDMODE(0, "write-tree", &o.mode,
> + N_("do a real merge instead of a trivial merge"),
> + 'w'),
> + OPT_CMDMODE(0, "trivial-merge", &o.mode,
> + N_("do a trivial merge only"), 't'),
> + OPT_END()
> + };In the review club last week I had mentioned I thought OPT_CMDMODE worked well with enums. I found some a reasonably nice example in builtin/replace.c:cmd_replace(), although I have some Opinions about the enum declaration placement there. Regardless, I think using an enum instead of a single character would make this more readable - otherwise I need to remember what 'w' means when I'm reasoning about how many args to expect below.
Show 20 quoted lines
> +
> + /* Parse arguments */
> + argc = parse_options(argc, argv, prefix, mt_options,
> + merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
> + if (o.mode) {
> + expected_remaining_argc = (o.mode == 'w' ? 2 : 3);
> + if (argc != expected_remaining_argc)
> + usage_with_options(merge_tree_usage, mt_options);
> + } else {
> + if (argc < 2 || argc > 3)
> + usage_with_options(merge_tree_usage, mt_options);
> + o.mode = (argc == 2 ? 'w' : 't');
> + }
> +
> + /* Do the relevant type of merge */
> + if (o.mode == 'w')
> + return real_merge(&o, argv[0], argv[1]);
> + else
> + return trivial_merge(argv[0], argv[1], argv[2]);
> }Sorry for the slow reply.
- Emily