* [PATCH] merge-tree: add -X strategy option
@ 2023-08-05 14:24 Izzy via GitGitGadget
2023-08-07 2:10 ` Junio C Hamano
2023-08-12 5:33 ` [PATCH v2] " Izzy via GitGitGadget
0 siblings, 2 replies; 18+ messages in thread
From: Izzy via GitGitGadget @ 2023-08-05 14:24 UTC (permalink / raw)
To: git; +Cc: Izzy, winglovet
From: winglovet <winglovet@gmail.com>
Add merge strategy option to produce more customizable merge result such
as automatically solve conflicts.
Signed-off-by: winglovet <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-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1565
builtin/merge-tree.c | 24 ++++++++++++++++++++++++
1 file changed, 24 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;
base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
--
gitgitgadget
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] merge-tree: add -X strategy option
2023-08-05 14:24 [PATCH] merge-tree: add -X strategy option Izzy via GitGitGadget
@ 2023-08-07 2:10 ` Junio C Hamano
2023-08-12 5:33 ` [PATCH v2] " Izzy via GitGitGadget
1 sibling, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2023-08-07 2:10 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Izzy, Elijah Newren
"Izzy via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: winglovet <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically solve conflicts.
>
> Signed-off-by: winglovet <winglovet@gmail.com>
cf. Documentation/SubmittingPatches::[real-name], a part of the DCO
section of the document.
> builtin/merge-tree.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
A new feature should be protected by new tests to make sure it will
not be broken accidentally by others. Probably a couple of new
tests to both t4300 and t4301?
$ git shortlog -sn --no-merges --since=6.months builtin/merge-tree.c t/t430[01]*.sh
tells me that Elijah and Ævar had been active, but by looking at the
output of
$ git log --author=Ævar --since=6.months builtin/merge-tree.c t/t430[01]*.sh
shows that the contribution by the latter was solely about code
clean-up with Coccinelle and not about code features and correctness,
so for a new-feature change like this, I'd ask comments by Elijah if
I were writing this patch.
Thanks.
> 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;
>
> base-commit: ac83bc5054c2ac489166072334b4147ce6d0fccb
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] merge-tree: add -X strategy option
2023-08-05 14:24 [PATCH] merge-tree: add -X strategy option Izzy via GitGitGadget
2023-08-07 2:10 ` Junio C Hamano
@ 2023-08-12 5:33 ` Izzy via GitGitGadget
2023-08-12 5:41 ` 唐宇奕
` (2 more replies)
1 sibling, 3 replies; 18+ messages in thread
From: Izzy via GitGitGadget @ 2023-08-12 5:33 UTC (permalink / raw)
To: git; +Cc: Izzy, winglovet
From: winglovet <winglovet@gmail.com>
Add merge strategy option to produce more customizable merge result such
as automatically solve conflicts.
Signed-off-by: winglovet <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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1565
Range-diff vs v1:
1: b843caed596 ! 1: 7d53d08381e merge-tree: add -X strategy option
@@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
/* Handle --stdin */
if (o.use_stdin) {
struct strbuf buf = STRBUF_INIT;
+
+ ## t/t4301-merge-tree-write-tree.sh ##
+@@ t/t4301-merge-tree-write-tree.sh: 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 &&
+@@ t/t4301-merge-tree-write-tree.sh: 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 &&
+@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few conflicts' '
+ test_cmp expect actual
+ '
+
++test_expect_success 'Auto resolve conflicts by "ours" stragety 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 &&
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..2718817628c 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" stragety 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] 18+ messages in thread
* Re: [PATCH v2] merge-tree: add -X strategy option
2023-08-12 5:33 ` [PATCH v2] " Izzy via GitGitGadget
@ 2023-08-12 5:41 ` 唐宇奕
2023-09-03 1:31 ` 唐宇奕
2023-09-12 15:03 ` Elijah Newren
2023-09-16 2:14 ` [PATCH v3] " Izzy via GitGitGadget
2 siblings, 1 reply; 18+ messages in thread
From: 唐宇奕 @ 2023-08-12 5:41 UTC (permalink / raw)
To: Izzy via GitGitGadget, Junio C Hamano; +Cc: git, newren
Thanks for your advice, I have uploaded a new patch including tests.
On Sat, Aug 12, 2023 at 1:33 PM Izzy via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: winglovet <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically solve conflicts.
>
> Signed-off-by: winglovet <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-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1565
>
> Range-diff vs v1:
>
> 1: b843caed596 ! 1: 7d53d08381e merge-tree: add -X strategy option
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
> /* Handle --stdin */
> if (o.use_stdin) {
> struct strbuf buf = STRBUF_INIT;
> +
> + ## t/t4301-merge-tree-write-tree.sh ##
> +@@ t/t4301-merge-tree-write-tree.sh: 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 &&
> +@@ t/t4301-merge-tree-write-tree.sh: 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 &&
> +@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few conflicts' '
> + test_cmp expect actual
> + '
> +
> ++test_expect_success 'Auto resolve conflicts by "ours" stragety 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 &&
>
>
> 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..2718817628c 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" stragety 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] 18+ messages in thread
* Re: [PATCH v2] merge-tree: add -X strategy option
2023-08-12 5:41 ` 唐宇奕
@ 2023-09-03 1:31 ` 唐宇奕
0 siblings, 0 replies; 18+ messages in thread
From: 唐宇奕 @ 2023-09-03 1:31 UTC (permalink / raw)
To: Izzy via GitGitGadget, Junio C Hamano; +Cc: git, newren
Hello, is there any problem with this patch?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] merge-tree: add -X strategy option
2023-08-12 5:33 ` [PATCH v2] " Izzy via GitGitGadget
2023-08-12 5:41 ` 唐宇奕
@ 2023-09-12 15:03 ` Elijah Newren
2023-09-16 2:14 ` [PATCH v3] " Izzy via GitGitGadget
2 siblings, 0 replies; 18+ messages in thread
From: Elijah Newren @ 2023-09-12 15:03 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, winglovet
Hi,
Sorry for the delay in responding; I've been busy with non-git things...
On Sat, Aug 12, 2023 at 12:19 AM Izzy via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: winglovet <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically solve conflicts.
>
> Signed-off-by: winglovet <winglovet@gmail.com>
Junio already flagged this line in your v1; you either need to correct
it or respond to his comments about it.
> ---
> 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-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1565/WingT/merge_tree_allow_strategy_option-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1565
>
> Range-diff vs v1:
>
> 1: b843caed596 ! 1: 7d53d08381e merge-tree: add -X strategy option
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char
> /* Handle --stdin */
> if (o.use_stdin) {
> struct strbuf buf = STRBUF_INIT;
> +
> + ## t/t4301-merge-tree-write-tree.sh ##
> +@@ t/t4301-merge-tree-write-tree.sh: 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 &&
> +@@ t/t4301-merge-tree-write-tree.sh: 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 &&
> +@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Content merge and a few conflicts' '
> + test_cmp expect actual
> + '
> +
> ++test_expect_success 'Auto resolve conflicts by "ours" stragety 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 &&
>
>
> 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;
I know we already have lots of globals, and I'm partially to blame
since I also copied this style from elsewhere into this file, but it'd
sure be nice to get rid of these needless globals rather than add
more. But, certainly not a blocker....
> 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;
> };
Probably the best we can do now, but feels a little icky to me that
parse_merge_opt() works on all of struct merge_options rather than
having a dedicated struct for the particular options it can set. Made
sense back in the merge-recursive.[ch] days, but it provides too many
extra unrelated and unnecessary fields now.
This isn't something you need to fix in this patch, though, just
something else I'll keep in mind when I get back to working on
removing merge-recursive.[ch].
> 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..2718817628c 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" stragety option' '
stragety -> strategy
> + 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 &&
Style: '>filename', not '> filename'.
> +
> + git merge-tree -X ours side1 side4 > actual &&
Same style issue with redirect.
Also, why is the leading spacing inconsistent with the surrounding lines?
> +
> + 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 &&
I personally don't see the value in passing `-X ours` or `-X theirs`
to merges. I've never really been a fan of either. Granted, I
understand the value in making merge-tree support the same things that
existing merges support, so I'm fine with supporting it, but I'd
rather have a useful example for the testcase such as
-Xignore-space-change or something (or maybe use it as an additional
example?) Not a blocker on its own, but just a personal preference.
Anyway, the general direction of the patch seems reasonable. Not
everything I commented on needs to be fixed, as I noted, but there are
a few small things to fix up.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3] merge-tree: add -X strategy option
2023-08-12 5:33 ` [PATCH v2] " Izzy via GitGitGadget
2023-08-12 5:41 ` 唐宇奕
2023-09-12 15:03 ` Elijah Newren
@ 2023-09-16 2:14 ` Izzy via GitGitGadget
2023-09-16 2:26 ` 唐宇奕
` (2 more replies)
2 siblings, 3 replies; 18+ messages in thread
From: Izzy via GitGitGadget @ 2023-09-16 2:14 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Izzy, Tang Yuyi
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3] merge-tree: add -X strategy option
2023-09-16 2:14 ` [PATCH v3] " Izzy via GitGitGadget
@ 2023-09-16 2:26 ` 唐宇奕
2023-09-16 3:21 ` Elijah Newren
2023-09-16 3:16 ` Elijah Newren
2023-09-16 3:47 ` [PATCH v4] " Izzy via GitGitGadget
2 siblings, 1 reply; 18+ messages in thread
From: 唐宇奕 @ 2023-09-16 2:26 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: git, Elijah Newren
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3] merge-tree: add -X strategy option
2023-09-16 2:14 ` [PATCH v3] " Izzy via GitGitGadget
2023-09-16 2:26 ` 唐宇奕
@ 2023-09-16 3:16 ` Elijah Newren
2023-09-16 3:47 ` [PATCH v4] " Izzy via GitGitGadget
2 siblings, 0 replies; 18+ messages in thread
From: Elijah Newren @ 2023-09-16 3:16 UTC (permalink / raw)
To: Izzy via GitGitGadget; +Cc: Git Mailing List, Tang Yuyi
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3] merge-tree: add -X strategy option
2023-09-16 2:26 ` 唐宇奕
@ 2023-09-16 3:21 ` Elijah Newren
0 siblings, 0 replies; 18+ messages in thread
From: Elijah Newren @ 2023-09-16 3:21 UTC (permalink / raw)
To: 唐宇奕; +Cc: Izzy via GitGitGadget, git
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?
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4] merge-tree: add -X strategy option
2023-09-16 2:14 ` [PATCH v3] " Izzy via GitGitGadget
2023-09-16 2:26 ` 唐宇奕
2023-09-16 3:16 ` Elijah Newren
@ 2023-09-16 3:47 ` Izzy via GitGitGadget
2023-09-16 3:55 ` Elijah Newren
` (3 more replies)
2 siblings, 4 replies; 18+ messages in thread
From: Izzy via GitGitGadget @ 2023-09-16 3:47 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, 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-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 related [flat|nested] 18+ 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 ` 唐宇奕
` (2 subsequent siblings)
3 siblings, 0 replies; 18+ 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] 18+ 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; 18+ 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] 18+ 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; 18+ 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] 18+ 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 ` 唐宇奕
2023-09-18 9:53 ` Phillip Wood
3 siblings, 2 replies; 18+ 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] 18+ 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
1 sibling, 0 replies; 18+ 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] 18+ 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
1 sibling, 1 reply; 18+ 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] 18+ 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; 18+ 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] 18+ messages in thread
end of thread, other threads:[~2023-09-18 16:10 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-05 14:24 [PATCH] merge-tree: add -X strategy option Izzy via GitGitGadget
2023-08-07 2:10 ` Junio C Hamano
2023-08-12 5:33 ` [PATCH v2] " Izzy via GitGitGadget
2023-08-12 5:41 ` 唐宇奕
2023-09-03 1:31 ` 唐宇奕
2023-09-12 15:03 ` Elijah Newren
2023-09-16 2:14 ` [PATCH v3] " Izzy via GitGitGadget
2023-09-16 2:26 ` 唐宇奕
2023-09-16 3:21 ` Elijah Newren
2023-09-16 3:16 ` Elijah Newren
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
2023-09-16 8:38 ` 唐宇奕
2023-09-18 9:53 ` Phillip Wood
2023-09-18 16:08 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/pub/scm/git/git.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).