* Re: [PATCH v4] merge-tree: add -X strategy option
2023-09-16 3:47 ` [PATCH v4] " Izzy via GitGitGadget
@ 2023-09-16 3:55 ` Elijah Newren
2023-09-16 4:04 ` 唐宇奕
` (2 subsequent siblings)
3 siblings, 0 replies; 27+ messages in thread
From: Elijah Newren @ 2023-09-16 3:55 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Tang Yuyi
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.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4] merge-tree: add -X strategy option
2023-09-16 3:47 ` [PATCH v4] " Izzy via GitGitGadget
2023-09-16 3:55 ` Elijah Newren
@ 2023-09-16 4:04 ` 唐宇奕
2023-09-16 6:11 ` Jeff King
2023-09-16 8:37 ` [PATCH v5] " Izzy via GitGitGadget
3 siblings, 0 replies; 27+ messages in thread
From: 唐宇奕 @ 2023-09-16 4:04 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Elijah Newren
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v4] merge-tree: add -X strategy option
2023-09-16 3:47 ` [PATCH v4] " Izzy via GitGitGadget
2023-09-16 3:55 ` Elijah Newren
2023-09-16 4:04 ` 唐宇奕
@ 2023-09-16 6:11 ` Jeff King
2023-09-16 8:37 ` [PATCH v5] " Izzy via GitGitGadget
3 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2023-09-16 6:11 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Elijah Newren, Izzy
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) {
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v5] merge-tree: add -X strategy option
2023-09-16 3:47 ` [PATCH v4] " Izzy via GitGitGadget
` (2 preceding siblings ...)
2023-09-16 6:11 ` Jeff King
@ 2023-09-16 8:37 ` Izzy via GitGitGadget
2023-09-16 8:38 ` 唐宇奕
` (2 more replies)
3 siblings, 3 replies; 27+ messages in thread
From: Izzy via GitGitGadget @ 2023-09-16 8:37 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Jeff King, Izzy, Tang Yuyi
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-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1565
Range-diff vs v4:
1: d2d8fcc2e9b ! 1: 28d4282e0d8 merge-tree: add -X strategy option
@@ Commit message
## builtin/merge-tree.c ##
@@
+ #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 {
@@ builtin/merge-tree.c: struct merge_tree_options {
int show_messages;
int name_only;
@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
opt.branch1 = branch1;
@@ builtin/merge-tree.c: 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;
@@ builtin/merge-tree.c: 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()
};
@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
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) {
@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few c
+ git merge --abort &&
+
+ git merge -X ours side4 &&
-+ git rev-parse HEAD^{tree} > expected &&
++ git rev-parse HEAD^{tree} >expected &&
+
-+ git merge-tree -X ours side1 side4 > actual &&
++ git merge-tree -X ours side1 side4 >actual &&
+
+ test_cmp expected actual
+'
builtin/merge-tree.c | 11 +++++++++++
t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 0de42aecf4b..97d0fe6c952 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,6 +18,7 @@
#include "quote.h"
#include "tree.h"
#include "config.h"
+#include "strvec.h"
static int line_termination = '\n';
@@ -414,6 +415,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 +441,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;
@@ -513,6 +517,7 @@ static int real_merge(struct merge_tree_options *o,
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;
@@ -548,6 +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_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+ N_("option for selected merge strategy")),
OPT_END()
};
@@ -556,6 +563,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.v[x]))
+ die(_("unknown strategy option: -X%s"), xopts.v[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..b2c8a43fce3 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v5] merge-tree: add -X strategy option
2023-09-16 8:37 ` [PATCH v5] " Izzy via GitGitGadget
@ 2023-09-16 8:38 ` 唐宇奕
2023-09-18 9:53 ` Phillip Wood
2023-09-24 2:23 ` [PATCH v6] " Izzy via GitGitGadget
2 siblings, 0 replies; 27+ messages in thread
From: 唐宇奕 @ 2023-09-16 8:38 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Elijah Newren, Jeff King
Thank you for your advice! I've uploaded new patch.
On Sat, Sep 16, 2023 at 4:37 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-v5
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v5
> Pull-Request: https://github.com/gitgitgadget/git/pull/1565
>
> Range-diff vs v4:
>
> 1: d2d8fcc2e9b ! 1: 28d4282e0d8 merge-tree: add -X strategy option
> @@ Commit message
>
> ## builtin/merge-tree.c ##
> @@
> + #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 {
> @@ builtin/merge-tree.c: struct merge_tree_options {
> int show_messages;
> int name_only;
> @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
>
> opt.branch1 = branch1;
> @@ builtin/merge-tree.c: 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;
> @@ builtin/merge-tree.c: 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()
> };
>
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
> 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) {
> @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few c
> + git merge --abort &&
> +
> + git merge -X ours side4 &&
> -+ git rev-parse HEAD^{tree} > expected &&
> ++ git rev-parse HEAD^{tree} >expected &&
> +
> -+ git merge-tree -X ours side1 side4 > actual &&
> ++ git merge-tree -X ours side1 side4 >actual &&
> +
> + test_cmp expected actual
> +'
>
>
> builtin/merge-tree.c | 11 +++++++++++
> t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 0de42aecf4b..97d0fe6c952 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -18,6 +18,7 @@
> #include "quote.h"
> #include "tree.h"
> #include "config.h"
> +#include "strvec.h"
>
> static int line_termination = '\n';
>
> @@ -414,6 +415,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 +441,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;
> @@ -513,6 +517,7 @@ static int real_merge(struct merge_tree_options *o,
> 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;
> @@ -548,6 +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_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> + N_("option for selected merge strategy")),
> OPT_END()
> };
>
> @@ -556,6 +563,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.v[x]))
> + die(_("unknown strategy option: -X%s"), xopts.v[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..b2c8a43fce3 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5] merge-tree: add -X strategy option
2023-09-16 8:37 ` [PATCH v5] " Izzy via GitGitGadget
2023-09-16 8:38 ` 唐宇奕
@ 2023-09-18 9:53 ` Phillip Wood
2023-09-18 16:08 ` Junio C Hamano
2023-09-24 2:23 ` [PATCH v6] " Izzy via GitGitGadget
2 siblings, 1 reply; 27+ messages in thread
From: Phillip Wood @ 2023-09-18 9:53 UTC (permalink / raw)
To: Izzy via GitGitGadget, git; +Cc: Elijah Newren, Jeff King, Izzy
On 16/09/2023 09:37, Izzy via GitGitGadget wrote:
> From: Tang Yuyi <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically resolving conflicts.
I think adding a merge strategy option to merge-tree is a good idea, but
have you tested this with anything apart from -Xours or -Xtheirs? It
looks to me like those are the only two that this patch supports. If you
look at parse_merge_opt() in merge-recursive.c you will see that there
are many other options. In order to support all the merge options I
think this patch needs a bit of refactoring.
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 0de42aecf4b..97d0fe6c952 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> static int real_merge(struct merge_tree_options *o,
> @@ -439,6 +441,8 @@ static int real_merge(struct merge_tree_options *o,
>
> init_merge_options(&opt, the_repository);
>
> + opt.recursive_variant = o->merge_options.recursive_variant;
> +
Rather that copying across individual members I think you should
initialize o->merge_options properly in cmd_merge_tree() by calling
init_merge_options() and then use o->merge_options instead of "opt" in
this function. That way all the strategy options will be supported.
> opt.show_rename_progress = 0;
>
> opt.branch1 = branch1;
> @@ -513,6 +517,7 @@ static int real_merge(struct merge_tree_options *o,
> 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;
> @@ -548,6 +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_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> + N_("option for selected merge strategy")),
> OPT_END()
> };
>
> @@ -556,6 +563,10 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
You should add
init_merge_options(&o.merge_options);
here to ensure it is properly initialized.
> argc = parse_options(argc, argv, prefix, mt_options,
> merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
This is the right place to call parse_merge_opt() but I think we should
first check that the user has requested a real merge rather than a
trivial merge.
if (xopts.nr && o.mode == MODE_TRIVIAL)
die(_("--trivial-merge is incompatible with all other options"));
Otherwise if the user passes in invalid strategy option to a trivial
merge they'll get an error about an invalid strategy option rather than
being told --strategy-option is not supported with --trivial-merge.
Ideally there would be a preparatory patch that moves the switch
statement that is below the "if(o.use_stdin)" block up to this point so
we'd always have set o.mode before checking if it is a trivial merge. (I
think you'd to change the code slightly when it is moved to add a check
for o.use_stdin)
Best Wishes
Phillip
> + 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) {
> 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..b2c8a43fce3 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v5] merge-tree: add -X strategy option
2023-09-18 9:53 ` Phillip Wood
@ 2023-09-18 16:08 ` Junio C Hamano
0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2023-09-18 16:08 UTC (permalink / raw)
To: Phillip Wood; +Cc: Izzy via GitGitGadget, git, Elijah Newren, Jeff King, Izzy
Phillip Wood <phillip.wood123@gmail.com> writes:
> ...
> This is the right place to call parse_merge_opt() but I think we
> should first check that the user has requested a real merge rather
> than a trivial merge.
>
> if (xopts.nr && o.mode == MODE_TRIVIAL)
> die(_("--trivial-merge is incompatible with all other options"));
>
> Otherwise if the user passes in invalid strategy option to a trivial
> merge they'll get an error about an invalid strategy option rather
> than being told --strategy-option is not supported with
> --trivial-merge.
I presume that another issue with not having that compatibility
checking with "--trivial" would be when the user passes a valid
strategy option and it would be silently ignored.
> Ideally there would be a preparatory patch that moves the switch
> statement that is below the "if(o.use_stdin)" block up to this point
> so we'd always have set o.mode before checking if it is a trivial
> merge. (I think you'd to change the code slightly when it is moved to
> add a check for o.use_stdin)
That sounds very good.
Thanks for a good "stepping back a bit" review. Highly appreciated.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v6] merge-tree: add -X strategy option
2023-09-16 8:37 ` [PATCH v5] " Izzy via GitGitGadget
2023-09-16 8:38 ` 唐宇奕
2023-09-18 9:53 ` Phillip Wood
@ 2023-09-24 2:23 ` Izzy via GitGitGadget
2023-09-24 2:26 ` 唐宇奕
2023-10-09 9:58 ` Phillip Wood
2 siblings, 2 replies; 27+ messages in thread
From: Izzy via GitGitGadget @ 2023-09-24 2:23 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Jeff King, Phillip Wood, Izzy, Tang Yuyi
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-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1565
Range-diff vs v5:
1: 28d4282e0d8 ! 1: 6a3dd8aeb13 merge-tree: add -X strategy option
@@ builtin/merge-tree.c: struct merge_tree_options {
static int real_merge(struct merge_tree_options *o,
@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
+ {
+ struct commit *parent1, *parent2;
+ struct commit_list *merge_bases = NULL;
+- struct merge_options opt;
++ struct merge_options opt = o->merge_options;
+ struct merge_result result = { 0 };
+ int show_messages = o->show_messages;
- init_merge_options(&opt, the_repository);
+@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
+ help_unknown_ref(branch2, "merge-tree",
+ _("not something we can merge"));
-+ opt.recursive_variant = o->merge_options.recursive_variant;
-+
+- init_merge_options(&opt, the_repository);
+-
opt.show_rename_progress = 0;
opt.branch1 = branch1;
@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
OPT_END()
};
-@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
++ /* Init merge options */
++ init_merge_options(&o.merge_options, the_repository);
++
+ /* Parse arguments */
+ original_argc = argc - 1; /* ignoring argv[0] */
argc = parse_options(argc, argv, prefix, mt_options,
merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
++ if (xopts.nr && o.mode == MODE_TRIVIAL)
++ die(_("--trivial-merge is incompatible with all other options"));
+ 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]);
builtin/merge-tree.c | 18 +++++++++++++++---
t/t4301-merge-tree-write-tree.sh | 23 +++++++++++++++++++++++
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 0de42aecf4b..7024b5ce2e4 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,6 +18,7 @@
#include "quote.h"
#include "tree.h"
#include "config.h"
+#include "strvec.h"
static int line_termination = '\n';
@@ -414,6 +415,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,
@@ -423,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
{
struct commit *parent1, *parent2;
struct commit_list *merge_bases = NULL;
- struct merge_options opt;
+ struct merge_options opt = o->merge_options;
struct merge_result result = { 0 };
int show_messages = o->show_messages;
@@ -437,8 +439,6 @@ static int real_merge(struct merge_tree_options *o,
help_unknown_ref(branch2, "merge-tree",
_("not something we can merge"));
- init_merge_options(&opt, the_repository);
-
opt.show_rename_progress = 0;
opt.branch1 = branch1;
@@ -513,6 +513,7 @@ static int real_merge(struct merge_tree_options *o,
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;
@@ -548,14 +549,25 @@ 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_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+ N_("option for selected merge strategy")),
OPT_END()
};
+ /* Init merge options */
+ init_merge_options(&o.merge_options, the_repository);
+
/* Parse arguments */
original_argc = argc - 1; /* ignoring argv[0] */
argc = parse_options(argc, argv, prefix, mt_options,
merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+ if (xopts.nr && o.mode == MODE_TRIVIAL)
+ die(_("--trivial-merge is incompatible with all other options"));
+ 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) {
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..b2c8a43fce3 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
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v6] merge-tree: add -X strategy option
2023-09-24 2:23 ` [PATCH v6] " Izzy via GitGitGadget
@ 2023-09-24 2:26 ` 唐宇奕
2023-10-09 9:58 ` Phillip Wood
1 sibling, 0 replies; 27+ messages in thread
From: 唐宇奕 @ 2023-09-24 2:26 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Elijah Newren, Jeff King, Phillip Wood
Thanks for your advice! I've uploaded a new patch to support more
strategy options and the parse option issue.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v6] merge-tree: add -X strategy option
2023-09-24 2:23 ` [PATCH v6] " Izzy via GitGitGadget
2023-09-24 2:26 ` 唐宇奕
@ 2023-10-09 9:58 ` Phillip Wood
2023-10-09 15:53 ` Jeff King
1 sibling, 1 reply; 27+ messages in thread
From: Phillip Wood @ 2023-10-09 9:58 UTC (permalink / raw)
To: Izzy via GitGitGadget, git; +Cc: Elijah Newren, Jeff King, Izzy
On 24/09/2023 03:23, Izzy via GitGitGadget wrote:
Thanks for updating the patch, sorry it has taken me a while to look at
this.
> 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>
> ---
> static int real_merge(struct merge_tree_options *o,
> @@ -423,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
> {
> struct commit *parent1, *parent2;
> struct commit_list *merge_bases = NULL;
> - struct merge_options opt;
> + struct merge_options opt = o->merge_options;
Copying struct merge_options by value here is unusual in our code base.
I don't think it introduces a bug (there is no function to free the
resources allocated in struct merge_options so we do not end up freeing
them twice for example) but it would be clearer that it was safe if you did
struct merge_options *opt = &o->merge_options;
and updated the code to reflect that opt is now a pointer or just
replaced all uses of "opt" with "o->merge_options"
> + if (xopts.nr && o.mode == MODE_TRIVIAL)
> + die(_("--trivial-merge is incompatible with all other options"));
This is good, thanks for adding it.
> + for (int x = 0; x < xopts.nr; x++)
This is not worth a re-roll on its own but as xopts.nr is a size_t we
should declare x to be the same type rather than an int.
The tests are pretty minimal, ideally we would check that "-X" works
with "--stdin", that it is rejected by a trivial merge and that one of
the other strategy options works.
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v6] merge-tree: add -X strategy option
2023-10-09 9:58 ` Phillip Wood
@ 2023-10-09 15:53 ` Jeff King
2023-10-09 17:10 ` Junio C Hamano
0 siblings, 1 reply; 27+ messages in thread
From: Jeff King @ 2023-10-09 15:53 UTC (permalink / raw)
To: phillip.wood; +Cc: Izzy via GitGitGadget, git, Elijah Newren, Izzy
On Mon, Oct 09, 2023 at 10:58:07AM +0100, Phillip Wood wrote:
> > @@ -423,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
> > {
> > struct commit *parent1, *parent2;
> > struct commit_list *merge_bases = NULL;
> > - struct merge_options opt;
> > + struct merge_options opt = o->merge_options;
>
> Copying struct merge_options by value here is unusual in our code base. I
> don't think it introduces a bug (there is no function to free the resources
> allocated in struct merge_options so we do not end up freeing them twice for
> example) but it would be clearer that it was safe if you did
>
> struct merge_options *opt = &o->merge_options;
>
> and updated the code to reflect that opt is now a pointer or just replaced
> all uses of "opt" with "o->merge_options"
I agree that struct-copying is an unusual pattern, and we'd potentially
run into problems with duplication. But I think it is even trickier than
that here. We also go on to actually _modify_ opt in this function,
assigning to various members (both directly, and I think the merge code
itself will write to opt->priv).
So if we use a pointer (rather than struct assignment), those changes
will persist in the merge_options struct that was passed in. Which is
also weird.
Between the two, I think using a pointer is probably the least-weird.
This real_merge() function is only called once, and is a static-local
helper for cmd_merge_tree(). So the two functions work as a single unit,
and munging "opt" is not a big deal.
-Peff
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v6] merge-tree: add -X strategy option
2023-10-09 15:53 ` Jeff King
@ 2023-10-09 17:10 ` Junio C Hamano
2023-10-09 18:52 ` Jeff King
0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2023-10-09 17:10 UTC (permalink / raw)
To: Jeff King; +Cc: phillip.wood, Izzy via GitGitGadget, git, Elijah Newren, Izzy
Jeff King <peff@peff.net> writes:
> I agree that struct-copying is an unusual pattern, and we'd potentially
> run into problems with duplication. But I think it is even trickier than
> that here. We also go on to actually _modify_ opt in this function,
> assigning to various members (both directly, and I think the merge code
> itself will write to opt->priv).
>
> So if we use a pointer (rather than struct assignment), those changes
> will persist in the merge_options struct that was passed in. Which is
> also weird.
>
> Between the two, I think using a pointer is probably the least-weird.
> This real_merge() function is only called once, and is a static-local
> helper for cmd_merge_tree(). So the two functions work as a single unit,
> and munging "opt" is not a big deal.
It is called once per --stdin input to perform many merges in a row.
The most obvious "structure to pointer to structure" conversion
below seems to break an assertion (which is not very surprising, as
it happens inside that --stdin loop), so I am tempted to revert the
whole thing for now.
Thanks.
git: merge-ort.c:5110: merge_incore_recursive: Assertion `opt->ancestor == NULL' failed.
./test-lib.sh: line 1067: 738791 Done printf "c1 c3\nc2 -- c1 c3\nc2 c3"
738792 Aborted | git -C repo merge-tree --stdin > actual
builtin/merge-tree.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git c/builtin/merge-tree.c w/builtin/merge-tree.c
index 7024b5ce2e..1cb1fba2de 100644
--- c/builtin/merge-tree.c
+++ w/builtin/merge-tree.c
@@ -425,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
{
struct commit *parent1, *parent2;
struct commit_list *merge_bases = NULL;
- struct merge_options opt = o->merge_options;
+ struct merge_options *opt = &o->merge_options;
struct merge_result result = { 0 };
int show_messages = o->show_messages;
@@ -439,10 +439,10 @@ static int real_merge(struct merge_tree_options *o,
help_unknown_ref(branch2, "merge-tree",
_("not something we can merge"));
- opt.show_rename_progress = 0;
+ opt->show_rename_progress = 0;
- opt.branch1 = branch1;
- opt.branch2 = branch2;
+ opt->branch1 = branch1;
+ opt->branch2 = branch2;
if (merge_base) {
struct commit *base_commit;
@@ -452,11 +452,11 @@ static int real_merge(struct merge_tree_options *o,
if (!base_commit)
die(_("could not lookup commit '%s'"), merge_base);
- opt.ancestor = merge_base;
+ opt->ancestor = merge_base;
base_tree = repo_get_commit_tree(the_repository, base_commit);
parent1_tree = repo_get_commit_tree(the_repository, parent1);
parent2_tree = repo_get_commit_tree(the_repository, parent2);
- merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
+ merge_incore_nonrecursive(opt, base_tree, parent1_tree, parent2_tree, &result);
} else {
/*
* Get the merge bases, in reverse order; see comment above
@@ -467,7 +467,7 @@ static int real_merge(struct merge_tree_options *o,
if (!merge_bases && !o->allow_unrelated_histories)
die(_("refusing to merge unrelated histories"));
merge_bases = reverse_commit_list(merge_bases);
- merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+ merge_incore_recursive(opt, merge_bases, parent1, parent2, &result);
}
if (result.clean < 0)
@@ -501,12 +501,12 @@ static int real_merge(struct merge_tree_options *o,
}
if (show_messages) {
putchar(line_termination);
- merge_display_update_messages(&opt, line_termination == '\0',
+ merge_display_update_messages(opt, line_termination == '\0',
&result);
}
if (o->use_stdin)
putchar(line_termination);
- merge_finalize(&opt, &result);
+ merge_finalize(opt, &result);
return !result.clean; /* result.clean < 0 handled above */
}
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v6] merge-tree: add -X strategy option
2023-10-09 17:10 ` Junio C Hamano
@ 2023-10-09 18:52 ` Jeff King
2023-10-11 19:38 ` Junio C Hamano
0 siblings, 1 reply; 27+ messages in thread
From: Jeff King @ 2023-10-09 18:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: phillip.wood, Izzy via GitGitGadget, git, Elijah Newren, Izzy
On Mon, Oct 09, 2023 at 10:10:28AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I agree that struct-copying is an unusual pattern, and we'd potentially
> > run into problems with duplication. But I think it is even trickier than
> > that here. We also go on to actually _modify_ opt in this function,
> > assigning to various members (both directly, and I think the merge code
> > itself will write to opt->priv).
> >
> > So if we use a pointer (rather than struct assignment), those changes
> > will persist in the merge_options struct that was passed in. Which is
> > also weird.
> >
> > Between the two, I think using a pointer is probably the least-weird.
> > This real_merge() function is only called once, and is a static-local
> > helper for cmd_merge_tree(). So the two functions work as a single unit,
> > and munging "opt" is not a big deal.
>
> It is called once per --stdin input to perform many merges in a row.
> The most obvious "structure to pointer to structure" conversion
> below seems to break an assertion (which is not very surprising, as
> it happens inside that --stdin loop), so I am tempted to revert the
> whole thing for now.
Oops, I totally missed the loop around the call to real_merge(). So
yeah, I think this is rather tricky.
Before Izzy's patch, real_merge() always makes its own fresh
merge_options. After, we have a template merge_options that we copy, but
we are assuming that a shallow struct copy is OK (probably true, but an
anti-pattern that may bite us later). If we add Phillip's suggestion on
top, then we do not copy at all, and end up reusing the same options
struct (which is definitely wrong).
I don't think there are any bugs with the state at the current tip of
ty/merge-tree-strategy-options, but if we want to make it safer, I think
we have two options:
- delay the conversion of the "xopts" list into merge_options until we
initialize it in real_merge(). This avoids breaking abstraction
boundaries, but does mean that the sanity-checking of the options
happens a little later (but not much in practice).
- provide a copy_merge_options() function, which makes this kind of
"set up a template and then copy it" pattern official. It can be a
struct assignment for now, but it at least alerts anybody adding new
options to the notion that a deep copy might be required.
Option 1 looks something like this (a lot of the hunks are just
reverting the tip of that branch, so squashed in it would be even
smaller):
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 7024b5ce2e..f9dbbdb867 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -415,7 +415,7 @@ struct merge_tree_options {
int show_messages;
int name_only;
int use_stdin;
- struct merge_options merge_options;
+ struct strvec xopts;
};
static int real_merge(struct merge_tree_options *o,
@@ -425,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
{
struct commit *parent1, *parent2;
struct commit_list *merge_bases = NULL;
- struct merge_options opt = o->merge_options;
+ struct merge_options opt;
struct merge_result result = { 0 };
int show_messages = o->show_messages;
@@ -439,11 +439,17 @@ static int real_merge(struct merge_tree_options *o,
help_unknown_ref(branch2, "merge-tree",
_("not something we can merge"));
+ init_merge_options(&opt, the_repository);
+
opt.show_rename_progress = 0;
opt.branch1 = branch1;
opt.branch2 = branch2;
+ for (size_t x = 0; x < o->xopts.nr; x++)
+ if (parse_merge_opt(&opt, o->xopts.v[x]))
+ die(_("unknown strategy option: -X%s"), o->xopts.v[x]);
+
if (merge_base) {
struct commit *base_commit;
struct tree *base_tree, *parent1_tree, *parent2_tree;
@@ -512,8 +518,7 @@ static int real_merge(struct merge_tree_options *o,
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;
+ struct merge_tree_options o = { .show_messages = -1,.xopts = STRVEC_INIT };
int expected_remaining_argc;
int original_argc;
const char *merge_base = NULL;
@@ -549,24 +554,18 @@ 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_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+ OPT_STRVEC('X', "strategy-option", &o.xopts, N_("option=value"),
N_("option for selected merge strategy")),
OPT_END()
};
- /* Init merge options */
- init_merge_options(&o.merge_options, the_repository);
-
/* Parse arguments */
original_argc = argc - 1; /* ignoring argv[0] */
argc = parse_options(argc, argv, prefix, mt_options,
merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
- if (xopts.nr && o.mode == MODE_TRIVIAL)
+ if (o.xopts.nr && o.mode == MODE_TRIVIAL)
die(_("--trivial-merge is incompatible with all other options"));
- 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) {
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v6] merge-tree: add -X strategy option
2023-10-09 18:52 ` Jeff King
@ 2023-10-11 19:38 ` Junio C Hamano
2023-10-11 21:43 ` Jeff King
0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2023-10-11 19:38 UTC (permalink / raw)
To: Jeff King; +Cc: phillip.wood, Izzy via GitGitGadget, git, Elijah Newren, Izzy
Jeff King <peff@peff.net> writes:
> Oops, I totally missed the loop around the call to real_merge(). So
> yeah, I think this is rather tricky.
> ...
> I don't think there are any bugs with the state at the current tip of
> ty/merge-tree-strategy-options, but if we want to make it safer, I think
> we have two options:
>
> - delay the conversion of the "xopts" list into merge_options until we
> initialize it in real_merge(). This avoids breaking abstraction
> boundaries, but does mean that the sanity-checking of the options
> happens a little later (but not much in practice).
>
> - provide a copy_merge_options() function, which makes this kind of
> "set up a template and then copy it" pattern official. It can be a
> struct assignment for now, but it at least alerts anybody adding new
> options to the notion that a deep copy might be required.
>
> Option 1 looks something like this (a lot of the hunks are just
> reverting the tip of that branch, so squashed in it would be even
> smaller):
If we have no plan and intention to extend "merge-tree" even more in
the future, then option 1 would be the approach with least patch
noise, and as your "something like this" shows, it is a nice and
clean solution. I very much like it.
But as the renovated "merge-tree" is a relatively young thing in our
toolbox, I suspect that more and more work may want to go into it.
And the other "official copy_merge_options()" approach would be a
more healthy solution in the longer run, I would think. If we were
to go that route, we should also give an interface to free the
resources held by the copy.
It is not that much code on top of the commit that is already queued
in 'next', I suspect. Perhaps something like this?
builtin/merge-tree.c | 4 +++-
merge-recursive.c | 16 ++++++++++++++++
merge-recursive.h | 3 +++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git c/builtin/merge-tree.c w/builtin/merge-tree.c
index 7024b5ce2e..a35e0452d6 100644
--- c/builtin/merge-tree.c
+++ w/builtin/merge-tree.c
@@ -425,10 +425,11 @@ static int real_merge(struct merge_tree_options *o,
{
struct commit *parent1, *parent2;
struct commit_list *merge_bases = NULL;
- struct merge_options opt = o->merge_options;
struct merge_result result = { 0 };
int show_messages = o->show_messages;
+ struct merge_options opt;
+ copy_merge_options(&opt, &o->merge_options);
parent1 = get_merge_parent(branch1);
if (!parent1)
help_unknown_ref(branch1, "merge-tree",
@@ -507,6 +508,7 @@ static int real_merge(struct merge_tree_options *o,
if (o->use_stdin)
putchar(line_termination);
merge_finalize(&opt, &result);
+ clear_merge_options(&opt);
return !result.clean; /* result.clean < 0 handled above */
}
diff --git c/merge-recursive.c w/merge-recursive.c
index 0d7e57e2df..e3beb0801b 100644
--- c/merge-recursive.c
+++ w/merge-recursive.c
@@ -3912,6 +3912,22 @@ void init_merge_options(struct merge_options *opt,
opt->buffer_output = 0;
}
+/*
+ * For now, members of merge_options do not need deep copying, but
+ * it may change in the future, in which case we would need to update
+ * this, and also make a matching change to clear_merge_options() to
+ * release the resources held by a copied instance.
+ */
+void copy_merge_options(struct merge_options *dst, struct merge_options *src)
+{
+ *dst = *src;
+}
+
+void clear_merge_options(struct merge_options *opt UNUSED)
+{
+ ; /* no-op as our copy is shallow right now */
+}
+
int parse_merge_opt(struct merge_options *opt, const char *s)
{
const char *arg;
diff --git c/merge-recursive.h w/merge-recursive.h
index b88000e3c2..3d3b3e3c29 100644
--- c/merge-recursive.h
+++ w/merge-recursive.h
@@ -55,6 +55,9 @@ struct merge_options {
void init_merge_options(struct merge_options *opt, struct repository *repo);
+void copy_merge_options(struct merge_options *dst, struct merge_options *src);
+void clear_merge_options(struct merge_options *opt);
+
/* parse the option in s and update the relevant field of opt */
int parse_merge_opt(struct merge_options *opt, const char *s);
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v6] merge-tree: add -X strategy option
2023-10-11 19:38 ` Junio C Hamano
@ 2023-10-11 21:43 ` Jeff King
2023-10-11 22:19 ` Junio C Hamano
0 siblings, 1 reply; 27+ messages in thread
From: Jeff King @ 2023-10-11 21:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: phillip.wood, Izzy via GitGitGadget, git, Elijah Newren, Izzy
On Wed, Oct 11, 2023 at 12:38:03PM -0700, Junio C Hamano wrote:
> If we have no plan and intention to extend "merge-tree" even more in
> the future, then option 1 would be the approach with least patch
> noise, and as your "something like this" shows, it is a nice and
> clean solution. I very much like it.
>
> But as the renovated "merge-tree" is a relatively young thing in our
> toolbox, I suspect that more and more work may want to go into it.
> And the other "official copy_merge_options()" approach would be a
> more healthy solution in the longer run, I would think. If we were
> to go that route, we should also give an interface to free the
> resources held by the copy.
I am happy with either, as they both resolve the "merge-tree knows
intimate details about merge_options" issue. The patch I showed would
require manually passing more details down to real_merge(), which is I
guess what you are getting at with the "more work may want to go into
it".
> It is not that much code on top of the commit that is already queued
> in 'next', I suspect. Perhaps something like this?
This looks OK, though...
> +void clear_merge_options(struct merge_options *opt UNUSED)
> +{
> + ; /* no-op as our copy is shallow right now */
> +}
Clearing is generally not just about copies, but any use of the struct.
so this invites the question of whether the original non-copy struct
should have a call to clear_merge_options() in cmd_merge_tree(). And
ditto for every other user.
I do not mind adding such calls, but of course we know that they are
currently noops (and we don't have any particular plan to change that).
-Peff
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v6] merge-tree: add -X strategy option
2023-10-11 21:43 ` Jeff King
@ 2023-10-11 22:19 ` Junio C Hamano
0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2023-10-11 22:19 UTC (permalink / raw)
To: Jeff King; +Cc: phillip.wood, Izzy via GitGitGadget, git, Elijah Newren, Izzy
Jeff King <peff@peff.net> writes:
> I am happy with either, as they both resolve the "merge-tree knows
> intimate details about merge_options" issue. The patch I showed would
> require manually passing more details down to real_merge(), which is I
> guess what you are getting at with the "more work may want to go into
> it".
I was alluding more about teaching "merge-tree" various optional
behaviour merge_options represents. In today's patch it may be
-X<options>, who knows what tomorrow's patch wants to bring
"merge-tree" to feature-parity with "merge". And the first approach
would mean we would add xopts today to the struct, but we will be
required passing more details as we add other things.
>> It is not that much code on top of the commit that is already queued
>> in 'next', I suspect. Perhaps something like this?
>
> This looks OK, though...
>
>> +void clear_merge_options(struct merge_options *opt UNUSED)
>> +{
>> + ; /* no-op as our copy is shallow right now */
>> +}
>
> Clearing is generally not just about copies, but any use of the struct.
> so this invites the question of whether the original non-copy struct
> should have a call to clear_merge_options() in cmd_merge_tree(). And
> ditto for every other user.
Yes, once we start leaking, somebody hopefully notice the lack of a
call to this on the original/template copy and add one. Until then...
^ permalink raw reply [flat|nested] 27+ messages in thread