checkout: cleanup --conflict=
cover checkout: cleanup --conflict=
Phillip Wood (4):
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
builtin/checkout.c | 40 +++++++++++++++++++++------------------- merge-ll.c | 6 ++++-- merge-ll.h | 5 +++++ merge-ort.c | 3 ++- merge-recursive.c | 5 ++++- merge-recursive.h | 1 + t/t7201-co.sh | 6 ++++++ xdiff-interface.c | 29 ++++++++++++++++++----------- xdiff-interface.h | 1 + 9 files changed, 62 insertions(+), 34 deletions(-)
base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1684
Signature · 1 line
gitgitgadget
PATCH 1/4 xdiff-interface: refactor parsing of merge.conflictstyle
From: Phillip Wood <[email protected]>
Factor out the code that parses of conflict style name so it can be reused in a later commit that wants to parse the name given on the command line.
--- xdiff-interface.c | 29 ++++++++++++++++++----------- xdiff-interface.h | 1 + 2 files changed, 19 insertions(+), 11 deletions(-)
xdiff-interface.c +18 -11
@@ -305,6 +305,22 @@ int xdiff_compare_lines(const char *l1, long s1,return xdl_recmatch (l1, s1, l2, s2, flags); } +int parse_conflict_style (const char *value) +{ +if (!strcmp (value,"diff3" )) +return XDL_MERGE_DIFF3 ; +else if (!strcmp (value,"zdiff3" )) +return XDL_MERGE_ZEALOUS_DIFF3 ; +else if (!strcmp (value,"merge" )) +return 0 ; +/* +* Please update _git_checkout() in git-completion.bash when +* you add new merge config +*/ +else +return -1 ; +} +int git_xmerge_style =-1 ; int git_xmerge_config(const char *var, const char *value,
@@ -313,17 +329,8 @@ int git_xmerge_config(const char *var, const char *value,if (!strcmp (var,"merge.conflictstyle" )) {if (!value)return config_error_nonbool (var); -if (!strcmp (value,"diff3" )) - git_xmerge_style =XDL_MERGE_DIFF3 ; -else if (!strcmp (value,"zdiff3" )) - git_xmerge_style =XDL_MERGE_ZEALOUS_DIFF3 ; -else if (!strcmp (value,"merge" )) - git_xmerge_style =0 ; -/* -* Please update _git_checkout() in -* git-completion.bash when you add new merge config -*/ -else + git_xmerge_style =parse_conflict_style (value); +if (git_xmerge_style ==-1 )return error (_ ("unknown style '%s' given for '%s'" ), value, var);return 0 ;
xdiff-interface.h +1 -0
@@ -51,6 +51,7 @@ 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 git_xmerge_config (const char *var,const char *value,const struct config_context *ctx,void *cb);extern int git_xmerge_style;
Signature · 1 line
gitgitgadget
PATCH 2/4 merge-ll: introduce LL_MERGE_OPTIONS_INIT
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Introduce a macro to initialize struct ll_merge_options in preparation for the next commit that will add a new member that needs to be initialized to a non-zero value.
--- builtin/checkout.c | 3 +-- merge-ll.c | 2 +- merge-ll.h | 2 ++ merge-ort.c | 2 +- merge-recursive.c | 2 +- 5 files changed, 6 insertions(+), 5 deletions(-)
builtin/checkout.c +1 -2
@@ -262,7 +262,7 @@ static int checkout_merged(int pos, const struct checkout *state,mmbuffer_t result_buf;struct object_id threeway[3 ];unsigned mode =0 ; -struct ll_merge_options ll_opts; +struct ll_merge_options ll_opts =LL_MERGE_OPTIONS_INIT ;int renormalize =0 ;memset (threeway,0 ,sizeof (threeway));
@@ -284,7 +284,6 @@ static int checkout_merged(int pos, const struct checkout *state,read_mmblob (&ours, &threeway[1 ]);read_mmblob (&theirs, &threeway[2 ]); -memset (&ll_opts,0 ,sizeof (ll_opts));git_config_get_bool ("merge.renormalize" , &renormalize); ll_opts.renormalize = renormalize; merge_status = ll_merge(&result_buf, path, &ancestor, "base",
merge-ll.c +1 -1
@@ -401,7 +401,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf, const struct ll_merge_options *opts) {struct attr_check *check =load_merge_attributes (); -static const struct ll_merge_options default_opts; +static const struct ll_merge_options default_opts =LL_MERGE_OPTIONS_INIT ;const char *ll_driver_name =NULL ;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;const struct ll_merge_driver *driver;
merge-ll.h +2 -0
@@ -82,6 +82,8 @@ struct ll_merge_options {long xdl_opts; }; +#define LL_MERGE_OPTIONS_INIT {0} +enum ll_merge_result {LL_MERGE_ERROR =-1 ,LL_MERGE_OK =0 ,
merge-ort.c +1 -1
@@ -1956,7 +1956,7 @@ static int merge_3way(struct merge_options *opt, mmbuffer_t *result_buf) {mmfile_t orig, src1, src2; -struct ll_merge_options ll_opts = {0 }; +struct ll_merge_options ll_opts =LL_MERGE_OPTIONS_INIT ;char *base, *name1, *name2;enum ll_merge_result merge_status;
merge-recursive.c +1 -1
@@ -1047,7 +1047,7 @@ static int merge_3way(struct merge_options *opt, const int extra_marker_size) {mmfile_t orig, src1, src2; -struct ll_merge_options ll_opts = {0 }; +struct ll_merge_options ll_opts =LL_MERGE_OPTIONS_INIT ;char *base, *name1, *name2;enum ll_merge_result merge_status;
Signature · 1 line
gitgitgadget
PATCH 3/4 merge options: add a conflict style member
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Add a conflict_style member to struct merge_options and struct ll_merge_options to allow callers to override the default conflict style. This will be used in the next commit.
--- merge-ll.c | 4 +++- merge-ll.h | 5 ++++- merge-ort.c | 1 + merge-recursive.c | 3 +++ merge-recursive.h | 1 + 5 files changed, 12 insertions(+), 2 deletions(-)
merge-ll.c +3 -1
@@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse xmp.level =XDL_MERGE_ZEALOUS ; xmp.favor = opts->variant; xmp.xpp.flags = opts->xdl_opts; -if (git_xmerge_style >=0 ) +if (opts->conflict_style >=0 ) + xmp.style = opts->conflict_style; +else if (git_xmerge_style >=0 ) xmp.style = git_xmerge_style;if (marker_size >0 ) xmp.marker_size = marker_size;
merge-ll.h +4 -1
@@ -78,11 +78,14 @@ struct ll_merge_options {*/ unsigned extra_marker_size; +/* Override the global conflict style. */ +int conflict_style; +/* Extra xpparam_t flags as defined in xdiff/xdiff.h. */ long xdl_opts; }; -#define LL_MERGE_OPTIONS_INIT {0} +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }enum ll_merge_result {LL_MERGE_ERROR =-1 ,
merge-ort.c +1 -0
@@ -1966,6 +1966,7 @@ static int merge_3way(struct merge_options *opt, ll_opts.renormalize = opt->renormalize; ll_opts.extra_marker_size = extra_marker_size; ll_opts.xdl_opts = opt->xdl_opts; + ll_opts.conflict_style = opt->conflict_style;if (opt->priv->call_depth) { ll_opts.virtual_ancestor =1 ;
merge-recursive.c +3 -0
@@ -1054,6 +1054,7 @@ static int merge_3way(struct merge_options *opt, ll_opts.renormalize = opt->renormalize; ll_opts.extra_marker_size = extra_marker_size; ll_opts.xdl_opts = opt->xdl_opts; + ll_opts.conflict_style = opt->conflict_style;if (opt->priv->call_depth) { ll_opts.virtual_ancestor =1 ;
@@ -3899,6 +3900,8 @@ void init_merge_options(struct merge_options *opt, opt->renormalize =0 ; + opt->conflict_style =-1 ; +merge_recursive_config (opt); merge_verbosity =getenv ("GIT_MERGE_VERBOSITY" );if (merge_verbosity)
merge-recursive.h +1 -0
@@ -31,6 +31,7 @@ struct merge_options {/* xdiff-related options (patience, ignore whitespace, ours/theirs) */ long xdl_opts; +int conflict_style;enum {MERGE_VARIANT_NORMAL =0 ,MERGE_VARIANT_OURS ,
Signature · 1 line
gitgitgadget
Re: [PATCH 3/4] merge options: add a conflict style member
merge-ll.c
Quoted 7 lines · Phillip Wood via GitGitGadget
merge-ll.c +1
-1-#define LL_MERGE_OPTIONS_INIT {0} +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }
Makes sense, and this obviously makes the previous step worth doing.
It looks quite wrong that low-level merge options definition is hosted in a file whose name is merge low-level. Is it too late to rename the file to fix this, by the way?
Thanks.
Re: [PATCH 3/4] merge options: add a conflict style member
Hi Junio
Quoted 12 lines · Junio C Hamano
merge-ll.c
Quoted 7 lines · Phillip Wood via GitGitGadget
merge-ll.c +1
-1-#define LL_MERGE_OPTIONS_INIT {0} +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }Makes sense, and this obviously makes the previous step worth doing.
Quoted 3 lines
It looks quite wrong that low-level merge options definition is hosted in a file whose name is merge low-level. Is it too late to rename the file to fix this, by the way?
I agree it is confusing, Elijah renamed it from ll-merge.c relatively recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It looks like the idea was to group it with the other merge* files:
merge-ll: rename from ll-merge
A long term (but rather minor) pet-peeve of mine was the name ll-merge.[ch]. I thought it made it harder to realize what stuff was related to merging when I was working on the merge machinery and trying to improve it.
Further, back in d1cbe1e6d8a ("hash-ll.h: split out of hash.h to remove dependency on repository.h", 2023-04-22), we have split the portions of hash.h that do not depend upon repository.h into a "hash-ll.h" (due to the recommendation to use "ll" for "low-level" in its name[1], but which I used as a suffix precisely because of my distaste for "ll-merge"). When we discussed adding additional "*-ll.h" files, a request was made that we use "ll" consistently as either a prefix or a suffix. Since it is already in use as both a prefix and a suffix, the only way to do so is to rename some files.
Besides my distaste for the ll-merge.[ch] name, let me also note that the files
ll-fsmonitor.h, ll-hash.h, ll-merge.h, ll-object-store.h, ll-read-cache.h
would have essentially nothing to do with each other and make no sense to group. But giving them the common "ll-" prefix would group them. Using "-ll" as a suffix thus seems just much more logical to me. Rename ll-merge.[ch] to merge-ll.[ch] to achieve this consistency, and to ensure we get a more logical grouping of files.
[1] https://lore.kernel.org/git/[email protected]/
Best Wishes
Phillip
Re: [PATCH 3/4] merge options: add a conflict style member
Quoted 3 lines · [email protected] writes:
I agree it is confusing, Elijah renamed it from
ll-merge.crelatively recently6723899932e(merge-ll: rename from ll-merge, 2023-05-16). It looks like the idea was to group it with the other merge* files:
That reasoning cuts both ways. When you are only interested in the upper level API functions, being able to skip ll-anything as "low level details" is a powerful thing. merge-ll & hash-ll separated far apart will make it impossible.
Re: [PATCH 3/4] merge options: add a conflict style member
Quoted 11 lines · Junio C Hamano
Quoted 3 lines · [email protected] writes:
I agree it is confusing, Elijah renamed it from
ll-merge.crelatively recently6723899932e(merge-ll: rename from ll-merge, 2023-05-16). It looks like the idea was to group it with the other merge* files:That reasoning cuts both ways. When you are only interested in the upper level API functions, being able to skip ll-anything as "low level details" is a powerful thing. merge-ll & hash-ll separated far apart will make it impossible.
merge-ll is wildly different than every other *-ll.h file we have in the tree; the latter set of files may be misnamed, for reasons other than what you are suggesting here. hash-ll, hex-ll, etc. exist due to the main include file having some rarely used API that require more #include statements, and most users of e.g. hex functions can get away with just including hex-ll.h rather than the full hex.h. Thus, hex-ll.h is _not_ "low level details that you can skip", but "the _primary_ data structures and functions". It doesn't get the name hex.h, though, because if we did that then the folks that need both primary parts of the API and the occasional additional function would need to have two hex-related includes. Also, every function declared within hex-ll.h is still defined within hex.c; there is no separate hex-ll.c file, and the same is true of all the other *-ll.h files other than merge-ll.h. As such, there is absolutely no relation between hex-ll.h, hash-ll.h, fsmonitor-ll.h, etc. Using an ll- prefix on those filenames thus makes no sense to me. (It's not clear that -ll even makes sense as a suffix for these files either, but it's not clear what else to use instead. If I recall correctly I originally put forward "-basics" as a possible name suffix for these files, but someone else suggested "-ll", and not having any better ideas or strong opinions I just went with it.)
merge-ll is different in that it is actually a separate level of the API, and there are both a merge-ll.h file and a merge-ll.c file.
I originally had proposed only adding the hex-ll.h, hash-ll.h, fsmonitor-ll.h, etc. files, but you suggested that ll-merge should either be renamed or that these new files should be
(https://lore.kernel.org/git/CABPp-BErrVUnuDjL73edDpmkKUvs6Ny6cYwvueXw1toB4JcF-Q@mail.gmail.com/).
Now, all that said, and assuming we were to go back to a world where the other *-ll.h files don't exist (or are renamed independently with a different suffix), I'm still not understanding why ll-merge makes more sense to you and Phillip than merge-ll. Could you explain more? If you're only interested in the upper-level API functions, that suggests you are already at the function level and looking within a given file. The low-level functions are already split out into a separate file, so you just don't go looking at that separate file. However, if you're interested in "where in this codebase do I find the stuff related to merging", then you aren't in a file but in a directory listing. The first usecase gains no benefit from renaming these files back to ll-merge.[ch], while the other usecase would have been much improved in my experience from being named merge-ll.[ch]. Granted, it's a really minor point, which was why I never brought it up until you suggested making all the other *-ll.h files and ll-merge.[ch] consistent; since renaming the other *-ll.h files made no sense at all to me, I went with renaming ll-merge.[ch] to merge-ll.[ch].
There's probably some other angle to this that you two are viewing this from that just isn't apparent to me. Sorry for not seeing it (yet). Hopefully the above context is at least helpful, though.
Re: [PATCH 3/4] merge options: add a conflict style member
Quoted 3 lines · Elijah Newren
a different suffix), I'm still not understanding why ll-merge makes more sense to you and Phillip than merge-ll.
Could you explain more?
Language and grammar? In other words, "low-level merge routines" is an understandable phrase, while "merge lo-level routines" is not.
PATCH 4/4 checkout: cleanup --conflict=<style> parsing
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Passing an invalid conflict style name such as "--conflict=bad" 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 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().
--- builtin/checkout.c | 37 ++++++++++++++++++++----------------- t/t7201-co.sh | 6 ++++++ 2 files changed, 26 insertions(+), 17 deletions(-)
builtin/checkout.c +20 -17
@@ -91,7 +91,8 @@ struct checkout_opts {int new_branch_log;enum branch_track track;struct diff_options diff_options; -char *conflict_style; +char *conflict_style_name; +int conflict_style;int branch_exists;const char *prefix;
@@ -100,6 +101,8 @@ struct checkout_opts {struct tree *source_tree; }; +#define CHECKOUT_OPTS_INIT { .conflict_style = -1 } +struct branch_info {char *name;/* The short name used */ char *path;/* The full name of a real branch */
@@ -251,7 +254,8 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos, }static int checkout_merged (int pos,const struct checkout *state, -int *nr_checkouts,struct mem_pool *ce_mem_pool) +int *nr_checkouts,struct mem_pool *ce_mem_pool, +int conflict_style) {struct cache_entry *ce = the_index.cache[pos];const char *path = ce->name;
@@ -286,6 +290,7 @@ static int checkout_merged(int pos, const struct checkout *state,git_config_get_bool ("merge.renormalize" , &renormalize); ll_opts.renormalize = renormalize; + ll_opts.conflict_style = conflict_style; merge_status =ll_merge (&result_buf, path, &ancestor,"base" , &ours,"ours" , &theirs,"theirs" , state->istate, &ll_opts);
@@ -416,7 +421,8 @@ static int checkout_worktree(const struct checkout_opts *opts,else if (opts->merge) errs |=checkout_merged (pos, &state, &nr_unmerged, - &ce_mem_pool); + &ce_mem_pool, + opts->conflict_style); pos =skip_same_name (ce, pos) -1 ; } }
@@ -886,6 +892,7 @@ static int merge_working_tree(const struct checkout_opts *opts, } o.branch1 = new_branch_info->name; o.branch2 ="local" ; + o.conflict_style = opts->conflict_style; ret = merge_trees(&o, new_tree, work,
@@ -1628,7 +1635,7 @@ static struct option *add_common_options(struct checkout_opts *opts, PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),OPT_BOOL (0 ,"progress" , &opts->show_progress,N_ ("force progress reporting" )),OPT_BOOL ('m' ,"merge" , &opts->merge,N_ ("perform a 3-way merge with the new branch" )), -OPT_STRING (0 ,"conflict" , &opts->conflict_style,N_ ("style" ), +OPT_STRING (0 ,"conflict" , &opts->conflict_style_name,N_ ("style" ),N_ ("conflict style (merge, diff3, or zdiff3)" )), OPT_END() };
@@ -1720,14 +1727,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix, opts->show_progress =isatty (2 ); } -if (opts->conflict_style) { -struct key_value_info kvi =KVI_INIT ; -struct config_context ctx = { - .kvi = &kvi, - }; +if (opts->conflict_style_name) { 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 ;
@@ -1893,7 +1899,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, int cmd_checkout(int argc, const char **argv, const char *prefix) { - struct checkout_opts opts; + struct checkout_opts opts = CHECKOUT_OPTS_INIT; struct option *options; struct option checkout_options[] = { OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
@@ -1909,7 +1915,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)int ret;struct branch_info new_branch_info = {0 }; -memset (&opts,0 ,sizeof (opts)); opts.dwim_new_local_branch =1 ; opts.switch_branch_doing_nothing_is_ok =1 ; opts.only_merge_on_switching_branches =0 ;
@@ -1948,7 +1953,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) int cmd_switch(int argc, const char **argv, const char *prefix) { - struct checkout_opts opts; + struct checkout_opts opts = CHECKOUT_OPTS_INIT; struct option *options = NULL; struct option switch_options[] = { OPT_STRING('c', "create", &opts.new_branch, N_("branch"),
@@ -1964,7 +1969,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)int ret;struct branch_info new_branch_info = {0 }; -memset (&opts,0 ,sizeof (opts)); opts.dwim_new_local_branch =1 ; opts.accept_ref =1 ; opts.accept_pathspec =0 ;
@@ -1990,7 +1994,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix) int cmd_restore(int argc, const char **argv, const char *prefix) { - struct checkout_opts opts; + struct checkout_opts opts = CHECKOUT_OPTS_INIT; struct option *options; struct option restore_options[] = { OPT_STRING('s', "source", &opts.from_treeish, "<tree-ish>",
@@ -2007,7 +2011,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)int ret;struct branch_info new_branch_info = {0 }; -memset (&opts,0 ,sizeof (opts)); opts.accept_ref =0 ; opts.accept_pathspec =1 ; opts.empty_pathspec_ok =0 ;
t/t7201-co.sh +6 -0
@@ -631,6 +631,12 @@ test_expect_success 'checkout --conflict=diff3' ' test_cmp merged file ' +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 && + test_cmp expect actual +' + test_expect_success 'failing checkout -b should not break working tree' ' git clean -fd && # Remove untracked files in the way git reset --hard main &&
Signature · 1 line
gitgitgadget
Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
builtin/checkout.c struct checkout_opts { · line 95
Quoted 7 lines · Phillip Wood via GitGitGadget
builtin/checkout.c +2
-1@@ -91,7 +91,8 @@ struct checkout_opts {int new_branch_log;enum branch_track track;struct diff_options diff_options; -char *conflict_style; +char *conflict_style_name; +int conflict_style;
Does the conflict_style_name need to be a member of this struct?
builtin/checkout.c static struct option *add_common_options(struct checkout_opts *opts, · line 1641
Quoted 9 lines
builtin/checkout.c +1
-1@@ -1628,7 +1635,7 @@ static struct option *add_common_options(struct checkout_opts *opts, PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),OPT_BOOL (0 ,"progress" , &opts->show_progress,N_ ("force progress reporting" )),OPT_BOOL ('m' ,"merge" , &opts->merge,N_ ("perform a 3-way merge with the new branch" )), -OPT_STRING (0 ,"conflict" , &opts->conflict_style,N_ ("style" ), +OPT_STRING (0 ,"conflict" , &opts->conflict_style_name,N_ ("style" ),N_ ("conflict style (merge, diff3, or zdiff3)" )), OPT_END() };
Ah, the options[] definition is not in the same scope as where the parse_options() is called, and that is the reason why we need to carry the extra member that we do not need after we are done with parsing (we use "int conflict_style") in the struct. Otherwise we would have just received OPT_STRING() into a local variable, called parse_options(), and post-processed the string into opts->conflict_style.
Yucky. I do not care much about wasted 8 bytes in the structure, but I find it disturbing that those functions called later with this struct has to know that conflict_style_name is a useless member and they are supposed to use conflict_style exclusively.
We could use OPT_CALLBACK() to accept the incoming string, parse it and store it in opts->conflict_style and that would be a way to avoid the extra member.
builtin/checkout.c static int checkout_main(int argc, const char **argv, const char *prefix, · line 1733
Quoted 2 lines
builtin/checkout.c +2
-0+ opts->conflict_style = +parse_conflict_style (opts->conflict_style_name);
When I saw the change to xdiff-interface in an earlier step, I thought parse_conflict_style() was a potentially confusing name. You can imagine a function that is fed a file with conflict markers and say "ah, this uses diff3 style with common ancestor version" vs "this uses merge style with only two sides" to have such a name.
parse_conflict_style_name() that takes a name and returns conflict_style enumeration constant would not risk such a confusion, I guess.
Thanks.
Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
Hi Junio
Quoted 15 lines · Junio C Hamano
"Phillip Wood via GitGitGadget" <[email protected]> writes:
We could use
OPT_CALLBACK()to accept the incoming string, parse it and store it inopts->conflict_styleand that would be a way to avoid the extra member.builtin/checkout.c static int checkout_main(int argc, const char **argv, const char *prefix, · line 1733
Quoted 2 lines
builtin/checkout.c +2
-0+ opts->conflict_style = +parse_conflict_style (opts->conflict_style_name);When I saw the change to xdiff-interface in an earlier step, I thought
parse_conflict_style()was a potentially confusing name. You can imagine a function that is fed a file with conflict markers and say "ah, this uses diff3 style with common ancestor version" vs "this uses merge style with only two sides" to have such a name.
Quoted 3 lines
parse_conflict_style_name()that takes a name and returnsconflict_styleenumeration constant would not risk such a confusion, I guess.
Those are both good suggestions - I'll re-roll next week
Thanks
Phillip
Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
Quoted 19 lines · [email protected] writes:
Hi Junio
Quoted 5 lines · Junio C Hamano
"Phillip Wood via GitGitGadget" <[email protected]> writes:
We could useOPT_CALLBACK()to accept the incoming string, parse it and store it inopts->conflict_styleand that would be a way to avoid the extra member.Quoted 10 lines
builtin/checkout.c static int checkout_main(int argc, const char **argv, const char *prefix, · line 1733
Quoted 2 lines
builtin/checkout.c +2
-0+ opts->conflict_style = +parse_conflict_style (opts->conflict_style_name);When I saw the change to xdiff-interface in an earlier step, I thought
parse_conflict_style()was a potentially confusing name. You can imagine a function that is fed a file with conflict markers and say "ah, this uses diff3 style with common ancestor version" vs "this uses merge style with only two sides" to have such a name.parse_conflict_style_name()that takes a name and returnsconflict_styleenumeration constant would not risk such a confusion, I guess.
Quoted 1 line
Those are both good suggestions - I'll re-roll next week
Thanks.
Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
In reply to Junio C HamanoHi Junio
Quoted 5 lines · Junio C Hamano
"Phillip Wood via GitGitGadget" <[email protected]> writes:
parse_conflict_style_name()that takes a name and returnsconflict_styleenumeration constant would not risk such a confusion, I guess.
Can I just check if you mean that we should convert XDL_MERGE_DIFF3 and friends to be an enum, or are you happy to leave them as pre-processor constants and have parse_conflict_style_name() return an int? I don't mind changing them to be an enum but I'm not sure it buys any type safety as C is happy to implicitly convert enums and integers. It would also raise the question of whether we should be converting the merge flavor and diff algorithm constants to enums as well.
Thanks
Phillip
Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
Quoted 10 lines · [email protected] writes:
Quoted 4 lines · Junio C Hamano
"Phillip Wood via GitGitGadget" <[email protected]> writes:
parse_conflict_style_name()that takes a name and returnsconflict_styleenumeration constant would not risk such a confusion, I guess.Can I just check if you mean that we should convert
XDL_MERGE_DIFF3and friends to be an enum, or are you happy to leave them as pre-processor constants and haveparse_conflict_style_name()return an int?
The latter. I was only wondering what the good name for the new function was and did not mean to imply that we want conversions from C preprocessor macros to enums.
Re: [PATCH 4/4] checkout: cleanup --conflict=<style> parsing
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · [email protected] writes:
Quoted 4 lines · Junio C Hamano
"Phillip Wood via GitGitGadget" <[email protected]> writes:
parse_conflict_style_name()that takes a name and returnsconflict_styleenumeration constant would not risk such a confusion, I guess.
Quoted 8 lines
Quoted 4 lines
Can I just check if you mean that we should convert
XDL_MERGE_DIFF3and friends to be an enum, or are you happy to leave them as pre-processor constants and haveparse_conflict_style_name()return an int?The latter. I was only wondering what the good name for the new function was and did not mean to imply that we want conversions from C preprocessor macros to enums.
Thanks
Phillip
"Phillip Wood via GitGitGadget" <[email protected]> writes:
Here is a place to say why this series exists. Saying things like "'checkout --conflict=bad' gives a wrong error message, as if the inalid conflict style were given by a configuration variable, and this is to fix that bug".
Quoted 22 lines
Phillip Wood (4):
xdiff-interface: refactor parsing of merge.conflictstyle
merge-ll: introduceLL_MERGE_OPTIONS_INIT
merge options: add a conflict style member
checkout: cleanup --conflict=<style> parsingbuiltin/checkout.c | 40 +++++++++++++++++++++------------------- merge-ll.c | 6 ++++-- merge-ll.h | 5 +++++ merge-ort.c | 3 ++- merge-recursive.c | 5 ++++- merge-recursive.h | 1 + t/t7201-co.sh | 6 ++++++ xdiff-interface.c | 29 ++++++++++++++++++----------- xdiff-interface.h | 1 + 9 files changed, 62 insertions(+), 34 deletions(-)base-commit:
b387623c12f3f4a376e4d35a610fd3e55d7ea907
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1684
Hi Junio
[cc Johannes for the gitgitgadget issue]
Quoted 6 lines · Junio C Hamano
"Phillip Wood via GitGitGadget" <[email protected]> writes:
Here is a place to say why this series exists. Saying things like "'checkout --conflict=bad' gives a wrong error message, as if the inalid conflict style were given by a configuration variable, and this is to fix that bug".
Sorry, I'm not sure what happen there, I definitely entered a cover-letter on the gitgitgadget PR:
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.
Best Wishes
Phillip
Quoted 22 lines
Quoted 22 lines
Phillip Wood (4):
xdiff-interface: refactor parsing of merge.conflictstyle
merge-ll: introduceLL_MERGE_OPTIONS_INIT
merge options: add a conflict style member
checkout: cleanup --conflict=<style> parsingbuiltin/checkout.c | 40 +++++++++++++++++++++------------------- merge-ll.c | 6 ++++-- merge-ll.h | 5 +++++ merge-ort.c | 3 ++- merge-recursive.c | 5 ++++- merge-recursive.h | 1 + t/t7201-co.sh | 6 ++++++ xdiff-interface.c | 29 ++++++++++++++++++----------- xdiff-interface.h | 1 + 9 files changed, 62 insertions(+), 34 deletions(-)base-commit:
b387623c12f3f4a376e4d35a610fd3e55d7ea907
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1684%2Fphillipwood%2Frefactor-conflict-style-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1684/phillipwood/refactor-conflict-style-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1684
v2 cover checkout: cleanup --conflict=
In reply to Phillip Wood via GitGitGadgetPhillip 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 <[email protected]>
@@ 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
Signature · 1 line
gitgitgadget
PATCH v2 1/5 xdiff-interface: refactor parsing of merge.conflictstyle
From: Phillip Wood <[email protected]>
Factor out the code that parses of conflict style name so it can be reused in a later commit that wants to parse the name given on the command line.
--- xdiff-interface.c | 29 ++++++++++++++++++----------- xdiff-interface.h | 1 + 2 files changed, 19 insertions(+), 11 deletions(-)
xdiff-interface.c +18 -11
@@ -305,6 +305,22 @@ int xdiff_compare_lines(const char *l1, long s1,return xdl_recmatch (l1, s1, l2, s2, flags); } +int parse_conflict_style_name (const char *value) +{ +if (!strcmp (value,"diff3" )) +return XDL_MERGE_DIFF3 ; +else if (!strcmp (value,"zdiff3" )) +return XDL_MERGE_ZEALOUS_DIFF3 ; +else if (!strcmp (value,"merge" )) +return 0 ; +/* +* Please update _git_checkout() in git-completion.bash when +* you add new merge config +*/ +else +return -1 ; +} +int git_xmerge_style =-1 ; int git_xmerge_config(const char *var, const char *value,
@@ -313,17 +329,8 @@ int git_xmerge_config(const char *var, const char *value,if (!strcmp (var,"merge.conflictstyle" )) {if (!value)return config_error_nonbool (var); -if (!strcmp (value,"diff3" )) - git_xmerge_style =XDL_MERGE_DIFF3 ; -else if (!strcmp (value,"zdiff3" )) - git_xmerge_style =XDL_MERGE_ZEALOUS_DIFF3 ; -else if (!strcmp (value,"merge" )) - git_xmerge_style =0 ; -/* -* Please update _git_checkout() in -* git-completion.bash when you add new merge config -*/ -else + git_xmerge_style =parse_conflict_style_name (value); +if (git_xmerge_style ==-1 )return error (_ ("unknown style '%s' given for '%s'" ), value, var);return 0 ;
xdiff-interface.h +1 -0
@@ -51,6 +51,7 @@ 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_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;
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/5] xdiff-interface: refactor parsing of merge.conflictstyle
Quoted 25 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Factor out the code that parses of conflict style name so it can be reused in a later commit that wants to parse the name given on the command line.
Signed-off-by Phillip Wood <[email protected]>--- xdiff-interface.c | 29 ++++++++++++++++++----------- xdiff-interface.h | 1 + 2 files changed, 19 insertions(+), 11 deletions(-)xdiff-interface.c +5
-0@@ -305,6 +305,22 @@ int xdiff_compare_lines(const char *l1, long s1,return xdl_recmatch (l1, s1, l2, s2, flags); } +int parse_conflict_style_name (const char *value) +{ +if (!strcmp (value,"diff3" )) +return XDL_MERGE_DIFF3 ; +else if (!strcmp (value,"zdiff3" ))
xdiff-interface.c int xdiff_compare_lines(const char *l1, long s1, · line 322
Quoted 10 lines
xdiff-interface.c +10
-0+return XDL_MERGE_ZEALOUS_DIFF3 ; + else if (!strcmp(value, "merge")) +return 0 ; +/* +* Please update _git_checkout() in git-completion.bash when +* you add new merge config +*/ + else + return -1; +}
As these symbols are now more public, it is tempting to leave a #leftoverbits mark to remind us to clean them up by adding XDL_MERGE_MERGE (instead of 0) and XDL_MERGE_UNKNOWN (instead of -1) after the dust settles. That would have made reading later patches in the series a little less puzzling.
But within the scope of this series, the above is perfect, faithful refactoring of the original.
PATCH v2 2/5 merge-ll: introduce LL_MERGE_OPTIONS_INIT
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Introduce a macro to initialize struct ll_merge_options in preparation for the next commit that will add a new member that needs to be initialized to a non-zero value.
--- builtin/checkout.c | 3 +-- merge-ll.c | 2 +- merge-ll.h | 2 ++ merge-ort.c | 2 +- merge-recursive.c | 2 +- 5 files changed, 6 insertions(+), 5 deletions(-)
builtin/checkout.c +1 -2
@@ -262,7 +262,7 @@ static int checkout_merged(int pos, const struct checkout *state,mmbuffer_t result_buf;struct object_id threeway[3 ];unsigned mode =0 ; -struct ll_merge_options ll_opts; +struct ll_merge_options ll_opts =LL_MERGE_OPTIONS_INIT ;int renormalize =0 ;memset (threeway,0 ,sizeof (threeway));
@@ -284,7 +284,6 @@ static int checkout_merged(int pos, const struct checkout *state,read_mmblob (&ours, &threeway[1 ]);read_mmblob (&theirs, &threeway[2 ]); -memset (&ll_opts,0 ,sizeof (ll_opts));git_config_get_bool ("merge.renormalize" , &renormalize); ll_opts.renormalize = renormalize; merge_status = ll_merge(&result_buf, path, &ancestor, "base",
merge-ll.c +1 -1
@@ -401,7 +401,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf, const struct ll_merge_options *opts) {struct attr_check *check =load_merge_attributes (); -static const struct ll_merge_options default_opts; +static const struct ll_merge_options default_opts =LL_MERGE_OPTIONS_INIT ;const char *ll_driver_name =NULL ;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;const struct ll_merge_driver *driver;
merge-ll.h +2 -0
@@ -82,6 +82,8 @@ struct ll_merge_options {long xdl_opts; }; +#define LL_MERGE_OPTIONS_INIT {0} +enum ll_merge_result {LL_MERGE_ERROR =-1 ,LL_MERGE_OK =0 ,
merge-ort.c +1 -1
@@ -1956,7 +1956,7 @@ static int merge_3way(struct merge_options *opt, mmbuffer_t *result_buf) {mmfile_t orig, src1, src2; -struct ll_merge_options ll_opts = {0 }; +struct ll_merge_options ll_opts =LL_MERGE_OPTIONS_INIT ;char *base, *name1, *name2;enum ll_merge_result merge_status;
merge-recursive.c +1 -1
@@ -1047,7 +1047,7 @@ static int merge_3way(struct merge_options *opt, const int extra_marker_size) {mmfile_t orig, src1, src2; -struct ll_merge_options ll_opts = {0 }; +struct ll_merge_options ll_opts =LL_MERGE_OPTIONS_INIT ;char *base, *name1, *name2;enum ll_merge_result merge_status;
Signature · 1 line
gitgitgadget
PATCH v2 3/5 merge options: add a conflict style member
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Add a conflict_style member to struct merge_options and struct ll_merge_options to allow callers to override the default conflict style. This will be used in the next commit.
--- merge-ll.c | 4 +++- merge-ll.h | 5 ++++- merge-ort.c | 1 + merge-recursive.c | 3 +++ merge-recursive.h | 1 + 5 files changed, 12 insertions(+), 2 deletions(-)
merge-ll.c +3 -1
@@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse xmp.level =XDL_MERGE_ZEALOUS ; xmp.favor = opts->variant; xmp.xpp.flags = opts->xdl_opts; -if (git_xmerge_style >=0 ) +if (opts->conflict_style >=0 ) + xmp.style = opts->conflict_style; +else if (git_xmerge_style >=0 ) xmp.style = git_xmerge_style;if (marker_size >0 ) xmp.marker_size = marker_size;
merge-ll.h +4 -1
@@ -78,11 +78,14 @@ struct ll_merge_options {*/ unsigned extra_marker_size; +/* Override the global conflict style. */ +int conflict_style; +/* Extra xpparam_t flags as defined in xdiff/xdiff.h. */ long xdl_opts; }; -#define LL_MERGE_OPTIONS_INIT {0} +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 }enum ll_merge_result {LL_MERGE_ERROR =-1 ,
merge-ort.c +1 -0
@@ -1966,6 +1966,7 @@ static int merge_3way(struct merge_options *opt, ll_opts.renormalize = opt->renormalize; ll_opts.extra_marker_size = extra_marker_size; ll_opts.xdl_opts = opt->xdl_opts; + ll_opts.conflict_style = opt->conflict_style;if (opt->priv->call_depth) { ll_opts.virtual_ancestor =1 ;
merge-recursive.c +3 -0
@@ -1054,6 +1054,7 @@ static int merge_3way(struct merge_options *opt, ll_opts.renormalize = opt->renormalize; ll_opts.extra_marker_size = extra_marker_size; ll_opts.xdl_opts = opt->xdl_opts; + ll_opts.conflict_style = opt->conflict_style;if (opt->priv->call_depth) { ll_opts.virtual_ancestor =1 ;
@@ -3899,6 +3900,8 @@ void init_merge_options(struct merge_options *opt, opt->renormalize =0 ; + opt->conflict_style =-1 ; +merge_recursive_config (opt); merge_verbosity =getenv ("GIT_MERGE_VERBOSITY" );if (merge_verbosity)
merge-recursive.h +1 -0
@@ -31,6 +31,7 @@ struct merge_options {/* xdiff-related options (patience, ignore whitespace, ours/theirs) */ long xdl_opts; +int conflict_style;enum {MERGE_VARIANT_NORMAL =0 ,MERGE_VARIANT_OURS ,
Signature · 1 line
gitgitgadget
Re: [PATCH v2 3/5] merge options: add a conflict style member
merge-ll.c static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse · line 134
Quoted 11 lines · Phillip Wood via GitGitGadget
merge-ll.c +3
-1@@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse xmp.level =XDL_MERGE_ZEALOUS ; xmp.favor = opts->variant; xmp.xpp.flags = opts->xdl_opts; -if (git_xmerge_style >=0 ) +if (opts->conflict_style >=0 ) + xmp.style = opts->conflict_style; +else if (git_xmerge_style >=0 ) xmp.style = git_xmerge_style;
The convention being negative ones are unknown/invalid/unspecified, this if/else cascade makes sense.
PATCH v2 4/5 checkout: cleanup --conflict=<style> parsing
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Passing an invalid conflict style name such as "--conflict=bad" 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 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_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.
--- builtin/checkout.c | 51 +++++++++++++++++++++++++++++----------------- t/t7201-co.sh | 6 ++++++ 2 files changed, 38 insertions(+), 19 deletions(-)
builtin/checkout.c +32 -19
@@ -91,7 +91,7 @@ struct checkout_opts {int new_branch_log;enum branch_track track;struct diff_options diff_options; -char *conflict_style; +int conflict_style;int branch_exists;const char *prefix;
@@ -100,6 +100,8 @@ struct checkout_opts {struct tree *source_tree; }; +#define CHECKOUT_OPTS_INIT { .conflict_style = -1 } +struct branch_info {char *name;/* The short name used */ char *path;/* The full name of a real branch */
@@ -251,7 +253,8 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos, }static int checkout_merged (int pos,const struct checkout *state, -int *nr_checkouts,struct mem_pool *ce_mem_pool) +int *nr_checkouts,struct mem_pool *ce_mem_pool, +int conflict_style) {struct cache_entry *ce = the_index.cache[pos];const char *path = ce->name;
@@ -286,6 +289,7 @@ static int checkout_merged(int pos, const struct checkout *state,git_config_get_bool ("merge.renormalize" , &renormalize); ll_opts.renormalize = renormalize; + ll_opts.conflict_style = conflict_style; merge_status =ll_merge (&result_buf, path, &ancestor,"base" , &ours,"ours" , &theirs,"theirs" , state->istate, &ll_opts);
@@ -416,7 +420,8 @@ static int checkout_worktree(const struct checkout_opts *opts,else if (opts->merge) errs |=checkout_merged (pos, &state, &nr_unmerged, - &ce_mem_pool); + &ce_mem_pool, + opts->conflict_style); pos =skip_same_name (ce, pos) -1 ; } }
@@ -886,6 +891,7 @@ static int merge_working_tree(const struct checkout_opts *opts, } o.branch1 = new_branch_info->name; o.branch2 ="local" ; + o.conflict_style = opts->conflict_style; ret = merge_trees(&o, new_tree, work,
@@ -1618,6 +1624,21 @@ 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) {
@@ -1628,8 +1649,9 @@ 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" ), -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);
@@ -1720,15 +1742,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix, opts->show_progress =isatty (2 ); } -if (opts->conflict_style) { -struct key_value_info kvi =KVI_INIT ; -struct config_context ctx = { - .kvi = &kvi, - }; +if (opts->conflict_style >=0 ) opts->merge =1 ;/* implied */ -git_xmerge_config ("merge.conflictstyle" , opts->conflict_style, - &ctx,NULL ); - } +if (opts->force) { opts->discard_changes =1 ; opts->ignore_unmerged_opt ="--force" ;
@@ -1893,7 +1909,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, int cmd_checkout(int argc, const char **argv, const char *prefix) { - struct checkout_opts opts; + struct checkout_opts opts = CHECKOUT_OPTS_INIT; struct option *options; struct option checkout_options[] = { OPT_STRING('b', NULL, &opts.new_branch, N_("branch"),
@@ -1909,7 +1925,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)int ret;struct branch_info new_branch_info = {0 }; -memset (&opts,0 ,sizeof (opts)); opts.dwim_new_local_branch =1 ; opts.switch_branch_doing_nothing_is_ok =1 ; opts.only_merge_on_switching_branches =0 ;
@@ -1948,7 +1963,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) int cmd_switch(int argc, const char **argv, const char *prefix) { - struct checkout_opts opts; + struct checkout_opts opts = CHECKOUT_OPTS_INIT; struct option *options = NULL; struct option switch_options[] = { OPT_STRING('c', "create", &opts.new_branch, N_("branch"),
@@ -1964,7 +1979,6 @@ int cmd_switch(int argc, const char **argv, const char *prefix)int ret;struct branch_info new_branch_info = {0 }; -memset (&opts,0 ,sizeof (opts)); opts.dwim_new_local_branch =1 ; opts.accept_ref =1 ; opts.accept_pathspec =0 ;
@@ -1990,7 +2004,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix) int cmd_restore(int argc, const char **argv, const char *prefix) { - struct checkout_opts opts; + struct checkout_opts opts = CHECKOUT_OPTS_INIT; struct option *options; struct option restore_options[] = { OPT_STRING('s', "source", &opts.from_treeish, "<tree-ish>",
@@ -2007,7 +2021,6 @@ int cmd_restore(int argc, const char **argv, const char *prefix)int ret;struct branch_info new_branch_info = {0 }; -memset (&opts,0 ,sizeof (opts)); opts.accept_ref =0 ; opts.accept_pathspec =1 ; opts.empty_pathspec_ok =0 ;
t/t7201-co.sh +6 -0
@@ -631,6 +631,12 @@ test_expect_success 'checkout --conflict=diff3' ' test_cmp merged file ' +test_expect_success 'checkout with invalid conflict style' ' + test_must_fail git checkout --conflict=bad 2>actual -- file && + echo "error: unknown conflict style ${SQ}bad${SQ}" >expect && + test_cmp expect actual +' + test_expect_success 'failing checkout -b should not break working tree' ' git clean -fd && # Remove untracked files in the way git reset --hard main &&
Signature · 1 line
gitgitgadget
Re: [PATCH v2 4/5] checkout: cleanup --conflict=<style> parsing
Quoted 20 lines · Phillip Wood via GitGitGadget
builtin/checkout.c +15
-0+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) {@@ -1628,8 +1649,9 @@ static struct option *add_common_options(struct checkout_opts *opts, PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater),
builtin/checkout.c static struct option *add_common_options(struct checkout_opts *opts, · line 1657
Quoted 10 lines
builtin/checkout.c +3
-2OPT_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" ), -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);
Looking good.
PATCH v2 5/5 checkout: fix interaction between --conflict and --merge
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
When using "git checkout" to recreate merge conflicts or merge uncommitted changes when switching branch "--conflict" sensibly implies "--merge". Unfortunately the way this is implemented means that "git checkout --conflict=diff3 --no-merge" implies "--merge" violating the usual last-one-wins rule. Fix this by only overriding the value of opts->merge if "--conflicts" comes after "--no-merge" or "-[-no]-merge" is not given on the command line.
The behavior of "git checkout --merge --no-conflict" is unchanged and will still merge on the basis that the "-[-no]-conflict" options are primarily intended to affect the conflict style and so "--no-conflict" should cancel a previous "--conflict" but not override "--merge".
Of the four new tests the second one tests the behavior change introduced by this commit, the other three check that this commit does not regress the existing behavior.
--- builtin/checkout.c | 10 +++++--- t/t7201-co.sh | 60 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-)
builtin/checkout.c +7 -3
@@ -100,7 +100,7 @@ struct checkout_opts {struct tree *source_tree; }; -#define CHECKOUT_OPTS_INIT { .conflict_style = -1 } +#define CHECKOUT_OPTS_INIT { .conflict_style = -1, .merge = -1 }struct branch_info {char *name;/* The short name used */
@@ -1635,6 +1635,9 @@ static int parse_opt_conflict(const struct option *o, const char *arg, int unset opts->conflict_style =parse_conflict_style_name (arg);if (opts->conflict_style <0 )return error (_ ("unknown conflict style '%s'" ), arg); +/* --conflict overrides a previous --no-merge */ +if (!opts->merge) + opts->merge =-1 ;return 0 ; }
@@ -1742,8 +1745,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix, opts->show_progress =isatty (2 ); } -if (opts->conflict_style >=0 ) - opts->merge =1 ;/* implied */ +/* --conflicts implies --merge */ +if (opts->merge ==-1 ) + opts->merge = opts->conflict_style >=0 ;if (opts->force) { opts->discard_changes =1 ;
t/t7201-co.sh +60 -0
66 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v2 5/5] checkout: fix interaction between --conflict and --merge
Quoted 9 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
When using "git checkout" to recreate merge conflicts or merge uncommitted changes when switching branch "--conflict" sensibly implies "--merge". Unfortunately the way this is implemented means that "git checkout --conflict=diff3 --no-merge" implies "--merge" violating the usual last-one-wins rule. Fix this by only overriding the value of
opts->mergeif "--conflicts" comes after "--no-merge" or "-[-no]-merge" is not given on the command line.
That smells like a convoluted logic but I think I can buy the argument. If "--conflict=diff3" implies "--conflict=diff3 --merge", then "--conflict=diff3 --no-merge" should imply "--conflict=diff3 --merge --no-merge" and the latter two cancels out with the last-one-wins rule, leaving only "--conflict=diff3" that does not imply anything about "--merge". The conflict style specification does not have any effect when we are not recreating any merge, so all of them are ignored in the end. So, it probably makes sense, even though I find it highly confusing.
Is it likely that "--conflict=diff3 --no-merge" signals that the user is confused and it is safer to abort the operation before doing further harm, though, I wonder?
Thanks.
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
Quoted 165 lines
Phillip Wood (5):
xdiff-interface: refactor parsing of merge.conflictstyle
merge-ll: introduceLL_MERGE_OPTIONS_INIT
merge options: add a conflict style member
checkout: cleanup --conflict=<style> parsing
checkout: fix interaction between --conflict and --mergebuiltin/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/1684Range-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 <[email protected]> @@ 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:
b771b29e45acheckout: fix interaction between --conflict and --merge