"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
