[PATCH 0/4] checkout: cleanup --conflict=

[PATCH 0/4] checkout: cleanup --conflict=

[Phillip Wood via GitGitGadget]

Phillip Wood (4):
  xdiff-interface: refactor parsing of merge.conflictstyle
  merge-ll: introduce LL_MERGE_OPTIONS_INIT
  merge options: add a conflict style member
  checkout: cleanup --conflict=<style> parsing

 builtin/checkout.c | 40 +++++++++++++++++++++-------------------
 merge-ll.c         |  6 ++++--
 merge-ll.h         |  5 +++++
 merge-ort.c        |  3 ++-
 merge-recursive.c  |  5 ++++-
 merge-recursive.h  |  1 +
 t/t7201-co.sh      |  6 ++++++
 xdiff-interface.c  | 29 ++++++++++++++++++-----------
 xdiff-interface.h  |  1 +
 9 files changed, 62 insertions(+), 34 deletions(-)


base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1684
-- 
gitgitgadget

[PATCH 1/4] xdiff-interface: refactor parsing of merge.conflictstyle

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Factor out the code that parses of conflict style name so it can be
reused in a later commit that wants to parse the name given on the
command line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 xdiff-interface.c | 29 ++++++++++++++++++-----------
 xdiff-interface.h |  1 +
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index 3162f517434..daee3c584e1 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -305,6 +305,22 @@ int xdiff_compare_lines(const char *l1, long s1,
 	return xdl_recmatch(l1, s1, l2, s2, flags);
 }
 
+int parse_conflict_style(const char *value)
+{
+	if (!strcmp(value, "diff3"))
+		return XDL_MERGE_DIFF3;
+	else if (!strcmp(value, "zdiff3"))
+		return XDL_MERGE_ZEALOUS_DIFF3;
+	else if (!strcmp(value, "merge"))
+		return 0;
+	/*
+	 * Please update _git_checkout() in git-completion.bash when
+	 * you add new merge config
+	 */
+	else
+		return -1;
+}
+
 int git_xmerge_style = -1;
 
 int git_xmerge_config(const char *var, const char *value,
@@ -313,17 +329,8 @@ int git_xmerge_config(const char *var, const char *value,
 	if (!strcmp(var, "merge.conflictstyle")) {
 		if (!value)
 			return config_error_nonbool(var);
-		if (!strcmp(value, "diff3"))
-			git_xmerge_style = XDL_MERGE_DIFF3;
-		else if (!strcmp(value, "zdiff3"))
-			git_xmerge_style = XDL_MERGE_ZEALOUS_DIFF3;
-		else if (!strcmp(value, "merge"))
-			git_xmerge_style = 0;
-		/*
-		 * Please update _git_checkout() in
-		 * git-completion.bash when you add new merge config
-		 */
-		else
+		git_xmerge_style = parse_conflict_style(value);
+		if (git_xmerge_style == -1)
 			return error(_("unknown style '%s' given for '%s'"),
 				     value, var);
 		return 0;
diff --git a/xdiff-interface.h b/xdiff-interface.h
index e6f80df0462..c569b7c5203 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -51,6 +51,7 @@ int buffer_is_binary(const char *ptr, unsigned long size);
 void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);
 void xdiff_clear_find_func(xdemitconf_t *xecfg);
 struct config_context;
+int parse_conflict_style(const char *value);
 int git_xmerge_config(const char *var, const char *value,
 		      const struct config_context *ctx, void *cb);
 extern int git_xmerge_style;
-- 
gitgitgadget

[PATCH 2/4] merge-ll: introduce LL_MERGE_OPTIONS_INIT

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Introduce a macro to initialize `struct ll_merge_options` in preparation
for the next commit that will add a new member that needs to be
initialized to a non-zero value.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/checkout.c | 3 +--
 merge-ll.c         | 2 +-
 merge-ll.h         | 2 ++
 merge-ort.c        | 2 +-
 merge-recursive.c  | 2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 067c2519334..6ded58bd95c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -262,7 +262,7 @@ static int checkout_merged(int pos, const struct checkout *state,
 	mmbuffer_t result_buf;
 	struct object_id threeway[3];
 	unsigned mode = 0;
-	struct ll_merge_options ll_opts;
+	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
 	int renormalize = 0;
 
 	memset(threeway, 0, sizeof(threeway));
@@ -284,7 +284,6 @@ static int checkout_merged(int pos, const struct checkout *state,
 	read_mmblob(&ours, &threeway[1]);
 	read_mmblob(&theirs, &threeway[2]);
 
-	memset(&ll_opts, 0, sizeof(ll_opts));
 	git_config_get_bool("merge.renormalize", &renormalize);
 	ll_opts.renormalize = renormalize;
 	merge_status = ll_merge(&result_buf, path, &ancestor, "base",
diff --git a/merge-ll.c b/merge-ll.c
index 61e0ae53981..6570707297d 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -401,7 +401,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
 	     const struct ll_merge_options *opts)
 {
 	struct attr_check *check = load_merge_attributes();
-	static const struct ll_merge_options default_opts;
+	static const struct ll_merge_options default_opts = LL_MERGE_OPTIONS_INIT;
 	const char *ll_driver_name = NULL;
 	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	const struct ll_merge_driver *driver;
diff --git a/merge-ll.h b/merge-ll.h
index e4a20e81a3a..af1ee36abdb 100644
--- a/merge-ll.h
+++ b/merge-ll.h
@@ -82,6 +82,8 @@ struct ll_merge_options {
 	long xdl_opts;
 };
 
+#define LL_MERGE_OPTIONS_INIT {0}
+
 enum ll_merge_result {
 	LL_MERGE_ERROR = -1,
 	LL_MERGE_OK = 0,
diff --git a/merge-ort.c b/merge-ort.c
index 8617babee41..4a02c3ecd99 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1956,7 +1956,7 @@ static int merge_3way(struct merge_options *opt,
 		      mmbuffer_t *result_buf)
 {
 	mmfile_t orig, src1, src2;
-	struct ll_merge_options ll_opts = {0};
+	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
 	char *base, *name1, *name2;
 	enum ll_merge_result merge_status;
 
diff --git a/merge-recursive.c b/merge-recursive.c
index a0c3e7a2d91..02b7b584f95 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1047,7 +1047,7 @@ static int merge_3way(struct merge_options *opt,
 		      const int extra_marker_size)
 {
 	mmfile_t orig, src1, src2;
-	struct ll_merge_options ll_opts = {0};
+	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
 	char *base, *name1, *name2;
 	enum ll_merge_result merge_status;
 
-- 
gitgitgadget

[PATCH 3/4] merge options: add a conflict style member

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a conflict_style member to `struct merge_options` and `struct
ll_merge_options` to allow callers to override the default conflict
style. This will be used in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 merge-ll.c        | 4 +++-
 merge-ll.h        | 5 ++++-
 merge-ort.c       | 1 +
 merge-recursive.c | 3 +++
 merge-recursive.h | 1 +
 5 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 6570707297d..bf1077ae092 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse
 	xmp.level = XDL_MERGE_ZEALOUS;
 	xmp.favor = opts->variant;
 	xmp.xpp.flags = opts->xdl_opts;
-	if (git_xmerge_style >= 0)
+	if (opts->conflict_style >= 0)
+		xmp.style = opts->conflict_style;
+	else if (git_xmerge_style >= 0)
 		xmp.style = git_xmerge_style;
 	if (marker_size > 0)
 		xmp.marker_size = marker_size;
diff --git a/merge-ll.h b/merge-ll.h
index af1ee36abdb..d038ee0c1e8 100644
--- a/merge-ll.h
+++ b/merge-ll.h
@@ -78,11 +78,14 @@ struct ll_merge_options {
 	 */
 	unsigned extra_marker_size;
 
+	/* Override the global conflict style. */
+	int conflict_style;
+
 	/* Extra xpparam_t flags as defined in xdiff/xdiff.h. */
 	long xdl_opts;
 };
 
-#define LL_MERGE_OPTIONS_INIT {0}
+#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }
 
 enum ll_merge_result {
 	LL_MERGE_ERROR = -1,
diff --git a/merge-ort.c b/merge-ort.c
index 4a02c3ecd99..a9ab4031451 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1966,6 +1966,7 @@ static int merge_3way(struct merge_options *opt,
 	ll_opts.renormalize = opt->renormalize;
 	ll_opts.extra_marker_size = extra_marker_size;
 	ll_opts.xdl_opts = opt->xdl_opts;
+	ll_opts.conflict_style = opt->conflict_style;
 
 	if (opt->priv->call_depth) {
 		ll_opts.virtual_ancestor = 1;
diff --git a/merge-recursive.c b/merge-recursive.c
index 02b7b584f95..33b5f9384e8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1054,6 +1054,7 @@ static int merge_3way(struct merge_options *opt,
 	ll_opts.renormalize = opt->renormalize;
 	ll_opts.extra_marker_size = extra_marker_size;
 	ll_opts.xdl_opts = opt->xdl_opts;
+	ll_opts.conflict_style = opt->conflict_style;
 
 	if (opt->priv->call_depth) {
 		ll_opts.virtual_ancestor = 1;
@@ -3899,6 +3900,8 @@ void init_merge_options(struct merge_options *opt,
 
 	opt->renormalize = 0;
 
+	opt->conflict_style = -1;
+
 	merge_recursive_config(opt);
 	merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
 	if (merge_verbosity)
diff --git a/merge-recursive.h b/merge-recursive.h
index 3d3b3e3c295..e67d38c3030 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -31,6 +31,7 @@ struct merge_options {
 
 	/* xdiff-related options (patience, ignore whitespace, ours/theirs) */
 	long xdl_opts;
+	int conflict_style;
 	enum {
 		MERGE_VARIANT_NORMAL = 0,
 		MERGE_VARIANT_OURS,
-- 
gitgitgadget

[PATCH 4/4] checkout: cleanup --conflict=<style> parsing

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Passing an invalid conflict style name such as "--conflict=bad" gives
the error message

    error: unknown style 'bad' given for 'merge.conflictstyle'

which is unfortunate as it talks about a config setting rather than
the option given on the command line. This happens because the
implementation calls git_xmerge_config() to set the conflict style
using the value given on the command line. Use the newly added
parse_conflict_style() instead and pass the value down the call chain
to override the config setting. This also means we can avoid setting
up a struct config_context required for calling git_xmerge_config().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/checkout.c | 37 ++++++++++++++++++++-----------------
 t/t7201-co.sh      |  6 ++++++
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6ded58bd95c..f5055f059ad 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -91,7 +91,8 @@ struct checkout_opts {
 	int new_branch_log;
 	enum branch_track track;
 	struct diff_options diff_options;
-	char *conflict_style;
+	char *conflict_style_name;
+	int conflict_style;
 
 	int branch_exists;
 	const char *prefix;
@@ -100,6 +101,8 @@ struct checkout_opts {
 	struct tree *source_tree;
 };
 
+#define CHECKOUT_OPTS_INIT { .conflict_style = -1 }
+
 struct branch_info {
 	char *name; /* The short name used */
 	char *path; /* The full name of a real branch */
@@ -251,7 +254,8 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,
 }
 
 static int checkout_merged(int pos, const struct checkout *state,
-			   int *nr_checkouts, struct mem_pool *ce_mem_pool)
+			   int *nr_checkouts, struct mem_pool *ce_mem_pool,
+			   int conflict_style)
 {
 	struct cache_entry *ce = the_index.cache[pos];
 	const char *path = ce->name;
@@ -286,6 +290,7 @@ static int checkout_merged(int pos, const struct checkout *state,
 
 	git_config_get_bool("merge.renormalize", &renormalize);
 	ll_opts.renormalize = renormalize;
+	ll_opts.conflict_style = conflict_style;
 	merge_status = ll_merge(&result_buf, path, &ancestor, "base",
 				&ours, "ours", &theirs, "theirs",
 				state->istate, &ll_opts);
@@ -416,7 +421,8 @@ static int checkout_worktree(const struct checkout_opts *opts,
 			else if (opts->merge)
 				errs |= checkout_merged(pos, &state,
 							&nr_unmerged,
-							&ce_mem_pool);
+							&ce_mem_pool,
+							opts->conflict_style);
 			pos = skip_same_name(ce, pos) - 1;
 		}
 	}
@@ -886,6 +892,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			}
 			o.branch1 = new_branch_info->name;
 			o.branch2 = "local";
+			o.conflict_style = opts->conflict_style;
 			ret = merge_trees(&o,
 					  new_tree,
 					  work,
@@ -1628,7 +1635,7 @@ static struct option *add_common_options(struct checkout_opts *opts,
 			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
 		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
 		OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")),
-		OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
+		OPT_STRING(0, "conflict", &opts->conflict_style_name, N_("style"),
 			   N_("conflict style (merge, diff3, or zdiff3)")),
 		OPT_END()
 	};
@@ -1720,14 +1727,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			opts->show_progress = isatty(2);
 	}
 
-	if (opts->conflict_style) {
-		struct key_value_info kvi = KVI_INIT;
-		struct config_context ctx = {
-			.kvi = &kvi,
-		};
+	if (opts->conflict_style_name) {
 		opts->merge = 1; /* implied */
-		git_xmerge_config("merge.conflictstyle", opts->conflict_style,
-				  &ctx, NULL);
+		opts->conflict_style =
+			parse_conflict_style(opts->conflict_style_name);
+		if (opts->conflict_style < 0)
+			die(_("unknown conflict style '%s'"),
+			    opts->conflict_style_name);
 	}
 	if (opts->force) {
 		opts->discard_changes = 1;
@@ -1893,7 +1899,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
-	struct checkout_opts opts;
+	struct checkout_opts opts = CHECKOUT_OPTS_INIT;
 	struct option *options;
 	struct option checkout_options[] = {
 		OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
@@ -1909,7 +1915,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	int ret;
 	struct branch_info new_branch_info = { 0 };
 
-	memset(&opts, 0, sizeof(opts));
 	opts.dwim_new_local_branch = 1;
 	opts.switch_branch_doing_nothing_is_ok = 1;
 	opts.only_merge_on_switching_branches = 0;
@@ -1948,7 +1953,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 
 int cmd_switch(int argc, const char **argv, const char *prefix)
 {
-	struct checkout_opts opts;
+	struct checkout_opts opts = CHECKOUT_OPTS_INIT;
 	struct option *options = NULL;
 	struct option switch_options[] = {
 		OPT_STRING('c', "create", &opts.new_branch, N_("branch"),
@@ -1964,7 +1969,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
 	int ret;
 	struct branch_info new_branch_info = { 0 };
 
-	memset(&opts, 0, sizeof(opts));
 	opts.dwim_new_local_branch = 1;
 	opts.accept_ref = 1;
 	opts.accept_pathspec = 0;
@@ -1990,7 +1994,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
 
 int cmd_restore(int argc, const char **argv, const char *prefix)
 {
-	struct checkout_opts opts;
+	struct checkout_opts opts = CHECKOUT_OPTS_INIT;
 	struct option *options;
 	struct option restore_options[] = {
 		OPT_STRING('s', "source", &opts.from_treeish, "<tree-ish>",
@@ -2007,7 +2011,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
 	int ret;
 	struct branch_info new_branch_info = { 0 };
 
-	memset(&opts, 0, sizeof(opts));
 	opts.accept_ref = 0;
 	opts.accept_pathspec = 1;
 	opts.empty_pathspec_ok = 0;
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 10cc6c46051..5746d152b6d 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -631,6 +631,12 @@ test_expect_success 'checkout --conflict=diff3' '
 	test_cmp merged file
 '
 
+test_expect_success 'checkout with invalid conflict style' '
+	test_must_fail git checkout --conflict=bad 2>actual -- file &&
+	echo "fatal: unknown conflict style ${SQ}bad${SQ}" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'failing checkout -b should not break working tree' '
 	git clean -fd &&  # Remove untracked files in the way
 	git reset --hard main &&
-- 
gitgitgadget

Re: [PATCH 0/4] checkout: cleanup --conflict=

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Here is a place to say why this series exists.  Saying things like
"'checkout --conflict=bad' gives a wrong error message, as if the
inalid conflict style were given by a configuration variable, and
this is to fix that bug".

> Phillip Wood (4):
>   xdiff-interface: refactor parsing of merge.conflictstyle
>   merge-ll: introduce LL_MERGE_OPTIONS_INIT
>   merge options: add a conflict style member
>   checkout: cleanup --conflict=<style> parsing
>
>  builtin/checkout.c | 40 +++++++++++++++++++++-------------------
>  merge-ll.c         |  6 ++++--
>  merge-ll.h         |  5 +++++
>  merge-ort.c        |  3 ++-
>  merge-recursive.c  |  5 ++++-
>  merge-recursive.h  |  1 +
>  t/t7201-co.sh      |  6 ++++++
>  xdiff-interface.c  | 29 ++++++++++++++++++-----------
>  xdiff-interface.h  |  1 +
>  9 files changed, 62 insertions(+), 34 deletions(-)
>
>
> base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1684

Re: [PATCH 3/4] merge options: add a conflict style member

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/merge-ll.c b/merge-ll.c
> index 6570707297d..bf1077ae092 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> ..
> -#define LL_MERGE_OPTIONS_INIT {0}
> +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }

Makes sense, and this obviously makes the previous step worth doing.

It looks quite wrong that low-level merge options definition is
hosted in a file whose name is merge low-level.  Is it too late to
rename the file to fix this, by the way?

Thanks.

Re: [PATCH 0/4] checkout: cleanup --conflict=

[phillip.wood123]

Hi Junio

[cc Johannes for the gitgitgadget issue]

On 08/03/2024 15:44, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> Here is a place to say why this series exists.  Saying things like
> "'checkout --conflict=bad' gives a wrong error message, as if the
> inalid conflict style were given by a configuration variable, and
> this is to fix that bug".

Sorry, I'm not sure what happen there, I definitely entered a 
cover-letter on the gitgitgadget PR:


Passing an invalid conflict style name such as "--conflict=bad" to "git 
checkout" gives the error message

error: unknown style 'bad' given for 'merge.conflictstyle'

which is unfortunate as it talks about a config setting rather than the 
option given on the command line. This series refactors the 
implementation to pass the conflict style down the call chain to the 
merge machinery rather than abusing the config setting.


Best Wishes

Phillip

>> Phillip Wood (4):
>>    xdiff-interface: refactor parsing of merge.conflictstyle
>>    merge-ll: introduce LL_MERGE_OPTIONS_INIT
>>    merge options: add a conflict style member
>>    checkout: cleanup --conflict=<style> parsing
>>
>>   builtin/checkout.c | 40 +++++++++++++++++++++-------------------
>>   merge-ll.c         |  6 ++++--
>>   merge-ll.h         |  5 +++++
>>   merge-ort.c        |  3 ++-
>>   merge-recursive.c  |  5 ++++-
>>   merge-recursive.h  |  1 +
>>   t/t7201-co.sh      |  6 ++++++
>>   xdiff-interface.c  | 29 ++++++++++++++++++-----------
>>   xdiff-interface.h  |  1 +
>>   9 files changed, 62 insertions(+), 34 deletions(-)
>>
>>
>> base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1684

Re: [PATCH 3/4] merge options: add a conflict style member

[phillip.wood123]

Hi Junio

On 08/03/2024 15:46, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> diff --git a/merge-ll.c b/merge-ll.c
>> index 6570707297d..bf1077ae092 100644
>> --- a/merge-ll.c
>> +++ b/merge-ll.c
>> ..
>> -#define LL_MERGE_OPTIONS_INIT {0}
>> +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }
> 
> Makes sense, and this obviously makes the previous step worth doing.
> 
> It looks quite wrong that low-level merge options definition is
> hosted in a file whose name is merge low-level.  Is it too late to
> rename the file to fix this, by the way?

I agree it is confusing, Elijah renamed it from ll-merge.c relatively
recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It
looks like the idea was to group it with the other merge* files:

     merge-ll: rename from ll-merge
     
     A long term (but rather minor) pet-peeve of mine was the name
     ll-merge.[ch].  I thought it made it harder to realize what stuff was
     related to merging when I was working on the merge machinery and trying
     to improve it.
     
     Further, back in d1cbe1e6d8a ("hash-ll.h: split out of hash.h to remove
     dependency on repository.h", 2023-04-22), we have split the portions of
     hash.h that do not depend upon repository.h into a "hash-ll.h" (due to
     the recommendation to use "ll" for "low-level" in its name[1], but which
     I used as a suffix precisely because of my distaste for "ll-merge").
     When we discussed adding additional "*-ll.h" files, a request was made
     that we use "ll" consistently as either a prefix or a suffix.  Since it
     is already in use as both a prefix and a suffix, the only way to do so
     is to rename some files.
     
     Besides my distaste for the ll-merge.[ch] name, let me also note that
     the files
       ll-fsmonitor.h, ll-hash.h, ll-merge.h, ll-object-store.h, ll-read-cache.h
     would have essentially nothing to do with each other and make no sense
     to group.  But giving them the common "ll-" prefix would group them.  Using
     "-ll" as a suffix thus seems just much more logical to me.  Rename
     ll-merge.[ch] to merge-ll.[ch] to achieve this consistency, and to
     ensure we get a more logical grouping of files.
     
     [1] https://lore.kernel.org/git/kl6lsfcu1g8w.fsf@chooglen-macbookpro.roam.corp.google.com/


Best Wishes

Phillip

Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -91,7 +91,8 @@ struct checkout_opts {
>  	int new_branch_log;
>  	enum branch_track track;
>  	struct diff_options diff_options;
> -	char *conflict_style;
> +	char *conflict_style_name;
> +	int conflict_style;

Does the conflict_style_name need to be a member of this struct?

> @@ -1628,7 +1635,7 @@ static struct option *add_common_options(struct checkout_opts *opts,
>  			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
>  		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
>  		OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")),
> -		OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
> +		OPT_STRING(0, "conflict", &opts->conflict_style_name, N_("style"),
>  			   N_("conflict style (merge, diff3, or zdiff3)")),
>  		OPT_END()
>  	};

Ah, the options[] definition is not in the same scope as where the
parse_options() is called, and that is the reason why we need to
carry the extra member that we do not need after we are done with
parsing (we use "int conflict_style") in the struct.  Otherwise we
would have just received OPT_STRING() into a local variable, called
parse_options(), and post-processed the string into
opts->conflict_style.

Yucky.  I do not care much about wasted 8 bytes in the structure,
but I find it disturbing that those functions called later with this
struct has to know that conflict_style_name is a useless member and
they are supposed to use conflict_style exclusively.

We could use OPT_CALLBACK() to accept the incoming string, parse it
and store it in opts->conflict_style and that would be a way to
avoid the extra member.

> +		opts->conflict_style =
> +			parse_conflict_style(opts->conflict_style_name);

When I saw the change to xdiff-interface in an earlier step, I
thought parse_conflict_style() was a potentially confusing name.
You can imagine a function that is fed a file with conflict markers
and say "ah, this uses diff3 style with common ancestor version" vs
"this uses merge style with only two sides" to have such a name.

parse_conflict_style_name() that takes a name and returns
conflict_style enumeration constant would not risk such a confusion,
I guess.

Thanks.

Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing

[phillip.wood123]

Hi Junio

On 08/03/2024 16:15, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> We could use OPT_CALLBACK() to accept the incoming string, parse it
> and store it in opts->conflict_style and that would be a way to
> avoid the extra member.
> 
>> +		opts->conflict_style =
>> +			parse_conflict_style(opts->conflict_style_name);
> 
> When I saw the change to xdiff-interface in an earlier step, I
> thought parse_conflict_style() was a potentially confusing name.
> You can imagine a function that is fed a file with conflict markers
> and say "ah, this uses diff3 style with common ancestor version" vs
> "this uses merge style with only two sides" to have such a name.
> 
> parse_conflict_style_name() that takes a name and returns
> conflict_style enumeration constant would not risk such a confusion,
> I guess.

Those are both good suggestions - I'll re-roll next week

Thanks

Phillip

Re: [PATCH 3/4] merge options: add a conflict style member

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> I agree it is confusing, Elijah renamed it from ll-merge.c relatively
> recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It
> looks like the idea was to group it with the other merge* files:

That reasoning cuts both ways.  When you are only interested in the
upper level API functions, being able to skip ll-anything as "low
level details" is a powerful thing.  merge-ll & hash-ll separated
far apart will make it impossible.

Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> Hi Junio
>
> On 08/03/2024 16:15, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> We could use OPT_CALLBACK() to accept the incoming string, parse it
>> and store it in opts->conflict_style and that would be a way to
>> avoid the extra member.
>> 
>>> +		opts->conflict_style =
>>> +			parse_conflict_style(opts->conflict_style_name);
>> When I saw the change to xdiff-interface in an earlier step, I
>> thought parse_conflict_style() was a potentially confusing name.
>> You can imagine a function that is fed a file with conflict markers
>> and say "ah, this uses diff3 style with common ancestor version" vs
>> "this uses merge style with only two sides" to have such a name.
>> parse_conflict_style_name() that takes a name and returns
>> conflict_style enumeration constant would not risk such a confusion,
>> I guess.
>
> Those are both good suggestions - I'll re-roll next week

Thanks.

Re: [PATCH 3/4] merge options: add a conflict style member

[Elijah Newren]

On Fri, Mar 8, 2024 at 8:48 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> phillip.wood123@gmail.com writes:
>
> > I agree it is confusing, Elijah renamed it from ll-merge.c relatively
> > recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It
> > looks like the idea was to group it with the other merge* files:
>
> That reasoning cuts both ways.  When you are only interested in the
> upper level API functions, being able to skip ll-anything as "low
> level details" is a powerful thing.  merge-ll & hash-ll separated
> far apart will make it impossible.

merge-ll is wildly different than every other *-ll.h file we have in
the tree; the latter set of files may be misnamed, for reasons other
than what you are suggesting here.  hash-ll, hex-ll, etc. exist due to
the main include file having some rarely used API that require more
#include statements, and most users of e.g. hex functions can get away
with just including hex-ll.h rather than the full hex.h.  Thus,
hex-ll.h is _not_ "low level details that you can skip", but "the
_primary_ data structures and functions".  It doesn't get the name
hex.h, though, because if we did that then the folks that need both
primary parts of the API and the occasional additional function would
need to have two hex-related includes.  Also, every function declared
within hex-ll.h is still defined within hex.c; there is no separate
hex-ll.c file, and the same is true of all the other *-ll.h files
other than merge-ll.h.  As such, there is absolutely no relation
between hex-ll.h, hash-ll.h, fsmonitor-ll.h, etc.  Using an ll- prefix
on those filenames thus makes no sense to me.  (It's not clear that
-ll even makes sense as a suffix for these files either, but it's not
clear what else to use instead.  If I recall correctly I originally
put forward "-basics" as a possible name suffix for these files, but
someone else suggested "-ll", and not having any better ideas or
strong opinions I just went with it.)

merge-ll is different in that it is actually a separate level of the
API, and there are both a merge-ll.h file and a merge-ll.c file.

I originally had proposed only adding the hex-ll.h, hash-ll.h,
fsmonitor-ll.h, etc. files, but you suggested that ll-merge should
either be renamed or that these new files should be
(https://lore.kernel.org/git/CABPp-BErrVUnuDjL73edDpmkKUvs6Ny6cYwvueXw1toB4JcF-Q@mail.gmail.com/).


Now, all that said, and assuming we were to go back to a world where
the other *-ll.h files don't exist (or are renamed independently with
a different suffix), I'm still not understanding why ll-merge makes
more sense to you and Phillip than merge-ll.  Could you explain more?
If you're only interested in the upper-level API functions, that
suggests you are already at the function level and looking within a
given file.  The low-level functions are already split out into a
separate file, so you just don't go looking at that separate file.
However, if you're interested in "where in this codebase do I find the
stuff related to merging", then you aren't in a file but in a
directory listing.  The first usecase gains no benefit from renaming
these files back to ll-merge.[ch], while the other usecase would have
been much improved in my experience from being named merge-ll.[ch].
Granted, it's a really minor point, which was why I never brought it
up until you suggested making all the other *-ll.h files and
ll-merge.[ch] consistent; since renaming the other *-ll.h files made
no sense at all to me, I went with renaming ll-merge.[ch] to
merge-ll.[ch].

There's probably some other angle to this that you two are viewing
this from that just isn't apparent to me.  Sorry for not seeing it
(yet).  Hopefully the above context is at least helpful, though.

Re: [PATCH 3/4] merge options: add a conflict style member

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> a different suffix), I'm still not understanding why ll-merge makes
> more sense to you and Phillip than merge-ll.
> Could you explain more?

Language and grammar?  In other words, "low-level merge routines" is
an understandable phrase, while "merge lo-level routines" is not.

Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing

[phillip.wood123]

Hi Junio

On 08/03/2024 16:15, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> parse_conflict_style_name() that takes a name and returns
> conflict_style enumeration constant would not risk such a confusion,
> I guess.

Can I just check if you mean that we should convert XDL_MERGE_DIFF3 and 
friends to be an enum, or are you happy to leave them as pre-processor 
constants and have parse_conflict_style_name() return an int? I don't 
mind changing them to be an enum but I'm not sure it buys any type 
safety as C is happy to implicitly convert enums and integers. It would 
also raise the question of whether we should be converting the merge 
flavor and diff algorithm constants to enums as well.

Thanks

Phillip

Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> On 08/03/2024 16:15, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> parse_conflict_style_name() that takes a name and returns
>> conflict_style enumeration constant would not risk such a confusion,
>> I guess.
>
> Can I just check if you mean that we should convert XDL_MERGE_DIFF3
> and friends to be an enum, or are you happy to leave them as
> pre-processor constants and have parse_conflict_style_name() return an
> int?

The latter.  I was only wondering what the good name for the new
function was and did not mean to imply that we want conversions from
C preprocessor macros to enums.

Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing

[Phillip Wood]

On 11/03/2024 17:41, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>> On 08/03/2024 16:15, Junio C Hamano wrote:
>>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>> parse_conflict_style_name() that takes a name and returns
>>> conflict_style enumeration constant would not risk such a confusion,
>>> I guess.
>>
>> Can I just check if you mean that we should convert XDL_MERGE_DIFF3
>> and friends to be an enum, or are you happy to leave them as
>> pre-processor constants and have parse_conflict_style_name() return an
>> int?
> 
> The latter.  I was only wondering what the good name for the new
> function was and did not mean to imply that we want conversions from
> C preprocessor macros to enums.

Thanks

Phillip

[PATCH v2 0/5] checkout: cleanup --conflict=

[Phillip Wood via GitGitGadget]

Phillip Wood (5):
  xdiff-interface: refactor parsing of merge.conflictstyle
  merge-ll: introduce LL_MERGE_OPTIONS_INIT
  merge options: add a conflict style member
  checkout: cleanup --conflict=<style> parsing
  checkout: fix interaction between --conflict and --merge

 builtin/checkout.c | 60 +++++++++++++++++++++++++----------------
 merge-ll.c         |  6 +++--
 merge-ll.h         |  5 ++++
 merge-ort.c        |  3 ++-
 merge-recursive.c  |  5 +++-
 merge-recursive.h  |  1 +
 t/t7201-co.sh      | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 xdiff-interface.c  | 29 ++++++++++++--------
 xdiff-interface.h  |  1 +
 9 files changed, 139 insertions(+), 37 deletions(-)


base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1684

Range-diff vs v1:

 1:  0263d001634 ! 1:  629e577879c xdiff-interface: refactor parsing of merge.conflictstyle
     @@ xdiff-interface.c: int xdiff_compare_lines(const char *l1, long s1,
       	return xdl_recmatch(l1, s1, l2, s2, flags);
       }
       
     -+int parse_conflict_style(const char *value)
     ++int parse_conflict_style_name(const char *value)
      +{
      +	if (!strcmp(value, "diff3"))
      +		return XDL_MERGE_DIFF3;
     @@ xdiff-interface.c: int git_xmerge_config(const char *var, const char *value,
      -		 * git-completion.bash when you add new merge config
      -		 */
      -		else
     -+		git_xmerge_style = parse_conflict_style(value);
     ++		git_xmerge_style = parse_conflict_style_name(value);
      +		if (git_xmerge_style == -1)
       			return error(_("unknown style '%s' given for '%s'"),
       				     value, var);
     @@ xdiff-interface.h: int buffer_is_binary(const char *ptr, unsigned long size);
       void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);
       void xdiff_clear_find_func(xdemitconf_t *xecfg);
       struct config_context;
     -+int parse_conflict_style(const char *value);
     ++int parse_conflict_style_name(const char *value);
       int git_xmerge_config(const char *var, const char *value,
       		      const struct config_context *ctx, void *cb);
       extern int git_xmerge_style;
 2:  4e05bc156bc = 2:  46e0f5a0af7 merge-ll: introduce LL_MERGE_OPTIONS_INIT
 3:  c0d7bafd438 = 3:  47d0ec28a55 merge options: add a conflict style member
 4:  317bb7a70d0 ! 4:  511e03d3db2 checkout: cleanup --conflict=<style> parsing
     @@ Commit message
          the option given on the command line. This happens because the
          implementation calls git_xmerge_config() to set the conflict style
          using the value given on the command line. Use the newly added
     -    parse_conflict_style() instead and pass the value down the call chain
     -    to override the config setting. This also means we can avoid setting
     -    up a struct config_context required for calling git_xmerge_config().
     +    parse_conflict_style_name() instead and pass the value down the call
     +    chain to override the config setting. This also means we can avoid
     +    setting up a struct config_context required for calling
     +    git_xmerge_config().
     +
     +    The option is now parsed in a callback to avoid having to store the
     +    option name. This is a change in behavior as now
     +
     +        git checkout --conflict=bad --conflict=diff3
     +
     +    will error out when parsing "--conflict=bad" whereas before this change
     +    it would succeed because it would only try to parse the value of the
     +    last "--conflict" option given on the command line.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ builtin/checkout.c: struct checkout_opts {
       	enum branch_track track;
       	struct diff_options diff_options;
      -	char *conflict_style;
     -+	char *conflict_style_name;
      +	int conflict_style;
       
       	int branch_exists;
     @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op
       			ret = merge_trees(&o,
       					  new_tree,
       					  work,
     +@@ builtin/checkout.c: static int checkout_branch(struct checkout_opts *opts,
     + 	return switch_branches(opts, new_branch_info);
     + }
     + 
     ++static int parse_opt_conflict(const struct option *o, const char *arg, int unset)
     ++{
     ++	struct checkout_opts *opts = o->value;
     ++
     ++	if (unset) {
     ++		opts->conflict_style = -1;
     ++		return 0;
     ++	}
     ++	opts->conflict_style = parse_conflict_style_name(arg);
     ++	if (opts->conflict_style < 0)
     ++		return error(_("unknown conflict style '%s'"), arg);
     ++
     ++	return 0;
     ++}
     ++
     + static struct option *add_common_options(struct checkout_opts *opts,
     + 					 struct option *prevopts)
     + {
      @@ builtin/checkout.c: static struct option *add_common_options(struct checkout_opts *opts,
       			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
       		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
       		OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")),
      -		OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
     -+		OPT_STRING(0, "conflict", &opts->conflict_style_name, N_("style"),
     - 			   N_("conflict style (merge, diff3, or zdiff3)")),
     +-			   N_("conflict style (merge, diff3, or zdiff3)")),
     ++		OPT_CALLBACK(0, "conflict", opts, N_("style"),
     ++			     N_("conflict style (merge, diff3, or zdiff3)"),
     ++			     parse_opt_conflict),
       		OPT_END()
       	};
     + 	struct option *newopts = parse_options_concat(prevopts, options);
      @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,
       			opts->show_progress = isatty(2);
       	}
     @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const
      -		struct config_context ctx = {
      -			.kvi = &kvi,
      -		};
     -+	if (opts->conflict_style_name) {
     ++	if (opts->conflict_style >= 0)
       		opts->merge = 1; /* implied */
      -		git_xmerge_config("merge.conflictstyle", opts->conflict_style,
      -				  &ctx, NULL);
     -+		opts->conflict_style =
     -+			parse_conflict_style(opts->conflict_style_name);
     -+		if (opts->conflict_style < 0)
     -+			die(_("unknown conflict style '%s'"),
     -+			    opts->conflict_style_name);
     - 	}
     +-	}
     ++
       	if (opts->force) {
       		opts->discard_changes = 1;
     + 		opts->ignore_unmerged_opt = "--force";
      @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,
       
       int cmd_checkout(int argc, const char **argv, const char *prefix)
     @@ t/t7201-co.sh: test_expect_success 'checkout --conflict=diff3' '
       
      +test_expect_success 'checkout with invalid conflict style' '
      +	test_must_fail git checkout --conflict=bad 2>actual -- file &&
     -+	echo "fatal: unknown conflict style ${SQ}bad${SQ}" >expect &&
     ++	echo "error: unknown conflict style ${SQ}bad${SQ}" >expect &&
      +	test_cmp expect actual
      +'
      +
 -:  ----------- > 5:  b771b29e45a checkout: fix interaction between --conflict and --merge

-- 
gitgitgadget

[PATCH v2 1/5] xdiff-interface: refactor parsing of merge.conflictstyle

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Factor out the code that parses of conflict style name so it can be
reused in a later commit that wants to parse the name given on the
command line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 xdiff-interface.c | 29 ++++++++++++++++++-----------
 xdiff-interface.h |  1 +
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index 3162f517434..16ed8ac4928 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -305,6 +305,22 @@ int xdiff_compare_lines(const char *l1, long s1,
 	return xdl_recmatch(l1, s1, l2, s2, flags);
 }
 
+int parse_conflict_style_name(const char *value)
+{
+	if (!strcmp(value, "diff3"))
+		return XDL_MERGE_DIFF3;
+	else if (!strcmp(value, "zdiff3"))
+		return XDL_MERGE_ZEALOUS_DIFF3;
+	else if (!strcmp(value, "merge"))
+		return 0;
+	/*
+	 * Please update _git_checkout() in git-completion.bash when
+	 * you add new merge config
+	 */
+	else
+		return -1;
+}
+
 int git_xmerge_style = -1;
 
 int git_xmerge_config(const char *var, const char *value,
@@ -313,17 +329,8 @@ int git_xmerge_config(const char *var, const char *value,
 	if (!strcmp(var, "merge.conflictstyle")) {
 		if (!value)
 			return config_error_nonbool(var);
-		if (!strcmp(value, "diff3"))
-			git_xmerge_style = XDL_MERGE_DIFF3;
-		else if (!strcmp(value, "zdiff3"))
-			git_xmerge_style = XDL_MERGE_ZEALOUS_DIFF3;
-		else if (!strcmp(value, "merge"))
-			git_xmerge_style = 0;
-		/*
-		 * Please update _git_checkout() in
-		 * git-completion.bash when you add new merge config
-		 */
-		else
+		git_xmerge_style = parse_conflict_style_name(value);
+		if (git_xmerge_style == -1)
 			return error(_("unknown style '%s' given for '%s'"),
 				     value, var);
 		return 0;
diff --git a/xdiff-interface.h b/xdiff-interface.h
index e6f80df0462..38537169b72 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -51,6 +51,7 @@ int buffer_is_binary(const char *ptr, unsigned long size);
 void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);
 void xdiff_clear_find_func(xdemitconf_t *xecfg);
 struct config_context;
+int parse_conflict_style_name(const char *value);
 int git_xmerge_config(const char *var, const char *value,
 		      const struct config_context *ctx, void *cb);
 extern int git_xmerge_style;
-- 
gitgitgadget

[PATCH v2 2/5] merge-ll: introduce LL_MERGE_OPTIONS_INIT

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Introduce a macro to initialize `struct ll_merge_options` in preparation
for the next commit that will add a new member that needs to be
initialized to a non-zero value.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/checkout.c | 3 +--
 merge-ll.c         | 2 +-
 merge-ll.h         | 2 ++
 merge-ort.c        | 2 +-
 merge-recursive.c  | 2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 067c2519334..6ded58bd95c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -262,7 +262,7 @@ static int checkout_merged(int pos, const struct checkout *state,
 	mmbuffer_t result_buf;
 	struct object_id threeway[3];
 	unsigned mode = 0;
-	struct ll_merge_options ll_opts;
+	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
 	int renormalize = 0;
 
 	memset(threeway, 0, sizeof(threeway));
@@ -284,7 +284,6 @@ static int checkout_merged(int pos, const struct checkout *state,
 	read_mmblob(&ours, &threeway[1]);
 	read_mmblob(&theirs, &threeway[2]);
 
-	memset(&ll_opts, 0, sizeof(ll_opts));
 	git_config_get_bool("merge.renormalize", &renormalize);
 	ll_opts.renormalize = renormalize;
 	merge_status = ll_merge(&result_buf, path, &ancestor, "base",
diff --git a/merge-ll.c b/merge-ll.c
index 61e0ae53981..6570707297d 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -401,7 +401,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
 	     const struct ll_merge_options *opts)
 {
 	struct attr_check *check = load_merge_attributes();
-	static const struct ll_merge_options default_opts;
+	static const struct ll_merge_options default_opts = LL_MERGE_OPTIONS_INIT;
 	const char *ll_driver_name = NULL;
 	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	const struct ll_merge_driver *driver;
diff --git a/merge-ll.h b/merge-ll.h
index e4a20e81a3a..af1ee36abdb 100644
--- a/merge-ll.h
+++ b/merge-ll.h
@@ -82,6 +82,8 @@ struct ll_merge_options {
 	long xdl_opts;
 };
 
+#define LL_MERGE_OPTIONS_INIT {0}
+
 enum ll_merge_result {
 	LL_MERGE_ERROR = -1,
 	LL_MERGE_OK = 0,
diff --git a/merge-ort.c b/merge-ort.c
index 8617babee41..4a02c3ecd99 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1956,7 +1956,7 @@ static int merge_3way(struct merge_options *opt,
 		      mmbuffer_t *result_buf)
 {
 	mmfile_t orig, src1, src2;
-	struct ll_merge_options ll_opts = {0};
+	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
 	char *base, *name1, *name2;
 	enum ll_merge_result merge_status;
 
diff --git a/merge-recursive.c b/merge-recursive.c
index a0c3e7a2d91..02b7b584f95 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1047,7 +1047,7 @@ static int merge_3way(struct merge_options *opt,
 		      const int extra_marker_size)
 {
 	mmfile_t orig, src1, src2;
-	struct ll_merge_options ll_opts = {0};
+	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
 	char *base, *name1, *name2;
 	enum ll_merge_result merge_status;
 
-- 
gitgitgadget

[PATCH v2 3/5] merge options: add a conflict style member

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a conflict_style member to `struct merge_options` and `struct
ll_merge_options` to allow callers to override the default conflict
style. This will be used in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 merge-ll.c        | 4 +++-
 merge-ll.h        | 5 ++++-
 merge-ort.c       | 1 +
 merge-recursive.c | 3 +++
 merge-recursive.h | 1 +
 5 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 6570707297d..bf1077ae092 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse
 	xmp.level = XDL_MERGE_ZEALOUS;
 	xmp.favor = opts->variant;
 	xmp.xpp.flags = opts->xdl_opts;
-	if (git_xmerge_style >= 0)
+	if (opts->conflict_style >= 0)
+		xmp.style = opts->conflict_style;
+	else if (git_xmerge_style >= 0)
 		xmp.style = git_xmerge_style;
 	if (marker_size > 0)
 		xmp.marker_size = marker_size;
diff --git a/merge-ll.h b/merge-ll.h
index af1ee36abdb..d038ee0c1e8 100644
--- a/merge-ll.h
+++ b/merge-ll.h
@@ -78,11 +78,14 @@ struct ll_merge_options {
 	 */
 	unsigned extra_marker_size;
 
+	/* Override the global conflict style. */
+	int conflict_style;
+
 	/* Extra xpparam_t flags as defined in xdiff/xdiff.h. */
 	long xdl_opts;
 };
 
-#define LL_MERGE_OPTIONS_INIT {0}
+#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }
 
 enum ll_merge_result {
 	LL_MERGE_ERROR = -1,
diff --git a/merge-ort.c b/merge-ort.c
index 4a02c3ecd99..a9ab4031451 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1966,6 +1966,7 @@ static int merge_3way(struct merge_options *opt,
 	ll_opts.renormalize = opt->renormalize;
 	ll_opts.extra_marker_size = extra_marker_size;
 	ll_opts.xdl_opts = opt->xdl_opts;
+	ll_opts.conflict_style = opt->conflict_style;
 
 	if (opt->priv->call_depth) {
 		ll_opts.virtual_ancestor = 1;
diff --git a/merge-recursive.c b/merge-recursive.c
index 02b7b584f95..33b5f9384e8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1054,6 +1054,7 @@ static int merge_3way(struct merge_options *opt,
 	ll_opts.renormalize = opt->renormalize;
 	ll_opts.extra_marker_size = extra_marker_size;
 	ll_opts.xdl_opts = opt->xdl_opts;
+	ll_opts.conflict_style = opt->conflict_style;
 
 	if (opt->priv->call_depth) {
 		ll_opts.virtual_ancestor = 1;
@@ -3899,6 +3900,8 @@ void init_merge_options(struct merge_options *opt,
 
 	opt->renormalize = 0;
 
+	opt->conflict_style = -1;
+
 	merge_recursive_config(opt);
 	merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
 	if (merge_verbosity)
diff --git a/merge-recursive.h b/merge-recursive.h
index 3d3b3e3c295..e67d38c3030 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -31,6 +31,7 @@ struct merge_options {
 
 	/* xdiff-related options (patience, ignore whitespace, ours/theirs) */
 	long xdl_opts;
+	int conflict_style;
 	enum {
 		MERGE_VARIANT_NORMAL = 0,
 		MERGE_VARIANT_OURS,
-- 
gitgitgadget

[PATCH v2 4/5] checkout: cleanup --conflict=<style> parsing

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Passing an invalid conflict style name such as "--conflict=bad" gives
the error message

    error: unknown style 'bad' given for 'merge.conflictstyle'

which is unfortunate as it talks about a config setting rather than
the option given on the command line. This happens because the
implementation calls git_xmerge_config() to set the conflict style
using the value given on the command line. Use the newly added
parse_conflict_style_name() instead and pass the value down the call
chain to override the config setting. This also means we can avoid
setting up a struct config_context required for calling
git_xmerge_config().

The option is now parsed in a callback to avoid having to store the
option name. This is a change in behavior as now

    git checkout --conflict=bad --conflict=diff3

will error out when parsing "--conflict=bad" whereas before this change
it would succeed because it would only try to parse the value of the
last "--conflict" option given on the command line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/checkout.c | 51 +++++++++++++++++++++++++++++-----------------
 t/t7201-co.sh      |  6 ++++++
 2 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6ded58bd95c..d6ab3b1d665 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -91,7 +91,7 @@ struct checkout_opts {
 	int new_branch_log;
 	enum branch_track track;
 	struct diff_options diff_options;
-	char *conflict_style;
+	int conflict_style;
 
 	int branch_exists;
 	const char *prefix;
@@ -100,6 +100,8 @@ struct checkout_opts {
 	struct tree *source_tree;
 };
 
+#define CHECKOUT_OPTS_INIT { .conflict_style = -1 }
+
 struct branch_info {
 	char *name; /* The short name used */
 	char *path; /* The full name of a real branch */
@@ -251,7 +253,8 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,
 }
 
 static int checkout_merged(int pos, const struct checkout *state,
-			   int *nr_checkouts, struct mem_pool *ce_mem_pool)
+			   int *nr_checkouts, struct mem_pool *ce_mem_pool,
+			   int conflict_style)
 {
 	struct cache_entry *ce = the_index.cache[pos];
 	const char *path = ce->name;
@@ -286,6 +289,7 @@ static int checkout_merged(int pos, const struct checkout *state,
 
 	git_config_get_bool("merge.renormalize", &renormalize);
 	ll_opts.renormalize = renormalize;
+	ll_opts.conflict_style = conflict_style;
 	merge_status = ll_merge(&result_buf, path, &ancestor, "base",
 				&ours, "ours", &theirs, "theirs",
 				state->istate, &ll_opts);
@@ -416,7 +420,8 @@ static int checkout_worktree(const struct checkout_opts *opts,
 			else if (opts->merge)
 				errs |= checkout_merged(pos, &state,
 							&nr_unmerged,
-							&ce_mem_pool);
+							&ce_mem_pool,
+							opts->conflict_style);
 			pos = skip_same_name(ce, pos) - 1;
 		}
 	}
@@ -886,6 +891,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			}
 			o.branch1 = new_branch_info->name;
 			o.branch2 = "local";
+			o.conflict_style = opts->conflict_style;
 			ret = merge_trees(&o,
 					  new_tree,
 					  work,
@@ -1618,6 +1624,21 @@ static int checkout_branch(struct checkout_opts *opts,
 	return switch_branches(opts, new_branch_info);
 }
 
+static int parse_opt_conflict(const struct option *o, const char *arg, int unset)
+{
+	struct checkout_opts *opts = o->value;
+
+	if (unset) {
+		opts->conflict_style = -1;
+		return 0;
+	}
+	opts->conflict_style = parse_conflict_style_name(arg);
+	if (opts->conflict_style < 0)
+		return error(_("unknown conflict style '%s'"), arg);
+
+	return 0;
+}
+
 static struct option *add_common_options(struct checkout_opts *opts,
 					 struct option *prevopts)
 {
@@ -1628,8 +1649,9 @@ static struct option *add_common_options(struct checkout_opts *opts,
 			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
 		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
 		OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")),
-		OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
-			   N_("conflict style (merge, diff3, or zdiff3)")),
+		OPT_CALLBACK(0, "conflict", opts, N_("style"),
+			     N_("conflict style (merge, diff3, or zdiff3)"),
+			     parse_opt_conflict),
 		OPT_END()
 	};
 	struct option *newopts = parse_options_concat(prevopts, options);
@@ -1720,15 +1742,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			opts->show_progress = isatty(2);
 	}
 
-	if (opts->conflict_style) {
-		struct key_value_info kvi = KVI_INIT;
-		struct config_context ctx = {
-			.kvi = &kvi,
-		};
+	if (opts->conflict_style >= 0)
 		opts->merge = 1; /* implied */
-		git_xmerge_config("merge.conflictstyle", opts->conflict_style,
-				  &ctx, NULL);
-	}
+
 	if (opts->force) {
 		opts->discard_changes = 1;
 		opts->ignore_unmerged_opt = "--force";
@@ -1893,7 +1909,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
-	struct checkout_opts opts;
+	struct checkout_opts opts = CHECKOUT_OPTS_INIT;
 	struct option *options;
 	struct option checkout_options[] = {
 		OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
@@ -1909,7 +1925,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	int ret;
 	struct branch_info new_branch_info = { 0 };
 
-	memset(&opts, 0, sizeof(opts));
 	opts.dwim_new_local_branch = 1;
 	opts.switch_branch_doing_nothing_is_ok = 1;
 	opts.only_merge_on_switching_branches = 0;
@@ -1948,7 +1963,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 
 int cmd_switch(int argc, const char **argv, const char *prefix)
 {
-	struct checkout_opts opts;
+	struct checkout_opts opts = CHECKOUT_OPTS_INIT;
 	struct option *options = NULL;
 	struct option switch_options[] = {
 		OPT_STRING('c', "create", &opts.new_branch, N_("branch"),
@@ -1964,7 +1979,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
 	int ret;
 	struct branch_info new_branch_info = { 0 };
 
-	memset(&opts, 0, sizeof(opts));
 	opts.dwim_new_local_branch = 1;
 	opts.accept_ref = 1;
 	opts.accept_pathspec = 0;
@@ -1990,7 +2004,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
 
 int cmd_restore(int argc, const char **argv, const char *prefix)
 {
-	struct checkout_opts opts;
+	struct checkout_opts opts = CHECKOUT_OPTS_INIT;
 	struct option *options;
 	struct option restore_options[] = {
 		OPT_STRING('s', "source", &opts.from_treeish, "<tree-ish>",
@@ -2007,7 +2021,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
 	int ret;
 	struct branch_info new_branch_info = { 0 };
 
-	memset(&opts, 0, sizeof(opts));
 	opts.accept_ref = 0;
 	opts.accept_pathspec = 1;
 	opts.empty_pathspec_ok = 0;
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 10cc6c46051..e1f85a91565 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -631,6 +631,12 @@ test_expect_success 'checkout --conflict=diff3' '
 	test_cmp merged file
 '
 
+test_expect_success 'checkout with invalid conflict style' '
+	test_must_fail git checkout --conflict=bad 2>actual -- file &&
+	echo "error: unknown conflict style ${SQ}bad${SQ}" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'failing checkout -b should not break working tree' '
 	git clean -fd &&  # Remove untracked files in the way
 	git reset --hard main &&
-- 
gitgitgadget

[PATCH v2 5/5] checkout: fix interaction between --conflict and --merge

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When using "git checkout" to recreate merge conflicts or merge
uncommitted changes when switching branch "--conflict" sensibly implies
"--merge". Unfortunately the way this is implemented means that "git
checkout --conflict=diff3 --no-merge" implies "--merge" violating the
usual last-one-wins rule. Fix this by only overriding the value of
opts->merge if "--conflicts" comes after "--no-merge" or "-[-no]-merge"
is not given on the command line.

The behavior of "git checkout --merge --no-conflict" is unchanged and
will still merge on the basis that the "-[-no]-conflict" options are
primarily intended to affect the conflict style and so "--no-conflict"
should cancel a previous "--conflict" but not override "--merge".

Of the four new tests the second one tests the behavior change
introduced by this commit, the other three check that this commit does
not regress the existing behavior.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/checkout.c | 10 +++++---
 t/t7201-co.sh      | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d6ab3b1d665..b8ce1904897 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -100,7 +100,7 @@ struct checkout_opts {
 	struct tree *source_tree;
 };
 
-#define CHECKOUT_OPTS_INIT { .conflict_style = -1 }
+#define CHECKOUT_OPTS_INIT { .conflict_style = -1, .merge = -1 }
 
 struct branch_info {
 	char *name; /* The short name used */
@@ -1635,6 +1635,9 @@ static int parse_opt_conflict(const struct option *o, const char *arg, int unset
 	opts->conflict_style = parse_conflict_style_name(arg);
 	if (opts->conflict_style < 0)
 		return error(_("unknown conflict style '%s'"), arg);
+	/* --conflict overrides a previous --no-merge */
+	if (!opts->merge)
+		opts->merge = -1;
 
 	return 0;
 }
@@ -1742,8 +1745,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			opts->show_progress = isatty(2);
 	}
 
-	if (opts->conflict_style >= 0)
-		opts->merge = 1; /* implied */
+	/* --conflicts implies --merge */
+	if (opts->merge == -1)
+		opts->merge = opts->conflict_style >= 0;
 
 	if (opts->force) {
 		opts->discard_changes = 1;
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index e1f85a91565..42352dc0dbe 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -631,6 +631,66 @@ test_expect_success 'checkout --conflict=diff3' '
 	test_cmp merged file
 '
 
+test_expect_success 'checkout --conflict=diff3 --no-conflict does not merge' '
+	setup_conflicting_index &&
+	echo "none of the above" >expect &&
+	cat expect >fild &&
+	cat expect >file &&
+	test_must_fail git checkout --conflict=diff3 --no-conflict -- fild file 2>err &&
+	test_cmp expect file &&
+	test_cmp expect fild &&
+	echo "error: path ${SQ}file${SQ} is unmerged" >expect &&
+	test_cmp expect err
+'
+
+test_expect_success 'checkout --conflict=diff3 --no-merge does not merge' '
+	setup_conflicting_index &&
+	echo "none of the above" >expect &&
+	cat expect >fild &&
+	cat expect >file &&
+	test_must_fail git checkout --conflict=diff3 --no-merge -- fild file 2>err &&
+	test_cmp expect file &&
+	test_cmp expect fild &&
+	echo "error: path ${SQ}file${SQ} is unmerged" >expect &&
+	test_cmp expect err
+'
+
+test_expect_success 'checkout --no-merge --conflict=diff3 does merge' '
+	setup_conflicting_index &&
+	echo "none of the above" >fild &&
+	echo "none of the above" >file &&
+	git checkout --no-merge --conflict=diff3 -- fild file &&
+	echo "ourside" >expect &&
+	test_cmp expect fild &&
+	cat >expect <<-\EOF &&
+	<<<<<<< ours
+	ourside
+	||||||| base
+	original
+	=======
+	theirside
+	>>>>>>> theirs
+	EOF
+	test_cmp expect file
+'
+
+test_expect_success 'checkout --merge --conflict=diff3 --no-conflict does merge' '
+	setup_conflicting_index &&
+	echo "none of the above" >fild &&
+	echo "none of the above" >file &&
+	git checkout --merge --conflict=diff3 --no-conflict -- fild file &&
+	echo "ourside" >expect &&
+	test_cmp expect fild &&
+	cat >expect <<-\EOF &&
+	<<<<<<< ours
+	ourside
+	=======
+	theirside
+	>>>>>>> theirs
+	EOF
+	test_cmp expect file
+'
+
 test_expect_success 'checkout with invalid conflict style' '
 	test_must_fail git checkout --conflict=bad 2>actual -- file &&
 	echo "error: unknown conflict style ${SQ}bad${SQ}" >expect &&
-- 
gitgitgadget

Re: [PATCH v2 0/5] checkout: cleanup --conflict=

[Phillip Wood]

On 14/03/2024 17:05, Phillip Wood via GitGitGadget wrote:

Here is the cover letter - I don't know why GGG keeps omitting it

Passing an invalid conflict style name such as "--conflict=bad" to "git 
checkout" gives the error message

     error: unknown style 'bad' given for 'merge.conflictstyle'

which is unfortunate as it talks about a config setting rather than the 
option given on the command line. This series refactors the 
implementation to pass the conflict style down the call chain to the 
merge machinery rather than abusing the config setting.

Thanks to Junio for his comments on v1, the changes in v2 are:
  - renamed parse_conflict_style() to parse_conflict_style_name()
  - parse --conflict using OPT_CALLBACK to avoid storing the string argument
  - added a new patch to fix the interaction of --conflict with --no-merge


> Phillip Wood (5):
>    xdiff-interface: refactor parsing of merge.conflictstyle
>    merge-ll: introduce LL_MERGE_OPTIONS_INIT
>    merge options: add a conflict style member
>    checkout: cleanup --conflict=<style> parsing
>    checkout: fix interaction between --conflict and --merge
> 
>   builtin/checkout.c | 60 +++++++++++++++++++++++++----------------
>   merge-ll.c         |  6 +++--
>   merge-ll.h         |  5 ++++
>   merge-ort.c        |  3 ++-
>   merge-recursive.c  |  5 +++-
>   merge-recursive.h  |  1 +
>   t/t7201-co.sh      | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>   xdiff-interface.c  | 29 ++++++++++++--------
>   xdiff-interface.h  |  1 +
>   9 files changed, 139 insertions(+), 37 deletions(-)
> 
> 
> base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1684
> 
> Range-diff vs v1:
> 
>   1:  0263d001634 ! 1:  629e577879c xdiff-interface: refactor parsing of merge.conflictstyle
>       @@ xdiff-interface.c: int xdiff_compare_lines(const char *l1, long s1,
>         	return xdl_recmatch(l1, s1, l2, s2, flags);
>         }
>         
>       -+int parse_conflict_style(const char *value)
>       ++int parse_conflict_style_name(const char *value)
>        +{
>        +	if (!strcmp(value, "diff3"))
>        +		return XDL_MERGE_DIFF3;
>       @@ xdiff-interface.c: int git_xmerge_config(const char *var, const char *value,
>        -		 * git-completion.bash when you add new merge config
>        -		 */
>        -		else
>       -+		git_xmerge_style = parse_conflict_style(value);
>       ++		git_xmerge_style = parse_conflict_style_name(value);
>        +		if (git_xmerge_style == -1)
>         			return error(_("unknown style '%s' given for '%s'"),
>         				     value, var);
>       @@ xdiff-interface.h: int buffer_is_binary(const char *ptr, unsigned long size);
>         void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);
>         void xdiff_clear_find_func(xdemitconf_t *xecfg);
>         struct config_context;
>       -+int parse_conflict_style(const char *value);
>       ++int parse_conflict_style_name(const char *value);
>         int git_xmerge_config(const char *var, const char *value,
>         		      const struct config_context *ctx, void *cb);
>         extern int git_xmerge_style;
>   2:  4e05bc156bc = 2:  46e0f5a0af7 merge-ll: introduce LL_MERGE_OPTIONS_INIT
>   3:  c0d7bafd438 = 3:  47d0ec28a55 merge options: add a conflict style member
>   4:  317bb7a70d0 ! 4:  511e03d3db2 checkout: cleanup --conflict=<style> parsing
>       @@ Commit message
>            the option given on the command line. This happens because the
>            implementation calls git_xmerge_config() to set the conflict style
>            using the value given on the command line. Use the newly added
>       -    parse_conflict_style() instead and pass the value down the call chain
>       -    to override the config setting. This also means we can avoid setting
>       -    up a struct config_context required for calling git_xmerge_config().
>       +    parse_conflict_style_name() instead and pass the value down the call
>       +    chain to override the config setting. This also means we can avoid
>       +    setting up a struct config_context required for calling
>       +    git_xmerge_config().
>       +
>       +    The option is now parsed in a callback to avoid having to store the
>       +    option name. This is a change in behavior as now
>       +
>       +        git checkout --conflict=bad --conflict=diff3
>       +
>       +    will error out when parsing "--conflict=bad" whereas before this change
>       +    it would succeed because it would only try to parse the value of the
>       +    last "--conflict" option given on the command line.
>        
>            Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>        
>       @@ builtin/checkout.c: struct checkout_opts {
>         	enum branch_track track;
>         	struct diff_options diff_options;
>        -	char *conflict_style;
>       -+	char *conflict_style_name;
>        +	int conflict_style;
>         
>         	int branch_exists;
>       @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op
>         			ret = merge_trees(&o,
>         					  new_tree,
>         					  work,
>       +@@ builtin/checkout.c: static int checkout_branch(struct checkout_opts *opts,
>       + 	return switch_branches(opts, new_branch_info);
>       + }
>       +
>       ++static int parse_opt_conflict(const struct option *o, const char *arg, int unset)
>       ++{
>       ++	struct checkout_opts *opts = o->value;
>       ++
>       ++	if (unset) {
>       ++		opts->conflict_style = -1;
>       ++		return 0;
>       ++	}
>       ++	opts->conflict_style = parse_conflict_style_name(arg);
>       ++	if (opts->conflict_style < 0)
>       ++		return error(_("unknown conflict style '%s'"), arg);
>       ++
>       ++	return 0;
>       ++}
>       ++
>       + static struct option *add_common_options(struct checkout_opts *opts,
>       + 					 struct option *prevopts)
>       + {
>        @@ builtin/checkout.c: static struct option *add_common_options(struct checkout_opts *opts,
>         			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
>         		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
>         		OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")),
>        -		OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
>       -+		OPT_STRING(0, "conflict", &opts->conflict_style_name, N_("style"),
>       - 			   N_("conflict style (merge, diff3, or zdiff3)")),
>       +-			   N_("conflict style (merge, diff3, or zdiff3)")),
>       ++		OPT_CALLBACK(0, "conflict", opts, N_("style"),
>       ++			     N_("conflict style (merge, diff3, or zdiff3)"),
>       ++			     parse_opt_conflict),
>         		OPT_END()
>         	};
>       + 	struct option *newopts = parse_options_concat(prevopts, options);
>        @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,
>         			opts->show_progress = isatty(2);
>         	}
>       @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const
>        -		struct config_context ctx = {
>        -			.kvi = &kvi,
>        -		};
>       -+	if (opts->conflict_style_name) {
>       ++	if (opts->conflict_style >= 0)
>         		opts->merge = 1; /* implied */
>        -		git_xmerge_config("merge.conflictstyle", opts->conflict_style,
>        -				  &ctx, NULL);
>       -+		opts->conflict_style =
>       -+			parse_conflict_style(opts->conflict_style_name);
>       -+		if (opts->conflict_style < 0)
>       -+			die(_("unknown conflict style '%s'"),
>       -+			    opts->conflict_style_name);
>       - 	}
>       +-	}
>       ++
>         	if (opts->force) {
>         		opts->discard_changes = 1;
>       + 		opts->ignore_unmerged_opt = "--force";
>        @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,
>         
>         int cmd_checkout(int argc, const char **argv, const char *prefix)
>       @@ t/t7201-co.sh: test_expect_success 'checkout --conflict=diff3' '
>         
>        +test_expect_success 'checkout with invalid conflict style' '
>        +	test_must_fail git checkout --conflict=bad 2>actual -- file &&
>       -+	echo "fatal: unknown conflict style ${SQ}bad${SQ}" >expect &&
>       ++	echo "error: unknown conflict style ${SQ}bad${SQ}" >expect &&
>        +	test_cmp expect actual
>        +'
>        +
>   -:  ----------- > 5:  b771b29e45a checkout: fix interaction between --conflict and --merge
> 

Re: [PATCH v2 1/5] xdiff-interface: refactor parsing of merge.conflictstyle

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Factor out the code that parses of conflict style name so it can be
> reused in a later commit that wants to parse the name given on the
> command line.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  xdiff-interface.c | 29 ++++++++++++++++++-----------
>  xdiff-interface.h |  1 +
>  2 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index 3162f517434..16ed8ac4928 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -305,6 +305,22 @@ int xdiff_compare_lines(const char *l1, long s1,
>  	return xdl_recmatch(l1, s1, l2, s2, flags);
>  }
>  
> +int parse_conflict_style_name(const char *value)
> +{
> +	if (!strcmp(value, "diff3"))
> +		return XDL_MERGE_DIFF3;
> +	else if (!strcmp(value, "zdiff3"))
> +		return XDL_MERGE_ZEALOUS_DIFF3;
> +	else if (!strcmp(value, "merge"))
> +		return 0;
> +	/*
> +	 * Please update _git_checkout() in git-completion.bash when
> +	 * you add new merge config
> +	 */
> +	else
> +		return -1;
> +}

As these symbols are now more public, it is tempting to leave a
#leftoverbits mark to remind us to clean them up by adding
XDL_MERGE_MERGE (instead of 0) and XDL_MERGE_UNKNOWN (instead of -1)
after the dust settles.  That would have made reading later patches
in the series a little less puzzling.

But within the scope of this series, the above is perfect, faithful
refactoring of the original.

Re: [PATCH v2 3/5] merge options: add a conflict style member

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse
>  	xmp.level = XDL_MERGE_ZEALOUS;
>  	xmp.favor = opts->variant;
>  	xmp.xpp.flags = opts->xdl_opts;
> -	if (git_xmerge_style >= 0)
> +	if (opts->conflict_style >= 0)
> +		xmp.style = opts->conflict_style;
> +	else if (git_xmerge_style >= 0)
>  		xmp.style = git_xmerge_style;

The convention being negative ones are unknown/invalid/unspecified,
this if/else cascade makes sense.

Re: [PATCH v2 4/5] checkout: cleanup --conflict=<style> parsing

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +static int parse_opt_conflict(const struct option *o, const char *arg, int unset)
> +{
> +	struct checkout_opts *opts = o->value;
> +
> +	if (unset) {
> +		opts->conflict_style = -1;
> +		return 0;
> +	}
> +	opts->conflict_style = parse_conflict_style_name(arg);
> +	if (opts->conflict_style < 0)
> +		return error(_("unknown conflict style '%s'"), arg);
> +
> +	return 0;
> +}
> +
>  static struct option *add_common_options(struct checkout_opts *opts,
>  					 struct option *prevopts)
>  {
> @@ -1628,8 +1649,9 @@ static struct option *add_common_options(struct checkout_opts *opts,
>  			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
>  		OPT_BOOL(0, "progress", &opts->show_progress, N_("force progress reporting")),
>  		OPT_BOOL('m', "merge", &opts->merge, N_("perform a 3-way merge with the new branch")),
> -		OPT_STRING(0, "conflict", &opts->conflict_style, N_("style"),
> -			   N_("conflict style (merge, diff3, or zdiff3)")),
> +		OPT_CALLBACK(0, "conflict", opts, N_("style"),
> +			     N_("conflict style (merge, diff3, or zdiff3)"),
> +			     parse_opt_conflict),
>  		OPT_END()
>  	};
>  	struct option *newopts = parse_options_concat(prevopts, options);

Looking good.

Re: [PATCH v2 5/5] checkout: fix interaction between --conflict and --merge

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When using "git checkout" to recreate merge conflicts or merge
> uncommitted changes when switching branch "--conflict" sensibly implies
> "--merge". Unfortunately the way this is implemented means that "git
> checkout --conflict=diff3 --no-merge" implies "--merge" violating the
> usual last-one-wins rule. Fix this by only overriding the value of
> opts->merge if "--conflicts" comes after "--no-merge" or "-[-no]-merge"
> is not given on the command line.

That smells like a convoluted logic but I think I can buy the
argument. If "--conflict=diff3" implies "--conflict=diff3 --merge",
then "--conflict=diff3 --no-merge" should imply "--conflict=diff3
--merge --no-merge" and the latter two cancels out with the
last-one-wins rule, leaving only "--conflict=diff3" that does not
imply anything about "--merge".  The conflict style specification
does not have any effect when we are not recreating any merge, so
all of them are ignored in the end.  So, it probably makes sense,
even though I find it highly confusing.

Is it likely that "--conflict=diff3 --no-merge" signals that the
user is confused and it is safer to abort the operation before doing
further harm, though, I wonder?

Thanks.