{"thread":{"id":"61075","subject":"[PATCH 0/4] checkout: cleanup --conflict=","startedAt":"2024-03-08T14:14:34Z","lastAt":"2024-03-14T17:32:46Z","messageCount":29,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","phillip.wood123@gmail.com","Elijah Newren","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"490244","messageId":"pull.1684.git.1709907270.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":null,"subject":"[PATCH 0/4] checkout: cleanup --conflict=","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-08T14:14:26Z","receivedAt":"2024-03-08T14:14:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Phillip Wood (4):\n  xdiff-interface: refactor parsing of merge.conflictstyle\n  merge-ll: introduce LL_MERGE_OPTIONS_INIT\n  merge options: add a conflict style member\n  checkout: cleanup --conflict=<style> parsing\n\n builtin/checkout.c | 40 +++++++++++++++++++++-------------------\n merge-ll.c         |  6 ++++--\n merge-ll.h         |  5 +++++\n merge-ort.c        |  3 ++-\n merge-recursive.c  |  5 ++++-\n merge-recursive.h  |  1 +\n t/t7201-co.sh      |  6 ++++++\n xdiff-interface.c  | 29 ++++++++++++++++++-----------\n xdiff-interface.h  |  1 +\n 9 files changed, 62 insertions(+), 34 deletions(-)\n\n\nbase-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1684\n-- \ngitgitgadget\n"},{"id":"490245","messageId":"0263d001634bd29970390be96a1df9b2251ef10c.1709907270.git.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.git.1709907270.gitgitgadget@gmail.com","subject":"[PATCH 1/4] xdiff-interface: refactor parsing of merge.conflictstyle","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-08T14:14:27Z","receivedAt":"2024-03-08T14:14:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nFactor out the code that parses of conflict style name so it can be\nreused in a later commit that wants to parse the name given on the\ncommand line.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n xdiff-interface.c | 29 ++++++++++++++++++-----------\n xdiff-interface.h |  1 +\n 2 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 3162f517434..daee3c584e1 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -305,6 +305,22 @@ int xdiff_compare_lines(const char *l1, long s1,\n \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n }\n \n+int parse_conflict_style(const char *value)\n+{\n+\tif (!strcmp(value, \"diff3\"))\n+\t\treturn XDL_MERGE_DIFF3;\n+\telse if (!strcmp(value, \"zdiff3\"))\n+\t\treturn XDL_MERGE_ZEALOUS_DIFF3;\n+\telse if (!strcmp(value, \"merge\"))\n+\t\treturn 0;\n+\t/*\n+\t * Please update _git_checkout() in git-completion.bash when\n+\t * you add new merge config\n+\t */\n+\telse\n+\t\treturn -1;\n+}\n+\n int git_xmerge_style = -1;\n \n int git_xmerge_config(const char *var, const char *value,\n@@ -313,17 +329,8 @@ int git_xmerge_config(const char *var, const char *value,\n \tif (!strcmp(var, \"merge.conflictstyle\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tif (!strcmp(value, \"diff3\"))\n-\t\t\tgit_xmerge_style = XDL_MERGE_DIFF3;\n-\t\telse if (!strcmp(value, \"zdiff3\"))\n-\t\t\tgit_xmerge_style = XDL_MERGE_ZEALOUS_DIFF3;\n-\t\telse if (!strcmp(value, \"merge\"))\n-\t\t\tgit_xmerge_style = 0;\n-\t\t/*\n-\t\t * Please update _git_checkout() in\n-\t\t * git-completion.bash when you add new merge config\n-\t\t */\n-\t\telse\n+\t\tgit_xmerge_style = parse_conflict_style(value);\n+\t\tif (git_xmerge_style == -1)\n \t\t\treturn error(_(\"unknown style '%s' given for '%s'\"),\n \t\t\t\t     value, var);\n \t\treturn 0;\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex e6f80df0462..c569b7c5203 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -51,6 +51,7 @@ int buffer_is_binary(const char *ptr, unsigned long size);\n void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);\n void xdiff_clear_find_func(xdemitconf_t *xecfg);\n struct config_context;\n+int parse_conflict_style(const char *value);\n int git_xmerge_config(const char *var, const char *value,\n \t\t      const struct config_context *ctx, void *cb);\n extern int git_xmerge_style;\n-- \ngitgitgadget\n\n"},{"id":"490246","messageId":"4e05bc156bc93e86f3f2ac878c39596bec722a56.1709907270.git.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.git.1709907270.gitgitgadget@gmail.com","subject":"[PATCH 2/4] merge-ll: introduce LL_MERGE_OPTIONS_INIT","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-08T14:14:28Z","receivedAt":"2024-03-08T14:14:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIntroduce a macro to initialize `struct ll_merge_options` in preparation\nfor the next commit that will add a new member that needs to be\ninitialized to a non-zero value.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/checkout.c | 3 +--\n merge-ll.c         | 2 +-\n merge-ll.h         | 2 ++\n merge-ort.c        | 2 +-\n merge-recursive.c  | 2 +-\n 5 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 067c2519334..6ded58bd95c 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -262,7 +262,7 @@ static int checkout_merged(int pos, const struct checkout *state,\n \tmmbuffer_t result_buf;\n \tstruct object_id threeway[3];\n \tunsigned mode = 0;\n-\tstruct ll_merge_options ll_opts;\n+\tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\n \tint renormalize = 0;\n \n \tmemset(threeway, 0, sizeof(threeway));\n@@ -284,7 +284,6 @@ static int checkout_merged(int pos, const struct checkout *state,\n \tread_mmblob(&ours, &threeway[1]);\n \tread_mmblob(&theirs, &threeway[2]);\n \n-\tmemset(&ll_opts, 0, sizeof(ll_opts));\n \tgit_config_get_bool(\"merge.renormalize\", &renormalize);\n \tll_opts.renormalize = renormalize;\n \tmerge_status = ll_merge(&result_buf, path, &ancestor, \"base\",\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 61e0ae53981..6570707297d 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -401,7 +401,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,\n \t     const struct ll_merge_options *opts)\n {\n \tstruct attr_check *check = load_merge_attributes();\n-\tstatic const struct ll_merge_options default_opts;\n+\tstatic const struct ll_merge_options default_opts = LL_MERGE_OPTIONS_INIT;\n \tconst char *ll_driver_name = NULL;\n \tint marker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \tconst struct ll_merge_driver *driver;\ndiff --git a/merge-ll.h b/merge-ll.h\nindex e4a20e81a3a..af1ee36abdb 100644\n--- a/merge-ll.h\n+++ b/merge-ll.h\n@@ -82,6 +82,8 @@ struct ll_merge_options {\n \tlong xdl_opts;\n };\n \n+#define LL_MERGE_OPTIONS_INIT {0}\n+\n enum ll_merge_result {\n \tLL_MERGE_ERROR = -1,\n \tLL_MERGE_OK = 0,\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8617babee41..4a02c3ecd99 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1956,7 +1956,7 @@ static int merge_3way(struct merge_options *opt,\n \t\t      mmbuffer_t *result_buf)\n {\n \tmmfile_t orig, src1, src2;\n-\tstruct ll_merge_options ll_opts = {0};\n+\tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\n \tchar *base, *name1, *name2;\n \tenum ll_merge_result merge_status;\n \ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex a0c3e7a2d91..02b7b584f95 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1047,7 +1047,7 @@ static int merge_3way(struct merge_options *opt,\n \t\t      const int extra_marker_size)\n {\n \tmmfile_t orig, src1, src2;\n-\tstruct ll_merge_options ll_opts = {0};\n+\tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\n \tchar *base, *name1, *name2;\n \tenum ll_merge_result merge_status;\n \n-- \ngitgitgadget\n\n"},{"id":"490247","messageId":"c0d7bafd43823ef9df5a73bc80b90cf003988bc9.1709907271.git.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.git.1709907270.gitgitgadget@gmail.com","subject":"[PATCH 3/4] merge options: add a conflict style member","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-08T14:14:29Z","receivedAt":"2024-03-08T14:14:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nAdd a conflict_style member to `struct merge_options` and `struct\nll_merge_options` to allow callers to override the default conflict\nstyle. This will be used in the next commit.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n merge-ll.c        | 4 +++-\n merge-ll.h        | 5 ++++-\n merge-ort.c       | 1 +\n merge-recursive.c | 3 +++\n merge-recursive.h | 1 +\n 5 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 6570707297d..bf1077ae092 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse\n \txmp.level = XDL_MERGE_ZEALOUS;\n \txmp.favor = opts->variant;\n \txmp.xpp.flags = opts->xdl_opts;\n-\tif (git_xmerge_style >= 0)\n+\tif (opts->conflict_style >= 0)\n+\t\txmp.style = opts->conflict_style;\n+\telse if (git_xmerge_style >= 0)\n \t\txmp.style = git_xmerge_style;\n \tif (marker_size > 0)\n \t\txmp.marker_size = marker_size;\ndiff --git a/merge-ll.h b/merge-ll.h\nindex af1ee36abdb..d038ee0c1e8 100644\n--- a/merge-ll.h\n+++ b/merge-ll.h\n@@ -78,11 +78,14 @@ struct ll_merge_options {\n \t */\n \tunsigned extra_marker_size;\n \n+\t/* Override the global conflict style. */\n+\tint conflict_style;\n+\n \t/* Extra xpparam_t flags as defined in xdiff/xdiff.h. */\n \tlong xdl_opts;\n };\n \n-#define LL_MERGE_OPTIONS_INIT {0}\n+#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }\n \n enum ll_merge_result {\n \tLL_MERGE_ERROR = -1,\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 4a02c3ecd99..a9ab4031451 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1966,6 +1966,7 @@ static int merge_3way(struct merge_options *opt,\n \tll_opts.renormalize = opt->renormalize;\n \tll_opts.extra_marker_size = extra_marker_size;\n \tll_opts.xdl_opts = opt->xdl_opts;\n+\tll_opts.conflict_style = opt->conflict_style;\n \n \tif (opt->priv->call_depth) {\n \t\tll_opts.virtual_ancestor = 1;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 02b7b584f95..33b5f9384e8 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1054,6 +1054,7 @@ static int merge_3way(struct merge_options *opt,\n \tll_opts.renormalize = opt->renormalize;\n \tll_opts.extra_marker_size = extra_marker_size;\n \tll_opts.xdl_opts = opt->xdl_opts;\n+\tll_opts.conflict_style = opt->conflict_style;\n \n \tif (opt->priv->call_depth) {\n \t\tll_opts.virtual_ancestor = 1;\n@@ -3899,6 +3900,8 @@ void init_merge_options(struct merge_options *opt,\n \n \topt->renormalize = 0;\n \n+\topt->conflict_style = -1;\n+\n \tmerge_recursive_config(opt);\n \tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n \tif (merge_verbosity)\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex 3d3b3e3c295..e67d38c3030 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -31,6 +31,7 @@ struct merge_options {\n \n \t/* xdiff-related options (patience, ignore whitespace, ours/theirs) */\n \tlong xdl_opts;\n+\tint conflict_style;\n \tenum {\n \t\tMERGE_VARIANT_NORMAL = 0,\n \t\tMERGE_VARIANT_OURS,\n-- \ngitgitgadget\n\n"},{"id":"490248","messageId":"317bb7a70d023278087f4370b843d7f28f9ee2f6.1709907271.git.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.git.1709907270.gitgitgadget@gmail.com","subject":"[PATCH 4/4] checkout: cleanup --conflict=<style> parsing","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-08T14:14:30Z","receivedAt":"2024-03-08T14:14:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nPassing an invalid conflict style name such as \"--conflict=bad\" gives\nthe error message\n\n    error: unknown style 'bad' given for 'merge.conflictstyle'\n\nwhich is unfortunate as it talks about a config setting rather than\nthe option given on the command line. This happens because the\nimplementation calls git_xmerge_config() to set the conflict style\nusing the value given on the command line. Use the newly added\nparse_conflict_style() instead and pass the value down the call chain\nto override the config setting. This also means we can avoid setting\nup a struct config_context required for calling git_xmerge_config().\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/checkout.c | 37 ++++++++++++++++++++-----------------\n t/t7201-co.sh      |  6 ++++++\n 2 files changed, 26 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6ded58bd95c..f5055f059ad 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -91,7 +91,8 @@ struct checkout_opts {\n \tint new_branch_log;\n \tenum branch_track track;\n \tstruct diff_options diff_options;\n-\tchar *conflict_style;\n+\tchar *conflict_style_name;\n+\tint conflict_style;\n \n \tint branch_exists;\n \tconst char *prefix;\n@@ -100,6 +101,8 @@ struct checkout_opts {\n \tstruct tree *source_tree;\n };\n \n+#define CHECKOUT_OPTS_INIT { .conflict_style = -1 }\n+\n struct branch_info {\n \tchar *name; /* The short name used */\n \tchar *path; /* The full name of a real branch */\n@@ -251,7 +254,8 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,\n }\n \n static int checkout_merged(int pos, const struct checkout *state,\n-\t\t\t   int *nr_checkouts, struct mem_pool *ce_mem_pool)\n+\t\t\t   int *nr_checkouts, struct mem_pool *ce_mem_pool,\n+\t\t\t   int conflict_style)\n {\n \tstruct cache_entry *ce = the_index.cache[pos];\n \tconst char *path = ce->name;\n@@ -286,6 +290,7 @@ static int checkout_merged(int pos, const struct checkout *state,\n \n \tgit_config_get_bool(\"merge.renormalize\", &renormalize);\n \tll_opts.renormalize = renormalize;\n+\tll_opts.conflict_style = conflict_style;\n \tmerge_status = ll_merge(&result_buf, path, &ancestor, \"base\",\n \t\t\t\t&ours, \"ours\", &theirs, \"theirs\",\n \t\t\t\tstate->istate, &ll_opts);\n@@ -416,7 +421,8 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \t\t\telse if (opts->merge)\n \t\t\t\terrs |= checkout_merged(pos, &state,\n \t\t\t\t\t\t\t&nr_unmerged,\n-\t\t\t\t\t\t\t&ce_mem_pool);\n+\t\t\t\t\t\t\t&ce_mem_pool,\n+\t\t\t\t\t\t\topts->conflict_style);\n \t\t\tpos = skip_same_name(ce, pos) - 1;\n \t\t}\n \t}\n@@ -886,6 +892,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t}\n \t\t\to.branch1 = new_branch_info->name;\n \t\t\to.branch2 = \"local\";\n+\t\t\to.conflict_style = opts->conflict_style;\n \t\t\tret = merge_trees(&o,\n \t\t\t\t\t  new_tree,\n \t\t\t\t\t  work,\n@@ -1628,7 +1635,7 @@ static struct option *add_common_options(struct checkout_opts *opts,\n \t\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),\n \t\tOPT_BOOL(0, \"progress\", &opts->show_progress, N_(\"force progress reporting\")),\n \t\tOPT_BOOL('m', \"merge\", &opts->merge, N_(\"perform a 3-way merge with the new branch\")),\n-\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style, N_(\"style\"),\n+\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style_name, N_(\"style\"),\n \t\t\t   N_(\"conflict style (merge, diff3, or zdiff3)\")),\n \t\tOPT_END()\n \t};\n@@ -1720,14 +1727,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\topts->show_progress = isatty(2);\n \t}\n \n-\tif (opts->conflict_style) {\n-\t\tstruct key_value_info kvi = KVI_INIT;\n-\t\tstruct config_context ctx = {\n-\t\t\t.kvi = &kvi,\n-\t\t};\n+\tif (opts->conflict_style_name) {\n \t\topts->merge = 1; /* implied */\n-\t\tgit_xmerge_config(\"merge.conflictstyle\", opts->conflict_style,\n-\t\t\t\t  &ctx, NULL);\n+\t\topts->conflict_style =\n+\t\t\tparse_conflict_style(opts->conflict_style_name);\n+\t\tif (opts->conflict_style < 0)\n+\t\t\tdie(_(\"unknown conflict style '%s'\"),\n+\t\t\t    opts->conflict_style_name);\n \t}\n \tif (opts->force) {\n \t\topts->discard_changes = 1;\n@@ -1893,7 +1899,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\n {\n-\tstruct checkout_opts opts;\n+\tstruct checkout_opts opts = CHECKOUT_OPTS_INIT;\n \tstruct option *options;\n \tstruct option checkout_options[] = {\n \t\tOPT_STRING('b', NULL, &opts.new_branch, N_(\"branch\"),\n@@ -1909,7 +1915,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tint ret;\n \tstruct branch_info new_branch_info = { 0 };\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n \topts.switch_branch_doing_nothing_is_ok = 1;\n \topts.only_merge_on_switching_branches = 0;\n@@ -1948,7 +1953,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \n int cmd_switch(int argc, const char **argv, const char *prefix)\n {\n-\tstruct checkout_opts opts;\n+\tstruct checkout_opts opts = CHECKOUT_OPTS_INIT;\n \tstruct option *options = NULL;\n \tstruct option switch_options[] = {\n \t\tOPT_STRING('c', \"create\", &opts.new_branch, N_(\"branch\"),\n@@ -1964,7 +1969,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \tint ret;\n \tstruct branch_info new_branch_info = { 0 };\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n \topts.accept_ref = 1;\n \topts.accept_pathspec = 0;\n@@ -1990,7 +1994,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \n int cmd_restore(int argc, const char **argv, const char *prefix)\n {\n-\tstruct checkout_opts opts;\n+\tstruct checkout_opts opts = CHECKOUT_OPTS_INIT;\n \tstruct option *options;\n \tstruct option restore_options[] = {\n \t\tOPT_STRING('s', \"source\", &opts.from_treeish, \"<tree-ish>\",\n@@ -2007,7 +2011,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \tint ret;\n \tstruct branch_info new_branch_info = { 0 };\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.accept_ref = 0;\n \topts.accept_pathspec = 1;\n \topts.empty_pathspec_ok = 0;\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 10cc6c46051..5746d152b6d 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -631,6 +631,12 @@ test_expect_success 'checkout --conflict=diff3' '\n \ttest_cmp merged file\n '\n \n+test_expect_success 'checkout with invalid conflict style' '\n+\ttest_must_fail git checkout --conflict=bad 2>actual -- file &&\n+\techo \"fatal: unknown conflict style ${SQ}bad${SQ}\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'failing checkout -b should not break working tree' '\n \tgit clean -fd &&  # Remove untracked files in the way\n \tgit reset --hard main &&\n-- \ngitgitgadget\n"},{"id":"490253","messageId":"xmqqplw4hgnq.fsf@gitster.g","threadId":"61075","inReplyTo":"pull.1684.git.1709907270.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] checkout: cleanup --conflict=","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T15:44:41Z","receivedAt":"2024-03-08T15:44:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\nHere is a place to say why this series exists.  Saying things like\n\"'checkout --conflict=bad' gives a wrong error message, as if the\ninalid conflict style were given by a configuration variable, and\nthis is to fix that bug\".\n\n> Phillip Wood (4):\n>   xdiff-interface: refactor parsing of merge.conflictstyle\n>   merge-ll: introduce LL_MERGE_OPTIONS_INIT\n>   merge options: add a conflict style member\n>   checkout: cleanup --conflict=<style> parsing\n>\n>  builtin/checkout.c | 40 +++++++++++++++++++++-------------------\n>  merge-ll.c         |  6 ++++--\n>  merge-ll.h         |  5 +++++\n>  merge-ort.c        |  3 ++-\n>  merge-recursive.c  |  5 ++++-\n>  merge-recursive.h  |  1 +\n>  t/t7201-co.sh      |  6 ++++++\n>  xdiff-interface.c  | 29 ++++++++++++++++++-----------\n>  xdiff-interface.h  |  1 +\n>  9 files changed, 62 insertions(+), 34 deletions(-)\n>\n>\n> base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1684\n"},{"id":"490254","messageId":"xmqqle6shgkw.fsf@gitster.g","threadId":"61075","inReplyTo":"c0d7bafd43823ef9df5a73bc80b90cf003988bc9.1709907271.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] merge options: add a conflict style member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T15:46:23Z","receivedAt":"2024-03-08T15:46:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/merge-ll.c b/merge-ll.c\n> index 6570707297d..bf1077ae092 100644\n> --- a/merge-ll.c\n> +++ b/merge-ll.c\n> ..\n> -#define LL_MERGE_OPTIONS_INIT {0}\n> +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }\n\nMakes sense, and this obviously makes the previous step worth doing.\n\nIt looks quite wrong that low-level merge options definition is\nhosted in a file whose name is merge low-level.  Is it too late to\nrename the file to fix this, by the way?\n\nThanks.\n"},{"id":"490256","messageId":"585668d2-2097-4e90-b5f0-abfdff633af6@gmail.com","threadId":"61075","inReplyTo":"xmqqplw4hgnq.fsf@gitster.g","subject":"Re: [PATCH 0/4] checkout: cleanup --conflict=","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-08T16:07:28Z","receivedAt":"2024-03-08T16:07:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\n[cc Johannes for the gitgitgadget issue]\n\nOn 08/03/2024 15:44, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> Here is a place to say why this series exists.  Saying things like\n> \"'checkout --conflict=bad' gives a wrong error message, as if the\n> inalid conflict style were given by a configuration variable, and\n> this is to fix that bug\".\n\nSorry, I'm not sure what happen there, I definitely entered a \ncover-letter on the gitgitgadget PR:\n\n\nPassing an invalid conflict style name such as \"--conflict=bad\" to \"git \ncheckout\" gives the error message\n\nerror: unknown style 'bad' given for 'merge.conflictstyle'\n\nwhich is unfortunate as it talks about a config setting rather than the \noption given on the command line. This series refactors the \nimplementation to pass the conflict style down the call chain to the \nmerge machinery rather than abusing the config setting.\n\n\nBest Wishes\n\nPhillip\n\n>> Phillip Wood (4):\n>>    xdiff-interface: refactor parsing of merge.conflictstyle\n>>    merge-ll: introduce LL_MERGE_OPTIONS_INIT\n>>    merge options: add a conflict style member\n>>    checkout: cleanup --conflict=<style> parsing\n>>\n>>   builtin/checkout.c | 40 +++++++++++++++++++++-------------------\n>>   merge-ll.c         |  6 ++++--\n>>   merge-ll.h         |  5 +++++\n>>   merge-ort.c        |  3 ++-\n>>   merge-recursive.c  |  5 ++++-\n>>   merge-recursive.h  |  1 +\n>>   t/t7201-co.sh      |  6 ++++++\n>>   xdiff-interface.c  | 29 ++++++++++++++++++-----------\n>>   xdiff-interface.h  |  1 +\n>>   9 files changed, 62 insertions(+), 34 deletions(-)\n>>\n>>\n>> base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1\n>> Pull-Request: https://github.com/gitgitgadget/git/pull/1684\n"},{"id":"490257","messageId":"48b2d5b1-a59f-44c3-94ba-e7f81913b7f5@gmail.com","threadId":"61075","inReplyTo":"xmqqle6shgkw.fsf@gitster.g","subject":"Re: [PATCH 3/4] merge options: add a conflict style member","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-08T16:13:44Z","receivedAt":"2024-03-08T16:13:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 08/03/2024 15:46, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> diff --git a/merge-ll.c b/merge-ll.c\n>> index 6570707297d..bf1077ae092 100644\n>> --- a/merge-ll.c\n>> +++ b/merge-ll.c\n>> ..\n>> -#define LL_MERGE_OPTIONS_INIT {0}\n>> +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }\n> \n> Makes sense, and this obviously makes the previous step worth doing.\n> \n> It looks quite wrong that low-level merge options definition is\n> hosted in a file whose name is merge low-level.  Is it too late to\n> rename the file to fix this, by the way?\n\nI agree it is confusing, Elijah renamed it from ll-merge.c relatively\nrecently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It\nlooks like the idea was to group it with the other merge* files:\n\n     merge-ll: rename from ll-merge\n     \n     A long term (but rather minor) pet-peeve of mine was the name\n     ll-merge.[ch].  I thought it made it harder to realize what stuff was\n     related to merging when I was working on the merge machinery and trying\n     to improve it.\n     \n     Further, back in d1cbe1e6d8a (\"hash-ll.h: split out of hash.h to remove\n     dependency on repository.h\", 2023-04-22), we have split the portions of\n     hash.h that do not depend upon repository.h into a \"hash-ll.h\" (due to\n     the recommendation to use \"ll\" for \"low-level\" in its name[1], but which\n     I used as a suffix precisely because of my distaste for \"ll-merge\").\n     When we discussed adding additional \"*-ll.h\" files, a request was made\n     that we use \"ll\" consistently as either a prefix or a suffix.  Since it\n     is already in use as both a prefix and a suffix, the only way to do so\n     is to rename some files.\n     \n     Besides my distaste for the ll-merge.[ch] name, let me also note that\n     the files\n       ll-fsmonitor.h, ll-hash.h, ll-merge.h, ll-object-store.h, ll-read-cache.h\n     would have essentially nothing to do with each other and make no sense\n     to group.  But giving them the common \"ll-\" prefix would group them.  Using\n     \"-ll\" as a suffix thus seems just much more logical to me.  Rename\n     ll-merge.[ch] to merge-ll.[ch] to achieve this consistency, and to\n     ensure we get a more logical grouping of files.\n     \n     [1] https://lore.kernel.org/git/kl6lsfcu1g8w.fsf@chooglen-macbookpro.roam.corp.google.com/\n\n\nBest Wishes\n\nPhillip\n"},{"id":"490258","messageId":"xmqq1q8khf8t.fsf@gitster.g","threadId":"61075","inReplyTo":"317bb7a70d023278087f4370b843d7f28f9ee2f6.1709907271.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T16:15:14Z","receivedAt":"2024-03-08T16:15:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> @@ -91,7 +91,8 @@ struct checkout_opts {\n>  \tint new_branch_log;\n>  \tenum branch_track track;\n>  \tstruct diff_options diff_options;\n> -\tchar *conflict_style;\n> +\tchar *conflict_style_name;\n> +\tint conflict_style;\n\nDoes the conflict_style_name need to be a member of this struct?\n\n> @@ -1628,7 +1635,7 @@ static struct option *add_common_options(struct checkout_opts *opts,\n>  \t\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),\n>  \t\tOPT_BOOL(0, \"progress\", &opts->show_progress, N_(\"force progress reporting\")),\n>  \t\tOPT_BOOL('m', \"merge\", &opts->merge, N_(\"perform a 3-way merge with the new branch\")),\n> -\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style, N_(\"style\"),\n> +\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style_name, N_(\"style\"),\n>  \t\t\t   N_(\"conflict style (merge, diff3, or zdiff3)\")),\n>  \t\tOPT_END()\n>  \t};\n\nAh, the options[] definition is not in the same scope as where the\nparse_options() is called, and that is the reason why we need to\ncarry the extra member that we do not need after we are done with\nparsing (we use \"int conflict_style\") in the struct.  Otherwise we\nwould have just received OPT_STRING() into a local variable, called\nparse_options(), and post-processed the string into\nopts->conflict_style.\n\nYucky.  I do not care much about wasted 8 bytes in the structure,\nbut I find it disturbing that those functions called later with this\nstruct has to know that conflict_style_name is a useless member and\nthey are supposed to use conflict_style exclusively.\n\nWe could use OPT_CALLBACK() to accept the incoming string, parse it\nand store it in opts->conflict_style and that would be a way to\navoid the extra member.\n\n> +\t\topts->conflict_style =\n> +\t\t\tparse_conflict_style(opts->conflict_style_name);\n\nWhen I saw the change to xdiff-interface in an earlier step, I\nthought parse_conflict_style() was a potentially confusing name.\nYou can imagine a function that is fed a file with conflict markers\nand say \"ah, this uses diff3 style with common ancestor version\" vs\n\"this uses merge style with only two sides\" to have such a name.\n\nparse_conflict_style_name() that takes a name and returns\nconflict_style enumeration constant would not risk such a confusion,\nI guess.\n\nThanks.\n"},{"id":"490260","messageId":"1fb1c84a-b6b3-4e3e-896f-5fb93cff57a6@gmail.com","threadId":"61075","inReplyTo":"xmqq1q8khf8t.fsf@gitster.g","subject":"Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-08T16:22:28Z","receivedAt":"2024-03-08T16:22:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 08/03/2024 16:15, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> We could use OPT_CALLBACK() to accept the incoming string, parse it\n> and store it in opts->conflict_style and that would be a way to\n> avoid the extra member.\n> \n>> +\t\topts->conflict_style =\n>> +\t\t\tparse_conflict_style(opts->conflict_style_name);\n> \n> When I saw the change to xdiff-interface in an earlier step, I\n> thought parse_conflict_style() was a potentially confusing name.\n> You can imagine a function that is fed a file with conflict markers\n> and say \"ah, this uses diff3 style with common ancestor version\" vs\n> \"this uses merge style with only two sides\" to have such a name.\n> \n> parse_conflict_style_name() that takes a name and returns\n> conflict_style enumeration constant would not risk such a confusion,\n> I guess.\n\nThose are both good suggestions - I'll re-roll next week\n\nThanks\n\nPhillip\n"},{"id":"490261","messageId":"xmqqwmqcfz4y.fsf@gitster.g","threadId":"61075","inReplyTo":"48b2d5b1-a59f-44c3-94ba-e7f81913b7f5@gmail.com","subject":"Re: [PATCH 3/4] merge options: add a conflict style member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T16:48:29Z","receivedAt":"2024-03-08T16:48:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> I agree it is confusing, Elijah renamed it from ll-merge.c relatively\n> recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It\n> looks like the idea was to group it with the other merge* files:\n\nThat reasoning cuts both ways.  When you are only interested in the\nupper level API functions, being able to skip ll-anything as \"low\nlevel details\" is a powerful thing.  merge-ll & hash-ll separated\nfar apart will make it impossible.\n\n"},{"id":"490272","messageId":"xmqqsf10flm2.fsf@gitster.g","threadId":"61075","inReplyTo":"1fb1c84a-b6b3-4e3e-896f-5fb93cff57a6@gmail.com","subject":"Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T21:40:37Z","receivedAt":"2024-03-08T21:40:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> Hi Junio\n>\n> On 08/03/2024 16:15, Junio C Hamano wrote:\n>> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> We could use OPT_CALLBACK() to accept the incoming string, parse it\n>> and store it in opts->conflict_style and that would be a way to\n>> avoid the extra member.\n>> \n>>> +\t\topts->conflict_style =\n>>> +\t\t\tparse_conflict_style(opts->conflict_style_name);\n>> When I saw the change to xdiff-interface in an earlier step, I\n>> thought parse_conflict_style() was a potentially confusing name.\n>> You can imagine a function that is fed a file with conflict markers\n>> and say \"ah, this uses diff3 style with common ancestor version\" vs\n>> \"this uses merge style with only two sides\" to have such a name.\n>> parse_conflict_style_name() that takes a name and returns\n>> conflict_style enumeration constant would not risk such a confusion,\n>> I guess.\n>\n> Those are both good suggestions - I'll re-roll next week\n\nThanks.\n"},{"id":"490286","messageId":"CABPp-BFBNDkdL5Z-VOrv2Z3aWaA3OpWFqd6ZGmU9YC8jBRsAog@mail.gmail.com","threadId":"61075","inReplyTo":"xmqqwmqcfz4y.fsf@gitster.g","subject":"Re: [PATCH 3/4] merge options: add a conflict style member","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-03-09T04:33:00Z","receivedAt":"2024-03-09T04:34:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Mar 8, 2024 at 8:48 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> phillip.wood123@gmail.com writes:\n>\n> > I agree it is confusing, Elijah renamed it from ll-merge.c relatively\n> > recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It\n> > looks like the idea was to group it with the other merge* files:\n>\n> That reasoning cuts both ways.  When you are only interested in the\n> upper level API functions, being able to skip ll-anything as \"low\n> level details\" is a powerful thing.  merge-ll & hash-ll separated\n> far apart will make it impossible.\n\nmerge-ll is wildly different than every other *-ll.h file we have in\nthe tree; the latter set of files may be misnamed, for reasons other\nthan what you are suggesting here.  hash-ll, hex-ll, etc. exist due to\nthe main include file having some rarely used API that require more\n#include statements, and most users of e.g. hex functions can get away\nwith just including hex-ll.h rather than the full hex.h.  Thus,\nhex-ll.h is _not_ \"low level details that you can skip\", but \"the\n_primary_ data structures and functions\".  It doesn't get the name\nhex.h, though, because if we did that then the folks that need both\nprimary parts of the API and the occasional additional function would\nneed to have two hex-related includes.  Also, every function declared\nwithin hex-ll.h is still defined within hex.c; there is no separate\nhex-ll.c file, and the same is true of all the other *-ll.h files\nother than merge-ll.h.  As such, there is absolutely no relation\nbetween hex-ll.h, hash-ll.h, fsmonitor-ll.h, etc.  Using an ll- prefix\non those filenames thus makes no sense to me.  (It's not clear that\n-ll even makes sense as a suffix for these files either, but it's not\nclear what else to use instead.  If I recall correctly I originally\nput forward \"-basics\" as a possible name suffix for these files, but\nsomeone else suggested \"-ll\", and not having any better ideas or\nstrong opinions I just went with it.)\n\nmerge-ll is different in that it is actually a separate level of the\nAPI, and there are both a merge-ll.h file and a merge-ll.c file.\n\nI originally had proposed only adding the hex-ll.h, hash-ll.h,\nfsmonitor-ll.h, etc. files, but you suggested that ll-merge should\neither be renamed or that these new files should be\n(https://lore.kernel.org/git/CABPp-BErrVUnuDjL73edDpmkKUvs6Ny6cYwvueXw1toB4JcF-Q@mail.gmail.com/).\n\n\nNow, all that said, and assuming we were to go back to a world where\nthe other *-ll.h files don't exist (or are renamed independently with\na different suffix), I'm still not understanding why ll-merge makes\nmore sense to you and Phillip than merge-ll.  Could you explain more?\nIf you're only interested in the upper-level API functions, that\nsuggests you are already at the function level and looking within a\ngiven file.  The low-level functions are already split out into a\nseparate file, so you just don't go looking at that separate file.\nHowever, if you're interested in \"where in this codebase do I find the\nstuff related to merging\", then you aren't in a file but in a\ndirectory listing.  The first usecase gains no benefit from renaming\nthese files back to ll-merge.[ch], while the other usecase would have\nbeen much improved in my experience from being named merge-ll.[ch].\nGranted, it's a really minor point, which was why I never brought it\nup until you suggested making all the other *-ll.h files and\nll-merge.[ch] consistent; since renaming the other *-ll.h files made\nno sense at all to me, I went with renaming ll-merge.[ch] to\nmerge-ll.[ch].\n\nThere's probably some other angle to this that you two are viewing\nthis from that just isn't apparent to me.  Sorry for not seeing it\n(yet).  Hopefully the above context is at least helpful, though.\n"},{"id":"490287","messageId":"xmqqplw4dkki.fsf@gitster.g","threadId":"61075","inReplyTo":"CABPp-BFBNDkdL5Z-VOrv2Z3aWaA3OpWFqd6ZGmU9YC8jBRsAog@mail.gmail.com","subject":"Re: [PATCH 3/4] merge options: add a conflict style member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-09T05:46:05Z","receivedAt":"2024-03-09T05:46:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> a different suffix), I'm still not understanding why ll-merge makes\n> more sense to you and Phillip than merge-ll.\n> Could you explain more?\n\nLanguage and grammar?  In other words, \"low-level merge routines\" is\nan understandable phrase, while \"merge lo-level routines\" is not.\n"},{"id":"490359","messageId":"f0c88900-63d3-46ed-9cb8-fc7c3fc07c28@gmail.com","threadId":"61075","inReplyTo":"xmqq1q8khf8t.fsf@gitster.g","subject":"Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-11T14:36:16Z","receivedAt":"2024-03-11T14:36:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 08/03/2024 16:15, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> parse_conflict_style_name() that takes a name and returns\n> conflict_style enumeration constant would not risk such a confusion,\n> I guess.\n\nCan I just check if you mean that we should convert XDL_MERGE_DIFF3 and \nfriends to be an enum, or are you happy to leave them as pre-processor \nconstants and have parse_conflict_style_name() return an int? I don't \nmind changing them to be an enum but I'm not sure it buys any type \nsafety as C is happy to implicitly convert enums and integers. It would \nalso raise the question of whether we should be converting the merge \nflavor and diff algorithm constants to enums as well.\n\nThanks\n\nPhillip\n"},{"id":"490373","messageId":"xmqqcys0sm22.fsf@gitster.g","threadId":"61075","inReplyTo":"f0c88900-63d3-46ed-9cb8-fc7c3fc07c28@gmail.com","subject":"Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-11T17:41:41Z","receivedAt":"2024-03-11T17:41:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> On 08/03/2024 16:15, Junio C Hamano wrote:\n>> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> parse_conflict_style_name() that takes a name and returns\n>> conflict_style enumeration constant would not risk such a confusion,\n>> I guess.\n>\n> Can I just check if you mean that we should convert XDL_MERGE_DIFF3\n> and friends to be an enum, or are you happy to leave them as\n> pre-processor constants and have parse_conflict_style_name() return an\n> int?\n\nThe latter.  I was only wondering what the good name for the new\nfunction was and did not mean to imply that we want conversions from\nC preprocessor macros to enums.\n\n"},{"id":"490488","messageId":"21c3904b-b533-4c6c-9f62-ca75c084ffca@gmail.com","threadId":"61075","inReplyTo":"xmqqcys0sm22.fsf@gitster.g","subject":"Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-12T15:50:28Z","receivedAt":"2024-03-12T15:50:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 11/03/2024 17:41, Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n> \n>> On 08/03/2024 16:15, Junio C Hamano wrote:\n>>> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>> parse_conflict_style_name() that takes a name and returns\n>>> conflict_style enumeration constant would not risk such a confusion,\n>>> I guess.\n>>\n>> Can I just check if you mean that we should convert XDL_MERGE_DIFF3\n>> and friends to be an enum, or are you happy to leave them as\n>> pre-processor constants and have parse_conflict_style_name() return an\n>> int?\n> \n> The latter.  I was only wondering what the good name for the new\n> function was and did not mean to imply that we want conversions from\n> C preprocessor macros to enums.\n\nThanks\n\nPhillip\n"},{"id":"490627","messageId":"pull.1684.v2.git.1710435907.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.git.1709907270.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] checkout: cleanup --conflict=","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-14T17:05:02Z","receivedAt":"2024-03-14T17:05:12Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Phillip Wood (5):\n  xdiff-interface: refactor parsing of merge.conflictstyle\n  merge-ll: introduce LL_MERGE_OPTIONS_INIT\n  merge options: add a conflict style member\n  checkout: cleanup --conflict=<style> parsing\n  checkout: fix interaction between --conflict and --merge\n\n builtin/checkout.c | 60 +++++++++++++++++++++++++----------------\n merge-ll.c         |  6 +++--\n merge-ll.h         |  5 ++++\n merge-ort.c        |  3 ++-\n merge-recursive.c  |  5 +++-\n merge-recursive.h  |  1 +\n t/t7201-co.sh      | 66 ++++++++++++++++++++++++++++++++++++++++++++++\n xdiff-interface.c  | 29 ++++++++++++--------\n xdiff-interface.h  |  1 +\n 9 files changed, 139 insertions(+), 37 deletions(-)\n\n\nbase-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1684\n\nRange-diff vs v1:\n\n 1:  0263d001634 ! 1:  629e577879c xdiff-interface: refactor parsing of merge.conflictstyle\n     @@ xdiff-interface.c: int xdiff_compare_lines(const char *l1, long s1,\n       \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n       }\n       \n     -+int parse_conflict_style(const char *value)\n     ++int parse_conflict_style_name(const char *value)\n      +{\n      +\tif (!strcmp(value, \"diff3\"))\n      +\t\treturn XDL_MERGE_DIFF3;\n     @@ xdiff-interface.c: int git_xmerge_config(const char *var, const char *value,\n      -\t\t * git-completion.bash when you add new merge config\n      -\t\t */\n      -\t\telse\n     -+\t\tgit_xmerge_style = parse_conflict_style(value);\n     ++\t\tgit_xmerge_style = parse_conflict_style_name(value);\n      +\t\tif (git_xmerge_style == -1)\n       \t\t\treturn error(_(\"unknown style '%s' given for '%s'\"),\n       \t\t\t\t     value, var);\n     @@ xdiff-interface.h: int buffer_is_binary(const char *ptr, unsigned long size);\n       void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);\n       void xdiff_clear_find_func(xdemitconf_t *xecfg);\n       struct config_context;\n     -+int parse_conflict_style(const char *value);\n     ++int parse_conflict_style_name(const char *value);\n       int git_xmerge_config(const char *var, const char *value,\n       \t\t      const struct config_context *ctx, void *cb);\n       extern int git_xmerge_style;\n 2:  4e05bc156bc = 2:  46e0f5a0af7 merge-ll: introduce LL_MERGE_OPTIONS_INIT\n 3:  c0d7bafd438 = 3:  47d0ec28a55 merge options: add a conflict style member\n 4:  317bb7a70d0 ! 4:  511e03d3db2 checkout: cleanup --conflict=<style> parsing\n     @@ Commit message\n          the option given on the command line. This happens because the\n          implementation calls git_xmerge_config() to set the conflict style\n          using the value given on the command line. Use the newly added\n     -    parse_conflict_style() instead and pass the value down the call chain\n     -    to override the config setting. This also means we can avoid setting\n     -    up a struct config_context required for calling git_xmerge_config().\n     +    parse_conflict_style_name() instead and pass the value down the call\n     +    chain to override the config setting. This also means we can avoid\n     +    setting up a struct config_context required for calling\n     +    git_xmerge_config().\n     +\n     +    The option is now parsed in a callback to avoid having to store the\n     +    option name. This is a change in behavior as now\n     +\n     +        git checkout --conflict=bad --conflict=diff3\n     +\n     +    will error out when parsing \"--conflict=bad\" whereas before this change\n     +    it would succeed because it would only try to parse the value of the\n     +    last \"--conflict\" option given on the command line.\n      \n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n     @@ builtin/checkout.c: struct checkout_opts {\n       \tenum branch_track track;\n       \tstruct diff_options diff_options;\n      -\tchar *conflict_style;\n     -+\tchar *conflict_style_name;\n      +\tint conflict_style;\n       \n       \tint branch_exists;\n     @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op\n       \t\t\tret = merge_trees(&o,\n       \t\t\t\t\t  new_tree,\n       \t\t\t\t\t  work,\n     +@@ builtin/checkout.c: static int checkout_branch(struct checkout_opts *opts,\n     + \treturn switch_branches(opts, new_branch_info);\n     + }\n     + \n     ++static int parse_opt_conflict(const struct option *o, const char *arg, int unset)\n     ++{\n     ++\tstruct checkout_opts *opts = o->value;\n     ++\n     ++\tif (unset) {\n     ++\t\topts->conflict_style = -1;\n     ++\t\treturn 0;\n     ++\t}\n     ++\topts->conflict_style = parse_conflict_style_name(arg);\n     ++\tif (opts->conflict_style < 0)\n     ++\t\treturn error(_(\"unknown conflict style '%s'\"), arg);\n     ++\n     ++\treturn 0;\n     ++}\n     ++\n     + static struct option *add_common_options(struct checkout_opts *opts,\n     + \t\t\t\t\t struct option *prevopts)\n     + {\n      @@ builtin/checkout.c: static struct option *add_common_options(struct checkout_opts *opts,\n       \t\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),\n       \t\tOPT_BOOL(0, \"progress\", &opts->show_progress, N_(\"force progress reporting\")),\n       \t\tOPT_BOOL('m', \"merge\", &opts->merge, N_(\"perform a 3-way merge with the new branch\")),\n      -\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style, N_(\"style\"),\n     -+\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style_name, N_(\"style\"),\n     - \t\t\t   N_(\"conflict style (merge, diff3, or zdiff3)\")),\n     +-\t\t\t   N_(\"conflict style (merge, diff3, or zdiff3)\")),\n     ++\t\tOPT_CALLBACK(0, \"conflict\", opts, N_(\"style\"),\n     ++\t\t\t     N_(\"conflict style (merge, diff3, or zdiff3)\"),\n     ++\t\t\t     parse_opt_conflict),\n       \t\tOPT_END()\n       \t};\n     + \tstruct option *newopts = parse_options_concat(prevopts, options);\n      @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,\n       \t\t\topts->show_progress = isatty(2);\n       \t}\n     @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const\n      -\t\tstruct config_context ctx = {\n      -\t\t\t.kvi = &kvi,\n      -\t\t};\n     -+\tif (opts->conflict_style_name) {\n     ++\tif (opts->conflict_style >= 0)\n       \t\topts->merge = 1; /* implied */\n      -\t\tgit_xmerge_config(\"merge.conflictstyle\", opts->conflict_style,\n      -\t\t\t\t  &ctx, NULL);\n     -+\t\topts->conflict_style =\n     -+\t\t\tparse_conflict_style(opts->conflict_style_name);\n     -+\t\tif (opts->conflict_style < 0)\n     -+\t\t\tdie(_(\"unknown conflict style '%s'\"),\n     -+\t\t\t    opts->conflict_style_name);\n     - \t}\n     +-\t}\n     ++\n       \tif (opts->force) {\n       \t\topts->discard_changes = 1;\n     + \t\topts->ignore_unmerged_opt = \"--force\";\n      @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,\n       \n       int cmd_checkout(int argc, const char **argv, const char *prefix)\n     @@ t/t7201-co.sh: test_expect_success 'checkout --conflict=diff3' '\n       \n      +test_expect_success 'checkout with invalid conflict style' '\n      +\ttest_must_fail git checkout --conflict=bad 2>actual -- file &&\n     -+\techo \"fatal: unknown conflict style ${SQ}bad${SQ}\" >expect &&\n     ++\techo \"error: unknown conflict style ${SQ}bad${SQ}\" >expect &&\n      +\ttest_cmp expect actual\n      +'\n      +\n -:  ----------- > 5:  b771b29e45a checkout: fix interaction between --conflict and --merge\n\n-- \ngitgitgadget\n"},{"id":"490628","messageId":"629e577879c550aafffeb0f75a64c9a2a95df512.1710435907.git.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.v2.git.1710435907.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] xdiff-interface: refactor parsing of merge.conflictstyle","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-14T17:05:03Z","receivedAt":"2024-03-14T17:05:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nFactor out the code that parses of conflict style name so it can be\nreused in a later commit that wants to parse the name given on the\ncommand line.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n xdiff-interface.c | 29 ++++++++++++++++++-----------\n xdiff-interface.h |  1 +\n 2 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 3162f517434..16ed8ac4928 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -305,6 +305,22 @@ int xdiff_compare_lines(const char *l1, long s1,\n \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n }\n \n+int parse_conflict_style_name(const char *value)\n+{\n+\tif (!strcmp(value, \"diff3\"))\n+\t\treturn XDL_MERGE_DIFF3;\n+\telse if (!strcmp(value, \"zdiff3\"))\n+\t\treturn XDL_MERGE_ZEALOUS_DIFF3;\n+\telse if (!strcmp(value, \"merge\"))\n+\t\treturn 0;\n+\t/*\n+\t * Please update _git_checkout() in git-completion.bash when\n+\t * you add new merge config\n+\t */\n+\telse\n+\t\treturn -1;\n+}\n+\n int git_xmerge_style = -1;\n \n int git_xmerge_config(const char *var, const char *value,\n@@ -313,17 +329,8 @@ int git_xmerge_config(const char *var, const char *value,\n \tif (!strcmp(var, \"merge.conflictstyle\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tif (!strcmp(value, \"diff3\"))\n-\t\t\tgit_xmerge_style = XDL_MERGE_DIFF3;\n-\t\telse if (!strcmp(value, \"zdiff3\"))\n-\t\t\tgit_xmerge_style = XDL_MERGE_ZEALOUS_DIFF3;\n-\t\telse if (!strcmp(value, \"merge\"))\n-\t\t\tgit_xmerge_style = 0;\n-\t\t/*\n-\t\t * Please update _git_checkout() in\n-\t\t * git-completion.bash when you add new merge config\n-\t\t */\n-\t\telse\n+\t\tgit_xmerge_style = parse_conflict_style_name(value);\n+\t\tif (git_xmerge_style == -1)\n \t\t\treturn error(_(\"unknown style '%s' given for '%s'\"),\n \t\t\t\t     value, var);\n \t\treturn 0;\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex e6f80df0462..38537169b72 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -51,6 +51,7 @@ int buffer_is_binary(const char *ptr, unsigned long size);\n void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);\n void xdiff_clear_find_func(xdemitconf_t *xecfg);\n struct config_context;\n+int parse_conflict_style_name(const char *value);\n int git_xmerge_config(const char *var, const char *value,\n \t\t      const struct config_context *ctx, void *cb);\n extern int git_xmerge_style;\n-- \ngitgitgadget\n\n"},{"id":"490629","messageId":"46e0f5a0af7d3ab406bd6dd4af9eb4e0a008ba59.1710435907.git.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.v2.git.1710435907.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] merge-ll: introduce LL_MERGE_OPTIONS_INIT","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-14T17:05:04Z","receivedAt":"2024-03-14T17:05:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIntroduce a macro to initialize `struct ll_merge_options` in preparation\nfor the next commit that will add a new member that needs to be\ninitialized to a non-zero value.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/checkout.c | 3 +--\n merge-ll.c         | 2 +-\n merge-ll.h         | 2 ++\n merge-ort.c        | 2 +-\n merge-recursive.c  | 2 +-\n 5 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 067c2519334..6ded58bd95c 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -262,7 +262,7 @@ static int checkout_merged(int pos, const struct checkout *state,\n \tmmbuffer_t result_buf;\n \tstruct object_id threeway[3];\n \tunsigned mode = 0;\n-\tstruct ll_merge_options ll_opts;\n+\tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\n \tint renormalize = 0;\n \n \tmemset(threeway, 0, sizeof(threeway));\n@@ -284,7 +284,6 @@ static int checkout_merged(int pos, const struct checkout *state,\n \tread_mmblob(&ours, &threeway[1]);\n \tread_mmblob(&theirs, &threeway[2]);\n \n-\tmemset(&ll_opts, 0, sizeof(ll_opts));\n \tgit_config_get_bool(\"merge.renormalize\", &renormalize);\n \tll_opts.renormalize = renormalize;\n \tmerge_status = ll_merge(&result_buf, path, &ancestor, \"base\",\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 61e0ae53981..6570707297d 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -401,7 +401,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,\n \t     const struct ll_merge_options *opts)\n {\n \tstruct attr_check *check = load_merge_attributes();\n-\tstatic const struct ll_merge_options default_opts;\n+\tstatic const struct ll_merge_options default_opts = LL_MERGE_OPTIONS_INIT;\n \tconst char *ll_driver_name = NULL;\n \tint marker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \tconst struct ll_merge_driver *driver;\ndiff --git a/merge-ll.h b/merge-ll.h\nindex e4a20e81a3a..af1ee36abdb 100644\n--- a/merge-ll.h\n+++ b/merge-ll.h\n@@ -82,6 +82,8 @@ struct ll_merge_options {\n \tlong xdl_opts;\n };\n \n+#define LL_MERGE_OPTIONS_INIT {0}\n+\n enum ll_merge_result {\n \tLL_MERGE_ERROR = -1,\n \tLL_MERGE_OK = 0,\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8617babee41..4a02c3ecd99 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1956,7 +1956,7 @@ static int merge_3way(struct merge_options *opt,\n \t\t      mmbuffer_t *result_buf)\n {\n \tmmfile_t orig, src1, src2;\n-\tstruct ll_merge_options ll_opts = {0};\n+\tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\n \tchar *base, *name1, *name2;\n \tenum ll_merge_result merge_status;\n \ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex a0c3e7a2d91..02b7b584f95 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1047,7 +1047,7 @@ static int merge_3way(struct merge_options *opt,\n \t\t      const int extra_marker_size)\n {\n \tmmfile_t orig, src1, src2;\n-\tstruct ll_merge_options ll_opts = {0};\n+\tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\n \tchar *base, *name1, *name2;\n \tenum ll_merge_result merge_status;\n \n-- \ngitgitgadget\n\n"},{"id":"490630","messageId":"47d0ec28a55a5c6604b30ad99953a078a789023a.1710435907.git.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.v2.git.1710435907.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] merge options: add a conflict style member","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-14T17:05:05Z","receivedAt":"2024-03-14T17:05:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nAdd a conflict_style member to `struct merge_options` and `struct\nll_merge_options` to allow callers to override the default conflict\nstyle. This will be used in the next commit.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n merge-ll.c        | 4 +++-\n merge-ll.h        | 5 ++++-\n merge-ort.c       | 1 +\n merge-recursive.c | 3 +++\n merge-recursive.h | 1 +\n 5 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 6570707297d..bf1077ae092 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse\n \txmp.level = XDL_MERGE_ZEALOUS;\n \txmp.favor = opts->variant;\n \txmp.xpp.flags = opts->xdl_opts;\n-\tif (git_xmerge_style >= 0)\n+\tif (opts->conflict_style >= 0)\n+\t\txmp.style = opts->conflict_style;\n+\telse if (git_xmerge_style >= 0)\n \t\txmp.style = git_xmerge_style;\n \tif (marker_size > 0)\n \t\txmp.marker_size = marker_size;\ndiff --git a/merge-ll.h b/merge-ll.h\nindex af1ee36abdb..d038ee0c1e8 100644\n--- a/merge-ll.h\n+++ b/merge-ll.h\n@@ -78,11 +78,14 @@ struct ll_merge_options {\n \t */\n \tunsigned extra_marker_size;\n \n+\t/* Override the global conflict style. */\n+\tint conflict_style;\n+\n \t/* Extra xpparam_t flags as defined in xdiff/xdiff.h. */\n \tlong xdl_opts;\n };\n \n-#define LL_MERGE_OPTIONS_INIT {0}\n+#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }\n \n enum ll_merge_result {\n \tLL_MERGE_ERROR = -1,\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 4a02c3ecd99..a9ab4031451 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1966,6 +1966,7 @@ static int merge_3way(struct merge_options *opt,\n \tll_opts.renormalize = opt->renormalize;\n \tll_opts.extra_marker_size = extra_marker_size;\n \tll_opts.xdl_opts = opt->xdl_opts;\n+\tll_opts.conflict_style = opt->conflict_style;\n \n \tif (opt->priv->call_depth) {\n \t\tll_opts.virtual_ancestor = 1;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 02b7b584f95..33b5f9384e8 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1054,6 +1054,7 @@ static int merge_3way(struct merge_options *opt,\n \tll_opts.renormalize = opt->renormalize;\n \tll_opts.extra_marker_size = extra_marker_size;\n \tll_opts.xdl_opts = opt->xdl_opts;\n+\tll_opts.conflict_style = opt->conflict_style;\n \n \tif (opt->priv->call_depth) {\n \t\tll_opts.virtual_ancestor = 1;\n@@ -3899,6 +3900,8 @@ void init_merge_options(struct merge_options *opt,\n \n \topt->renormalize = 0;\n \n+\topt->conflict_style = -1;\n+\n \tmerge_recursive_config(opt);\n \tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n \tif (merge_verbosity)\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex 3d3b3e3c295..e67d38c3030 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -31,6 +31,7 @@ struct merge_options {\n \n \t/* xdiff-related options (patience, ignore whitespace, ours/theirs) */\n \tlong xdl_opts;\n+\tint conflict_style;\n \tenum {\n \t\tMERGE_VARIANT_NORMAL = 0,\n \t\tMERGE_VARIANT_OURS,\n-- \ngitgitgadget\n\n"},{"id":"490631","messageId":"511e03d3db26b174fa6585310b9866a06ac2dff0.1710435907.git.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.v2.git.1710435907.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] checkout: cleanup --conflict=<style> parsing","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-14T17:05:06Z","receivedAt":"2024-03-14T17:05:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nPassing an invalid conflict style name such as \"--conflict=bad\" gives\nthe error message\n\n    error: unknown style 'bad' given for 'merge.conflictstyle'\n\nwhich is unfortunate as it talks about a config setting rather than\nthe option given on the command line. This happens because the\nimplementation calls git_xmerge_config() to set the conflict style\nusing the value given on the command line. Use the newly added\nparse_conflict_style_name() instead and pass the value down the call\nchain to override the config setting. This also means we can avoid\nsetting up a struct config_context required for calling\ngit_xmerge_config().\n\nThe option is now parsed in a callback to avoid having to store the\noption name. This is a change in behavior as now\n\n    git checkout --conflict=bad --conflict=diff3\n\nwill error out when parsing \"--conflict=bad\" whereas before this change\nit would succeed because it would only try to parse the value of the\nlast \"--conflict\" option given on the command line.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/checkout.c | 51 +++++++++++++++++++++++++++++-----------------\n t/t7201-co.sh      |  6 ++++++\n 2 files changed, 38 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6ded58bd95c..d6ab3b1d665 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -91,7 +91,7 @@ struct checkout_opts {\n \tint new_branch_log;\n \tenum branch_track track;\n \tstruct diff_options diff_options;\n-\tchar *conflict_style;\n+\tint conflict_style;\n \n \tint branch_exists;\n \tconst char *prefix;\n@@ -100,6 +100,8 @@ struct checkout_opts {\n \tstruct tree *source_tree;\n };\n \n+#define CHECKOUT_OPTS_INIT { .conflict_style = -1 }\n+\n struct branch_info {\n \tchar *name; /* The short name used */\n \tchar *path; /* The full name of a real branch */\n@@ -251,7 +253,8 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos,\n }\n \n static int checkout_merged(int pos, const struct checkout *state,\n-\t\t\t   int *nr_checkouts, struct mem_pool *ce_mem_pool)\n+\t\t\t   int *nr_checkouts, struct mem_pool *ce_mem_pool,\n+\t\t\t   int conflict_style)\n {\n \tstruct cache_entry *ce = the_index.cache[pos];\n \tconst char *path = ce->name;\n@@ -286,6 +289,7 @@ static int checkout_merged(int pos, const struct checkout *state,\n \n \tgit_config_get_bool(\"merge.renormalize\", &renormalize);\n \tll_opts.renormalize = renormalize;\n+\tll_opts.conflict_style = conflict_style;\n \tmerge_status = ll_merge(&result_buf, path, &ancestor, \"base\",\n \t\t\t\t&ours, \"ours\", &theirs, \"theirs\",\n \t\t\t\tstate->istate, &ll_opts);\n@@ -416,7 +420,8 @@ static int checkout_worktree(const struct checkout_opts *opts,\n \t\t\telse if (opts->merge)\n \t\t\t\terrs |= checkout_merged(pos, &state,\n \t\t\t\t\t\t\t&nr_unmerged,\n-\t\t\t\t\t\t\t&ce_mem_pool);\n+\t\t\t\t\t\t\t&ce_mem_pool,\n+\t\t\t\t\t\t\topts->conflict_style);\n \t\t\tpos = skip_same_name(ce, pos) - 1;\n \t\t}\n \t}\n@@ -886,6 +891,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t}\n \t\t\to.branch1 = new_branch_info->name;\n \t\t\to.branch2 = \"local\";\n+\t\t\to.conflict_style = opts->conflict_style;\n \t\t\tret = merge_trees(&o,\n \t\t\t\t\t  new_tree,\n \t\t\t\t\t  work,\n@@ -1618,6 +1624,21 @@ static int checkout_branch(struct checkout_opts *opts,\n \treturn switch_branches(opts, new_branch_info);\n }\n \n+static int parse_opt_conflict(const struct option *o, const char *arg, int unset)\n+{\n+\tstruct checkout_opts *opts = o->value;\n+\n+\tif (unset) {\n+\t\topts->conflict_style = -1;\n+\t\treturn 0;\n+\t}\n+\topts->conflict_style = parse_conflict_style_name(arg);\n+\tif (opts->conflict_style < 0)\n+\t\treturn error(_(\"unknown conflict style '%s'\"), arg);\n+\n+\treturn 0;\n+}\n+\n static struct option *add_common_options(struct checkout_opts *opts,\n \t\t\t\t\t struct option *prevopts)\n {\n@@ -1628,8 +1649,9 @@ static struct option *add_common_options(struct checkout_opts *opts,\n \t\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),\n \t\tOPT_BOOL(0, \"progress\", &opts->show_progress, N_(\"force progress reporting\")),\n \t\tOPT_BOOL('m', \"merge\", &opts->merge, N_(\"perform a 3-way merge with the new branch\")),\n-\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style, N_(\"style\"),\n-\t\t\t   N_(\"conflict style (merge, diff3, or zdiff3)\")),\n+\t\tOPT_CALLBACK(0, \"conflict\", opts, N_(\"style\"),\n+\t\t\t     N_(\"conflict style (merge, diff3, or zdiff3)\"),\n+\t\t\t     parse_opt_conflict),\n \t\tOPT_END()\n \t};\n \tstruct option *newopts = parse_options_concat(prevopts, options);\n@@ -1720,15 +1742,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\topts->show_progress = isatty(2);\n \t}\n \n-\tif (opts->conflict_style) {\n-\t\tstruct key_value_info kvi = KVI_INIT;\n-\t\tstruct config_context ctx = {\n-\t\t\t.kvi = &kvi,\n-\t\t};\n+\tif (opts->conflict_style >= 0)\n \t\topts->merge = 1; /* implied */\n-\t\tgit_xmerge_config(\"merge.conflictstyle\", opts->conflict_style,\n-\t\t\t\t  &ctx, NULL);\n-\t}\n+\n \tif (opts->force) {\n \t\topts->discard_changes = 1;\n \t\topts->ignore_unmerged_opt = \"--force\";\n@@ -1893,7 +1909,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\n {\n-\tstruct checkout_opts opts;\n+\tstruct checkout_opts opts = CHECKOUT_OPTS_INIT;\n \tstruct option *options;\n \tstruct option checkout_options[] = {\n \t\tOPT_STRING('b', NULL, &opts.new_branch, N_(\"branch\"),\n@@ -1909,7 +1925,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tint ret;\n \tstruct branch_info new_branch_info = { 0 };\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n \topts.switch_branch_doing_nothing_is_ok = 1;\n \topts.only_merge_on_switching_branches = 0;\n@@ -1948,7 +1963,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \n int cmd_switch(int argc, const char **argv, const char *prefix)\n {\n-\tstruct checkout_opts opts;\n+\tstruct checkout_opts opts = CHECKOUT_OPTS_INIT;\n \tstruct option *options = NULL;\n \tstruct option switch_options[] = {\n \t\tOPT_STRING('c', \"create\", &opts.new_branch, N_(\"branch\"),\n@@ -1964,7 +1979,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \tint ret;\n \tstruct branch_info new_branch_info = { 0 };\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.dwim_new_local_branch = 1;\n \topts.accept_ref = 1;\n \topts.accept_pathspec = 0;\n@@ -1990,7 +2004,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix)\n \n int cmd_restore(int argc, const char **argv, const char *prefix)\n {\n-\tstruct checkout_opts opts;\n+\tstruct checkout_opts opts = CHECKOUT_OPTS_INIT;\n \tstruct option *options;\n \tstruct option restore_options[] = {\n \t\tOPT_STRING('s', \"source\", &opts.from_treeish, \"<tree-ish>\",\n@@ -2007,7 +2021,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)\n \tint ret;\n \tstruct branch_info new_branch_info = { 0 };\n \n-\tmemset(&opts, 0, sizeof(opts));\n \topts.accept_ref = 0;\n \topts.accept_pathspec = 1;\n \topts.empty_pathspec_ok = 0;\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 10cc6c46051..e1f85a91565 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -631,6 +631,12 @@ test_expect_success 'checkout --conflict=diff3' '\n \ttest_cmp merged file\n '\n \n+test_expect_success 'checkout with invalid conflict style' '\n+\ttest_must_fail git checkout --conflict=bad 2>actual -- file &&\n+\techo \"error: unknown conflict style ${SQ}bad${SQ}\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'failing checkout -b should not break working tree' '\n \tgit clean -fd &&  # Remove untracked files in the way\n \tgit reset --hard main &&\n-- \ngitgitgadget\n\n"},{"id":"490632","messageId":"b771b29e45abd1992e46c174dcaebe20ca8a41f9.1710435907.git.gitgitgadget@gmail.com","threadId":"61075","inReplyTo":"pull.1684.v2.git.1710435907.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] checkout: fix interaction between --conflict and --merge","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-14T17:05:07Z","receivedAt":"2024-03-14T17:05:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen using \"git checkout\" to recreate merge conflicts or merge\nuncommitted changes when switching branch \"--conflict\" sensibly implies\n\"--merge\". Unfortunately the way this is implemented means that \"git\ncheckout --conflict=diff3 --no-merge\" implies \"--merge\" violating the\nusual last-one-wins rule. Fix this by only overriding the value of\nopts->merge if \"--conflicts\" comes after \"--no-merge\" or \"-[-no]-merge\"\nis not given on the command line.\n\nThe behavior of \"git checkout --merge --no-conflict\" is unchanged and\nwill still merge on the basis that the \"-[-no]-conflict\" options are\nprimarily intended to affect the conflict style and so \"--no-conflict\"\nshould cancel a previous \"--conflict\" but not override \"--merge\".\n\nOf the four new tests the second one tests the behavior change\nintroduced by this commit, the other three check that this commit does\nnot regress the existing behavior.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n builtin/checkout.c | 10 +++++---\n t/t7201-co.sh      | 60 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 67 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex d6ab3b1d665..b8ce1904897 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -100,7 +100,7 @@ struct checkout_opts {\n \tstruct tree *source_tree;\n };\n \n-#define CHECKOUT_OPTS_INIT { .conflict_style = -1 }\n+#define CHECKOUT_OPTS_INIT { .conflict_style = -1, .merge = -1 }\n \n struct branch_info {\n \tchar *name; /* The short name used */\n@@ -1635,6 +1635,9 @@ static int parse_opt_conflict(const struct option *o, const char *arg, int unset\n \topts->conflict_style = parse_conflict_style_name(arg);\n \tif (opts->conflict_style < 0)\n \t\treturn error(_(\"unknown conflict style '%s'\"), arg);\n+\t/* --conflict overrides a previous --no-merge */\n+\tif (!opts->merge)\n+\t\topts->merge = -1;\n \n \treturn 0;\n }\n@@ -1742,8 +1745,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\topts->show_progress = isatty(2);\n \t}\n \n-\tif (opts->conflict_style >= 0)\n-\t\topts->merge = 1; /* implied */\n+\t/* --conflicts implies --merge */\n+\tif (opts->merge == -1)\n+\t\topts->merge = opts->conflict_style >= 0;\n \n \tif (opts->force) {\n \t\topts->discard_changes = 1;\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex e1f85a91565..42352dc0dbe 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -631,6 +631,66 @@ test_expect_success 'checkout --conflict=diff3' '\n \ttest_cmp merged file\n '\n \n+test_expect_success 'checkout --conflict=diff3 --no-conflict does not merge' '\n+\tsetup_conflicting_index &&\n+\techo \"none of the above\" >expect &&\n+\tcat expect >fild &&\n+\tcat expect >file &&\n+\ttest_must_fail git checkout --conflict=diff3 --no-conflict -- fild file 2>err &&\n+\ttest_cmp expect file &&\n+\ttest_cmp expect fild &&\n+\techo \"error: path ${SQ}file${SQ} is unmerged\" >expect &&\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success 'checkout --conflict=diff3 --no-merge does not merge' '\n+\tsetup_conflicting_index &&\n+\techo \"none of the above\" >expect &&\n+\tcat expect >fild &&\n+\tcat expect >file &&\n+\ttest_must_fail git checkout --conflict=diff3 --no-merge -- fild file 2>err &&\n+\ttest_cmp expect file &&\n+\ttest_cmp expect fild &&\n+\techo \"error: path ${SQ}file${SQ} is unmerged\" >expect &&\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success 'checkout --no-merge --conflict=diff3 does merge' '\n+\tsetup_conflicting_index &&\n+\techo \"none of the above\" >fild &&\n+\techo \"none of the above\" >file &&\n+\tgit checkout --no-merge --conflict=diff3 -- fild file &&\n+\techo \"ourside\" >expect &&\n+\ttest_cmp expect fild &&\n+\tcat >expect <<-\\EOF &&\n+\t<<<<<<< ours\n+\tourside\n+\t||||||| base\n+\toriginal\n+\t=======\n+\ttheirside\n+\t>>>>>>> theirs\n+\tEOF\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'checkout --merge --conflict=diff3 --no-conflict does merge' '\n+\tsetup_conflicting_index &&\n+\techo \"none of the above\" >fild &&\n+\techo \"none of the above\" >file &&\n+\tgit checkout --merge --conflict=diff3 --no-conflict -- fild file &&\n+\techo \"ourside\" >expect &&\n+\ttest_cmp expect fild &&\n+\tcat >expect <<-\\EOF &&\n+\t<<<<<<< ours\n+\tourside\n+\t=======\n+\ttheirside\n+\t>>>>>>> theirs\n+\tEOF\n+\ttest_cmp expect file\n+'\n+\n test_expect_success 'checkout with invalid conflict style' '\n \ttest_must_fail git checkout --conflict=bad 2>actual -- file &&\n \techo \"error: unknown conflict style ${SQ}bad${SQ}\" >expect &&\n-- \ngitgitgadget\n"},{"id":"490633","messageId":"329d3570-8aca-4f12-9843-d351133eff15@gmail.com","threadId":"61075","inReplyTo":"pull.1684.v2.git.1710435907.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] checkout: cleanup --conflict=","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-03-14T17:07:13Z","receivedAt":"2024-03-14T17:07:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/03/2024 17:05, Phillip Wood via GitGitGadget wrote:\n\nHere is the cover letter - I don't know why GGG keeps omitting it\n\nPassing an invalid conflict style name such as \"--conflict=bad\" to \"git \ncheckout\" gives the error message\n\n     error: unknown style 'bad' given for 'merge.conflictstyle'\n\nwhich is unfortunate as it talks about a config setting rather than the \noption given on the command line. This series refactors the \nimplementation to pass the conflict style down the call chain to the \nmerge machinery rather than abusing the config setting.\n\nThanks to Junio for his comments on v1, the changes in v2 are:\n  - renamed parse_conflict_style() to parse_conflict_style_name()\n  - parse --conflict using OPT_CALLBACK to avoid storing the string argument\n  - added a new patch to fix the interaction of --conflict with --no-merge\n\n\n> Phillip Wood (5):\n>    xdiff-interface: refactor parsing of merge.conflictstyle\n>    merge-ll: introduce LL_MERGE_OPTIONS_INIT\n>    merge options: add a conflict style member\n>    checkout: cleanup --conflict=<style> parsing\n>    checkout: fix interaction between --conflict and --merge\n> \n>   builtin/checkout.c | 60 +++++++++++++++++++++++++----------------\n>   merge-ll.c         |  6 +++--\n>   merge-ll.h         |  5 ++++\n>   merge-ort.c        |  3 ++-\n>   merge-recursive.c  |  5 +++-\n>   merge-recursive.h  |  1 +\n>   t/t7201-co.sh      | 66 ++++++++++++++++++++++++++++++++++++++++++++++\n>   xdiff-interface.c  | 29 ++++++++++++--------\n>   xdiff-interface.h  |  1 +\n>   9 files changed, 139 insertions(+), 37 deletions(-)\n> \n> \n> base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1684\n> \n> Range-diff vs v1:\n> \n>   1:  0263d001634 ! 1:  629e577879c xdiff-interface: refactor parsing of merge.conflictstyle\n>       @@ xdiff-interface.c: int xdiff_compare_lines(const char *l1, long s1,\n>         \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n>         }\n>         \n>       -+int parse_conflict_style(const char *value)\n>       ++int parse_conflict_style_name(const char *value)\n>        +{\n>        +\tif (!strcmp(value, \"diff3\"))\n>        +\t\treturn XDL_MERGE_DIFF3;\n>       @@ xdiff-interface.c: int git_xmerge_config(const char *var, const char *value,\n>        -\t\t * git-completion.bash when you add new merge config\n>        -\t\t */\n>        -\t\telse\n>       -+\t\tgit_xmerge_style = parse_conflict_style(value);\n>       ++\t\tgit_xmerge_style = parse_conflict_style_name(value);\n>        +\t\tif (git_xmerge_style == -1)\n>         \t\t\treturn error(_(\"unknown style '%s' given for '%s'\"),\n>         \t\t\t\t     value, var);\n>       @@ xdiff-interface.h: int buffer_is_binary(const char *ptr, unsigned long size);\n>         void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);\n>         void xdiff_clear_find_func(xdemitconf_t *xecfg);\n>         struct config_context;\n>       -+int parse_conflict_style(const char *value);\n>       ++int parse_conflict_style_name(const char *value);\n>         int git_xmerge_config(const char *var, const char *value,\n>         \t\t      const struct config_context *ctx, void *cb);\n>         extern int git_xmerge_style;\n>   2:  4e05bc156bc = 2:  46e0f5a0af7 merge-ll: introduce LL_MERGE_OPTIONS_INIT\n>   3:  c0d7bafd438 = 3:  47d0ec28a55 merge options: add a conflict style member\n>   4:  317bb7a70d0 ! 4:  511e03d3db2 checkout: cleanup --conflict=<style> parsing\n>       @@ Commit message\n>            the option given on the command line. This happens because the\n>            implementation calls git_xmerge_config() to set the conflict style\n>            using the value given on the command line. Use the newly added\n>       -    parse_conflict_style() instead and pass the value down the call chain\n>       -    to override the config setting. This also means we can avoid setting\n>       -    up a struct config_context required for calling git_xmerge_config().\n>       +    parse_conflict_style_name() instead and pass the value down the call\n>       +    chain to override the config setting. This also means we can avoid\n>       +    setting up a struct config_context required for calling\n>       +    git_xmerge_config().\n>       +\n>       +    The option is now parsed in a callback to avoid having to store the\n>       +    option name. This is a change in behavior as now\n>       +\n>       +        git checkout --conflict=bad --conflict=diff3\n>       +\n>       +    will error out when parsing \"--conflict=bad\" whereas before this change\n>       +    it would succeed because it would only try to parse the value of the\n>       +    last \"--conflict\" option given on the command line.\n>        \n>            Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>        \n>       @@ builtin/checkout.c: struct checkout_opts {\n>         \tenum branch_track track;\n>         \tstruct diff_options diff_options;\n>        -\tchar *conflict_style;\n>       -+\tchar *conflict_style_name;\n>        +\tint conflict_style;\n>         \n>         \tint branch_exists;\n>       @@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *op\n>         \t\t\tret = merge_trees(&o,\n>         \t\t\t\t\t  new_tree,\n>         \t\t\t\t\t  work,\n>       +@@ builtin/checkout.c: static int checkout_branch(struct checkout_opts *opts,\n>       + \treturn switch_branches(opts, new_branch_info);\n>       + }\n>       +\n>       ++static int parse_opt_conflict(const struct option *o, const char *arg, int unset)\n>       ++{\n>       ++\tstruct checkout_opts *opts = o->value;\n>       ++\n>       ++\tif (unset) {\n>       ++\t\topts->conflict_style = -1;\n>       ++\t\treturn 0;\n>       ++\t}\n>       ++\topts->conflict_style = parse_conflict_style_name(arg);\n>       ++\tif (opts->conflict_style < 0)\n>       ++\t\treturn error(_(\"unknown conflict style '%s'\"), arg);\n>       ++\n>       ++\treturn 0;\n>       ++}\n>       ++\n>       + static struct option *add_common_options(struct checkout_opts *opts,\n>       + \t\t\t\t\t struct option *prevopts)\n>       + {\n>        @@ builtin/checkout.c: static struct option *add_common_options(struct checkout_opts *opts,\n>         \t\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),\n>         \t\tOPT_BOOL(0, \"progress\", &opts->show_progress, N_(\"force progress reporting\")),\n>         \t\tOPT_BOOL('m', \"merge\", &opts->merge, N_(\"perform a 3-way merge with the new branch\")),\n>        -\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style, N_(\"style\"),\n>       -+\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style_name, N_(\"style\"),\n>       - \t\t\t   N_(\"conflict style (merge, diff3, or zdiff3)\")),\n>       +-\t\t\t   N_(\"conflict style (merge, diff3, or zdiff3)\")),\n>       ++\t\tOPT_CALLBACK(0, \"conflict\", opts, N_(\"style\"),\n>       ++\t\t\t     N_(\"conflict style (merge, diff3, or zdiff3)\"),\n>       ++\t\t\t     parse_opt_conflict),\n>         \t\tOPT_END()\n>         \t};\n>       + \tstruct option *newopts = parse_options_concat(prevopts, options);\n>        @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,\n>         \t\t\topts->show_progress = isatty(2);\n>         \t}\n>       @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const\n>        -\t\tstruct config_context ctx = {\n>        -\t\t\t.kvi = &kvi,\n>        -\t\t};\n>       -+\tif (opts->conflict_style_name) {\n>       ++\tif (opts->conflict_style >= 0)\n>         \t\topts->merge = 1; /* implied */\n>        -\t\tgit_xmerge_config(\"merge.conflictstyle\", opts->conflict_style,\n>        -\t\t\t\t  &ctx, NULL);\n>       -+\t\topts->conflict_style =\n>       -+\t\t\tparse_conflict_style(opts->conflict_style_name);\n>       -+\t\tif (opts->conflict_style < 0)\n>       -+\t\t\tdie(_(\"unknown conflict style '%s'\"),\n>       -+\t\t\t    opts->conflict_style_name);\n>       - \t}\n>       +-\t}\n>       ++\n>         \tif (opts->force) {\n>         \t\topts->discard_changes = 1;\n>       + \t\topts->ignore_unmerged_opt = \"--force\";\n>        @@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,\n>         \n>         int cmd_checkout(int argc, const char **argv, const char *prefix)\n>       @@ t/t7201-co.sh: test_expect_success 'checkout --conflict=diff3' '\n>         \n>        +test_expect_success 'checkout with invalid conflict style' '\n>        +\ttest_must_fail git checkout --conflict=bad 2>actual -- file &&\n>       -+\techo \"fatal: unknown conflict style ${SQ}bad${SQ}\" >expect &&\n>       ++\techo \"error: unknown conflict style ${SQ}bad${SQ}\" >expect &&\n>        +\ttest_cmp expect actual\n>        +'\n>        +\n>   -:  ----------- > 5:  b771b29e45a checkout: fix interaction between --conflict and --merge\n> \n"},{"id":"490634","messageId":"xmqqwmq4vihw.fsf@gitster.g","threadId":"61075","inReplyTo":"629e577879c550aafffeb0f75a64c9a2a95df512.1710435907.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/5] xdiff-interface: refactor parsing of merge.conflictstyle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T17:19:23Z","receivedAt":"2024-03-14T17:19:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Factor out the code that parses of conflict style name so it can be\n> reused in a later commit that wants to parse the name given on the\n> command line.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  xdiff-interface.c | 29 ++++++++++++++++++-----------\n>  xdiff-interface.h |  1 +\n>  2 files changed, 19 insertions(+), 11 deletions(-)\n>\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index 3162f517434..16ed8ac4928 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -305,6 +305,22 @@ int xdiff_compare_lines(const char *l1, long s1,\n>  \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n>  }\n>  \n> +int parse_conflict_style_name(const char *value)\n> +{\n> +\tif (!strcmp(value, \"diff3\"))\n> +\t\treturn XDL_MERGE_DIFF3;\n> +\telse if (!strcmp(value, \"zdiff3\"))\n> +\t\treturn XDL_MERGE_ZEALOUS_DIFF3;\n> +\telse if (!strcmp(value, \"merge\"))\n> +\t\treturn 0;\n> +\t/*\n> +\t * Please update _git_checkout() in git-completion.bash when\n> +\t * you add new merge config\n> +\t */\n> +\telse\n> +\t\treturn -1;\n> +}\n\nAs these symbols are now more public, it is tempting to leave a\n#leftoverbits mark to remind us to clean them up by adding\nXDL_MERGE_MERGE (instead of 0) and XDL_MERGE_UNKNOWN (instead of -1)\nafter the dust settles.  That would have made reading later patches\nin the series a little less puzzling.\n\nBut within the scope of this series, the above is perfect, faithful\nrefactoring of the original.\n"},{"id":"490635","messageId":"xmqqsf0svie1.fsf@gitster.g","threadId":"61075","inReplyTo":"47d0ec28a55a5c6604b30ad99953a078a789023a.1710435907.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/5] merge options: add a conflict style member","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T17:21:42Z","receivedAt":"2024-03-14T17:21:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> --- a/merge-ll.c\n> +++ b/merge-ll.c\n> @@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse\n>  \txmp.level = XDL_MERGE_ZEALOUS;\n>  \txmp.favor = opts->variant;\n>  \txmp.xpp.flags = opts->xdl_opts;\n> -\tif (git_xmerge_style >= 0)\n> +\tif (opts->conflict_style >= 0)\n> +\t\txmp.style = opts->conflict_style;\n> +\telse if (git_xmerge_style >= 0)\n>  \t\txmp.style = git_xmerge_style;\n\nThe convention being negative ones are unknown/invalid/unspecified,\nthis if/else cascade makes sense.\n"},{"id":"490636","messageId":"xmqqmsr0via6.fsf@gitster.g","threadId":"61075","inReplyTo":"511e03d3db26b174fa6585310b9866a06ac2dff0.1710435907.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] checkout: cleanup --conflict=<style> parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T17:24:01Z","receivedAt":"2024-03-14T17:24:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +static int parse_opt_conflict(const struct option *o, const char *arg, int unset)\n> +{\n> +\tstruct checkout_opts *opts = o->value;\n> +\n> +\tif (unset) {\n> +\t\topts->conflict_style = -1;\n> +\t\treturn 0;\n> +\t}\n> +\topts->conflict_style = parse_conflict_style_name(arg);\n> +\tif (opts->conflict_style < 0)\n> +\t\treturn error(_(\"unknown conflict style '%s'\"), arg);\n> +\n> +\treturn 0;\n> +}\n> +\n>  static struct option *add_common_options(struct checkout_opts *opts,\n>  \t\t\t\t\t struct option *prevopts)\n>  {\n> @@ -1628,8 +1649,9 @@ static struct option *add_common_options(struct checkout_opts *opts,\n>  \t\t\t    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),\n>  \t\tOPT_BOOL(0, \"progress\", &opts->show_progress, N_(\"force progress reporting\")),\n>  \t\tOPT_BOOL('m', \"merge\", &opts->merge, N_(\"perform a 3-way merge with the new branch\")),\n> -\t\tOPT_STRING(0, \"conflict\", &opts->conflict_style, N_(\"style\"),\n> -\t\t\t   N_(\"conflict style (merge, diff3, or zdiff3)\")),\n> +\t\tOPT_CALLBACK(0, \"conflict\", opts, N_(\"style\"),\n> +\t\t\t     N_(\"conflict style (merge, diff3, or zdiff3)\"),\n> +\t\t\t     parse_opt_conflict),\n>  \t\tOPT_END()\n>  \t};\n>  \tstruct option *newopts = parse_options_concat(prevopts, options);\n\nLooking good.\n"},{"id":"490637","messageId":"xmqqcyrwvhvr.fsf@gitster.g","threadId":"61075","inReplyTo":"b771b29e45abd1992e46c174dcaebe20ca8a41f9.1710435907.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/5] checkout: fix interaction between --conflict and --merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T17:32:40Z","receivedAt":"2024-03-14T17:32:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> When using \"git checkout\" to recreate merge conflicts or merge\n> uncommitted changes when switching branch \"--conflict\" sensibly implies\n> \"--merge\". Unfortunately the way this is implemented means that \"git\n> checkout --conflict=diff3 --no-merge\" implies \"--merge\" violating the\n> usual last-one-wins rule. Fix this by only overriding the value of\n> opts->merge if \"--conflicts\" comes after \"--no-merge\" or \"-[-no]-merge\"\n> is not given on the command line.\n\nThat smells like a convoluted logic but I think I can buy the\nargument. If \"--conflict=diff3\" implies \"--conflict=diff3 --merge\",\nthen \"--conflict=diff3 --no-merge\" should imply \"--conflict=diff3\n--merge --no-merge\" and the latter two cancels out with the\nlast-one-wins rule, leaving only \"--conflict=diff3\" that does not\nimply anything about \"--merge\".  The conflict style specification\ndoes not have any effect when we are not recreating any merge, so\nall of them are ignored in the end.  So, it probably makes sense,\neven though I find it highly confusing.\n\nIs it likely that \"--conflict=diff3 --no-merge\" signals that the\nuser is confused and it is safer to abort the operation before doing\nfurther harm, though, I wonder?\n\nThanks.\n"}]}