From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 08 Mar 2024 16:15:14 GMT
Subject: Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
Message-ID: <xmqq1q8khf8t.fsf@gitster.g>
In-Reply-To: <317bb7a70d023278087f4370b843d7f28f9ee2f6.1709907271.git.gitgitgadget@gmail.com>

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

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

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

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

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

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

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

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

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

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

Thanks.