| Message ID | pull.1565.v3.git.1694830462463.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] merge-tree: add -X strategy option | expand |
Thanks for your advice! I've fixed those blocking issues. However, regarding the global variable issue, I'm not familiar with C and git code and don't know how to solve this. I think perhaps we need something like closure to parse opt into a local variable? Our usecase is to achieve something like 'range-diff', we first use merge-tree to merge new patchset's base commit with old patchset's source commit, then use the merge result to diff against new patchset's source commit. So we only need to make sure conflict's are handled automatically, leaving other diff features to the second step. On Sat, Sep 16, 2023 at 10:14 AM Izzy via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Tang Yuyi <winglovet@gmail.com> > > Add merge strategy option to produce more customizable merge result such > as automatically solve conflicts. > > Signed-off-by: Tang Yuyi <winglovet@gmail.com> > --- > merge-tree: add -X strategy option > > Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76 > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v3 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v3 > Pull-Request: https://github.com/gitgitgadget/git/pull/1565 > > Range-diff vs v2: > > 1: 7d53d08381e ! 1: d64a774fa7c merge-tree: add -X strategy option > @@ > ## Metadata ## > -Author: winglovet <winglovet@gmail.com> > +Author: Tang Yuyi <winglovet@gmail.com> > > ## Commit message ## > merge-tree: add -X strategy option > @@ Commit message > Add merge strategy option to produce more customizable merge result such > as automatically solve conflicts. > > - Signed-off-by: winglovet <winglovet@gmail.com> > + Signed-off-by: Tang Yuyi <winglovet@gmail.com> > > ## builtin/merge-tree.c ## > @@ > @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few c > test_cmp expect actual > ' > > -+test_expect_success 'Auto resolve conflicts by "ours" stragety option' ' > ++test_expect_success 'Auto resolve conflicts by "ours" strategy option' ' > + git checkout side1^0 && > + > + # make sure merge conflict exists > > > builtin/merge-tree.c | 24 ++++++++++++++++++++++++ > t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c > index 0de42aecf4b..2ec6ec0d39a 100644 > --- a/builtin/merge-tree.c > +++ b/builtin/merge-tree.c > @@ -19,6 +19,8 @@ > #include "tree.h" > #include "config.h" > > +static const char **xopts; > +static size_t xopts_nr, xopts_alloc; > static int line_termination = '\n'; > > struct merge_list { > @@ -414,6 +416,7 @@ struct merge_tree_options { > int show_messages; > int name_only; > int use_stdin; > + struct merge_options merge_options; > }; > > static int real_merge(struct merge_tree_options *o, > @@ -439,6 +442,8 @@ static int real_merge(struct merge_tree_options *o, > > init_merge_options(&opt, the_repository); > > + opt.recursive_variant = o->merge_options.recursive_variant; > + > opt.show_rename_progress = 0; > > opt.branch1 = branch1; > @@ -510,6 +515,17 @@ static int real_merge(struct merge_tree_options *o, > return !result.clean; /* result.clean < 0 handled above */ > } > > +static int option_parse_x(const struct option *opt, > + const char *arg, int unset) > +{ > + if (unset) > + return 0; > + > + ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc); > + xopts[xopts_nr++] = xstrdup(arg); > + return 0; > +} > + > int cmd_merge_tree(int argc, const char **argv, const char *prefix) > { > struct merge_tree_options o = { .show_messages = -1 }; > @@ -548,6 +564,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > &merge_base, > N_("commit"), > N_("specify a merge-base for the merge")), > + OPT_CALLBACK('X', "strategy-option", &xopts, > + N_("option=value"), > + N_("option for selected merge strategy"), > + option_parse_x), > OPT_END() > }; > > @@ -556,6 +576,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > argc = parse_options(argc, argv, prefix, mt_options, > merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION); > > + for (int x = 0; x < xopts_nr; x++) > + if (parse_merge_opt(&o.merge_options, xopts[x])) > + die(_("unknown strategy option: -X%s"), xopts[x]); > + > /* Handle --stdin */ > if (o.use_stdin) { > struct strbuf buf = STRBUF_INIT; > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh > index 250f721795b..4125bb101ec 100755 > --- a/t/t4301-merge-tree-write-tree.sh > +++ b/t/t4301-merge-tree-write-tree.sh > @@ -22,6 +22,7 @@ test_expect_success setup ' > git branch side1 && > git branch side2 && > git branch side3 && > + git branch side4 && > > git checkout side1 && > test_write_lines 1 2 3 4 5 6 >numbers && > @@ -46,6 +47,13 @@ test_expect_success setup ' > test_tick && > git commit -m rename-numbers && > > + git checkout side4 && > + test_write_lines 0 1 2 3 4 5 >numbers && > + echo yo >greeting && > + git add numbers greeting && > + test_tick && > + git commit -m other-content-modifications && > + > git switch --orphan unrelated && > >something-else && > git add something-else && > @@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' ' > test_cmp expect actual > ' > > +test_expect_success 'Auto resolve conflicts by "ours" strategy option' ' > + git checkout side1^0 && > + > + # make sure merge conflict exists > + test_must_fail git merge side4 && > + git merge --abort && > + > + git merge -X ours side4 && > + git rev-parse HEAD^{tree} > expected && > + > + git merge-tree -X ours side1 side4 > actual && > + > + test_cmp expected actual > +' > + > test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' ' > # Mis-spell with single "s" instead of double "s" > test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect && > > base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb > -- > gitgitgadget
Hi, On Fri, Sep 15, 2023 at 7:14 PM Izzy via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Tang Yuyi <winglovet@gmail.com> > > Add merge strategy option to produce more customizable merge result such > as automatically solve conflicts. s/solve/resolving/ I think "solve" should be "solving" here, except that "solve" isn't really the right word. It's not solving (figuring out) anything, it's using a big hammer to make the problem just go away. So, I think "resolving" is a better choice. > Signed-off-by: Tang Yuyi <winglovet@gmail.com> > --- > merge-tree: add -X strategy option > > Change-Id: I16be592262d13cebcff8726eb043f7ecdb313b76 > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1565%2FWingT%2Fmerge_tree_allow_strategy_option-v3 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v3 > Pull-Request: https://github.com/gitgitgadget/git/pull/1565 > > Range-diff vs v2: > > 1: 7d53d08381e ! 1: d64a774fa7c merge-tree: add -X strategy option > @@ > ## Metadata ## > -Author: winglovet <winglovet@gmail.com> > +Author: Tang Yuyi <winglovet@gmail.com> > > ## Commit message ## > merge-tree: add -X strategy option > @@ Commit message > Add merge strategy option to produce more customizable merge result such > as automatically solve conflicts. > > - Signed-off-by: winglovet <winglovet@gmail.com> > + Signed-off-by: Tang Yuyi <winglovet@gmail.com> > > ## builtin/merge-tree.c ## > @@ > @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few c > test_cmp expect actual > ' > > -+test_expect_success 'Auto resolve conflicts by "ours" stragety option' ' > ++test_expect_success 'Auto resolve conflicts by "ours" strategy option' ' > + git checkout side1^0 && > + > + # make sure merge conflict exists > > > builtin/merge-tree.c | 24 ++++++++++++++++++++++++ > t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c > index 0de42aecf4b..2ec6ec0d39a 100644 > --- a/builtin/merge-tree.c > +++ b/builtin/merge-tree.c > @@ -19,6 +19,8 @@ > #include "tree.h" > #include "config.h" > > +static const char **xopts; > +static size_t xopts_nr, xopts_alloc; > static int line_termination = '\n'; > > struct merge_list { > @@ -414,6 +416,7 @@ struct merge_tree_options { > int show_messages; > int name_only; > int use_stdin; > + struct merge_options merge_options; > }; > > static int real_merge(struct merge_tree_options *o, > @@ -439,6 +442,8 @@ static int real_merge(struct merge_tree_options *o, > > init_merge_options(&opt, the_repository); > > + opt.recursive_variant = o->merge_options.recursive_variant; > + > opt.show_rename_progress = 0; > > opt.branch1 = branch1; > @@ -510,6 +515,17 @@ static int real_merge(struct merge_tree_options *o, > return !result.clean; /* result.clean < 0 handled above */ > } > > +static int option_parse_x(const struct option *opt, > + const char *arg, int unset) > +{ > + if (unset) > + return 0; > + > + ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc); > + xopts[xopts_nr++] = xstrdup(arg); > + return 0; > +} > + > int cmd_merge_tree(int argc, const char **argv, const char *prefix) > { > struct merge_tree_options o = { .show_messages = -1 }; > @@ -548,6 +564,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > &merge_base, > N_("commit"), > N_("specify a merge-base for the merge")), > + OPT_CALLBACK('X', "strategy-option", &xopts, > + N_("option=value"), > + N_("option for selected merge strategy"), > + option_parse_x), > OPT_END() > }; > > @@ -556,6 +576,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > argc = parse_options(argc, argv, prefix, mt_options, > merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION); > > + for (int x = 0; x < xopts_nr; x++) > + if (parse_merge_opt(&o.merge_options, xopts[x])) > + die(_("unknown strategy option: -X%s"), xopts[x]); > + > /* Handle --stdin */ > if (o.use_stdin) { > struct strbuf buf = STRBUF_INIT; > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh > index 250f721795b..4125bb101ec 100755 > --- a/t/t4301-merge-tree-write-tree.sh > +++ b/t/t4301-merge-tree-write-tree.sh > @@ -22,6 +22,7 @@ test_expect_success setup ' > git branch side1 && > git branch side2 && > git branch side3 && > + git branch side4 && > > git checkout side1 && > test_write_lines 1 2 3 4 5 6 >numbers && > @@ -46,6 +47,13 @@ test_expect_success setup ' > test_tick && > git commit -m rename-numbers && > > + git checkout side4 && > + test_write_lines 0 1 2 3 4 5 >numbers && > + echo yo >greeting && > + git add numbers greeting && > + test_tick && > + git commit -m other-content-modifications && > + > git switch --orphan unrelated && > >something-else && > git add something-else && > @@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' ' > test_cmp expect actual > ' > > +test_expect_success 'Auto resolve conflicts by "ours" strategy option' ' > + git checkout side1^0 && > + > + # make sure merge conflict exists > + test_must_fail git merge side4 && > + git merge --abort && > + > + git merge -X ours side4 && > + git rev-parse HEAD^{tree} > expected && > + > + git merge-tree -X ours side1 side4 > actual && Multiple style problems still here from V2: * There should be no space between the redirection operator ('>') and the filename following it. * You have indented most lines with tabs, but the line just above this one with 4 spaces. > + > + test_cmp expected actual > +' > + > test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' ' > # Mis-spell with single "s" instead of double "s" > test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect && > > base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
On Fri, Sep 15, 2023 at 7:27 PM 唐宇奕 <winglovet@gmail.com> wrote: > > Thanks for your advice! > I've fixed those blocking issues. Thanks, you did fix most of them. I left a comment on a few things on V3. > However, regarding the global variable issue, I'm not familiar with C > and git code and don't know how to solve this. I think perhaps we need > something like closure to parse opt into a local variable? You could merely make xopts, xopts_nr, and xopts_alloc local variables, and pass them around as needed. Closures would make things look cleaner perhaps, but certainly aren't necessarily. But, I don't think the globals thing is a blocking issue, especially since we already have a number of unnecessary globals in that file already. > Our usecase is to achieve something like 'range-diff', we first use > merge-tree to merge new patchset's base commit with old patchset's > source commit, then use the merge result to diff against new > patchset's source commit. So we only need to make sure conflict's are > handled automatically, leaving other diff features to the second step. If you're trying to do something like range-diff, then I don't see why there's any point in creating a merge at all. I'm afraid I don't follow your explanations about the steps you are taking and more importantly why you are taking them, and how it helps you achieve your goal of doing "something like 'range-diff'". Could you elaborate?
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 0de42aecf4b..2ec6ec0d39a 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -19,6 +19,8 @@ #include "tree.h" #include "config.h" +static const char **xopts; +static size_t xopts_nr, xopts_alloc; static int line_termination = '\n'; struct merge_list { @@ -414,6 +416,7 @@ struct merge_tree_options { int show_messages; int name_only; int use_stdin; + struct merge_options merge_options; }; static int real_merge(struct merge_tree_options *o, @@ -439,6 +442,8 @@ static int real_merge(struct merge_tree_options *o, init_merge_options(&opt, the_repository); + opt.recursive_variant = o->merge_options.recursive_variant; + opt.show_rename_progress = 0; opt.branch1 = branch1; @@ -510,6 +515,17 @@ static int real_merge(struct merge_tree_options *o, return !result.clean; /* result.clean < 0 handled above */ } +static int option_parse_x(const struct option *opt, + const char *arg, int unset) +{ + if (unset) + return 0; + + ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc); + xopts[xopts_nr++] = xstrdup(arg); + return 0; +} + int cmd_merge_tree(int argc, const char **argv, const char *prefix) { struct merge_tree_options o = { .show_messages = -1 }; @@ -548,6 +564,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) &merge_base, N_("commit"), N_("specify a merge-base for the merge")), + OPT_CALLBACK('X', "strategy-option", &xopts, + N_("option=value"), + N_("option for selected merge strategy"), + option_parse_x), OPT_END() }; @@ -556,6 +576,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, mt_options, merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION); + for (int x = 0; x < xopts_nr; x++) + if (parse_merge_opt(&o.merge_options, xopts[x])) + die(_("unknown strategy option: -X%s"), xopts[x]); + /* Handle --stdin */ if (o.use_stdin) { struct strbuf buf = STRBUF_INIT; diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh index 250f721795b..4125bb101ec 100755 --- a/t/t4301-merge-tree-write-tree.sh +++ b/t/t4301-merge-tree-write-tree.sh @@ -22,6 +22,7 @@ test_expect_success setup ' git branch side1 && git branch side2 && git branch side3 && + git branch side4 && git checkout side1 && test_write_lines 1 2 3 4 5 6 >numbers && @@ -46,6 +47,13 @@ test_expect_success setup ' test_tick && git commit -m rename-numbers && + git checkout side4 && + test_write_lines 0 1 2 3 4 5 >numbers && + echo yo >greeting && + git add numbers greeting && + test_tick && + git commit -m other-content-modifications && + git switch --orphan unrelated && >something-else && git add something-else && @@ -97,6 +105,21 @@ test_expect_success 'Content merge and a few conflicts' ' test_cmp expect actual ' +test_expect_success 'Auto resolve conflicts by "ours" strategy option' ' + git checkout side1^0 && + + # make sure merge conflict exists + test_must_fail git merge side4 && + git merge --abort && + + git merge -X ours side4 && + git rev-parse HEAD^{tree} > expected && + + git merge-tree -X ours side1 side4 > actual && + + test_cmp expected actual +' + test_expect_success 'Barf on misspelled option, with exit code other than 0 or 1' ' # Mis-spell with single "s" instead of double "s" test_expect_code 129 git merge-tree --write-tree --mesages FOOBAR side1 side2 2>expect &&