threads / patch / 61075

patch, 4 partscheckout: cleanup --conflict=

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

## tl;dr

29 messages between Mar 8, 2024 and Mar 14, 2024. Diffs are folded; open one to read it.

replies: 28people: 3as markdown or json

Phillip Wood via GitGitGadget· Mar 8, 2024, 14:14 UTC · lore
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
Phillip Wood via GitGitGadget· Mar 8, 2024, 14:14 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to 2 files +19 −11

xdiff-interface.c, xdiff-interface.h

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
Phillip Wood via GitGitGadget· Mar 8, 2024, 14:14 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to 5 files +6 −5

builtin/checkout.c, merge-ll.c, merge-ll.h, merge-ort.c, merge-recursive.c

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
Phillip Wood via GitGitGadget· Mar 8, 2024, 14:14 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to 5 files +12 −2

merge-ll.c, merge-ll.h, merge-ort.c, merge-recursive.c, merge-recursive.h

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
Junio C Hamano· Mar 8, 2024, 15:46 UTC · re: Phillip Wood via GitGitGadget · lore

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

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> 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.
phillip.wood123@gmail.com· Mar 8, 2024, 16:13 UTC · re: Junio C Hamano · lore

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

Hi Junio
On 08/03/2024 15:46, Junio C Hamano wrote:
Show 15 quoted lines
> "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
Junio C Hamano· Mar 8, 2024, 16:48 UTC · re: phillip.wood123@gmail.com · lore

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

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.

Elijah Newren· Mar 9, 2024, 04:33 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 8, 2024 at 8:48 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
>
> 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.

Junio C Hamano· Mar 9, 2024, 05:46 UTC · re: Elijah Newren · lore

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

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.

Phillip Wood via GitGitGadget· Mar 8, 2024, 14:14 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to 2 files +26 −17

builtin/checkout.c, t/t7201-co.sh

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
Junio C Hamano· Mar 8, 2024, 16:15 UTC · re: Phillip Wood via GitGitGadget · lore

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

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> @@ -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?
Show 9 quoted lines
> @@ -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.
phillip.wood123@gmail.com· Mar 8, 2024, 16:22 UTC · re: Junio C Hamano · lore

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

Hi Junio
On 08/03/2024 16:15, Junio C Hamano wrote:
Show 18 quoted lines
> "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
Junio C Hamano· Mar 8, 2024, 21:40 UTC · re: phillip.wood123@gmail.com · lore

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

phillip.wood123@gmail.com writes:
Show 20 quoted lines
> 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.wood123@gmail.com· Mar 11, 2024, 14:36 UTC · re: Junio C Hamano · lore

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

Hi Junio
On 08/03/2024 16:15, Junio C Hamano wrote:
Show 5 quoted lines
> "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
Junio C Hamano· Mar 11, 2024, 17:41 UTC · re: phillip.wood123@gmail.com · lore

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

phillip.wood123@gmail.com writes:
Show 10 quoted lines
> 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.

Phillip Wood· Mar 12, 2024, 15:50 UTC · re: Junio C Hamano · lore

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

On 11/03/2024 17:41, Junio C Hamano wrote:
Show 16 quoted lines
> 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
Junio C Hamano· Mar 8, 2024, 15:44 UTC · re: Phillip Wood via GitGitGadget · lore

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

"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".

Show 22 quoted lines
> 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
phillip.wood123@gmail.com· Mar 8, 2024, 16:07 UTC · re: Junio C Hamano · lore

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

Hi Junio
[cc Johannes for the gitgitgadget issue]
On 08/03/2024 15:44, Junio C Hamano wrote:
Show 6 quoted lines
> "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
Show 22 quoted lines
>> 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
Phillip Wood via GitGitGadget· Mar 14, 2024, 17:05 UTC · re: Phillip Wood via GitGitGadget · lore

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

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
Phillip Wood via GitGitGadget· Mar 14, 2024, 17:05 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to 2 files +19 −11

xdiff-interface.c, xdiff-interface.h

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
Junio C Hamano· Mar 14, 2024, 17:19 UTC · re: Phillip Wood via GitGitGadget · lore

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

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 35 quoted lines
> 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.

Phillip Wood via GitGitGadget· Mar 14, 2024, 17:05 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to 5 files +6 −5

builtin/checkout.c, merge-ll.c, merge-ll.h, merge-ort.c, merge-recursive.c

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
Phillip Wood via GitGitGadget· Mar 14, 2024, 17:05 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to 5 files +12 −2

merge-ll.c, merge-ll.h, merge-ort.c, merge-recursive.c, merge-recursive.h

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
Junio C Hamano· Mar 14, 2024, 17:21 UTC · re: Phillip Wood via GitGitGadget · lore

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

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 11 quoted lines
> --- 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.

Phillip Wood via GitGitGadget· Mar 14, 2024, 17:05 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to 2 files +38 −19

builtin/checkout.c, t/t7201-co.sh

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
Junio C Hamano· Mar 14, 2024, 17:24 UTC · re: Phillip Wood via GitGitGadget · lore

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

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 30 quoted lines
> +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.
Phillip Wood via GitGitGadget· Mar 14, 2024, 17:05 UTC · re: Phillip Wood via GitGitGadget · lore

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

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(-)
Show changes to 2 files +67 −3

builtin/checkout.c, t/t7201-co.sh

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
Junio C Hamano· Mar 14, 2024, 17:32 UTC · re: Phillip Wood via GitGitGadget · lore

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

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 9 quoted lines
> 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.
Phillip Wood· Mar 14, 2024, 17:07 UTC · re: Phillip Wood via GitGitGadget · lore

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

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
Show 165 quoted lines
> 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
> 

← back to recent threads