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

Commit Message

Phillip Wood March 8, 2024, 2:14 p.m. UTC

From: Phillip Wood <[e-mail]>

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 <[e-mail]>
---
 builtin/checkout.c | 37 ++++++++++++++++++++-----------------
 t/t7201-co.sh      |  6 ++++++
 2 files changed, 26 insertions(+), 17 deletions(-)

Comments

Junio C Hamano March 8, 2024, 4:15 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> 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.

Phillip Wood March 8, 2024, 4:22 p.m. UTC | #2

Hi Junio

On 08/03/2024 16:15, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> 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 March 8, 2024, 9:40 p.m. UTC | #3

[e-mail] writes:

> Hi Junio
>
> On 08/03/2024 16:15, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <[e-mail]> 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 Wood March 11, 2024, 2:36 p.m. UTC | #4

Hi Junio

On 08/03/2024 16:15, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> 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 March 11, 2024, 5:41 p.m. UTC | #5

[e-mail] writes:

> On 08/03/2024 16:15, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <[e-mail]> 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 March 12, 2024, 3:50 p.m. UTC | #6

On 11/03/2024 17:41, Junio C Hamano wrote:
> [e-mail] writes:
> 
>> On 08/03/2024 16:15, Junio C Hamano wrote:
>>> "Phillip Wood via GitGitGadget" <[e-mail]> 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

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 &&