| Message ID | pull.1565.v4.git.1694836025469.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] merge-tree: add -X strategy option | expand |
Hi, On Fri, Sep 15, 2023 at 8:47 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 resolving 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-v4 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v4 > Pull-Request: https://github.com/gitgitgadget/git/pull/1565 > > Range-diff vs v3: > > 1: d64a774fa7c ! 1: d2d8fcc2e9b merge-tree: add -X strategy option > @@ Commit message > merge-tree: add -X strategy option > > Add merge strategy option to produce more customizable merge result such > - as automatically solve conflicts. > + as automatically resolving conflicts. Thanks for fixing this part of what I pointed out in the review. > Signed-off-by: Tang Yuyi <winglovet@gmail.com> > > > > 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 && Please fix the problems in these last two lines as pointed out in both of my last two reviews.
Thanks, what we want to do is to compare two patchsets' diff while providing capabilities like normal commit diff. The existing range-diff command provide an effective solution, however: * The output isn't suitable for machine processing * The command take's number of commits and commit messages into count, while what we really want is merely the content diff * The command doesn't support features like word diff, ignoring whitespace changes, etc So what comes to our mind is to simulate user behavior. We first merge the old patchset into the new patchset's base commit, then use the merge result to diff against the new patchset's source commit. By doing this, the diff introduced between two patchsets' bases won't be shown. Thus we get the 'real diff' between two patchsets. In the first step, we use git-merge-tree to produce the merge result since its performance's better than git-merge. However, sometimes there's conflict between the new patchset's base and the old patchset's source.So we need automatic conflict resolving - only use content from 'their' side specifically. Hope that makes sense. On Sat, Sep 16, 2023 at 11:47 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 resolving 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-v4 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v4 > Pull-Request: https://github.com/gitgitgadget/git/pull/1565 > > Range-diff vs v3: > > 1: d64a774fa7c ! 1: d2d8fcc2e9b merge-tree: add -X strategy option > @@ Commit message > merge-tree: add -X strategy option > > Add merge strategy option to produce more customizable merge result such > - as automatically solve conflicts. > + as automatically resolving conflicts. > > Signed-off-by: Tang Yuyi <winglovet@gmail.com> > > > > 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
On Sat, Sep 16, 2023 at 03:47:05AM +0000, Izzy via GitGitGadget wrote: > +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; > +} This callback was presumably copied from the one in builtin/merge.c, and it suffers from the same "--no-strategy-option" bug. You should make a similar change here to the one we did in dee02da826 (merge: make xopts a strvec, 2023-08-31). And as a bonus, it will make your patch even shorter. :) It would also make it easier to get rid of the global variables, I think. Something like (squashed into your patch): diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 2ec6ec0d39..e13dbc4c79 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -18,9 +18,8 @@ #include "quote.h" #include "tree.h" #include "config.h" +#include "strvec.h" -static const char **xopts; -static size_t xopts_nr, xopts_alloc; static int line_termination = '\n'; struct merge_list { @@ -515,20 +514,10 @@ 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 }; + struct strvec xopts = STRVEC_INIT; int expected_remaining_argc; int original_argc; const char *merge_base = NULL; @@ -564,10 +553,8 @@ 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_STRVEC('X', "strategy-option", &xopts, N_("option=value"), + N_("option for selected merge strategy")), OPT_END() }; @@ -576,9 +563,9 @@ 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]); + for (int x = 0; x < xopts.nr; x++) + if (parse_merge_opt(&o.merge_options, xopts.v[x])) + die(_("unknown strategy option: -X%s"), xopts.v[x]); /* Handle --stdin */ if (o.use_stdin) {
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 &&