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

[Phillip Wood <phillip.wood123@gmail.com>]

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


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