| Message ID | 20210609192842.696646-6-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make diff3 the default conflict style | expand |
On 09/06/2021 20:28, Felipe Contreras wrote: The subject would make more sense as 'xdiff: rename XDL_MERGE_DIFF3 to XDL_MERGE_STYLE_DIFF3' rather than using the new name of the constant alone. > If we don't specify we are talking about a style, XDL_MERGE_MINIMAL > could be confused with a valid value instead of XDL_MERGE_DIFF3, which > it isn't. I don't object to the rename but what is the source of the confusion with XDL_MERGE_MINIMAL? Best Wishes Phillip > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/merge-file.c | 2 +- > xdiff-interface.c | 2 +- > xdiff/xdiff.h | 2 +- > xdiff/xmerge.c | 4 ++-- > 4 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/builtin/merge-file.c b/builtin/merge-file.c > index 06a2f90c48..a4097a596f 100644 > --- a/builtin/merge-file.c > +++ b/builtin/merge-file.c > @@ -33,7 +33,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) > int quiet = 0; > struct option options[] = { > OPT_BOOL('p', "stdout", &to_stdout, N_("send results to standard output")), > - OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_DIFF3), > + OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_STYLE_DIFF3), > OPT_SET_INT(0, "ours", &xmp.favor, N_("for conflicts, use our version"), > XDL_MERGE_FAVOR_OURS), > OPT_SET_INT(0, "theirs", &xmp.favor, N_("for conflicts, use their version"), > diff --git a/xdiff-interface.c b/xdiff-interface.c > index 609615db2c..64e2c4e301 100644 > --- a/xdiff-interface.c > +++ b/xdiff-interface.c > @@ -307,7 +307,7 @@ int git_xmerge_config(const char *var, const char *value, void *cb) > if (!value) > die("'%s' is not a boolean", var); > if (!strcmp(value, "diff3")) > - git_xmerge_style = XDL_MERGE_DIFF3; > + git_xmerge_style = XDL_MERGE_STYLE_DIFF3; > else if (!strcmp(value, "merge")) > git_xmerge_style = 0; > /* > diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h > index 7a04605146..45883f5eb3 100644 > --- a/xdiff/xdiff.h > +++ b/xdiff/xdiff.h > @@ -64,7 +64,7 @@ extern "C" { > #define XDL_MERGE_FAVOR_UNION 3 > > /* merge output styles */ > -#define XDL_MERGE_DIFF3 1 > +#define XDL_MERGE_STYLE_DIFF3 1 > > typedef struct s_mmfile { > char *ptr; > diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c > index 1659edb453..f6916a4ba4 100644 > --- a/xdiff/xmerge.c > +++ b/xdiff/xmerge.c > @@ -230,7 +230,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1, > size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1, > dest ? dest + size : NULL); > > - if (style == XDL_MERGE_DIFF3) { > + if (style == XDL_MERGE_STYLE_DIFF3) { > /* Shared preimage */ > if (!dest) { > size += marker_size + 1 + needs_cr + marker3_size; > @@ -482,7 +482,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, > int style = xmp->style; > int favor = xmp->favor; > > - if (style == XDL_MERGE_DIFF3) { > + if (style == XDL_MERGE_STYLE_DIFF3) { > /* > * "diff3 -m" output does not make sense for anything > * more aggressive than XDL_MERGE_EAGER. >
Phillip Wood wrote: > On 09/06/2021 20:28, Felipe Contreras wrote: > > The subject would make more sense as 'xdiff: rename XDL_MERGE_DIFF3 to > XDL_MERGE_STYLE_DIFF3' rather than using the new name of the constant alone. That is 55 characters, more than the recommended commit title length. > > If we don't specify we are talking about a style, XDL_MERGE_MINIMAL > > could be confused with a valid value instead of XDL_MERGE_DIFF3, which > > it isn't. > > I don't object to the rename but what is the source of the confusion > with XDL_MERGE_MINIMAL? XDL_MERGE_MINIMAL and other XDL_MERGE_FOO constants go into xmparam_t.level, XDL_MERGE_DIFF3 does not. As stated in the commit message, the name XDL_MERGE_DIFF3 doesn't distinguish it as a style. Chers.
Phillip Wood <phillip.wood123@gmail.com> writes: > The subject would make more sense as 'xdiff: rename XDL_MERGE_DIFF3 to > XDL_MERGE_STYLE_DIFF3' rather than using the new name of the constant > alone. True. >> If we don't specify we are talking about a style, XDL_MERGE_MINIMAL >> could be confused with a valid value instead of XDL_MERGE_DIFF3, which >> it isn't. > > I don't object to the rename but what is the source of the confusion > with XDL_MERGE_MINIMAL? I do not see any confusion, either, but the current XDL_MERGE_DIFF3 being a boolean (i.e. if false, use the output style of the 'merge' command) and our lack of an enumeration constant for 'merge' means that a future addition of the third output style would require us to add XDL_MERGE_$STYLE for both the new style and the traditional 'merge' style. And If we would end up with XDL_MERGE_DIFF3, XDL_MERGE_MERGE and XDL_MERGE_FOO for that third output style. The 'merge' one simply looks strange in that context. And from that point of view, this change might be a good way to futureproof the codebase. Thanks.
Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > > The subject would make more sense as 'xdiff: rename XDL_MERGE_DIFF3 to > > XDL_MERGE_STYLE_DIFF3' rather than using the new name of the constant > > alone. > > True. But why? When we look back in history few people would care what the previous name of XDL_MERGE_STYLE_DIFF3 was, and if they do, they don't necessarily need it in the title. > >> If we don't specify we are talking about a style, XDL_MERGE_MINIMAL > >> could be confused with a valid value instead of XDL_MERGE_DIFF3, which > >> it isn't. > > > > I don't object to the rename but what is the source of the confusion > > with XDL_MERGE_MINIMAL? > > I do not see any confusion, either, but the current XDL_MERGE_DIFF3 > being a boolean But it's not a boolean: git_xmerge_style is currently -1 by default. > (i.e. if false, use the output style of the 'merge' > command) and our lack of an enumeration constant for 'merge' means > that a future addition of the third output style would require us to > add XDL_MERGE_$STYLE for both the new style and the traditional > 'merge' style. And If we would end up with XDL_MERGE_DIFF3, > XDL_MERGE_MERGE and XDL_MERGE_FOO for that third output style. But can you put XDL_MERGE_FOO in xmp.level? Or XDL_MERGE_BAR in xmp.style? > The 'merge' one simply looks strange in that context. And from that > point of view, this change might be a good way to futureproof the > codebase. Yes.
diff --git a/builtin/merge-file.c b/builtin/merge-file.c index 06a2f90c48..a4097a596f 100644 --- a/builtin/merge-file.c +++ b/builtin/merge-file.c @@ -33,7 +33,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) int quiet = 0; struct option options[] = { OPT_BOOL('p', "stdout", &to_stdout, N_("send results to standard output")), - OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_DIFF3), + OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_STYLE_DIFF3), OPT_SET_INT(0, "ours", &xmp.favor, N_("for conflicts, use our version"), XDL_MERGE_FAVOR_OURS), OPT_SET_INT(0, "theirs", &xmp.favor, N_("for conflicts, use their version"), diff --git a/xdiff-interface.c b/xdiff-interface.c index 609615db2c..64e2c4e301 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -307,7 +307,7 @@ int git_xmerge_config(const char *var, const char *value, void *cb) if (!value) die("'%s' is not a boolean", var); if (!strcmp(value, "diff3")) - git_xmerge_style = XDL_MERGE_DIFF3; + git_xmerge_style = XDL_MERGE_STYLE_DIFF3; else if (!strcmp(value, "merge")) git_xmerge_style = 0; /* diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h index 7a04605146..45883f5eb3 100644 --- a/xdiff/xdiff.h +++ b/xdiff/xdiff.h @@ -64,7 +64,7 @@ extern "C" { #define XDL_MERGE_FAVOR_UNION 3 /* merge output styles */ -#define XDL_MERGE_DIFF3 1 +#define XDL_MERGE_STYLE_DIFF3 1 typedef struct s_mmfile { char *ptr; diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index 1659edb453..f6916a4ba4 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c @@ -230,7 +230,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1, size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1, dest ? dest + size : NULL); - if (style == XDL_MERGE_DIFF3) { + if (style == XDL_MERGE_STYLE_DIFF3) { /* Shared preimage */ if (!dest) { size += marker_size + 1 + needs_cr + marker3_size; @@ -482,7 +482,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, int style = xmp->style; int favor = xmp->favor; - if (style == XDL_MERGE_DIFF3) { + if (style == XDL_MERGE_STYLE_DIFF3) { /* * "diff3 -m" output does not make sense for anything * more aggressive than XDL_MERGE_EAGER.
If we don't specify we are talking about a style, XDL_MERGE_MINIMAL could be confused with a valid value instead of XDL_MERGE_DIFF3, which it isn't. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/merge-file.c | 2 +- xdiff-interface.c | 2 +- xdiff/xdiff.h | 2 +- xdiff/xmerge.c | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-)