Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 8, 2024, 16:15 UTC
- Message-ID
- <xmqq1q8khf8t.fsf@gitster.g>
- In-Reply-To
- <317bb7a70d023278087f4370b843d7f28f9ee2f6.1709907271.git.gitgitgadget@gmail.com>
"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.