diff mbox series

[5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3

Message ID 20210609192842.696646-6-felipe.contreras@gmail.com (mailing list archive)
State New
Headers show
Series Make diff3 the default conflict style | expand

Commit Message

Felipe Contreras June 9, 2021, 7:28 p.m. UTC
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(-)

Comments

Phillip Wood June 10, 2021, 9:21 a.m. UTC | #1
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.
>
Felipe Contreras June 10, 2021, 1:33 p.m. UTC | #2
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.
Junio C Hamano June 11, 2021, 3:17 a.m. UTC | #3
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.
Felipe Contreras June 11, 2021, 1:42 p.m. UTC | #4
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 mbox series

Patch

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.